search

aiidateam / aiida-quantumespresso

The official AiiDA plugin for Quantum ESPRESSO
https://aiida-quantumespresso.readthedocs.io
Other
52 stars 77 forks source link

Bump default SSSP version in protocol to v1.3 #994

Closed unkcpz closed 6 months ago

unkcpz commented 6 months ago

This is required by https://github.com/aiidalab/aiidalab-qe/pull/578. It is not that easy to decouple reading protocol from aiida-quatumespresso in QeApp, if the override is not provided, it will always fallback to the default pseudo family which may not aligned between the qe plugin and the qe app.

It would be great after this change, a new patch release can be made. Or it has to be a minor version bump for aiida-quantumespresso?

mbercx commented 6 months ago

Thanks @unkcpz! Also updated the documentation where I found references to the SSSP pseudos.

A patch release is a bit tedious since we have everything on main. But doing a minor release isn't so much work. How urgently do you need it? ^^

unkcpz commented 6 months ago

Thanks for such a quick update!

How urgently do you need it?

Can I say very very urgent? 😉 My mistake was that I somehow missed this requirement for qeapp and it is what our big boss ask to include for the qeapp before release exactly.

mbercx commented 6 months ago

Can I say very very urgent? 😉

I'll start prepping a release branch then ^^

mbercx commented 6 months ago

Interesting. Re-requesting a review does not dismiss my old one. Was curious what would happen there. ^^

Anyways, let me also check the aiida-pseudo version where SSSP v1.3 was added, we should probably bump our dependency in case it's below that one.

unkcpz commented 6 months ago

let me also check the aiida-pseudo version where SSSP v1.3 was added, we should probably bump our dependency in case it's below that one.

I think aiida-pseudo~=1.1 should work. I find it from changelog https://github.com/aiidateam/aiida-pseudo/blob/a4face268b9a911caacecedbcbc605db389dc68e/CHANGELOG.md?plain=1#L31

EDIT: nevermind, you come first 😜

mbercx commented 6 months ago

Seems SSSP v1.3 support was only added in aiida-pseudo==1.1.0.

mbercx commented 6 months ago

let me also check the aiida-pseudo version where SSSP v1.3 was added, we should probably bump our dependency in case it's below that one.

I think aiida-pseudo~=1.1 should work. I find it from changelog https://github.com/aiidateam/aiida-pseudo/blob/a4face268b9a911caacecedbcbc605db389dc68e/CHANGELOG.md?plain=1#L31

Hehe, beat me to it. ;)

unkcpz commented 6 months ago

Test locally on qeapp with these changes and works as expected.

sphuber commented 6 months ago

Now I only pray that I haven't missed something and @sphuber hits me with the spanking gifs in the morning. ^^

Nah, I already spent all my creative energy on another post today.

Changes look fine to me. It is just that people with existing profiles will now probably face an exception when updating. I think the PwBaseWorkChain.get_builder_from_protocol will now require the v1.3 PBEsol SSSP to be installed. The error will just provide a very generic message Please useaiida-pseudo installto install it. But users won't automatically now what the exact command is. We should improve this by either: 1) improve the error message to be explicit and give the full exact command 2) allow any installed SSSP version as a fallback and issue a warning if that is used

I think for most cases option 2) would probably be most user-friendly. Just for users that actually want a very specific version and forgot to install it (or mistyped it in the protocol) could be surprises, but not sure how common this will be.

unkcpz commented 6 months ago

I think the PwBaseWorkChain.get_builder_from_protocol will now require the v1.3 PBEsol SSSP to be installed.

@sphuber True! this is the problem I want to avoid in QeApp by this change here, because in QeApp we install the SSSP and dojo by default, after bumping the version the get_builder_from_protocol all fails with not able to find the pseudo-family.

But I think option 1 might be better. The user anyway needs to set up pseudo libraries the first time why bother to do it again, it also pushes users to use the new libraries as default. What do you think? @mbercx

mbercx commented 6 months ago

I'm also leaning towards (1). Seems more straightforward to motivate the user to just install the new pseudos. But then it'd nice to have a more general command that converts a certain configuration into the corresponding CLI command to install it. And I suppose this would be more at home in the aiida-pseudo package. ^^