-
Notifications
You must be signed in to change notification settings - Fork 14
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
Define daemon restart command based on the aiida-core version #424
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #424 +/- ##
==========================================
- Coverage 60.09% 59.95% -0.14%
==========================================
Files 23 23
Lines 1466 1471 +5
==========================================
+ Hits 881 882 +1
- Misses 585 589 +4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Hi @danielhollas, This is a critical bug. In the QEApp case, this blocks running the latest QEApp in image-2.4.3. When this PR gets merged, could you make a new release of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the change.
Not sure I understand how this affects QeApp. Are you calling this function from QeApp? If that's the case, I would advise to simply copy it to reduce coupling between this package and QeApp.
aiidalab/utils.py
Outdated
# however, in the latest aiida-core branch, this is now the default. | ||
# Define command based on aiida-core version | ||
aiida_version = version("aiida-core") | ||
if aiida_version <= "2.5.1": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you need this distinction? I believe the --reset flag is still present in upcoming version no? Or am I wrong?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure about this. This at least covers all our docker images and the latest stable AiiDA. We may update it when the new AiiDA version is available. I can open an issue for reminder.
Do you have any better suggestions to handle it?
Hi @danielhollas , thanks for your review.
No. I install QEApp from AiiDAlab App Store. And
This is a problem not only for QEApp, |
Hi @danielhollas , I added Though, I believe it's still good to fix this issue. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @superstar54. I am just really surprised that we did not run into this earlier.
Good call on integrating this to QeApp for now, because we don't need to rush releasiing this package (and we would need to release new image with it as well!).
aiidalab/utils.py
Outdated
return subprocess.Popen( | ||
["verdi", "daemon", "restart"], | ||
command, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just double checked that on current aiida-core main the --reset
flag still exist (albeit is deprecated) so let's keep this simple
command, | |
["verdi", "daemon", "restart", "--reset"], |
We can remove it once we switch to v2.6. Note in general we don't need to worry about supporting multiple aiida-core versions, since this package is baked into the image and cannot be changed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK to me! I removed the version check and simply added the --reset
tag.
Fix #423 .
After installing or updating a plugin package, one needs to restart the daemon with the
--reset
flag for changes to take effect.Besides, in the latest aiida-core branch, this is now the default. aiidateam/aiida-core#6317, thus we need to define the restart command based on aiida-core version