`RecommendedCutoffMixin`: Add support for `aiida-pseudo>=0.4.0` families

mbercx commented 3 years ago

Currently, pseudo families installed with aiida-pseudo>=0.4.0 will fail to work with the new API features in two ways, both related to the addition of units for the recommended cutoffs:

delete_cutoffs expects the units to be stored in the '_cutoffs_unit' extras dictionary. Simply adding None as a default to return in the .pop() call fixes this issue.
get_cutoffs_unit will return a KeyError since the previously configured stringencies did not include units. Here we can add an if clause that check if the requested stringency does have its cutoffs set. Since the default unit was and is eV, we set this as the new unit for the stringency. From then on the pseudo family will be "fixed".

Note: families installed with aiida-pseudo<=0.3.0 will still cause problems since the units for the recommended cutoffs were set to eV in v0.4.0.

mbercx commented 3 years ago

Note that these workarounds can be temporary, since if the user actually uses the get_cutoffs_unit method at some point (directly or indirectly), the default unit (eV) will be added to the pseudo family.

mbercx commented 3 years ago

@sphuber If you agree with adding these workarounds, I'll also open an issue on removing them so we don't forget.

sphuber commented 3 years ago

If you agree with adding these workarounds, I'll also open an issue on removing them so we don't forget.

Good to add an issue, but the tricky part will be deciding when we can remove it. Consider the example where someone is running aiida-pseudo==0.5.0 now and installed some families. They work with this version and not until a long time do they decide to upgrade and most likely will upgrade to the latest version. If that version happens to have removed the workaround, they will run into the exception. This shows that in principle we can never know when it is safe to remove them since we don't know when "everyone" that was on v0.5 will have upgraded to a newer version with the implicit data migration.

mbercx commented 3 years ago

Good to add an issue, but the tricky part will be deciding when we can remove it.

Indeed. v1.0.0 seems like a reasonable choice, but that might still be a long ways off. Still, better to have a bit of nastiness for some time that fixes the issue for most people in our massive user base ;).

sphuber commented 3 years ago

Indeed. v1.0.0 seems like a reasonable choice, but that might still be a long ways off. Still, better to have a bit of nastiness for some time that fixes the issue for most people in our massive user base ;).

Haha, still I wouldn't underestimate it. "Everyone" running aiida-quantumespresso and aiida-abinit will be affected and especially for QE there are now quite some users. Anyway, I think this workaround is not that invasive and is more than fine to keep in given the benefits it provides. No rush to get it out either.

Final thing, you verified that this then works with existing installations of aiida-quantumespresso? If so, I will approve merge and update the release. I think we are then ready for a release, arent we?

mbercx commented 3 years ago

Final thing, you verified that this then works with existing installations of aiida-quantumespresso? If so, I will approve merge and update the release. I think we are then ready for a release, arent we?

I opened a PR for providing support for aiida-pseudo==0.6.0 in aiida-quantumespresso:

https://github.com/aiidateam/aiida-quantumespresso/pull/671

And tested the get_builder_from_protocol() method using both pseudos installed with aiida-pseudo v0.5.0 and this branch (reverting back to this branch after installing with v0.5.0). Both tests worked, so I think we're good to go!

aiidateam / aiida-pseudo

`RecommendedCutoffMixin`: Add support for `aiida-pseudo>=0.4.0` families #75