-
Notifications
You must be signed in to change notification settings - Fork 117
[bugfix] Combine access and job options for slurm '--constraint' #1363
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
[bugfix] Combine access and job options for slurm '--constraint' #1363
Conversation
* The `--constraint` options passed via `access` and `job.options` are combined in the resulting job script. * Add unittests to test the expected behavior.
Codecov Report
@@ Coverage Diff @@
## master #1363 +/- ##
==========================================
+ Coverage 91.66% 91.69% +0.02%
==========================================
Files 83 83
Lines 12673 12712 +39
==========================================
+ Hits 11617 11656 +39
Misses 1056 1056
Continue to review full report at Codecov.
|
ekouts
left a comment
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.
@teojgo I think you should add some comment in the documentation (maybe here) that explains that the constraint job option will be combined and not just added after the "access" options.
Currently, when you pass -J c=c1, -J -c=c1, -J constraint=c1, -J --constraint=c1 it is combined, but when you pass -J "#..." this will be passed verbatim in the script and slurm will only consider the last job option.
|
@ekouts ok I will add a note text block in the documentation |
|
@vkarak in which section do you think I should add the a documentation note about this? |
vkarak
left a comment
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 will fix the remaining issue.
The
--constraintoptions passed viaaccessandjob.optionsare combined in the resulting job script.
Add unittests to test the expected behavior.
Fixes #1348