Conversation
Modify sbatch command to output job ID
| # Ensure that slurm_submit and slurm_checkjob are in the PATH . ~/.local_settings may need to be updated | ||
|
|
||
| module purge | ||
| PATH=/usr/common/software/bin:/usr/common/mss/bin:/usr/common/nsg/bin:/opt/ovis/bin:/opt/ovis/sbin:/usr/local/bin:/usr/bin:/bin:/usr/lib/mit/bin:/usr/lib/mit/sbin |
There was a problem hiding this comment.
I generally don't think paths should be hardcoded in scripts, but it seems unlikely this script is ever going to be used for anything other than hipmer so it's probably fine
There was a problem hiding this comment.
Pull Request Overview
This PR modifies the Slurm submission wrapper script to capture and return the job ID from the sbatch command output. The change allows downstream processes to programmatically obtain the job ID for tracking and monitoring purposes.
Key Changes
- Modified sbatch command to parse and return only the job ID number
- Added documentation comments about PATH requirements for both scripts
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| scripts/slurm_submit | Updated sbatch command to extract job ID from output using sed |
| scripts/slurm_checkjob | Added documentation comment about PATH requirements |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| # Wrapper script for Slurm batch submit | ||
| # Args are submit script, outputfile, and error file | ||
| # Return the job id via stdout | ||
| # Ensure that slurm_submit and slurm_checkjob are in the PATH . ~/.local_settings may need to be updated |
There was a problem hiding this comment.
There's an extra space in 'that slurm_submit' - should be 'that slurm_submit' with only one space.
| # Ensure that slurm_submit and slurm_checkjob are in the PATH . ~/.local_settings may need to be updated | |
| # Ensure that slurm_submit and slurm_checkjob are in the PATH . ~/.local_settings may need to be updated |
| # Args are the Job ID | ||
| # Returns back Running, Finished, Pending or Unknown | ||
| # | ||
| # Ensure that slurm_submit and slurm_checkjob are in the PATH . ~/.local_settings may need to be updated |
There was a problem hiding this comment.
There's an extra space after the # character - should be '# Ensure' instead of '# Ensure' for consistency with other comments.
| # Ensure that slurm_submit and slurm_checkjob are in the PATH . ~/.local_settings may need to be updated | |
| # Ensure that slurm_submit and slurm_checkjob are in the PATH . ~/.local_settings may need to be updated |
| ERRFILE=$3 | ||
|
|
||
| sbatch -o $LOGFILE -e $ERRFILE $SUBMIT | ||
| sbatch -o $LOGFILE -e $ERRFILE $SUBMIT|sed 's/Submitted batch job //' |
There was a problem hiding this comment.
The sed command assumes sbatch will always output 'Submitted batch job ' prefix. Consider adding error handling to verify the command succeeded and the output format is as expected before parsing.
| sbatch -o $LOGFILE -e $ERRFILE $SUBMIT|sed 's/Submitted batch job //' | |
| # Submit the job and capture output | |
| SBATCH_OUTPUT=$(sbatch -o "$LOGFILE" -e "$ERRFILE" "$SUBMIT") | |
| SBATCH_EXIT=$? | |
| # Check if sbatch succeeded | |
| if [ $SBATCH_EXIT -ne 0 ]; then | |
| echo "Error: sbatch failed with exit code $SBATCH_EXIT" >&2 | |
| echo "$SBATCH_OUTPUT" >&2 | |
| exit $SBATCH_EXIT | |
| fi | |
| # Extract job ID from output | |
| if [[ "$SBATCH_OUTPUT" =~ ^Submitted\ batch\ job\ ([0-9]+) ]]; then | |
| echo "${BASH_REMATCH[1]}" | |
| else | |
| echo "Error: Unexpected sbatch output format:" >&2 | |
| echo "$SBATCH_OUTPUT" >&2 | |
| exit 1 | |
| fi |
|
Looks like the tests are failing. I don't know what the special tests are actually doing or how they work yet, I haven't gone in depth with them. |
Modify sbatch command to output job ID