[feat] Allow ReFrame to pass the access options in command line instead of the script for Slurm #3156
Conversation
|
Hello @ekouts, Thank you for updating! Cheers! There are no PEP8 issues in this Pull Request!Do see the ReFrame Coding Style Guide Comment last updated at 2024-04-19 16:22:53 UTC |
ekouts
changed the title
There was a problem hiding this comment.
I would suggest to (a) extend this to all the schedulers and (b) do not emit the options in the job script.
Also I don't know how would be able to test this fully, but at least we need unit tests that set this option and verify that the sched_access options are not passed in the script if this configuration option is set.
| job.sched_access | ||
| ) | ||
| if parsed_options.constraint: | ||
| constraints.append(parsed_options.constraint.strip()) |
There was a problem hiding this comment.
I think that we should do the same thing with constraints in both cases.
There was a problem hiding this comment.
But in the case the -C c1 is passed in with sbatchand then they are joined in the script -C c1&c2 the option in the script will probably be ignored completely, right?
So are you suggesting that we join them in the submission command as well? Then the behaviour would be that we only add the constraint options from the test in the job, while passing the rest in the script?
There was a problem hiding this comment.
I think additional constraints should be combined with sched_access and passed from the command line in this case, because this is the equivalent of what we are doing now.
There was a problem hiding this comment.
The logic becomes a bit complicated to properly support this, so I suggest postponing this to 4.7 to properly test. Maybe we should change the logic and stop treating constraints specially and pass the responsibility of combining the constraints to the test.
reframe/core/schedulers/slurm.py
Outdated
| ' '.join(job.sched_access) if self._sched_access_in_submit | ||
| else '' | ||
| ) | ||
| cmd = f'sbatch {cmd_opts} {job.script_filename}' |
There was a problem hiding this comment.
I'd use the cmd_parts pattern again.
Co-authored-by: Vasileios Karakasis <vkarak@gmail.com>
Co-authored-by: Vasileios Karakasis <vkarak@gmail.com>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #3156 +/- ##
===========================================
- Coverage 86.66% 86.56% -0.11%
===========================================
Files 61 61
Lines 12134 12167 +33
===========================================
+ Hits 10516 10532 +16
- Misses 1618 1635 +17 ☔ View full report in Codecov by Sentry. |
vkarak
changed the title
Right now I am only changing this for Slurm. I leave these options in the slurm script, mostly to remind whoever might be checking the script that the options are not ignored. Do you think I should remove them from the script?Closes #2970 .