Modify the batch and slurm scripts to support additonal per-subject parameters

[dkuegler]

A current limitation of both srun_fastsurfer and brun_fastsurfer is that it is not possible to define additional subject-specific parameters to run_fastsurfer.sh.
This PR adds the feature to add such options to both srun_fastsurfer and brun_fastsurfer.

In particular, brun_fastsurfer.sh can have additional parameters (format =,). This also passed through to srun_fastsurfer, for example:
Example 1:
--subject_list_delim "," --subject_list_awk_code_args '\$2 ",--vox_size," \$4'
to implement from the subject_list line
subject-101,raw/T1w-101A.nii.gz,study-1,0.9
to
--sid subject-101 --t1 <data-path>/raw/T1w-101A.nii.gz --vox_size 0.9
Example 2:
--subject_list_delim "," --subject_list_awk_code '\$2' --subject_list_awk_code_args '\$4,--asegdkt_segfile,\$3'
to implement from the subject_list line
4,sub_A1,mri/asegdkt.mgz,412463-t1.nii.gz
to
--sid sub_A1 --t1 <data-path>/412463-t1.nii.gz --asegdkt_segfile mri/asegdkt.mgz

[dkuegler]

The PR is tested and can be reviewed now.

[ClePol]

Thanks for the PR, it's working well out of the box now.
I ran into two issues while testing:

1. I am not sure what is supposed to happen when a job crashes. Right now it seems like if the FastSurferCNN module crashes (e.g. QC doesnt pass) the slurm jobs become zombies, because the dependency is never met. Is this expected behaviour?
   It would be great if the cleanup job is executed in any case, so we don't have to hunt down the jobs and files on the slurm cluster and cancel them manually.
2. The HPC work directory can not be modified while the processing is running because any files placed there would be deleted. We could instead check and remove the three folder created ("cases", "script", "logs", "images"). I think this only needs modifications in the check_hpcwork and make_cleanup_job functions.

[ClePol]

I believe I found a bug - the surface pipeline can not run, because --parralel_subjects is passed to brun_fastsrufer.sh without argument.
Putting --parallel_subjects -1 for maximum parallelism

FastSurfer/srun_fastsurfer.sh

Line 669 in 0d4dbb5

--parallel_subjects

allows the surface pipeline to start

[dkuegler]

--parallel_subjects is supposed to be a valid setting and equal to max.
I'll look into it.

With regards to your earlier comments.

1. Initially, if a job crashed, all jobs in following jobs were supposed to fail via slurm. (The dependency is never fulfilled). But this creates unwanted side-effects for the other cases that are merged to be processed in the same batch. So I implemented a monitoring file for successful completion of different stages, the subject_success file. So from then on, the expected behavior was that failed cases are skipped for failed prior steps. But I have not checked this recently, just when I implemented this.
2. Users are not supposed to modify/edit the work directory, so this is not really a concern. All data should be copied back in the cleanup script to the folder specified via --sd.
   Also, you do not need to specify --work if $HPCWORK is set, as would be set on most clusters.

[dkuegler]

There seems to be a not missing

FastSurfer/brun_fastsurfer.sh

Line 148 in 0d4dbb5

then

.

[dkuegler]

I am a bit surprised about the zombification of jobs though. brun_fastsurfer is supposed to always return 0 (success)

FastSurfer/brun_fastsurfer.sh

Line 466 in 0d4dbb5

# always exit successful

I'll have to read up on the slurm docs, whether a non-zero return code for a child process supercedes a zero return code of the main process.

[ClePol]

1. Initially, if a job crashed, all jobs in following jobs were supposed to fail via slurm. (The dependency is never fulfilled). But this creates unwanted side-effects for the other cases that are merged to be processed in the same batch. So I implemented a monitoring file for successful completion of different stages, the subject_success file. So from then on, the expected behavior was that failed cases are skipped for failed prior steps. But I have not checked this recently, just when I implemented this.

I think this is broken atm, I can keep an eye on this when I run some batches with failing subjects and let you know what happens currently. IIRC the jobs stayed IDLE indefinetly with "dependency not met" (zombie processes), but I didn't look into it.

2. Users are not supposed to modify/edit the work directory, so this is not really a concern. All data should be copied back in the cleanup script to the folder specified via --sd.
   Also, you do not need to specify --work if $HPCWORK is set, as would be set on most clusters.

The issue is not that files used by the script cant be modified, but that the script esentially blocks the HCP work directory, so no other scripts can use it. (e.g. what if I want to also run freesurfer on slurm at the same time as fastsrufer). Why not only check and delete the folders that are actually used?
A workaround would be to set --work manually to a subfolder of $HPCWORK (which I didn't do yet, but might need to do in the future).

[dkuegler]

For proper scheduling between Jobarrays, there needs to be a dependency between in corresponding seg and surf tasks.

[ClePol]

In case this pops up during testing, just FYI - I ran into an issues on line

FastSurfer/brun_fastsurfer.sh

Line 438 in 0d4dbb5

$run_fastsurfer "$seg_surf_only" "${args[@]}" "${POSITIONAL_FASTSURFER[@]}" | prepend "$subject_id: " &

where there seemed to be some issue with the formatting of the srun commend. I did a quick and dirty workaround by building the command into a string and calling eval on it.

[dkuegler]

In case this pops up during testing, just FYI - I ran into an issues on line

FastSurfer/brun_fastsurfer.sh

Line 438 in 0d4dbb5

$run_fastsurfer "$seg_surf_only" "${args[@]}" "${POSITIONAL_FASTSURFER[@]}" | prepend "$subject_id: " &

where there seemed to be some issue with the formatting of the srun commend. I did a quick and dirty workaround by building the command into a string and calling eval on it.

I don't particularly know what you are running into, but #438 identifies an issue in exactly that line, $seg_surg_only is empty. The question is what error you were getting. In #438 it was "unrecognized option" -- the empty string.

[dkuegler]

Currently, the default option for check_hpc_work is none, i.e. there is no check, if the directory is in fact empty, which can lead to unexpected deletion of files by the cleanup job.

Also, the cleanup job should just delete cases, logs, scripts, and images, not *.

[dkuegler]

I can confirm that slurm inherits the return code from job steps, i.e. if a srun call fails, for example a surface or a segmentation command, this interrupts the whole processing pipeline.

This can be avoided by manually setting the derived return code with sjobexitmod https://slurm.schedmd.com/job_exit_code.html.

It would probably be best to have a verbose error message at the end of the script, but always return 0, so proceeding tasks are always executed.