feat: pass a shell script to sbatch rather than the command directly #380
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
|
Warning Rate limit exceeded@dmwnz has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 23 minutes and 57 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)
WalkthroughAdds a public boolean setting Changes
Sequence Diagram(s)sequenceDiagram
participant Executor as Executor Code
participant Popen as subprocess.Popen
participant sbatch as sbatch
Note over Executor: Build sbatch_script (#!/bin/sh + exec_job) and log it
Executor->>Popen: start "sbatch /dev/stdin" with stdin piping (input=sbatch_script)
Popen->>sbatch: stream sbatch_script via stdin
sbatch->>sbatch: parse & execute script
alt submission success
sbatch-->>Popen: stdout (job id)
Popen-->>Executor: return output
else submission failure
sbatch-->>Popen: stderr (error)
Popen-->>Executor: return error including sbatch_script
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
dmwnz
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
snakemake_executor_plugin_slurm/__init__.py (2)
388-388: Consider truncating or gating full sbatch script loggingLogging the entire
sbatch_scriptboth at debug level and inside error messages can produce extremely long log entries for large commands and may expose sensitive arguments if users put credentials or tokens on the CLI.You might want to:
- Log the full script only under a dedicated debug flag or test mode, or
- Truncate the logged script (e.g., first N characters) and indicate truncation.
This keeps diagnostics useful without risking excessively large or sensitive logs.
Also applies to: 410-410
390-398:subprocess.Popen(..., shell=True)keeps existing injection surface; consider tighteningRuff’s S602 warning on this call is valid in general, but this code path already relied on
shell=Truewith the samecallstring before the change, so the security posture is not worsened by this PR. That said, ifcallever incorporates user-controlled fragments beyond what you alreadyshlex.quote, it remains a potential shell-injection vector.If
get_submit_commandand the subsequent mutations ofcalldon’t depend on shell features (expansions,&&, pipes, etc.), you could reduce risk and silence S602 by switching toshell=False, e.g.:- process = subprocess.Popen( - f"{call} /dev/stdin", - shell=True, - text=True, - stdin=subprocess.PIPE, - stdout=subprocess.PIPE, - stderr=subprocess.PIPE, - ) + process = subprocess.Popen( + shlex.split(call) + ["/dev/stdin"], + text=True, + stdin=subprocess.PIPE, + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, + )That change is optional and might be better suited for a dedicated hardening PR once you’ve confirmed
callnever needs a shell.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
snakemake_executor_plugin_slurm/__init__.py(2 hunks)
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 0
File: :0-0
Timestamp: 2025-01-13T09:54:22.950Z
Learning: PR #173 (adding gres resource specification) depends on PR #28 in snakemake-executor-plugin-slurm-jobstep repository, as changes were required in the cpu-settings function of the jobstep-Executor module.
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 0
File: :0-0
Timestamp: 2025-09-09T15:55:55.669Z
Learning: In the Snakemake SLURM executor plugin, Snakemake submits itself as a job using the --wrap parameter in the sbatch command, rather than creating or submitting a separate job.sh script file. The actual implementation uses `call += f' --wrap="{exec_job}"'` where exec_job is the formatted snakemake execution command.
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 187
File: .github/workflows/post_to_mastodon.yml:0-0
Timestamp: 2025-01-17T17:27:32.446Z
Learning: In the mastodon publishing workflow for snakemake-executor-plugin-slurm, the PR_TITLE environment variable is required by the post_to_mastodon.sh script and should be preserved.
Learnt from: johanneskoester
Repo: snakemake/snakemake-executor-plugin-slurm PR: 173
File: docs/further.md:96-96
Timestamp: 2025-03-10T15:20:51.829Z
Learning: PR #173 in snakemake-executor-plugin-slurm implements GPU job support by adding resources: `gres` for generic resource specifications (e.g., 'gpu:1'), `gpu`/`gpus` for specifying GPU counts, and `gpu_model`/`gpu_manufacturer` for specifying GPU types, allowing users to request GPU resources directly rather than having to use slurm_extra.
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 193
File: .github/workflows/post_to_mastodon.yml:26-26
Timestamp: 2025-01-20T09:13:26.443Z
Learning: In the snakemake-executor-plugin-slurm repository, release PRs follow the naming pattern "chore(main): release X.Y.Z" where X.Y.Z is the version number.
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 183
File: .github/workflows/post_to_mastodon.sh:6-17
Timestamp: 2025-01-15T14:23:31.720Z
Learning: The Mastodon instance used by the Snakemake project has a higher character limit than the standard 500 characters, making strict character limit validation unnecessary in the post_to_mastodon.sh script.
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 178
File: snakemake_executor_plugin_slurm/__init__.py:0-0
Timestamp: 2024-12-11T14:17:08.749Z
Learning: In the `snakemake-executor-plugin-slurm` project, when handling exceptions in `snakemake_executor_plugin_slurm/__init__.py`, prefer concise error messages and avoid unnecessary verbosity or exception chaining when it's not necessary.
📚 Learning: 2025-09-09T15:55:55.669Z
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 0
File: :0-0
Timestamp: 2025-09-09T15:55:55.669Z
Learning: In the Snakemake SLURM executor plugin, Snakemake submits itself as a job using the --wrap parameter in the sbatch command, rather than creating or submitting a separate job.sh script file. The actual implementation uses `call += f' --wrap="{exec_job}"'` where exec_job is the formatted snakemake execution command.
Applied to files:
snakemake_executor_plugin_slurm/__init__.py
📚 Learning: 2024-12-11T14:17:08.749Z
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 178
File: snakemake_executor_plugin_slurm/__init__.py:0-0
Timestamp: 2024-12-11T14:17:08.749Z
Learning: In the `snakemake-executor-plugin-slurm` project, when handling exceptions in `snakemake_executor_plugin_slurm/__init__.py`, prefer concise error messages and avoid unnecessary verbosity or exception chaining when it's not necessary.
Applied to files:
snakemake_executor_plugin_slurm/__init__.py
📚 Learning: 2025-03-10T15:20:51.829Z
Learnt from: johanneskoester
Repo: snakemake/snakemake-executor-plugin-slurm PR: 173
File: docs/further.md:96-96
Timestamp: 2025-03-10T15:20:51.829Z
Learning: PR #173 in snakemake-executor-plugin-slurm implements GPU job support by adding resources: `gres` for generic resource specifications (e.g., 'gpu:1'), `gpu`/`gpus` for specifying GPU counts, and `gpu_model`/`gpu_manufacturer` for specifying GPU types, allowing users to request GPU resources directly rather than having to use slurm_extra.
Applied to files:
snakemake_executor_plugin_slurm/__init__.py
📚 Learning: 2025-03-31T11:52:05.373Z
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 249
File: snakemake_executor_plugin_slurm/submit_string.py:21-29
Timestamp: 2025-03-31T11:52:05.373Z
Learning: In the snakemake-executor-plugin-slurm project, account and partition parameters are validated in the submit function before reaching the get_submit_command function. By design, these parameters are either empty strings or already properly formatted with SLURM parameter tags, making additional validation in get_submit_command unnecessary.
Applied to files:
snakemake_executor_plugin_slurm/__init__.py
📚 Learning: 2025-01-20T09:13:26.443Z
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 193
File: .github/workflows/post_to_mastodon.yml:26-26
Timestamp: 2025-01-20T09:13:26.443Z
Learning: In the snakemake-executor-plugin-slurm repository, release PRs follow the naming pattern "chore(main): release X.Y.Z" where X.Y.Z is the version number.
Applied to files:
snakemake_executor_plugin_slurm/__init__.py
📚 Learning: 2025-06-03T17:10:51.292Z
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 221
File: snakemake_executor_plugin_slurm/__init__.py:0-0
Timestamp: 2025-06-03T17:10:51.292Z
Learning: In the snakemake-executor-plugin-slurm repository, the create_efficiency_report method in snakemake_executor_plugin_slurm/__init__.py contains temporary testing/debugging code that logs directory contents using os.listdir() and directory paths. This code was inserted for testing purposes only and should be removed from the production implementation.
Applied to files:
snakemake_executor_plugin_slurm/__init__.py
📚 Learning: 2025-03-10T13:34:59.270Z
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 173
File: tests/tests.py:79-86
Timestamp: 2025-03-10T13:34:59.270Z
Learning: In the Snakemake executor plugin for SLURM, all GPU resources (both simple numeric values and model:number specifications) should be translated to the `--gpus` flag (plural form) to match SLURM's command line interface expectations.
Applied to files:
snakemake_executor_plugin_slurm/__init__.py
🪛 Ruff (0.14.6)
snakemake_executor_plugin_slurm/__init__.py
390-390: subprocess call with shell=True identified, security issue
(S602)
🔇 Additional comments (1)
snakemake_executor_plugin_slurm/__init__.py (1)
382-399: Script-based sbatch submission nicely addresses CLI length limitsMoving the long
exec_jobpayload from--wrapintosbatch_scriptand piping it tosbatchvia stdin cleanly solves the SLURM command-line length problem while keeping the rest of the submission flow intact. This is consistent with SLURM’s own--wrapbehavior (a small shell script wrapper) and aligns with the prior design that used--wrap="{exec_job}". Based on learnings, this looks like the right direction to fix issue #379.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
snakemake_executor_plugin_slurm/__init__.py (2)
384-388: Align comment with actual shell used in the sbatch scriptThe comment says “bash script” but the shebang is
#!/bin/sh. To avoid confusion, either change the comment to “shell script” or update the shebang to match the intended shell.
390-410: Use a single variable for the full sbatch command including/dev/stdinThe new stdin-based submission nicely removes the long
--wrapargument and should address the SLURM CLI length limit (aligning with the linked issue and prior usage of--wrap. Based on learnings, this is the right direction.)For clarity and easier debugging, consider constructing a dedicated command string that includes
/dev/stdinand reusing it consistently:- self.logger.debug(f"sbatch call: {call}") + sbatch_command = f"{call} /dev/stdin" + self.logger.debug(f"sbatch call: {sbatch_command}") @@ - process = subprocess.Popen( - f"{call} /dev/stdin", # tell sbatch to read the sbatch script from stdin + process = subprocess.Popen( + sbatch_command, # tell sbatch to read the sbatch script from stdin @@ - raise subprocess.CalledProcessError( - process.returncode, call, output=err - ) + raise subprocess.CalledProcessError( + process.returncode, sbatch_command, output=err + ) @@ - f" sbatch call:\n {call}\n" + f" sbatch call:\n {sbatch_command}\n"This keeps logs and exceptions in sync with the actual executed command while preserving current behavior.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
snakemake_executor_plugin_slurm/__init__.py(2 hunks)
🧰 Additional context used
🧠 Learnings (12)
📓 Common learnings
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 0
File: :0-0
Timestamp: 2025-09-09T15:55:55.669Z
Learning: In the Snakemake SLURM executor plugin, Snakemake submits itself as a job using the --wrap parameter in the sbatch command, rather than creating or submitting a separate job.sh script file. The actual implementation uses `call += f' --wrap="{exec_job}"'` where exec_job is the formatted snakemake execution command.
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 0
File: :0-0
Timestamp: 2025-01-13T09:54:22.950Z
Learning: PR #173 (adding gres resource specification) depends on PR #28 in snakemake-executor-plugin-slurm-jobstep repository, as changes were required in the cpu-settings function of the jobstep-Executor module.
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 187
File: .github/workflows/post_to_mastodon.yml:0-0
Timestamp: 2025-01-17T17:27:32.446Z
Learning: In the mastodon publishing workflow for snakemake-executor-plugin-slurm, the PR_TITLE environment variable is required by the post_to_mastodon.sh script and should be preserved.
Learnt from: johanneskoester
Repo: snakemake/snakemake-executor-plugin-slurm PR: 173
File: docs/further.md:96-96
Timestamp: 2025-03-10T15:20:51.829Z
Learning: PR #173 in snakemake-executor-plugin-slurm implements GPU job support by adding resources: `gres` for generic resource specifications (e.g., 'gpu:1'), `gpu`/`gpus` for specifying GPU counts, and `gpu_model`/`gpu_manufacturer` for specifying GPU types, allowing users to request GPU resources directly rather than having to use slurm_extra.
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 183
File: .github/workflows/post_to_mastodon.sh:6-17
Timestamp: 2025-01-15T14:23:31.720Z
Learning: The Mastodon instance used by the Snakemake project has a higher character limit than the standard 500 characters, making strict character limit validation unnecessary in the post_to_mastodon.sh script.
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 193
File: .github/workflows/post_to_mastodon.yml:26-26
Timestamp: 2025-01-20T09:13:26.443Z
Learning: In the snakemake-executor-plugin-slurm repository, release PRs follow the naming pattern "chore(main): release X.Y.Z" where X.Y.Z is the version number.
📚 Learning: 2025-09-09T15:55:55.669Z
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 0
File: :0-0
Timestamp: 2025-09-09T15:55:55.669Z
Learning: In the Snakemake SLURM executor plugin, Snakemake submits itself as a job using the --wrap parameter in the sbatch command, rather than creating or submitting a separate job.sh script file. The actual implementation uses `call += f' --wrap="{exec_job}"'` where exec_job is the formatted snakemake execution command.
Applied to files:
snakemake_executor_plugin_slurm/__init__.py
📚 Learning: 2024-12-11T14:17:08.749Z
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 178
File: snakemake_executor_plugin_slurm/__init__.py:0-0
Timestamp: 2024-12-11T14:17:08.749Z
Learning: In the `snakemake-executor-plugin-slurm` project, when handling exceptions in `snakemake_executor_plugin_slurm/__init__.py`, prefer concise error messages and avoid unnecessary verbosity or exception chaining when it's not necessary.
Applied to files:
snakemake_executor_plugin_slurm/__init__.py
📚 Learning: 2025-03-10T15:20:51.829Z
Learnt from: johanneskoester
Repo: snakemake/snakemake-executor-plugin-slurm PR: 173
File: docs/further.md:96-96
Timestamp: 2025-03-10T15:20:51.829Z
Learning: PR #173 in snakemake-executor-plugin-slurm implements GPU job support by adding resources: `gres` for generic resource specifications (e.g., 'gpu:1'), `gpu`/`gpus` for specifying GPU counts, and `gpu_model`/`gpu_manufacturer` for specifying GPU types, allowing users to request GPU resources directly rather than having to use slurm_extra.
Applied to files:
snakemake_executor_plugin_slurm/__init__.py
📚 Learning: 2025-01-20T09:13:26.443Z
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 193
File: .github/workflows/post_to_mastodon.yml:26-26
Timestamp: 2025-01-20T09:13:26.443Z
Learning: In the snakemake-executor-plugin-slurm repository, release PRs follow the naming pattern "chore(main): release X.Y.Z" where X.Y.Z is the version number.
Applied to files:
snakemake_executor_plugin_slurm/__init__.py
📚 Learning: 2025-06-03T17:10:51.292Z
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 221
File: snakemake_executor_plugin_slurm/__init__.py:0-0
Timestamp: 2025-06-03T17:10:51.292Z
Learning: In the snakemake-executor-plugin-slurm repository, the create_efficiency_report method in snakemake_executor_plugin_slurm/__init__.py contains temporary testing/debugging code that logs directory contents using os.listdir() and directory paths. This code was inserted for testing purposes only and should be removed from the production implementation.
Applied to files:
snakemake_executor_plugin_slurm/__init__.py
📚 Learning: 2025-01-13T09:54:22.950Z
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 0
File: :0-0
Timestamp: 2025-01-13T09:54:22.950Z
Learning: PR #173 (adding gres resource specification) depends on PR #28 in snakemake-executor-plugin-slurm-jobstep repository, as changes were required in the cpu-settings function of the jobstep-Executor module.
Applied to files:
snakemake_executor_plugin_slurm/__init__.py
📚 Learning: 2025-03-31T11:52:05.373Z
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 249
File: snakemake_executor_plugin_slurm/submit_string.py:21-29
Timestamp: 2025-03-31T11:52:05.373Z
Learning: In the snakemake-executor-plugin-slurm project, account and partition parameters are validated in the submit function before reaching the get_submit_command function. By design, these parameters are either empty strings or already properly formatted with SLURM parameter tags, making additional validation in get_submit_command unnecessary.
Applied to files:
snakemake_executor_plugin_slurm/__init__.py
📚 Learning: 2025-01-17T17:27:32.446Z
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 187
File: .github/workflows/post_to_mastodon.yml:0-0
Timestamp: 2025-01-17T17:27:32.446Z
Learning: In the mastodon publishing workflow for snakemake-executor-plugin-slurm, the PR_TITLE environment variable is required by the post_to_mastodon.sh script and should be preserved.
Applied to files:
snakemake_executor_plugin_slurm/__init__.py
📚 Learning: 2025-03-10T13:34:59.270Z
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 173
File: tests/tests.py:79-86
Timestamp: 2025-03-10T13:34:59.270Z
Learning: In the Snakemake executor plugin for SLURM, all GPU resources (both simple numeric values and model:number specifications) should be translated to the `--gpus` flag (plural form) to match SLURM's command line interface expectations.
Applied to files:
snakemake_executor_plugin_slurm/__init__.py
📚 Learning: 2025-06-24T12:41:31.837Z
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 221
File: tests/tests.py:62-70
Timestamp: 2025-06-24T12:41:31.837Z
Learning: In the snakemake-executor-plugin-slurm efficiency report feature, the executor is responsible for creating the efficiency_report_path directory when generating efficiency reports. Tests should not manually check for directory existence but should rely on the executor to properly create necessary directories as part of its normal operation.
Applied to files:
snakemake_executor_plugin_slurm/__init__.py
📚 Learning: 2025-03-10T13:34:59.270Z
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 173
File: tests/tests.py:79-86
Timestamp: 2025-03-10T13:34:59.270Z
Learning: In the Snakemake executor plugin for SLURM, all GPU resources (even simple numeric ones) should be translated to the `--gpus` flag (plural) to match SLURM's expectations, not `--gpu` (singular).
Applied to files:
snakemake_executor_plugin_slurm/__init__.py
🪛 Ruff (0.14.6)
snakemake_executor_plugin_slurm/__init__.py
390-390: subprocess call with shell=True identified, security issue
(S602)
dmwnz
changed the title
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
snakemake_executor_plugin_slurm/__init__.py (2)
387-402: Consider capturing the full sbatch command (with/dev/stdin) for clearer diagnosticsThe
sbatch /dev/stdinpattern withstdin=PIPEandcommunicate(input=sbatch_script)is a solid way to stream the script and avoid command‑line length limits, and keeps the existingshell=Trueusage (S602) within the same risk profile as before (sincecallis assembled and validated upstream).For slightly clearer error reporting and debugging, you could construct the full command once and use it consistently:
- self.logger.debug(f"sbatch call: {call}") + full_sbatch_cmd = f"{call} /dev/stdin" + self.logger.debug(f"sbatch call: {full_sbatch_cmd}") try: process = subprocess.Popen( - f"{call} /dev/stdin", # tell sbatch to read the sbatch script from stdin + full_sbatch_cmd, # tell sbatch to read the sbatch script from stdin shell=True, text=True, stdin=subprocess.PIPE, stdout=subprocess.PIPE, stderr=subprocess.PIPE, ) out, err = process.communicate(input=sbatch_script) # feed the sbatch shell script through stdin if process.returncode != 0: raise subprocess.CalledProcessError( - process.returncode, call, output=err + process.returncode, full_sbatch_cmd, output=err )and in the error message below:
- f" sbatch call:\n {call}\n" + f" sbatch call:\n {full_sbatch_cmd}\n"This way, logs and exceptions always reflect the exact command that was executed.
410-410: Error message including the full sbatch script is very helpful (optionally consider truncation)Including
sbatch_scriptin the reported error context will make diagnosing submission failures much easier. If you ever see excessively large log entries from this, you could optionally truncate the script to the first/last N lines, but the current behavior is reasonable for most clusters.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
snakemake_executor_plugin_slurm/__init__.py(2 hunks)
🧰 Additional context used
🧠 Learnings (11)
📓 Common learnings
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 0
File: :0-0
Timestamp: 2025-09-09T15:55:55.669Z
Learning: In the Snakemake SLURM executor plugin, Snakemake submits itself as a job using the --wrap parameter in the sbatch command, rather than creating or submitting a separate job.sh script file. The actual implementation uses `call += f' --wrap="{exec_job}"'` where exec_job is the formatted snakemake execution command.
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 0
File: :0-0
Timestamp: 2025-01-13T09:54:22.950Z
Learning: PR #173 (adding gres resource specification) depends on PR #28 in snakemake-executor-plugin-slurm-jobstep repository, as changes were required in the cpu-settings function of the jobstep-Executor module.
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 187
File: .github/workflows/post_to_mastodon.yml:0-0
Timestamp: 2025-01-17T17:27:32.446Z
Learning: In the mastodon publishing workflow for snakemake-executor-plugin-slurm, the PR_TITLE environment variable is required by the post_to_mastodon.sh script and should be preserved.
Learnt from: johanneskoester
Repo: snakemake/snakemake-executor-plugin-slurm PR: 173
File: docs/further.md:96-96
Timestamp: 2025-03-10T15:20:51.829Z
Learning: PR #173 in snakemake-executor-plugin-slurm implements GPU job support by adding resources: `gres` for generic resource specifications (e.g., 'gpu:1'), `gpu`/`gpus` for specifying GPU counts, and `gpu_model`/`gpu_manufacturer` for specifying GPU types, allowing users to request GPU resources directly rather than having to use slurm_extra.
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 193
File: .github/workflows/post_to_mastodon.yml:26-26
Timestamp: 2025-01-20T09:13:26.443Z
Learning: In the snakemake-executor-plugin-slurm repository, release PRs follow the naming pattern "chore(main): release X.Y.Z" where X.Y.Z is the version number.
📚 Learning: 2025-09-09T15:55:55.669Z
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 0
File: :0-0
Timestamp: 2025-09-09T15:55:55.669Z
Learning: In the Snakemake SLURM executor plugin, Snakemake submits itself as a job using the --wrap parameter in the sbatch command, rather than creating or submitting a separate job.sh script file. The actual implementation uses `call += f' --wrap="{exec_job}"'` where exec_job is the formatted snakemake execution command.
Applied to files:
snakemake_executor_plugin_slurm/__init__.py
📚 Learning: 2024-12-11T14:17:08.749Z
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 178
File: snakemake_executor_plugin_slurm/__init__.py:0-0
Timestamp: 2024-12-11T14:17:08.749Z
Learning: In the `snakemake-executor-plugin-slurm` project, when handling exceptions in `snakemake_executor_plugin_slurm/__init__.py`, prefer concise error messages and avoid unnecessary verbosity or exception chaining when it's not necessary.
Applied to files:
snakemake_executor_plugin_slurm/__init__.py
📚 Learning: 2025-03-10T15:20:51.829Z
Learnt from: johanneskoester
Repo: snakemake/snakemake-executor-plugin-slurm PR: 173
File: docs/further.md:96-96
Timestamp: 2025-03-10T15:20:51.829Z
Learning: PR #173 in snakemake-executor-plugin-slurm implements GPU job support by adding resources: `gres` for generic resource specifications (e.g., 'gpu:1'), `gpu`/`gpus` for specifying GPU counts, and `gpu_model`/`gpu_manufacturer` for specifying GPU types, allowing users to request GPU resources directly rather than having to use slurm_extra.
Applied to files:
snakemake_executor_plugin_slurm/__init__.py
📚 Learning: 2025-01-20T09:13:26.443Z
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 193
File: .github/workflows/post_to_mastodon.yml:26-26
Timestamp: 2025-01-20T09:13:26.443Z
Learning: In the snakemake-executor-plugin-slurm repository, release PRs follow the naming pattern "chore(main): release X.Y.Z" where X.Y.Z is the version number.
Applied to files:
snakemake_executor_plugin_slurm/__init__.py
📚 Learning: 2025-06-03T17:10:51.292Z
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 221
File: snakemake_executor_plugin_slurm/__init__.py:0-0
Timestamp: 2025-06-03T17:10:51.292Z
Learning: In the snakemake-executor-plugin-slurm repository, the create_efficiency_report method in snakemake_executor_plugin_slurm/__init__.py contains temporary testing/debugging code that logs directory contents using os.listdir() and directory paths. This code was inserted for testing purposes only and should be removed from the production implementation.
Applied to files:
snakemake_executor_plugin_slurm/__init__.py
📚 Learning: 2025-03-31T11:52:05.373Z
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 249
File: snakemake_executor_plugin_slurm/submit_string.py:21-29
Timestamp: 2025-03-31T11:52:05.373Z
Learning: In the snakemake-executor-plugin-slurm project, account and partition parameters are validated in the submit function before reaching the get_submit_command function. By design, these parameters are either empty strings or already properly formatted with SLURM parameter tags, making additional validation in get_submit_command unnecessary.
Applied to files:
snakemake_executor_plugin_slurm/__init__.py
📚 Learning: 2025-01-17T17:27:32.446Z
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 187
File: .github/workflows/post_to_mastodon.yml:0-0
Timestamp: 2025-01-17T17:27:32.446Z
Learning: In the mastodon publishing workflow for snakemake-executor-plugin-slurm, the PR_TITLE environment variable is required by the post_to_mastodon.sh script and should be preserved.
Applied to files:
snakemake_executor_plugin_slurm/__init__.py
📚 Learning: 2025-01-13T09:54:22.950Z
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 0
File: :0-0
Timestamp: 2025-01-13T09:54:22.950Z
Learning: PR #173 (adding gres resource specification) depends on PR #28 in snakemake-executor-plugin-slurm-jobstep repository, as changes were required in the cpu-settings function of the jobstep-Executor module.
Applied to files:
snakemake_executor_plugin_slurm/__init__.py
📚 Learning: 2025-03-10T13:34:59.270Z
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 173
File: tests/tests.py:79-86
Timestamp: 2025-03-10T13:34:59.270Z
Learning: In the Snakemake executor plugin for SLURM, all GPU resources (even simple numeric ones) should be translated to the `--gpus` flag (plural) to match SLURM's expectations, not `--gpu` (singular).
Applied to files:
snakemake_executor_plugin_slurm/__init__.py
📚 Learning: 2025-03-10T13:34:59.270Z
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 173
File: tests/tests.py:79-86
Timestamp: 2025-03-10T13:34:59.270Z
Learning: In the Snakemake executor plugin for SLURM, all GPU resources (both simple numeric values and model:number specifications) should be translated to the `--gpus` flag (plural form) to match SLURM's command line interface expectations.
Applied to files:
snakemake_executor_plugin_slurm/__init__.py
🪛 Ruff (0.14.6)
snakemake_executor_plugin_slurm/__init__.py
390-390: subprocess call with shell=True identified, security issue
(S602)
🔇 Additional comments (2)
snakemake_executor_plugin_slurm/__init__.py (2)
384-385: Using a small shebang script instead of--wrapcleanly solves the CLI-length issueConstructing
sbatch_scriptas#!/bin/shplus the formattedexec_joband feeding that tosbatchkeeps the long Snakemake command out of the command line while preserving shell semantics; this aligns well with the goal of avoiding SLURM’s command-line length limit and looks functionally sound. Based on learnings, this appropriately replaces the previous--wrap-based submission.
388-388: Debug‑logging the generated sbatch script is a good additionEmitting the full
sbatch_scriptat debug level is very helpful when diagnosing why a submission failed or behaved unexpectedly, and it stays out of normal log noise.
dmwnz
force-pushed
the
feat/sbatch_script
branch
2 times, most recently
from
79f430f to
676f27f
Compare
…mand directly to circumvent SLURM command-line character limit
dmwnz
force-pushed
the
feat/sbatch_script
branch
from
676f27f to
144c300
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
snakemake_executor_plugin_slurm/__init__.py (1)
520-530: Implementation looks correct.Using
/dev/stdinto stream the script avoids temporary file management. Minor suggestion: consider adding a trailing newline for script completeness.- sbatch_script = "\n".join(["#!/bin/sh", exec_job]) + sbatch_script = "\n".join(["#!/bin/sh", exec_job, ""])
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
snakemake_executor_plugin_slurm/__init__.py(4 hunks)
🧰 Additional context used
🧠 Learnings (9)
📓 Common learnings
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 0
File: :0-0
Timestamp: 2025-09-09T15:55:55.669Z
Learning: In the Snakemake SLURM executor plugin, Snakemake submits itself as a job using the --wrap parameter in the sbatch command, rather than creating or submitting a separate job.sh script file. The actual implementation uses `call += f' --wrap="{exec_job}"'` where exec_job is the formatted snakemake execution command.
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 0
File: :0-0
Timestamp: 2025-01-13T09:54:22.950Z
Learning: PR #173 (adding gres resource specification) depends on PR #28 in snakemake-executor-plugin-slurm-jobstep repository, as changes were required in the cpu-settings function of the jobstep-Executor module.
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 187
File: .github/workflows/post_to_mastodon.yml:0-0
Timestamp: 2025-01-17T17:27:32.446Z
Learning: In the mastodon publishing workflow for snakemake-executor-plugin-slurm, the PR_TITLE environment variable is required by the post_to_mastodon.sh script and should be preserved.
Learnt from: johanneskoester
Repo: snakemake/snakemake-executor-plugin-slurm PR: 173
File: docs/further.md:96-96
Timestamp: 2025-03-10T15:20:51.829Z
Learning: PR #173 in snakemake-executor-plugin-slurm implements GPU job support by adding resources: `gres` for generic resource specifications (e.g., 'gpu:1'), `gpu`/`gpus` for specifying GPU counts, and `gpu_model`/`gpu_manufacturer` for specifying GPU types, allowing users to request GPU resources directly rather than having to use slurm_extra.
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 193
File: .github/workflows/post_to_mastodon.yml:26-26
Timestamp: 2025-01-20T09:13:26.443Z
Learning: In the snakemake-executor-plugin-slurm repository, release PRs follow the naming pattern "chore(main): release X.Y.Z" where X.Y.Z is the version number.
📚 Learning: 2025-03-10T15:20:51.829Z
Learnt from: johanneskoester
Repo: snakemake/snakemake-executor-plugin-slurm PR: 173
File: docs/further.md:96-96
Timestamp: 2025-03-10T15:20:51.829Z
Learning: PR #173 in snakemake-executor-plugin-slurm implements GPU job support by adding resources: `gres` for generic resource specifications (e.g., 'gpu:1'), `gpu`/`gpus` for specifying GPU counts, and `gpu_model`/`gpu_manufacturer` for specifying GPU types, allowing users to request GPU resources directly rather than having to use slurm_extra.
Applied to files:
snakemake_executor_plugin_slurm/__init__.py
📚 Learning: 2025-09-09T15:55:55.669Z
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 0
File: :0-0
Timestamp: 2025-09-09T15:55:55.669Z
Learning: In the Snakemake SLURM executor plugin, Snakemake submits itself as a job using the --wrap parameter in the sbatch command, rather than creating or submitting a separate job.sh script file. The actual implementation uses `call += f' --wrap="{exec_job}"'` where exec_job is the formatted snakemake execution command.
Applied to files:
snakemake_executor_plugin_slurm/__init__.py
📚 Learning: 2024-12-11T14:17:08.749Z
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 178
File: snakemake_executor_plugin_slurm/__init__.py:0-0
Timestamp: 2024-12-11T14:17:08.749Z
Learning: In the `snakemake-executor-plugin-slurm` project, when handling exceptions in `snakemake_executor_plugin_slurm/__init__.py`, prefer concise error messages and avoid unnecessary verbosity or exception chaining when it's not necessary.
Applied to files:
snakemake_executor_plugin_slurm/__init__.py
📚 Learning: 2025-01-13T09:54:22.950Z
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 0
File: :0-0
Timestamp: 2025-01-13T09:54:22.950Z
Learning: PR #173 (adding gres resource specification) depends on PR #28 in snakemake-executor-plugin-slurm-jobstep repository, as changes were required in the cpu-settings function of the jobstep-Executor module.
Applied to files:
snakemake_executor_plugin_slurm/__init__.py
📚 Learning: 2025-01-17T17:27:32.446Z
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 187
File: .github/workflows/post_to_mastodon.yml:0-0
Timestamp: 2025-01-17T17:27:32.446Z
Learning: In the mastodon publishing workflow for snakemake-executor-plugin-slurm, the PR_TITLE environment variable is required by the post_to_mastodon.sh script and should be preserved.
Applied to files:
snakemake_executor_plugin_slurm/__init__.py
📚 Learning: 2025-03-10T13:34:59.270Z
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 173
File: tests/tests.py:79-86
Timestamp: 2025-03-10T13:34:59.270Z
Learning: In the Snakemake executor plugin for SLURM, all GPU resources (even simple numeric ones) should be translated to the `--gpus` flag (plural) to match SLURM's expectations, not `--gpu` (singular).
Applied to files:
snakemake_executor_plugin_slurm/__init__.py
📚 Learning: 2025-03-10T13:34:59.270Z
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 173
File: tests/tests.py:79-86
Timestamp: 2025-03-10T13:34:59.270Z
Learning: In the Snakemake executor plugin for SLURM, all GPU resources (both simple numeric values and model:number specifications) should be translated to the `--gpus` flag (plural form) to match SLURM's command line interface expectations.
Applied to files:
snakemake_executor_plugin_slurm/__init__.py
📚 Learning: 2025-06-03T17:10:51.292Z
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 221
File: snakemake_executor_plugin_slurm/__init__.py:0-0
Timestamp: 2025-06-03T17:10:51.292Z
Learning: In the snakemake-executor-plugin-slurm repository, the create_efficiency_report method in snakemake_executor_plugin_slurm/__init__.py contains temporary testing/debugging code that logs directory contents using os.listdir() and directory paths. This code was inserted for testing purposes only and should be removed from the production implementation.
Applied to files:
snakemake_executor_plugin_slurm/__init__.py
🪛 Ruff (0.14.7)
snakemake_executor_plugin_slurm/__init__.py
534-534: subprocess call with shell=True identified, security issue
(S602)
🔇 Additional comments (4)
snakemake_executor_plugin_slurm/__init__.py (4)
272-284: LGTM!The new setting is well-documented with a clear explanation of when to use it. Defaulting to
Falsemaintains backward compatibility with existing workflows.
534-544: Subprocess stdin handling is correct.The
communicate(input=...)pattern is the proper way to feed data to a subprocess's stdin. Thestdin=subprocess.PIPEaddition is necessary for this to work.
556-560: Good addition for debugging failed submissions.Including the script content in error messages will help users diagnose submission failures. Minor note: you could use
subprocess_stdindirectly instead ofsbatch_scriptfor consistency with the condition check, though both have the same value.
429-433: Verify that the companion implementation of--slurm-jobstep-pass-command-as-scriptexists in snakemake-executor-plugin-slurm-jobstep.This flag is passed to the jobstep executor when
pass_command_as_scriptis enabled. Confirm the corresponding argument parser and handler exist in the jobstep repository and that this PR includes or depends on the necessary changes there.
Member
|
@dmwnz thank you for the contribution! I'm afraid, due to teaching obligations and other stuff, I need to postpone testing to the end of this week. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #379
Summary by CodeRabbit
New Features
Refactor
Bug Fixes / Reliability
✏️ Tip: You can customize this high-level summary in your review settings.