feat: pass a bash script to srun rather than the command directly #37
Conversation
|
Warning Rate limit exceeded@dmwnz has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 14 minutes and 58 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds a new public dataclass Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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
dmwnz
force-pushed
the
feat/srun_script
branch
from
2f9d19b to
96653b2
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
snakemake_executor_plugin_slurm_jobstep/__init__.py (1)
163-166: Add error handling for stdin write operation.Writing to stdin can raise
BrokenPipeErrorif the subprocess terminates prematurely. Without error handling, this causes an unhandled exception with unclear error messaging.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 as e: + # subprocess terminated before reading stdin + self.logger.error( + f"Failed to write script to stdin for job {job}. " + f"Subprocess may have terminated prematurely. Error: {e}" + ) + self.report_job_error(SubmittedJobInfo(job)) + raise WorkflowError( + f"Job {job} failed: subprocess terminated before reading script" + )Based on past review comments, this error handling is essential for robust execution.
🧹 Nitpick comments (1)
snakemake_executor_plugin_slurm_jobstep/__init__.py (1)
160-162: Consider conditionally setting stdin=PIPE.Currently,
stdin=PIPEis set unconditionally even whensrun_scriptis None. While this doesn't cause incorrect behavior, it's slightly inefficient.Apply this diff to optimize:
jobsteps[job] = subprocess.Popen( - call, shell=True, text=True, stdin=subprocess.PIPE + call, + shell=True, + text=True, + stdin=subprocess.PIPE if srun_script is not None else None )Note: The static analysis warning about
shell=Trueappears to be a false positive, as the call string is constructed from controlled sources rather than direct user input.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
snakemake_executor_plugin_slurm_jobstep/__init__.py(4 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.7)
snakemake_executor_plugin_slurm_jobstep/__init__.py
160-160: subprocess call with shell=True identified, security issue
(S602)
🔇 Additional comments (4)
snakemake_executor_plugin_slurm_jobstep/__init__.py (4)
10-10: LGTM!The imports are correctly structured to support the new
ExecutorSettingsdataclass.Also applies to: 16-20
46-63: LGTM!The
ExecutorSettingsdataclass is well-structured with clear documentation and appropriate defaults that preserve backward compatibility.
85-85: LGTM!Proper initialization of
srun_scriptto enable conditional logic.
156-157: LGTM!The conditional debug logging for the script content is properly implemented.
| if self.workflow.executor_settings.pass_command_as_script: | ||
| # format the job to execute with all the snakemake parameters | ||
| # into a script | ||
| srun_script = self.format_job_exec(job) | ||
| # the process will read the srun script from stdin | ||
| call += " sh -s" |
There was a problem hiding this comment.
Critical: Missing else branch breaks default execution path.
When pass_command_as_script is False (the default), the call variable is left incomplete without appending the actual job command. This means srun will execute with no job specified, causing execution to fail.
Apply this diff to add the missing else branch:
if self.workflow.executor_settings.pass_command_as_script:
# format the job to execute with all the snakemake parameters
# into a script
srun_script = self.format_job_exec(job)
# the process will read the srun script from stdin
call += " sh -s"
+ else:
+ # append the formatted job command directly to the call
+ call += self.format_job_exec(job)📝 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 self.workflow.executor_settings.pass_command_as_script: | |
| # format the job to execute with all the snakemake parameters | |
| # into a script | |
| srun_script = self.format_job_exec(job) | |
| # the process will read the srun script from stdin | |
| call += " sh -s" | |
| if self.workflow.executor_settings.pass_command_as_script: | |
| # format the job to execute with all the snakemake parameters | |
| # into a script | |
| srun_script = self.format_job_exec(job) | |
| # the process will read the srun script from stdin | |
| call += " sh -s" | |
| else: | |
| # append the formatted job command directly to the call | |
| call += self.format_job_exec(job) |
🤖 Prompt for AI Agents
In snakemake_executor_plugin_slurm_jobstep/__init__.py around lines 146 to 151,
the if block handles pass_command_as_script but lacks an else branch so when
pass_command_as_script is False the srun call is left without the actual job
command; add an else branch that appends the formatted job command (the same
output as format_job_exec(job)) to the call string so srun receives the job to
run (ensure spacing/quoting matches existing call construction and that you do
not duplicate the script/stdin handling).
96653b2 to
960a7f9
Compare
…ly to circumvent SLURM command-line character limit
dmwnz
force-pushed
the
feat/srun_script
branch
from
960a7f9 to
1e9ffff
Compare
1 participant
Related to snakemake/snakemake-executor-plugin-slurm#379 and matching PR snakemake/snakemake-executor-plugin-slurm#380
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.