Skip to content

Add --extra_slurm_options to srun_fastsurfer.sh for sbatch scheduling options - #853

Open
eastagiletracker wants to merge 1 commit into
Deep-MI:devfrom
eastagiletracker:agile-board/srun-extra-slurm-options
Open

Add --extra_slurm_options to srun_fastsurfer.sh for sbatch scheduling options#853
eastagiletracker wants to merge 1 commit into
Deep-MI:devfrom
eastagiletracker:agile-board/srun-extra-slurm-options

Conversation

@eastagiletracker

Copy link
Copy Markdown

This PR proposes --extra_slurm_options, --extra_slurm_options_seg and --extra_slurm_options_surf for srun_fastsurfer.sh, so scheduling options such as --reservation actually reach the sbatch calls of the segmentation and surface jobs (fixes #586). We include this PR work along with a full history of your repo at https://eastagiletracker.com/projects/219. You can sign in with your GitHub ID to claim ownership of the project.

What was wrong

srun_fastsurfer.sh forwards every option it does not recognize to FastSurfer itself (POSITIONAL_FASTSURFER), so a scheduling option is not rejected — it is silently moved into the FastSurfer call inside the container. On dev at ef190b9, asking for a reserved node produces jobs that are scheduled without the reservation and then abort inside the container:

$ ./srun_fastsurfer.sh --data data --sd out --work work --pattern '*.nii.gz' \
    --fs_license license.txt --singularity_image fastsurfer.sif \
    --reservation gpu_nodes --dry
sbatch --parsable --mem=7G --cpus-per-task=16 -J FastSurfer-Seg- --time=20 -o WORK/logs/seg_%A_%a.log WORK/scripts/slurm_cmd_seg.sh --gpus-per-task=1
...
  /data/scripts/brun_fastsurfer.sh  --subject_list /data/scripts/subject_list --statusfile /data/scripts/subject_success --sd /data/cases --threads 16 --seg_only --reservation gpu_nodes --fs_license /data/scripts/.fs_license

The sbatch line carries no reservation, while --reservation gpu_nodes ends up on the brun_fastsurfer.sh command line, where run_fastsurfer.sh stops with ERROR: Flag '--reservation' unrecognized. for every case — after the jobs have already been submitted and started.

What this changes

Following the suggestion in #586 to generalize this in the style of --extra_singularity_options, rather than adding a single --reservation flag, this adds three options to srun_fastsurfer.sh: --extra_slurm_options for both jobs, plus --extra_slurm_options_seg and --extra_slurm_options_surf for one of them. Both the segmentation job and the surface job are covered, as requested in the issue. The options are inserted in front of the batch script path, because sbatch stops parsing options at the batch script and passes everything after it to the script instead. Option strings are split at whitespace, so several options can be given at once (documented in --help, which doc/scripts/SLURM.md renders). The cleanup and copy helper jobs are deliberately left alone, matching how --partition behaves today.

$ ./srun_fastsurfer.sh ... --extra_slurm_options_seg "--reservation=gpu_nodes --qos=high"
sbatch --parsable --mem=7G --cpus-per-task=16 -J FastSurfer-Seg- --time=20 -o WORK/logs/seg_%A_%a.log --reservation=gpu_nodes --qos=high WORK/scripts/slurm_cmd_seg.sh --gpus-per-task=1
sbatch --parsable --mem-per-cpu=3G --cpus-per-task=2 --ntasks=1 --nodes=1-1 --hint=nomultithread --depend=afterok:SEG_JOB_ID -J FastSurfer-Surf- -o WORK/logs/surf_%A_%a.log WORK/scripts/slurm_cmd_surf.sh

How it was verified

The PR adds test/scripts/test_srun_fastsurfer.py, which runs srun_fastsurfer.sh --dry in a temporary directory and inspects the sbatch commands it prints — no SLURM, singularity or image data needed, and it skips itself off Linux since the script needs GNU coreutils. Six of the seven tests fail against dev at ef190b9 and pass with this change; the seventh asserts that the generated sbatch commands are byte-for-byte what they are today when none of the new options are given, and passes both before and after, which is the backward-compatibility check. test/image reports the same 387 passing before and after (394 with the new suite), and ruff check . is clean.

$ python -m pytest -q test/image test/scripts
394 passed, 192 warnings in 9.34s

The new suite is not wired into .github/workflows/unittest.yaml; adding "scripts" to the tests matrix there runs it alongside test/image, since the job already takes the directory from the matrix. Glad to include that one-line change here if you would prefer it in this PR.

How this was managed

This work was tracked on a board imported from this repository's own issues and pull requests, as the story Added Support for --reservation Flag for srun_fastsurfer.sh on the FastSurfer board — 844 stories imported, with milestones as epics.

board

If you'd rather not receive contributions like this, reply no-more-prs on this pull request and we won't open any further ones on your repositories.


Lawrence W. Sinclair
CEO / East Agile
linkedin.com/in/lwsinclair/
eastagile.com

… options

Options passed to srun_fastsurfer.sh that it does not recognize are forwarded
to run_fastsurfer.sh, so scheduling options such as --reservation never reach
sbatch and abort the run inside the container instead. --extra_slurm_options,
--extra_slurm_options_seg and --extra_slurm_options_surf pass arbitrary options
to the sbatch call of both or only one of the two pipelines, in the style of
--extra_singularity_options.

The new test/scripts suite checks the sbatch commands that --dry prints.
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.

Added Support for --reservation Flag for srun_fastsurfer.sh

1 participant