👌 Protocols: support pseudo families without recommended cutoffs

[elinscott]

Summary

This PR allows non-SSSP/PseudoDojo pseudo families to be used with the builders.

aiida-pseudo install family <folder> <label> installs a family the builders then refuse:

ValueError: required pseudo family `MyPseudos/local` is not installed.
Please use `aiida-pseudo install` toinstall it.

It is installed. PwBaseWorkChain.get_builder_from_protocol looks only for the family classes that can recommend cutoffs, and asks for those cutoffs before fetching the pseudos, so a family without them cannot be used at all — even when the overrides already give both (which take precedence anyway). A CutoffsPseudoPotentialFamily whose stringency is never set fails the same way, with no default stringency has been defined.

Changes

First commit. The recommended cutoffs are requested only when the overrides do not already define both; the pseudos are fetched either way. The error still fires when nothing provides them, now naming the alternative.

Second commit. The query now names the base class rather than the three cutoff-recommending subclasses, and the capability is checked where the cutoffs are needed (isinstance(..., RecommendedCutoffMixin)):

ValueError: pseudo family `PseudoPotentialFamily<MyPseudos/local>` cannot recommend cutoffs.
Provide both `ecutwfc` and `ecutrho` in the `overrides`, or use a family that recommends them.

The old query's line was not "families whose cutoffs track the protocol": get_recommended_cutoffs is called with no stringency, and only stringent varies the family — by naming a different one, which a cutoffs family follows no better than a plain one.

A question for the maintainers

Is the intent that get_builder_from_protocol always returns a builder with cutoffs set — so a plain family plus override cutoffs is legitimate, as implemented — or that only cutoff-recommending families are supported at all, in which case the right fix is just a clearer error and no widening? I have taken the first; the second commit can be dropped on its own.

Notes

• this adds the first direct aiida_pseudo import to src/ (entry points cover group classes, not mixins)
• this drops the now-unused module-level SsspFamily / PseudoDojoFamily / CutoffsPseudoPotentialFamily names from workflows/pw/base.py. Say if you would rather keep them.
• Protocols: Add aiida-pseudo install command to ValueError #931 proposes improving the neighbouring "not installed" error. This does not reword it, but it does stop it firing for families that are installed.

[t-reents]

I'll have a closer look later today.

Regarding your question:

Is the intent that get_builder_from_protocol always returns a builder with cutoffs set — so a plain family plus override cutoffs is legitimate, as implemented — or that only cutoff-recommending families are supported at all, in which case the right fix is just a clearer error and no widening?

Based on the historical implementation, I think the latter is/was true (not sure about the intention). Nonetheless, I think that the most important aspect is that the builder should always be populated when it gets returned. Hence, I don't see a reason why one should limit it to cutoff recommending families.

I think there are/should be 3 valid options:

1. A cutoff recommending family is used: nothing else needed
2. A PP family without recommended cutoffs: Check that cutoffs are provided in the overrides. If not, raise. So the important part is indeed that this should not work without the overrides.
3. Custom pseudos via overrides: As done in 🐛 get_builder_from_protocol: Fix bugs when overriding pseudopotentials #1102, ensure that pseudos are provided for all elements via the overrides and that the cutoffs are also manually specified.

As long as those 3 cases are covered, I don't see a problem. Based on the approach that you selected, I guess that you agree with my interpretation, so no need to change anything.

As I said, I will have a look at the actual implementation later, but if there are no further issues, I'm happy to continue with your "design".

[t-reents]

This was not introduced by this PR, but would you mind adding another exception for MultipleObjectsError, in case multiple families with the same label are found?

Other than that, I reviewed the implementation and all looks good.

[elinscott]

Added in the latest commit. To be fair, it's not actually entirely unrelated: group labels are unique per type string, so a plain PseudoPotentialFamily and an SsspFamily/CutoffsPseudoPotentialFamily can share one. so this PR makes this clash more likely.

Now gives an error such as

    ValueError: the label `custom/shared-label` matches more than one installed pseudo family:
    `PseudoPotentialFamily<custom/shared-label>`, `CutoffsPseudoPotentialFamily<custom/shared-label>`.
    Please delete or relabel all but one, or pass the `pseudos` in the `overrides` instead.

[elinscott]

Thanks @t-reents !

I think there are/should be 3 valid options:

1. A cutoff recommending family is used: nothing else needed
2. A PP family without recommended cutoffs: Check that cutoffs are provided in the overrides. If not, raise. So the important part is indeed that this should not work without the overrides.
3. Custom pseudos via overrides: As done in 🐛 get_builder_from_protocol: Fix bugs when overriding pseudopotentials #1102, ensure that pseudos are provided for all elements via the overrides and that the cutoffs are also manually specified.

Agreed. Cases 1 and 2 are what the PR implements; I checked case 3 against the current code rather than assuming it, and it behaves as you describe. The pseudos-in-overrides branch compares sorted(pseudos.keys()) with the structure's kind names, so pseudos for only some kinds are refused, as are keys naming something the structure does not contain, and both ecutwfc and ecutrho have to be in the overrides. On a water molecule I get:

# pseudos for O only, both cutoffs given
ValueError: `pseudos` override needs one value for each of the 2 kinds.

# pseudos for both kinds, ecutwfc only
ValueError: When overriding the pseudo potentials, both `ecutwfc` and `ecutrho` cutoffs should be provided in the `overrides`: ...

[t-reents]

Thanks for adding the test and also confirming the expected behavior!
I'll merge once the CI passes (which was already the case before I merged main).

[elinscott]

Thanks for adding the test and also confirming the expected behavior! I'll merge once the CI passes (which was already the case before I merged main).

No problem — thanks! (In case you missed it, see my in-thread reply I just posted)

[t-reents]

Ah yes, just saw the comment. Indeed, this is also why I raised it. While it was already possible before, the changes made it a bit more likely to actually occur.

[elinscott]

Ah yes, just saw the comment. Indeed, this is also why I raised it. While it was already possible before, the changes made it a bit more likely to actually occur.

Turns out this is an edge case but it can happen if you're sadistic: aiidateam/aiida-pseudo#206

[t-reents]

Haha, yes, I also played with it yesterday before asking for the test. I will have a look at aiida-pseudo next week, but I also think that this should be handled differently