Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix SLURM env setup #193

Merged
merged 4 commits into from
May 27, 2020
Merged

fix SLURM env setup #193

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
1b0bdbb
fix SLURM env setup
johrstrom May 26, 2020
2dc5ef7
add copy_environment to yardoc param list
johrstrom May 26, 2020
31a7f8f
refactor --export argument to a function
johrstrom May 26, 2020
1907861
add --export NONE to each test case instead of function call
johrstrom May 27, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 15 additions & 4 deletions lib/ood_core/job/adapters/slurm.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -394,10 +394,21 @@ def submit(script, after: [], afterok: [], afternotok: [], afterany: [])

# Set environment variables
env = script.job_environment || {}
unless (script.job_environment.nil? || script.job_environment.empty?)
prefix = script.copy_environment? ? "ALL," : "NONE," # NONE if false or nil
args += ["--export", prefix + script.job_environment.keys.join(",")]
end
# we default to export NONE, but SLURM defaults to ALL.
# we do this bc SLURM setups a new environment loading /etc/profile (modules)
# where the PUN (the env sbatch uses) did not.
export = if !env.empty? && !script.copy_environment?
johrstrom marked this conversation as resolved.
Show resolved Hide resolved
env.keys.join(",")
elsif !env.empty? && script.copy_environment?
"ALL," + env.keys.join(",")
elsif env.empty? && script.copy_environment?
# only this option changes behaivor dramatically
"ALL"
else
"NONE"
end

args += ["--export", export]

# Set native options
args += script.native if script.native
Expand Down
1 change: 1 addition & 0 deletions lib/ood_core/job/script.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,7 @@ class Script
# @param wall_time [#to_i, nil] max real time
# @param accounting_id [#to_s, nil] accounting id
# @param native [Object, nil] native specifications
# @param copy_environment [Boolean, nil] copy the environment
def initialize(content:, args: nil, submit_as_hold: nil, rerunnable: nil,
job_environment: nil, workdir: nil, email: nil,
email_on_started: nil, email_on_terminated: nil,
Expand Down
66 changes: 38 additions & 28 deletions spec/job/adapters/slurm_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,10 @@ def build_script(opts = {})
)
end

def base_args(arr)
arr + ["--export", "NONE"]
end
Copy link
Contributor

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.

Copy link
Contributor

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


let(:slurm) { double(submit_string: "job.123") }
let(:content) { "my batch script" }

Expand All @@ -54,53 +58,53 @@ def build_script(opts = {})

it "returns job id" do
is_expected.to eq("job.123")
expect(slurm).to have_received(:submit_string).with(content, args: [], env: {})
expect(slurm).to have_received(:submit_string).with(content, args: base_args([]), env: {})
end

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: base_args(["-p", "queue"]), env: {}) }
end

context "with :args" do
before { adapter.submit(build_script(args: ["arg1", "arg2"])) }

it { expect(slurm).to have_received(:submit_string).with(content, args: [], env: {}) }
it { expect(slurm).to have_received(:submit_string).with(content, args: base_args([]), env: {}) }
end

context "with :submit_as_hold" do
context "as true" do
before { adapter.submit(build_script(submit_as_hold: true)) }

it { expect(slurm).to have_received(:submit_string).with(content, args: ["-H"], env: {}) }
it { expect(slurm).to have_received(:submit_string).with(content, args: base_args(["-H"]), env: {}) }
end

context "as false" do
before { adapter.submit(build_script(submit_as_hold: false)) }

it { expect(slurm).to have_received(:submit_string).with(content, args: [], env: {}) }
it { expect(slurm).to have_received(:submit_string).with(content, args: base_args([]), env: {}) }
end
end

context "with :rerunnable" do
context "as true" do
before { adapter.submit(build_script(rerunnable: true)) }

it { expect(slurm).to have_received(:submit_string).with(content, args: ["--requeue"], env: {}) }
it { expect(slurm).to have_received(:submit_string).with(content, args: base_args(["--requeue"]), env: {}) }
end

context "as false" do
before { adapter.submit(build_script(rerunnable: false)) }

it { expect(slurm).to have_received(:submit_string).with(content, args: ["--no-requeue"], env: {}) }
it { expect(slurm).to have_received(:submit_string).with(content, args: base_args(["--no-requeue"]), env: {}) }
end
end

context "with :job_environment" do
before { adapter.submit(build_script(job_environment: {"key" => "value"})) }

it { expect(slurm).to have_received(:submit_string).with(content, args: ["--export", "NONE,key"], env: {"key" => "value"}) }
it { expect(slurm).to have_received(:submit_string).with(content, args: ["--export", "key"], env: {"key" => "value"}) }
end

context "with :job_environment where copy_environment is true" do
Expand All @@ -109,129 +113,135 @@ def build_script(opts = {})
it { expect(slurm).to have_received(:submit_string).with(content, args: ["--export", "ALL,key"], env: {"key" => "value"}) }
end

context "with :copy_environment and no :job_environment" do
before { adapter.submit(build_script(copy_environment: true)) }

it { expect(slurm).to have_received(:submit_string).with(content, args: ["--export", "ALL"], env: {}) }
end

context "with :workdir" do
before { adapter.submit(build_script(workdir: "/path/to/workdir")) }

it { expect(slurm).to have_received(:submit_string).with(content, args: ["-D", "/path/to/workdir"], env: {}) }
it { expect(slurm).to have_received(:submit_string).with(content, args: base_args(["-D", "/path/to/workdir"]), env: {}) }
end

context "with :email" do
before { adapter.submit(build_script(email: ["email1", "email2"])) }

it { expect(slurm).to have_received(:submit_string).with(content, args: ["--mail-user", "email1,email2"], env: {}) }
it { expect(slurm).to have_received(:submit_string).with(content, args: base_args(["--mail-user", "email1,email2"]), env: {}) }
end

context "with :email_on_started" do
context "as true" do
before { adapter.submit(build_script(email_on_started: true)) }

it { expect(slurm).to have_received(:submit_string).with(content, args: ["--mail-type", "BEGIN"], env: {}) }
it { expect(slurm).to have_received(:submit_string).with(content, args: base_args(["--mail-type", "BEGIN"]), env: {}) }
end

context "as false" do
before { adapter.submit(build_script(email_on_started: false)) }

it { expect(slurm).to have_received(:submit_string).with(content, args: [], env: {}) }
it { expect(slurm).to have_received(:submit_string).with(content, args: base_args([]), env: {}) }
end
end

context "with :email_on_terminated" do
context "as true" do
before { adapter.submit(build_script(email_on_terminated: true)) }

it { expect(slurm).to have_received(:submit_string).with(content, args: ["--mail-type", "END"], env: {}) }
it { expect(slurm).to have_received(:submit_string).with(content, args: base_args(["--mail-type", "END"]), env: {}) }
end

context "as false" do
before { adapter.submit(build_script(email_on_terminated: false)) }

it { expect(slurm).to have_received(:submit_string).with(content, args: [], env: {}) }
it { expect(slurm).to have_received(:submit_string).with(content, args: base_args([]), env: {}) }
end
end

context "with :email_on_started and :email_on_terminated" do
before { adapter.submit(build_script(email_on_started: true, email_on_terminated: true)) }

it { expect(slurm).to have_received(:submit_string).with(content, args: ["--mail-type", "ALL"], env: {}) }
it { expect(slurm).to have_received(:submit_string).with(content, args: base_args(["--mail-type", "ALL"]), env: {}) }
end

context "with :job_name" do
before { adapter.submit(build_script(job_name: "my_job")) }

it { expect(slurm).to have_received(:submit_string).with(content, args: ["-J", "my_job"], env: {}) }
it { expect(slurm).to have_received(:submit_string).with(content, args: base_args(["-J", "my_job"]), env: {}) }
end

context "with :shell_path" do
before { adapter.submit(build_script(shell_path: "/path/to/shell")) }

it { expect(slurm).to have_received(:submit_string).with("#!/path/to/shell\n#{content}", args: [], env: {}) }
it { expect(slurm).to have_received(:submit_string).with("#!/path/to/shell\n#{content}", args: base_args([]), env: {}) }
end

context "with :input_path" do
before { adapter.submit(build_script(input_path: "/path/to/input")) }

it { expect(slurm).to have_received(:submit_string).with(content, args: ["-i", Pathname.new("/path/to/input")], env: {}) }
it { expect(slurm).to have_received(:submit_string).with(content, args: base_args(["-i", Pathname.new("/path/to/input")]), env: {}) }
end

context "with :output_path" do
before { adapter.submit(build_script(output_path: "/path/to/output")) }

it { expect(slurm).to have_received(:submit_string).with(content, args: ["-o", Pathname.new("/path/to/output")], env: {}) }
it { expect(slurm).to have_received(:submit_string).with(content, args: base_args(["-o", Pathname.new("/path/to/output")]), env: {}) }
end

context "with :error_path" do
before { adapter.submit(build_script(error_path: "/path/to/error")) }

it { expect(slurm).to have_received(:submit_string).with(content, args: ["-e", Pathname.new("/path/to/error")], env: {}) }
it { expect(slurm).to have_received(:submit_string).with(content, args: base_args(["-e", Pathname.new("/path/to/error")]), env: {}) }
end

context "with :reservation_id" do
before { adapter.submit(build_script(reservation_id: "my_rsv")) }

it { expect(slurm).to have_received(:submit_string).with(content, args: ["--reservation", "my_rsv"], env: {}) }
it { expect(slurm).to have_received(:submit_string).with(content, args: base_args(["--reservation", "my_rsv"]), env: {}) }
end

context "with :priority" do
before { adapter.submit(build_script(priority: 123)) }

it { expect(slurm).to have_received(:submit_string).with(content, args: ["--priority", 123], env: {}) }
it { expect(slurm).to have_received(:submit_string).with(content, args: base_args(["--priority", 123]), env: {}) }
end

context "with :start_time" do
before { adapter.submit(build_script(start_time: Time.new(2016, 11, 8, 13, 53, 54).to_i)) }

it { expect(slurm).to have_received(:submit_string).with(content, args: ["--begin", "2016-11-08T13:53:54"], env: {}) }
it { expect(slurm).to have_received(:submit_string).with(content, args: base_args(["--begin", "2016-11-08T13:53:54"]), env: {}) }
end

context "with :accounting_id" do
before { adapter.submit(build_script(accounting_id: "my_account")) }

it { expect(slurm).to have_received(:submit_string).with(content, args: ["-A", "my_account"], env: {}) }
it { expect(slurm).to have_received(:submit_string).with(content, args: base_args(["-A", "my_account"]), env: {}) }
end

context "with :wall_time" do
before { adapter.submit(build_script(wall_time: 94534)) }

it { expect(slurm).to have_received(:submit_string).with(content, args: ["-t", "26:15:34"], env: {}) }
it { expect(slurm).to have_received(:submit_string).with(content, args: base_args(["-t", "26:15:34"]), env: {}) }
end

context "with :native" do
before { adapter.submit(build_script(native: ["A", "B", "C"])) }

it { expect(slurm).to have_received(:submit_string).with(content, args: ["A", "B", "C"], env: {}) }
it { expect(slurm).to have_received(:submit_string).with(content, args: ["--export", "NONE", "A", "B", "C"], env: {}) }
end

%i(after afterok afternotok afterany).each do |after|
context "and :#{after} is defined as a single job id" do
before { adapter.submit(build_script, after => "job_id") }

it { expect(slurm).to have_received(:submit_string).with(content, args: ["-d", "#{after}:job_id"], env: {}) }
it { expect(slurm).to have_received(:submit_string).with(content, args: base_args(["-d", "#{after}:job_id"]), env: {}) }
end

context "and :#{after} is defined as multiple job ids" do
before { adapter.submit(build_script, after => ["job1", "job2"]) }

it { expect(slurm).to have_received(:submit_string).with(content, args: ["-d", "#{after}:job1:job2"], env: {}) }
it { expect(slurm).to have_received(:submit_string).with(content, args: base_args(["-d", "#{after}:job1:job2"]), env: {}) }
end
end

Expand Down