-
Notifications
You must be signed in to change notification settings - Fork 29
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
fix SLURM env setup #193
fix SLURM env setup #193
Conversation
fix SLURM environment setup such that --export=NONE is the default (as it was before with SBATCH_EXPORT=NONE), job_environment adds correctly to --export, and copy_environment correctly prefixes with ALL. comment also left in the code as to why we default to NONE.
By fixing it so we default to export NONE, how will that impact sites that already observed this problem and "fixed it". Will their "fixes" still work, or will they negatively affected by the switch again? |
spec/job/adapters/slurm_spec.rb
Outdated
@@ -41,6 +41,10 @@ def build_script(opts = {}) | |||
) | |||
end | |||
|
|||
def base_args(arr) | |||
arr + ["--export", "NONE"] | |||
end |
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.
Is there a better name than base_args? Just by going by the name of the method, it isn't clear why every test wraps the args to test against with base_args except for the test context "with :job_environment" do
where it is omitted and args: ["--export", "key"]
is used. It looks like the fact that it isn't used in the latter test is a bug but of course adding it would break the test.
I don't know the answer besides a larger refactor that would change/simplify how we test these things.
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.
It would be better to get rid of base_args and inline with the rest of the tests. Since we arbitrarily add base args to the tests, it isn't the right abstraction. Better to have duplication than the wrong abstraction. A better architecture for the adapter code in the future will address the problems with the tests.
So
context "with :queue_name" do
before { adapter.submit(build_script(queue_name: "queue")) }
- it { expect(slurm).to have_received(:submit_string).with(content, args: ["-p", "queue"], env: {}) }
+ it { expect(slurm).to have_received(:submit_string).with(content, args: ["-p", "queue", "--export", "NONE"], env: {}) }
end
I can test it to be doubly sure, but the 'fix' was to source files under Which is to say, sites with the 'fix' wouldn't be impacted other than having a little bit of tech debt to clean up. But I'll confirm. |
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.
Besides getting rid of the base_args in the test, I'm okay with this merging when you are satisfied that the sites that have addressed the temporary behavior change will not be negatively affected.
I've been double checking this morning and I think we're OK with the 2nd sourcing of files. The site I was working on even had it in an if block (if |
fix SLURM environment setup such that --export=NONE is the default
(as it was before with SBATCH_EXPORT=NONE), job_environment adds
correctly to --export, and copy_environment correctly prefixes with ALL.
comment also left in the code as to why we default to NONE.
Fixes the issues raised in and created by #109 and #158.