-
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
Adding the max_wallclock_seconds setting #743
Adding the max_wallclock_seconds setting #743
Conversation
Missing: documentation on how to do it in a plugin.
…inside a generic plugin.
for more information, see https://pre-commit.ci
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 feature!
LGTM!
@mikibonacci please fix the failed tests. |
@mikibonacci i think you are also trying to upload the export folder |
@mikibonacci is it possible to add a test for this option? |
Sure, I will add a test. |
src/aiidalab_qe/common/widgets.py
Outdated
value=3600 * 12, | ||
step=3600, | ||
min=1800, | ||
max=3600 * 24 * 10, |
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.
This could bring problems depending on the computer , for example piz daint has a limit of 24 hours max.
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.
The logic should somehow consider the restrictions of the computer (it they are present) , or there should be a help text saying the user to set the walltime depending on the restrictions of the computer you have selected
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 added a sentence, and set as max time 24h (10 days was indeed a bit too much).
Adding a sentence which tells the user to carefully set the advance code resources (included the time) `test_qeapp_computational_resources_widget` test function adapted to work with the new wallclock setting. Removed the export folder.
for more information, see https://pre-commit.ci
@AndresOrtegaGuerrero, I am not sure we need a test for this setting? I adapted the |
Maybe a for the builder , just to confirm that the builder is indeed generating the walltime change |
@AndresOrtegaGuerrero my last commit should be the one to put also the check for the builder. |
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.
LGTM!
Addressing #709
I have to fix the layout, but in the meanwhile useful to see if there is some major issue.