feat: pass a bash script to srun rather than the command directly #37
Conversation
📝 WalkthroughWalkthroughAdds a 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)
✨ 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.
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.
dmwnz
force-pushed
the
feat/srun_script
branch
from
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
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
snakemake_executor_plugin_slurm_jobstep/__init__.py (2)
162-164: Consider conditionally setting stdin parameter.When
pass_command_as_script=False, the subprocess is created withstdin=subprocess.PIPEbut stdin is never used. While harmless, you could optimize by conditionally setting stdin only when needed.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 + )
165-179: Error handling for stdin write implemented correctly.The try-except block properly catches
BrokenPipeErrorand handles the failure appropriately. However, consider these improvements from static analysis:Apply this diff to improve error handling:
except BrokenPipeError: # subprocess terminated before reading stdin - self.logger.error( + self.logger.exception( 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" - ) + raise WorkflowError( + f"Job {job} failed: subprocess terminated before reading script" + ) from NoneChanges:
logging.exceptionautomatically includes the tracebackfrom Noneexplicitly suppresses the original exception context since we're re-raising with a clearer message
📜 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.8)
snakemake_executor_plugin_slurm_jobstep/__init__.py
162-162: subprocess call with shell=True identified, security issue
(S602)
172-175: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
177-179: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
177-179: Avoid specifying long messages outside the exception class
(TRY003)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: testing
🔇 Additional comments (3)
snakemake_executor_plugin_slurm_jobstep/__init__.py (3)
10-10: LGTM! Imports are correct.The new imports properly support the
ExecutorSettingsdataclass and are consistent with the existing import style.Also applies to: 16-20
46-62: Well-defined executor settings.The
ExecutorSettingsdataclass is properly structured with clear metadata and a sensible default that maintains backward compatibility.
85-85: Bothpass_command_as_scriptexecution paths are complete.When
True:srun_scriptis set and" sh -s"is appended to read from stdin.
WhenFalse: The formatted command is appended directly to the call.
…TestWorkflowsBase
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/tests.py (1)
14-16: Returning a concreteExecutorSettingsinstance is appropriateUsing
ExecutorSettings()here correctly instantiates the plugin-specific settings while still satisfying theOptional[ExecutorSettingsBase]return type. As a tiny nit, consider fixing the typo in the comment (“instatiate” → “instantiate”) when you next touch this file.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/tests.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:
tests/tests.py
🧬 Code graph analysis (1)
tests/tests.py (1)
snakemake_executor_plugin_slurm_jobstep/__init__.py (1)
ExecutorSettings(47-62)
🔇 Additional comments (1)
tests/tests.py (1)
5-5: ImportingExecutorSettingsfrom the plugin looks correctThe import matches the plugin’s public API and keeps the tests aligned with the new settings class.
|
@cmeesters Thank you for merging snakemake/snakemake-executor-plugin-slurm#380 |
|
@dmwnz absolutely! The thing is, which PRs like these, I need to publish in sync. This means, I need to publish the jobstep plugin first, update the slurm plugin's dependencies and publish that plugin. Need to pay attention, that both are passing bioconda updates at about the same time, too. Here, though, the update requires updating the test-suite too. And the documentation. This and my other workload, caused me to write, that I will need some time. edit: I just did not get further, because it was the weekend and I fell sick. |
🤖 I have created a release *beep* *boop* --- ## [0.4.0](v0.3.0...v0.4.0) (2025-12-15) ### Features * pass a bash script to srun rather than the command directly ([#37](#37)) ([46b39e7](46b39e7)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
2 participants
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
Tests
✏️ Tip: You can customize this high-level summary in your review settings.