Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 3 commits into from
Dec 12, 2023

Conversation

unkcpz
Copy link
Member

@unkcpz unkcpz commented Dec 12, 2023

This is required by aiidalab/aiidalab-qe#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?

@unkcpz unkcpz force-pushed the fea/xx/sssp-version-bump-to-v13 branch from d078009 to 159fd57 Compare December 12, 2023 20:56
@unkcpz unkcpz requested a review from mbercx December 12, 2023 20:56
@mbercx mbercx force-pushed the fea/xx/sssp-version-bump-to-v13 branch from 0b7d482 to be82377 Compare December 12, 2023 21:05
@mbercx
Copy link
Member

mbercx commented Dec 12, 2023

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? ^^

@mbercx mbercx force-pushed the fea/xx/sssp-version-bump-to-v13 branch from be82377 to a5e2eb8 Compare December 12, 2023 21:21
@unkcpz
Copy link
Member Author

unkcpz commented Dec 12, 2023

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
Copy link
Member

mbercx commented Dec 12, 2023

Can I say very very urgent? 😉

I'll start prepping a release branch then ^^

mbercx
mbercx previously approved these changes Dec 12, 2023
@mbercx mbercx self-requested a review December 12, 2023 21:53
@mbercx
Copy link
Member

mbercx commented Dec 12, 2023

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.

@mbercx mbercx force-pushed the fea/xx/sssp-version-bump-to-v13 branch from a5e2eb8 to 9bbaedf Compare December 12, 2023 22:01
@unkcpz
Copy link
Member Author

unkcpz commented Dec 12, 2023

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 mbercx force-pushed the fea/xx/sssp-version-bump-to-v13 branch from 9bbaedf to b910d3c Compare December 12, 2023 22:04
@mbercx
Copy link
Member

mbercx commented Dec 12, 2023

@mbercx
Copy link
Member

mbercx commented Dec 12, 2023

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. ;)

Copy link
Member

@mbercx mbercx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@mbercx mbercx merged commit 49d503d into aiidateam:main Dec 12, 2023
7 checks passed
@unkcpz unkcpz deleted the fea/xx/sssp-version-bump-to-v13 branch December 12, 2023 22:16
@unkcpz
Copy link
Member Author

unkcpz commented Dec 13, 2023

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

@sphuber
Copy link
Contributor

sphuber commented Dec 13, 2023

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 use aiida-pseudo install to 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
Copy link
Member Author

unkcpz commented Dec 13, 2023

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
Copy link
Member

mbercx commented Dec 13, 2023

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. ^^

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants