feat: pass a bash script to srun rather than the command directly #37
Conversation
…vent SLURM command-line character limit
📝 WalkthroughWalkthroughRefactors job script invocation for SMP jobs to stream bash scripts via stdin instead of embedding them in command lines. Creates a local variable to hold the srun bash script, appends "bash -s" to the srun call, and passes the script through subprocess stdin in text mode. Changes
Sequence Diagram(s)sequenceDiagram
participant Code
participant Subprocess
participant Bash as Bash Process
rect rgba(100, 150, 200, 0.1)
note over Code: New Flow - Stdin-based Script Streaming
Code->>Code: Format job script into srun_script
Code->>Code: Append "bash -s" to srun call
Code->>Subprocess: Create subprocess with stdin=PIPE, text=True
Subprocess->>Bash: Launch bash -s
Code->>Bash: Write srun_script to stdin
Code->>Bash: Close stdin
Bash->>Bash: Execute script from stdin
Bash-->>Subprocess: Return exit code
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Possibly related issues
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
snakemake_executor_plugin_slurm_jobstep/__init__.py (2)
129-130: Consider limiting script logging for very large scripts.While logging the script content is valuable for debugging, very large scripts could bloat logs. Consider logging the script length or first few lines instead.
Apply this diff to log script metadata instead:
- srun_script_str = '\n' + srun_script if srun_script else 'N/A' - self.logger.debug(f"The script for this job is: {srun_script_str}") + if srun_script: + script_lines = srun_script.count('\n') + 1 + script_preview = srun_script[:200] + ('...' if len(srun_script) > 200 else '') + self.logger.debug(f"The script for this job ({script_lines} lines, {len(srun_script)} chars): {script_preview}") + else: + self.logger.debug("The script for this job is: N/A")
133-133: Static analysis warning is a false positive, but consider conditional stdin.The static analysis warning about
shell=True(S602) is a false positive here. This change actually improves security by moving the job script (user-controlled content) from the shell command line to stdin. Thecallvariable is constructed entirely from controlled sources.However,
stdin=subprocess.PIPEis set for all jobs but only used for SMP jobs. Consider setting it conditionally for minor efficiency improvement.Apply this diff to conditionally set stdin:
- jobsteps[job] = subprocess.Popen(call, shell=True, text=True, stdin=subprocess.PIPE) + stdin_arg = subprocess.PIPE if srun_script is not None else None + jobsteps[job] = subprocess.Popen( + call, shell=True, text=True, stdin=stdin_arg + )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
snakemake_executor_plugin_slurm_jobstep/__init__.py(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
⚙️ CodeRabbit configuration file
**/*.py: Do not try to improve formatting.
Do not suggest type annotations for functions that are defined inside of functions or methods.
Do not suggest type annotation of theselfargument of methods.
Do not suggest type annotation of theclsargument of classmethods.
Do not suggest return type annotation if a function or method does not contain areturnstatement.
Files:
snakemake_executor_plugin_slurm_jobstep/__init__.py
🪛 Ruff (0.14.6)
snakemake_executor_plugin_slurm_jobstep/__init__.py
133-133: subprocess call with shell=True identified, security issue
(S602)
🔇 Additional comments (2)
snakemake_executor_plugin_slurm_jobstep/__init__.py (2)
61-61: LGTM!The introduction of
srun_scriptto hold the bash script is clear and follows the PR objective of passing scripts via stdin.
122-124: Clarification needed: SLURM stdin forwarding limitation may affect this approach.The web search revealed that "Slurm job scripts do not get stdin forwarded — if your rule expects stdin (bash reading from stdin), write input to a file (or use here-doc / bash -c "…") inside the job script". This directly challenges the assumption that
bash -swill read the script from stdin in a SLURM context.Before approving this change, confirm:
- How is
srun_scriptactually passed to bash via stdin if SLURM doesn't forward stdin by default?- Is there explicit configuration or a workaround being used that enables stdin forwarding?
- Verify this matches the approach in the referenced upstream PR #380
Without clarification on how stdin is actually provided to bash within the SLURM job execution context, the logic of storing the script in
srun_scriptand usingbash -smay not work as intended.
| if srun_script is not None: | ||
| # pass the srun bash script via stdin | ||
| jobsteps[job].stdin.write(srun_script) | ||
| jobsteps[job].stdin.close() |
There was a problem hiding this comment.
Add error handling for stdin write operation.
Writing to stdin can raise BrokenPipeError if the subprocess terminates prematurely. Without error handling, this would cause an unhandled exception with unclear error messaging. Wrap the write operation in try-except for better error reporting and robustness.
Apply this diff to add error handling:
if srun_script is not None:
- # pass the srun bash script via stdin
- jobsteps[job].stdin.write(srun_script)
- jobsteps[job].stdin.close()
+ try:
+ # pass the srun bash script via stdin
+ jobsteps[job].stdin.write(srun_script)
+ jobsteps[job].stdin.close()
+ except BrokenPipeError:
+ # subprocess terminated before reading stdin
+ self.logger.error(
+ f"Failed to write script to stdin for job {job}. "
+ "Subprocess may have terminated prematurely."
+ )
+ self.report_job_error(SubmittedJobInfo(job))
+ raise WorkflowError(
+ f"Job {job} failed: subprocess terminated before reading script"
+ )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if srun_script is not None: | |
| # pass the srun bash script via stdin | |
| jobsteps[job].stdin.write(srun_script) | |
| jobsteps[job].stdin.close() | |
| if srun_script is not None: | |
| try: | |
| # pass the srun bash script via stdin | |
| jobsteps[job].stdin.write(srun_script) | |
| jobsteps[job].stdin.close() | |
| except BrokenPipeError: | |
| # subprocess terminated before reading stdin | |
| self.logger.error( | |
| f"Failed to write script to stdin for job {job}. " | |
| "Subprocess may have terminated prematurely." | |
| ) | |
| self.report_job_error(SubmittedJobInfo(job)) | |
| raise WorkflowError( | |
| f"Job {job} failed: subprocess terminated before reading script" | |
| ) |
🤖 Prompt for AI Agents
In snakemake_executor_plugin_slurm_jobstep/__init__.py around lines 134-137, the
direct write to jobsteps[job].stdin can raise BrokenPipeError if the subprocess
exits; wrap the write and close in a try/except/finally block that catches
BrokenPipeError (and optionally OSError), logs a clear error message including
the exception details using the module's existing logger (e.g., logger or
process_logger), and ensures stdin is closed in finally to avoid resource leaks;
do not swallow unexpected exceptions unless re-raised after logging.
dmwnz
changed the title
1 participant
Related to snakemake/snakemake-executor-plugin-slurm#379 and matching PR snakemake/snakemake-executor-plugin-slurm#380
Summary by CodeRabbit
Bug Fixes
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.