Skip to content

fix: correct kwargs key names in SlurmJob fluent setters#11

Open
prabindersinghh wants to merge 1 commit intoouterbounds:mainfrom
prabindersinghh:fix/slurm-job-kwargs-key-mismatch
Open

fix: correct kwargs key names in SlurmJob fluent setters#11
prabindersinghh wants to merge 1 commit intoouterbounds:mainfrom
prabindersinghh:fix/slurm-job-kwargs-key-mismatch

Conversation

@prabindersinghh
Copy link
Copy Markdown

@prabindersinghh prabindersinghh commented Mar 12, 2026

Summary

Fixes #10

The cpus_per_task(), memory(), and run_time_limit() fluent setter
methods wrote incorrect keys into self.kwargs, but
create_slurm_script() reads different keys. The mismatch meant all
three values were silently dropped — every job ran with default CPUs,
memory, and time limit regardless of what the user specified.

Setter method Key written (before) Key read by create_slurm_script()
cpus_per_task() "cpus-per-task" (hyphen) "cpus_per_task" (underscore)
memory() "mem" "memory"
run_time_limit() "time" "run_time_limit"

Changes

  • metaflow_extensions/slurm_ext/plugins/slurm/slurm_job.py — 3 key renames
  • test/unit/test_slurm_job.py — new file, 8 unit tests

Testing

8 new unit tests in test/unit/test_slurm_job.py:

Test What it verifies
test_cpus_per_task_setter_writes_correct_key setter writes correct key
test_memory_setter_writes_correct_key setter writes correct key
test_run_time_limit_setter_writes_correct_key setter writes correct key
test_create_slurm_script_includes_cpus_per_task_directive script contains correct #SBATCH directive
test_create_slurm_script_includes_mem_directive script contains correct #SBATCH directive
test_create_slurm_script_includes_time_directive script contains correct #SBATCH directive
test_create_slurm_script_chained_fluent_api all three directives present when chained
test_kwargs_constructor_path_still_works existing constructor path unaffected

All 8 pass. Zero regressions.

The cpus_per_task(), memory(), and run_time_limit() setter methods
wrote wrong keys into self.kwargs:

  cpus_per_task() wrote 'cpus-per-task' (hyphen)
  memory()        wrote 'mem'
  run_time_limit() wrote 'time'

But create_slurm_script() reads 'cpus_per_task', 'memory', and
'run_time_limit' (underscores). The mismatch meant all three values
were silently dropped — every job ran with default CPUs, memory,
and time limit regardless of what the user specified.

Fix: rename the three keys to match what create_slurm_script() reads.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug: SlurmJob fluent setters write wrong kwargs keys — cpus/memory/time silently dropped

1 participant