Add --extra_slurm_options to srun_fastsurfer.sh for sbatch scheduling options - #853
Open
eastagiletracker wants to merge 1 commit into
Open
Conversation
… 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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR proposes
--extra_slurm_options,--extra_slurm_options_segand--extra_slurm_options_surfforsrun_fastsurfer.sh, so scheduling options such as--reservationactually reach thesbatchcalls 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.shforwards 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. Ondevatef190b9, asking for a reserved node produces jobs that are scheduled without the reservation and then abort inside the container:The
sbatchline carries no reservation, while--reservation gpu_nodesends up on thebrun_fastsurfer.shcommand line, whererun_fastsurfer.shstops withERROR: 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--reservationflag, this adds three options tosrun_fastsurfer.sh:--extra_slurm_optionsfor both jobs, plus--extra_slurm_options_segand--extra_slurm_options_surffor 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, becausesbatchstops 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, whichdoc/scripts/SLURM.mdrenders). The cleanup and copy helper jobs are deliberately left alone, matching how--partitionbehaves today.How it was verified
The PR adds
test/scripts/test_srun_fastsurfer.py, which runssrun_fastsurfer.sh --dryin a temporary directory and inspects thesbatchcommands 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 againstdevatef190b9and pass with this change; the seventh asserts that the generatedsbatchcommands 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/imagereports the same 387 passing before and after (394 with the new suite), andruff check .is clean.The new suite is not wired into
.github/workflows/unittest.yaml; adding"scripts"to thetestsmatrix there runs it alongsidetest/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.
If you'd rather not receive contributions like this, reply
no-more-prson 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