feat: preventing overwrites #358
Conversation
WalkthroughCentralizes SLURM extra validation into a new validation module; updates docs to remove QoS from the Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User as User
participant Exec as Executor
participant Val as Validator
participant SLURM as sbatch
User->>Exec: run_job(job)
Exec->>Val: validate_slurm_extra(job)
alt Forbidden option present
Val-->>Exec: raises WorkflowError
Exec-->>User: validation error
else OK
Exec->>Exec: build sbatch args (account/partition as single-string)
Exec->>SLURM: sbatch <args> (submit)
SLURM-->>Exec: jobid / error
Exec-->>User: submission result
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
snakemake_executor_plugin_slurm/__init__.py (3)
700-710: Partition is not quoted; short-circuit command injection.Users can set slurm_partition via profiles/CLI. Quote it before concatenating into a shell command.
- if partition: - return f" -p {partition}" + if partition: + return f" -p {shlex.quote(str(partition))}"
574-576: Bug: Path has no attribute _str; breaks error reporting on failures.This throws AttributeError on failed jobs and hides the real failure.
- self.report_job_error( - j, msg=msg, aux_logs=[j.aux["slurm_logfile"]._str] - ) + self.report_job_error( + j, msg=msg, aux_logs=[str(j.aux["slurm_logfile"])] + )
601-607: shell=True usage — quote inserted variables or avoid shell=True.Several subprocess calls build shell strings with unquoted variables; this enables command-injection risk. Fix by quoting variables (shlex.quote or existing safe_quote), validating/sanitizing job ids, or by using subprocess with args (shell=False).
- snakemake_executor_plugin_slurm/init.py — cancel_jobs(): scancel_command = f"scancel {jobids} --clusters=all" (≈ lines 595–607): quote/sanitize jobids or call subprocess.run([...], shell=False).
- snakemake_executor_plugin_slurm/efficiency_report.py — get_sacct_data(): cmd = f"sacct --name={run_uuid} --parsable2 --noheader" (≈ lines 59–62): quote run_uuid or pass args list.
- snakemake_executor_plugin_slurm/init.py — sacct_command built at ≈466–468 and executed with shell=True at ≈630: ensure any inserted names/clusters are quoted.
Use safe_quote (submit_string.py), shlex.quote, or args-lists; where shell is required (pipes/wrap), rigorously quote/validate inputs and chunk jobids to avoid overly long command lines.
🧹 Nitpick comments (5)
docs/further.md (1)
492-495: Tighten wording and point users to supported settings instead of slurm_extra.The current note is a bit vague. Explicitly list common managed flags and where to configure them instead (resources or executor settings) to reduce confusion.
Apply:
-.. note:: We prevent you from overwriting all flags, which are used or configurable internally. This prevents conflicts and breaking job submission. +.. note:: Do not pass SLURM options that the plugin manages via ``slurm_extra`` (e.g., ``--job-name``, ``--output/--error``, ``--account/-A``, ``--partition/-p``, ``--mem/--mem-per-cpu``, ``--time/-t``, ``--gpus/-G``/``--gres``, ``--constraint/-C``, ``--nodes/-N``, ``--ntasks/-n``, ``--cpus-per-task/-c``, ``--clusters/-M``). Such overrides are rejected to avoid broken submissions. Use Snakemake resources and executor settings instead (e.g., ``slurm_partition``, ``slurm_account``, ``runtime``, ``mem_mb``/``mem_mb_per_cpu``, ``gpu``/``gres``, and the executor settings ``qos``/``reservation``).snakemake_executor_plugin_slurm/validation.py (1)
63-75: Make error messages precise and shorter; include the exact offending token.This helps users locate the problem and satisfies linters warning about long exception strings.
Apply:
@@ - for pattern, option_name in forbidden_options.items(): - if re.search(pattern, job.resources.slurm_extra): - raise WorkflowError( - f"The --{option_name.replace(' ', '-')} option is not allowed in the 'slurm_extra' parameter. " - f"The {option_name} is set by the snakemake executor plugin and must not be overwritten. " - f"Please use the appropriate snakemake resource specification instead. " - f"Consult the documentation for proper resource configuration." - ) + for pattern, option_name in forbidden_options.items(): + m = re.search(pattern, job.resources.slurm_extra) + if m: + offending = m.group(0).strip() + raise WorkflowError( + f"Forbidden SLURM option in slurm_extra: '{offending}'. " + f"The plugin manages {option_name}; use Snakemake resources/executor settings instead." + )snakemake_executor_plugin_slurm/__init__.py (3)
312-315: Passing preformatted account/partition strings couples to get_submit_command.Not a blocker, but consider passing raw values and formatting in get_submit_command to keep one quoting policy.
Would you like a follow-up patch to move formatting into get_submit_command for consistency?
345-354: Minor: fix spacing in concatenated warning string.Currently renders “…'tasks_per_node=1'.Probably…”. Add a space/newline.
- "specified. Assuming 'tasks_per_node=1'." - "Probably not what you want." + "specified. Assuming 'tasks_per_node=1'. " + "Probably not what you want."
252-263: Unused parameter/state in warn_on_jobcontext().The done parameter is unused; setting done=True has no effect. Simplify the signature.
- def warn_on_jobcontext(self, done=None): - if not done: + def warn_on_jobcontext(self): + if True: if "SLURM_JOB_ID" in os.environ: ... - done = True
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
docs/further.md(1 hunks)snakemake_executor_plugin_slurm/__init__.py(8 hunks)snakemake_executor_plugin_slurm/validation.py(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: cmeesters
PR: snakemake/snakemake-executor-plugin-slurm#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: johanneskoester
PR: snakemake/snakemake-executor-plugin-slurm#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
PR: snakemake/snakemake-executor-plugin-slurm#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.
📚 Learning: 2025-03-10T15:20:51.829Z
Learnt from: johanneskoester
PR: snakemake/snakemake-executor-plugin-slurm#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/validation.py
📚 Learning: 2024-12-11T14:17:08.749Z
Learnt from: cmeesters
PR: snakemake/snakemake-executor-plugin-slurm#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-31T11:52:05.373Z
Learnt from: cmeesters
PR: snakemake/snakemake-executor-plugin-slurm#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
🧬 Code graph analysis (1)
snakemake_executor_plugin_slurm/__init__.py (1)
snakemake_executor_plugin_slurm/validation.py (1)
validate_slurm_extra(53-75)
🪛 Ruff (0.12.2)
snakemake_executor_plugin_slurm/validation.py
70-75: Avoid specifying long messages outside the exception class
(TRY003)
snakemake_executor_plugin_slurm/__init__.py
737-737: subprocess call with shell=True identified, security issue
(S602)
750-750: subprocess call with shell=True identified, security issue
(S602)
⏰ 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 (4)
docs/further.md (1)
489-490: Good change: remove qos from slurm_extra example.This aligns the example with the new policy of not overriding plugin-managed flags.
snakemake_executor_plugin_slurm/__init__.py (3)
38-39: Good: centralize slurm_extra validation.Importing and delegating to validation.validate_slurm_extra reduces duplication.
304-307: Call validation unconditionally if slurm_extra exists.Looks good. Ensure this runs before constructing the sbatch string.
809-811: LGTM: validation is now single-sourced.This nicely removes duplicated regex checks from the executor.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/tests.py (1)
659-675: Bug: test body is outside the function due to missing indentation.
The statements after the docstring run at module import; the test method is empty.Apply:
def test_wildcard_slash_replacement(self): """ Test that slashes in wildcards are correctly replaced with underscores in log directory paths. """ - - # Just test the wildcard sanitization logic directly - wildcards = ["/leading_slash", "middle/slash", "trailing/"] - - # This is the actual logic from the Executor.run_job method - wildcard_str = "_".join(wildcards).replace("/", "_") if wildcards else "" - - # Assert that slashes are correctly replaced with underscores - assert wildcard_str == "_leading_slash_middle_slash_trailing_" - - # Verify no slashes remain in the wildcard string - assert "/" not in wildcard_str + # Just test the wildcard sanitization logic directly + wildcards = ["/leading_slash", "middle/slash", "trailing/"] + # This is the actual logic from the Executor.run_job method + wildcard_str = "_".join(wildcards).replace("/", "_") if wildcards else "" + # Assert that slashes are correctly replaced with underscores + assert wildcard_str == "_leading_slash_middle_slash_trailing_" + # Verify no slashes remain in the wildcard string + assert "/" not in wildcard_str
♻️ Duplicate comments (1)
snakemake_executor_plugin_slurm/validation.py (1)
16-45: Harden regex: catch compact short options and missing aliases (-pdebug, -o%j.log, -M, --chdir, -G, etc.).
As flagged before, current patterns miss unspaced short forms and some aliases, allowing bypass of the check.Apply:
- return { - # Job identification and output - r"--job-name[=\s]|-J\s?": "job name", - r"--output[=\s]|-o\s": "output file", - r"--error[=\s]|-e\s": "error file", - r"--parsable": "parsable output", - r"--export[=\s]": "environment export", - r"--comment[=\s]": "job comment", - r"--workdir[=\s]|-D\s": "working directory", - # Account and partition - r"--account[=\s]|-A\s": "account", - r"--partition[=\s]|-p\s": "partition", - # Memory options - r"--mem[=\s]": "memory", - r"--mem-per-cpu[=\s]": "memory per CPU", - # CPU and task options - r"--ntasks[=\s]|-n\s": "number of tasks", - r"--ntasks-per-gpu[=\s]": "tasks per GPU", - r"--cpus-per-task[=\s]|-c\s": "CPUs per task", - r"--cpus-per-gpu[=\s]": "CPUs per GPU", - # Time and resource constraints - r"--time[=\s]|-t\s": "runtime/time limit", - r"--constraint[=\s]|-C\s": "node constraints", - r"--qos[=\s]": "quality of service", - r"--nodes[=\s]|-N\s": "number of nodes", - r"--clusters[=\s]": "cluster specification", - # GPU options - r"--gres[=\s]": "generic resources (GRES)", - r"--gpus[=\s]": "GPU allocation", - } + return { + # Job identification and output + r"(?<!\S)(?:--job-name(?:=|\s)|-J(?:\S|\s))": "job name", + r"(?<!\S)(?:--output(?:=|\s)|-o(?:\S|\s))": "output file", + r"(?<!\S)(?:--error(?:=|\s)|-e(?:\S|\s))": "error file", + r"(?<!\S)--parsable": "parsable output", + r"(?<!\S)--export(?:=|\s)": "environment export", + r"(?<!\S)--comment(?:=|\s)": "job comment", + r"(?<!\S)(?:--chdir(?:=|\s)|--workdir(?:=|\s)|-D(?:\S|\s))": "working directory", + # Account and partition + r"(?<!\S)(?:--account(?:=|\s)|-A(?:\S|\s))": "account", + r"(?<!\S)(?:--partition(?:=|\s)|-p(?:\S|\s))": "partition", + # Memory options + r"(?<!\S)--mem(?:=|\s)": "memory", + r"(?<!\S)--mem-per-cpu(?:=|\s)": "memory per CPU", + # CPU and task options + r"(?<!\S)(?:--ntasks(?:=|\s)|-n(?:\d+|\s+\d+))": "number of tasks", + r"(?<!\S)--ntasks-per-gpu(?:=|\s)": "tasks per GPU", + r"(?<!\S)(?:--cpus-per-task(?:=|\s)|-c(?:\d+|\s+\d+))": "CPUs per task", + r"(?<!\S)--cpus-per-gpu(?:=|\s)": "CPUs per GPU", + # Time and resource constraints + r"(?<!\S)(?:--time(?:=|\s)|-t(?:\S+|\s+\S+))": "runtime/time limit", + r"(?<!\S)(?:--constraint(?:=|\s)|-C(?:\S+|\s+\S+))": "node constraints", + r"(?<!\S)--qos(?:=|\s)": "quality of service", + r"(?<!\S)(?:--nodes(?:=|\s)|-N(?:\d+|\s+\d+))": "number of nodes", + r"(?<!\S)(?:--clusters(?:=|\s)|-M(?:\S+|\s+\S+))": "cluster specification", + # GPU options + r"(?<!\S)--gres(?:=|\s)": "generic resources (GRES)", + r"(?<!\S)(?:--gpus(?:=|\s)|-G(?:\d+|\s+\d+))": "GPU allocation", + }
🧹 Nitpick comments (6)
tests/tests.py (3)
681-700: Tighten mock for “no slurm_extra” and fix Ruff ARG005.
Else-branch lambda ignores key; also MagicMock will still synthesize attributes, which can mask bugs.Apply:
@@ - else: - # No slurm_extra attribute - del mock_resources.slurm_extra - mock_resources.get.side_effect = lambda key, default=None: default + else: + # No slurm_extra attribute + if hasattr(mock_resources, "slurm_extra"): + delattr(mock_resources, "slurm_extra") + mock_resources.get.side_effect = lambda _key, default=None: default
524-524: Typo: rename test for clarity.
“test_taks” → “test_tasks”.- def test_taks(self, mock_job): + def test_tasks(self, mock_job):
367-385: Optional: avoid injecting “ None” into sbatch.
Tests pass, but passing None for account/partition gets stringified into the command. Use empty strings to mirror production behavior.- "account": None, - "partition": None, + "account": "", + "partition": "",Also applies to: 398-407, 421-432, 452-461, 503-513, 527-536, 551-560, 575-584, 599-607, 622-631
snakemake_executor_plugin_slurm/validation.py (3)
64-74: Keep message concise; satisfy TRY003 while preserving “not allowed”.
Optional cleanup; content stays the same.- raise WorkflowError( - f"The --{option_name.replace(' ', '-')} option is not " - f"allowed in the 'slurm_extra' parameter. " - f"The {option_name} is set by the snakemake executor plugin " - f"and must not be overwritten. " - f"Please use the appropriate snakemake resource " - f"specification instead. " - f"Consult the documentation for proper resource configuration." - ) + raise WorkflowError( + f"The --{option_name.replace(' ', '-')} option is not allowed in 'slurm_extra'. " + "It is managed by the Snakemake SLURM executor; remove it and use resources instead." + )
58-61: Remove stray “no need to test?” comment.
It’s confusing in a validator that is tested.- # no need to test?
9-15: Optional: precompile patterns once.
Minor perf/clarity improvement.I can refactor to return a list of (compiled_regex, name) and use those in validate_slurm_extra.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
snakemake_executor_plugin_slurm/validation.py(1 hunks)tests/tests.py(5 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: cmeesters
PR: snakemake/snakemake-executor-plugin-slurm#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
PR: snakemake/snakemake-executor-plugin-slurm#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.
📚 Learning: 2025-03-31T11:52:05.373Z
Learnt from: cmeesters
PR: snakemake/snakemake-executor-plugin-slurm#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:
tests/tests.pysnakemake_executor_plugin_slurm/validation.py
📚 Learning: 2025-09-16T17:25:38.450Z
Learnt from: cmeesters
PR: snakemake/snakemake-executor-plugin-slurm#355
File: snakemake_executor_plugin_slurm/submit_string.py:0-0
Timestamp: 2025-09-16T17:25:38.450Z
Learning: In the snakemake-executor-plugin-slurm project, constraint and qos parameters follow a three-state logic: None (not set, no flag added), empty string (explicitly set to empty, adds flag with empty quotes like --constraint ''), and non-empty string (adds flag with quoted value). The correct implementation uses `constraint = job.resources.get("constraint")` followed by `if constraint is not None:` to distinguish between "not provided" and "explicitly set to empty".
Applied to files:
tests/tests.py
📚 Learning: 2025-03-10T15:20:51.829Z
Learnt from: johanneskoester
PR: snakemake/snakemake-executor-plugin-slurm#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/validation.py
🧬 Code graph analysis (1)
tests/tests.py (2)
snakemake_executor_plugin_slurm/validation.py (1)
validate_slurm_extra(48-74)snakemake_executor_plugin_slurm/submit_string.py (1)
get_submit_command(18-100)
🪛 Ruff (0.12.2)
tests/tests.py
694-694: Unused lambda argument: key
(ARG005)
snakemake_executor_plugin_slurm/validation.py
66-74: 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 (6)
tests/tests.py (6)
15-15: Good: tests exercise new validation API.
Importing validate_slurm_extra here is correct and keeps tests focused on the public surface.
392-392: Good: constraint value is now quoted in sbatch.
Assertion matches the safe_quote behavior and prevents regressions.
416-416: Good: qos is validated with quotes.
Matches current submit string construction.
443-444: Good: combined constraint + qos assertions.
Covers ordering-agnostic inclusion of both flags.
678-761: Consider adding coverage for compact short options (e.g., -pdebug, -n8).
These forms are valid in SLURM and should be rejected by validation.I can add parametrized tests once the regex is updated in validation.py.
678-761: No duplicate TestSlurmExtraValidation found.
Single match at tests/tests.py:678.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
snakemake_executor_plugin_slurm/__init__.py (1)
321-325: Quote qos/reservation values appended to the sbatch call.- if self.workflow.executor_settings.qos: - call += f" --qos={self.workflow.executor_settings.qos}" + if self.workflow.executor_settings.qos: + call += f" --qos={shlex.quote(str(self.workflow.executor_settings.qos))}" @@ - if self.workflow.executor_settings.reservation: - call += f" --reservation={self.workflow.executor_settings.reservation}" + if self.workflow.executor_settings.reservation: + call += f" --reservation={shlex.quote(str(self.workflow.executor_settings.reservation))}"
♻️ Duplicate comments (6)
snakemake_executor_plugin_slurm/__init__.py (4)
762-777: Normalize account-case for membership test; keep originals for messaging.- accounts = set(_.strip() for _ in accounts.split("\n") if _) + raw_accounts = [_.strip() for _ in accounts.split("\n") if _] + accounts = set(a.lower() for a in raw_accounts) @@ - if account.lower() not in accounts: + if account.lower() not in accounts: raise WorkflowError( - f"The given account {account} appears to be invalid. Available " - f"accounts:\n{', '.join(accounts)}" + f"The given account {account} appears to be invalid. Available " + f"accounts:\n{', '.join(raw_accounts)}" )
675-676: Quote user-supplied account; shell=True makes this critical.
Use shlex.quote to avoid injection/whitespace issues.- return f" -A '{job.resources.slurm_account}'" + return f" -A {shlex.quote(str(job.resources.slurm_account))}"
681-684: Fallback account not quoted.- self.test_account(f"{account}") - self._fallback_account_arg = f" -A {account}" + self.test_account(f"{account}") + self._fallback_account_arg = f" -A {shlex.quote(str(account))}"
705-707: Quote partition to be consistent and safe.- if partition: - return f" -p {partition}" + if partition: + return f" -p {shlex.quote(str(partition))}"snakemake_executor_plugin_slurm/validation.py (2)
58-66: Prevent crash for missing/None/mock slurm_extra; accept only non-empty strings.
Current getattr on a MagicMock yields a child mock; re.search then raises TypeError. Also treat empty string as “not set”.Apply:
- # skip testing if no slurm_extra is set - slurm_extra = getattr(job.resources, "slurm_extra", None) - if not slurm_extra: - return + # Obtain slurm_extra robustly + slurm_extra = getattr(job.resources, "slurm_extra", None) + if slurm_extra is None and hasattr(job.resources, "get"): + slurm_extra = job.resources.get("slurm_extra") + # Accept missing or explicitly empty values + if slurm_extra in (None, ""): + return + if not isinstance(slurm_extra, str): + raise WorkflowError("The 'slurm_extra' resource must be a string.")
16-45: Bypass risk: short-form options without whitespace and missing aliases aren’t caught.
Patterns like -pdebug, -o%j.log, -n8, -c4, -N2, -D/path, -Ctag, -G2, -Mcluster slip through. Also missing --chdir and short -M/-G forms.Harden the map:
- r"--job-name[=\s]|-J\s?": "job name", - r"--output[=\s]|-o\s": "output file", - r"--error[=\s]|-e\s": "error file", + r"--job-name(?:=|\s)|-J(?:\s|\S)": "job name", + r"--output(?:=|\s)|-o(?:\s|\S)": "output file", + r"--error(?:=|\s)|-e(?:\s|\S)": "error file", r"--parsable": "parsable output", - r"--export[=\s]": "environment export", - r"--comment[=\s]": "job comment", - r"--workdir[=\s]|-D\s": "working directory", + r"--export(?:=|\s)": "environment export", + r"--comment(?:=|\s)": "job comment", + r"--chdir(?:=|\s)|--workdir(?:=|\s)|-D(?:\s|\S)": "working directory", - r"--account[=\s]|-A\s": "account", - r"--partition[=\s]|-p\s": "partition", + r"--account(?:=|\s)|-A(?:\s|\S)": "account", + r"--partition(?:=|\s)|-p(?:\s|\S)": "partition", - r"--mem[=\s]": "memory", - r"--mem-per-cpu[=\s]": "memory per CPU", + r"--mem(?:=|\s)": "memory", + r"--mem-per-cpu(?:=|\s)": "memory per CPU", - r"--ntasks[=\s]|-n\s": "number of tasks", - r"--ntasks-per-gpu[=\s]": "tasks per GPU", - r"--cpus-per-task[=\s]|-c\s": "CPUs per task", - r"--cpus-per-gpu[=\s]": "CPUs per GPU", + r"--ntasks(?:=|\s)|-n(?:\s|\d)": "number of tasks", + r"--ntasks-per-gpu(?:=|\s)": "tasks per GPU", + r"--cpus-per-task(?:=|\s)|-c(?:\s|\d)": "CPUs per task", + r"--cpus-per-gpu(?:=|\s)": "CPUs per GPU", - r"--time[=\s]|-t\s": "runtime/time limit", - r"--constraint[=\s]|-C\s": "node constraints", - r"--qos[=\s]": "quality of service", - r"--nodes[=\s]|-N\s": "number of nodes", - r"--clusters[=\s]": "cluster specification", + r"--time(?:=|\s)|-t(?:\s|\S)": "runtime/time limit", + r"--constraint(?:=|\s)|-C(?:\s|\S)": "node constraints", + r"--qos(?:=|\s)": "quality of service", + r"--nodes(?:=|\s)|-N(?:\s|\d)": "number of nodes", + r"--clusters(?:=|\s)|-M(?:\s|\S)": "cluster specification", - r"--gres[=\s]": "generic resources (GRES)", - r"--gpus[=\s]": "GPU allocation", + r"--gres(?:=|\s)": "generic-resources", + r"--gpus(?:=|\s)|-G(?:\s|\d)": "GPU allocation",
🧹 Nitpick comments (2)
snakemake_executor_plugin_slurm/validation.py (1)
67-75: Trim exception text; return concise guidance.
Consider shortening the message per TRY003 and link to docs rather than embedding multi-sentence text.snakemake_executor_plugin_slurm/__init__.py (1)
359-367: Optional: prefer shell=False where feasible.
Construct argv lists for sbatch/scancel/sacct invocations to drop shell=True and reduce injection surface.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
snakemake_executor_plugin_slurm/__init__.py(8 hunks)snakemake_executor_plugin_slurm/validation.py(1 hunks)tests/tests.py(5 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: cmeesters
PR: snakemake/snakemake-executor-plugin-slurm#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: johanneskoester
PR: snakemake/snakemake-executor-plugin-slurm#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
PR: snakemake/snakemake-executor-plugin-slurm#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.
📚 Learning: 2025-03-31T11:52:05.373Z
Learnt from: cmeesters
PR: snakemake/snakemake-executor-plugin-slurm#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__.pysnakemake_executor_plugin_slurm/validation.pytests/tests.py
📚 Learning: 2024-12-11T14:17:08.749Z
Learnt from: cmeesters
PR: snakemake/snakemake-executor-plugin-slurm#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__.pysnakemake_executor_plugin_slurm/validation.py
📚 Learning: 2025-03-10T15:20:51.829Z
Learnt from: johanneskoester
PR: snakemake/snakemake-executor-plugin-slurm#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__.pysnakemake_executor_plugin_slurm/validation.py
📚 Learning: 2025-09-16T17:25:38.450Z
Learnt from: cmeesters
PR: snakemake/snakemake-executor-plugin-slurm#355
File: snakemake_executor_plugin_slurm/submit_string.py:0-0
Timestamp: 2025-09-16T17:25:38.450Z
Learning: In the snakemake-executor-plugin-slurm project, constraint and qos parameters follow a three-state logic: None (not set, no flag added), empty string (explicitly set to empty, adds flag with empty quotes like --constraint ''), and non-empty string (adds flag with quoted value). The correct implementation uses `constraint = job.resources.get("constraint")` followed by `if constraint is not None:` to distinguish between "not provided" and "explicitly set to empty".
Applied to files:
snakemake_executor_plugin_slurm/validation.pytests/tests.py
🧬 Code graph analysis (2)
snakemake_executor_plugin_slurm/__init__.py (1)
snakemake_executor_plugin_slurm/validation.py (1)
validate_slurm_extra(48-75)
tests/tests.py (2)
snakemake_executor_plugin_slurm/validation.py (1)
validate_slurm_extra(48-75)snakemake_executor_plugin_slurm/submit_string.py (1)
get_submit_command(18-100)
🪛 Ruff (0.12.2)
snakemake_executor_plugin_slurm/__init__.py
736-736: subprocess call with shell=True identified, security issue
(S602)
749-749: subprocess call with shell=True identified, security issue
(S602)
snakemake_executor_plugin_slurm/validation.py
67-75: Avoid specifying long messages outside the exception class
(TRY003)
tests/tests.py
695-695: Unused lambda argument: key
(ARG005)
⏰ 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 (6)
tests/tests.py (4)
15-15: Import looks good; aligns tests with new validator.
392-392: Quote-aware expectation confirmed.
The constraint is now always shell-quoted; assertion matches.
416-416: Quote-aware expectation confirmed.
QoS is now always shell-quoted; assertion matches.
441-445: Both flags asserted with quoting — good.snakemake_executor_plugin_slurm/__init__.py (2)
311-314: Passing account/partition fragments as strings is fine; ensure they’re shell-escaped.
808-810: Centralizing slurm_extra checks here is the right call.
Accidentally, the last account escaping and checking behaviour was erased - this commit corrects this mistake.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
snakemake_executor_plugin_slurm/__init__.py (1)
676-704: Avoid re-runningtest_accountin the yield loopLine 687 calls
self.test_account(account)even though we already validated the same account in the preceding loop. Because eachtest_accountinvocation shells out tosacctmgr/sshare, this doubles the external calls and noticeably slows down submissions on busy clusters. Drop the second call and keep the quoting as-is.- for account in accounts: - self.test_account(account) - yield f" -A {shlex.quote(account)}" + for account in accounts: + yield f" -A {shlex.quote(account)}"
📜 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 (3)
📓 Common learnings
Learnt from: cmeesters
PR: snakemake/snakemake-executor-plugin-slurm#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: johanneskoester
PR: snakemake/snakemake-executor-plugin-slurm#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
PR: snakemake/snakemake-executor-plugin-slurm#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.
📚 Learning: 2025-03-31T11:52:05.373Z
Learnt from: cmeesters
PR: snakemake/snakemake-executor-plugin-slurm#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: 2024-12-11T14:17:08.749Z
Learnt from: cmeesters
PR: snakemake/snakemake-executor-plugin-slurm#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
🧬 Code graph analysis (1)
snakemake_executor_plugin_slurm/__init__.py (1)
snakemake_executor_plugin_slurm/validation.py (1)
validate_slurm_extra(48-75)
⏰ 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
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/tests.py(5 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: cmeesters
PR: snakemake/snakemake-executor-plugin-slurm#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: johanneskoester
PR: snakemake/snakemake-executor-plugin-slurm#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
PR: snakemake/snakemake-executor-plugin-slurm#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.
📚 Learning: 2025-03-31T11:52:05.373Z
Learnt from: cmeesters
PR: snakemake/snakemake-executor-plugin-slurm#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:
tests/tests.py
📚 Learning: 2025-09-16T17:25:38.450Z
Learnt from: cmeesters
PR: snakemake/snakemake-executor-plugin-slurm#355
File: snakemake_executor_plugin_slurm/submit_string.py:0-0
Timestamp: 2025-09-16T17:25:38.450Z
Learning: In the snakemake-executor-plugin-slurm project, constraint and qos parameters follow a three-state logic: None (not set, no flag added), empty string (explicitly set to empty, adds flag with empty quotes like --constraint ''), and non-empty string (adds flag with quoted value). The correct implementation uses `constraint = job.resources.get("constraint")` followed by `if constraint is not None:` to distinguish between "not provided" and "explicitly set to empty".
Applied to files:
tests/tests.py
🧬 Code graph analysis (1)
tests/tests.py (2)
snakemake_executor_plugin_slurm/validation.py (1)
validate_slurm_extra(48-75)snakemake_executor_plugin_slurm/submit_string.py (1)
get_submit_command(18-100)
🪛 Ruff (0.13.1)
tests/tests.py
723-723: Pattern passed to match= contains metacharacters but is neither escaped nor raw
(RUF043)
729-729: Pattern passed to match= contains metacharacters but is neither escaped nor raw
(RUF043)
735-735: Pattern passed to match= contains metacharacters but is neither escaped nor raw
(RUF043)
741-741: Pattern passed to match= contains metacharacters but is neither escaped nor raw
(RUF043)
747-747: Pattern passed to match= contains metacharacters but is neither escaped nor raw
(RUF043)
753-753: Pattern passed to match= contains metacharacters but is neither escaped nor raw
(RUF043)
760-760: Pattern passed to match= contains metacharacters but is neither escaped nor raw
(RUF043)
⏰ 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 (1)
tests/tests.py (1)
685-699: Replace the MagicMock resources with a plain helper object.
Whenslurm_extrais absent,getattr(job.resources, "slurm_extra", None)on aMagicMockstill fabricates a child mock instead of returning the default. The validator then receives that mock,re.search(...)raisesTypeError: expected string or bytes-like object, andtest_no_slurm_extrafails. Please back the fixture with a trivial resources object so missing attributes stay missing.Apply this diff to rework the fixture:
- def _create_job(slurm_extra=None): - mock_resources = MagicMock() - if slurm_extra is not None: - mock_resources.slurm_extra = slurm_extra - mock_resources.get.side_effect = lambda _key, default=None: ( - slurm_extra if _key == "slurm_extra" else default - ) - else: - # Do not set slurm_extra attribute at all - mock_resources.get.side_effect = lambda _key, default=None: default - - mock_job = MagicMock() - mock_job.resources = mock_resources - return mock_job + def _create_job(slurm_extra=None): + class Resources: + def __init__(self, value): + self._value = value + if value is not None: + self.slurm_extra = value + + def get(self, key, default=None): + if key == "slurm_extra": + return self._value + return default + + mock_job = MagicMock() + mock_job.resources = Resources(slurm_extra) + return mock_job
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)
tests/tests.py (2)
705-711: Add coverage for “no/empty slurm_extra” early‑return path.Prevents regressions and catches MagicMock pitfalls; passes with the Resources refactor above.
Apply this diff to add tests:
def test_valid_slurm_extra(self, mock_job): """Test that validation passes with allowed SLURM options.""" job = mock_job(slurm_extra="--mail-type=END [email protected]") # Should not raise any exception validate_slurm_extra(job) + def test_no_slurm_extra(self, mock_job): + """Validator should no-op when slurm_extra is not set.""" + job = mock_job() # no slurm_extra attribute + validate_slurm_extra(job) + + def test_empty_slurm_extra(self, mock_job): + """Validator should no-op when slurm_extra is explicitly empty.""" + job = mock_job(slurm_extra="") + validate_slurm_extra(job) +
747-752: Test relies on mapping iteration order; confirm determinism.If get_forbidden_slurm_options changes order, this test can become flaky. Ensure the mapping preserves insertion order or assert on a set of messages instead.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/tests.py(5 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: cmeesters
PR: snakemake/snakemake-executor-plugin-slurm#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
PR: snakemake/snakemake-executor-plugin-slurm#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.
📚 Learning: 2025-03-31T11:52:05.373Z
Learnt from: cmeesters
PR: snakemake/snakemake-executor-plugin-slurm#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:
tests/tests.py
📚 Learning: 2025-09-16T17:25:38.450Z
Learnt from: cmeesters
PR: snakemake/snakemake-executor-plugin-slurm#355
File: snakemake_executor_plugin_slurm/submit_string.py:0-0
Timestamp: 2025-09-16T17:25:38.450Z
Learning: In the snakemake-executor-plugin-slurm project, constraint and qos parameters follow a three-state logic: None (not set, no flag added), empty string (explicitly set to empty, adds flag with empty quotes like --constraint ''), and non-empty string (adds flag with quoted value). The correct implementation uses `constraint = job.resources.get("constraint")` followed by `if constraint is not None:` to distinguish between "not provided" and "explicitly set to empty".
Applied to files:
tests/tests.py
🧬 Code graph analysis (1)
tests/tests.py (2)
snakemake_executor_plugin_slurm/validation.py (1)
validate_slurm_extra(48-75)snakemake_executor_plugin_slurm/submit_string.py (1)
get_submit_command(18-100)
⏰ 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 (4)
tests/tests.py (4)
15-15: Centralized validation import: LGTM.Importing validate_slurm_extra aligns tests with the new validation flow.
392-392: Quoting expectations for constraint/qos: LGTM.Assertions match the safe_quote behavior and the 3‑state logic for None/""/value.
Also applies to: 416-416, 443-445
685-703: Avoid MagicMock attribute auto-creation in resources; use a tiny Resources class.MagicMock returns child mocks for missing attributes. If a test omits slurm_extra, validate_slurm_extra(getattr(...)) can receive a MagicMock, leading to TypeError in re.search. Use a simple Resources class to ensure missing attrs raise AttributeError and get(...) works predictably.
Apply this diff to the fixture:
def _create_job(**resources): - mock_resources = MagicMock() - # Configure get method to return values from resources dict - mock_resources.get.side_effect = lambda key, default=None: resources.get( - key, default - ) - # Add direct attribute access for certain resources - for key, value in resources.items(): - setattr(mock_resources, key, value) - - mock_job = MagicMock() - mock_job.resources = mock_resources + class Resources: + def __init__(self, d): + self._d = d + for k, v in d.items(): + setattr(self, k, v) + def get(self, key, default=None): + return self._d.get(key, default) + + mock_job = MagicMock() + mock_job.resources = Resources(resources) mock_job.name = "test_job" mock_job.wildcards = {} mock_job.is_group.return_value = False mock_job.jobid = 1 return mock_job
714-715: Raw regex in pytest.raises(match): LGTM.Switch to r"...” resolves Ruff RUF043 warnings.
Also applies to: 720-721, 726-727, 732-733, 738-739, 744-745, 751-752
…utor-plugin-slurm into fix/slurm_extra
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/tests.py (1)
747-752: Avoid order-dependent assertion; make the test robust to pattern iteration order.The validator may report either option first depending on the order returned by get_forbidden_slurm_options(). Don’t pin to job-name specifically.
Apply one of:
Option A (regex alternation):
- with pytest.raises(WorkflowError, match=r"job-name.*not allowed"): + with pytest.raises(WorkflowError, match=r"(job-name|account).*not allowed"):Option B (inspect exception message):
- with pytest.raises(WorkflowError, match=r"job-name.*not allowed"): - validate_slurm_extra(job) + with pytest.raises(WorkflowError) as excinfo: + validate_slurm_extra(job) + assert re.search(r"(job-name|account).*not allowed", str(excinfo.value))
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/tests.py(3 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: cmeesters
PR: snakemake/snakemake-executor-plugin-slurm#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
PR: snakemake/snakemake-executor-plugin-slurm#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.
📚 Learning: 2025-03-31T11:52:05.373Z
Learnt from: cmeesters
PR: snakemake/snakemake-executor-plugin-slurm#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:
tests/tests.py
🧬 Code graph analysis (1)
tests/tests.py (2)
snakemake_executor_plugin_slurm/validation.py (1)
validate_slurm_extra(48-75)snakemake_executor_plugin_slurm/submit_string.py (1)
get_submit_command(18-100)
⏰ 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)
tests/tests.py (3)
15-15: Import is correct and used in tests.The new import aligns with the added validation tests.
681-704: Fixture setup for slurm_extra looks solid.Defining only provided attributes and using resources.get keeps semantics aligned with validator’s getattr(job.resources, "slurm_extra", None) access.
711-746: Good: raw regex strings in pytest.raises(match=...).Using raw string patterns avoids RUF043 and ensures intended regex behavior.
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 (1)
tests/tests.py (1)
747-752: Broaden validation tests: cover partition and missing slurm_extra.Add tests for
--partition/-pforbiddance and the no-op case whenslurm_extrais absent to guard regressions.def test_multiple_forbidden_options(self, mock_job): """Test that the first forbidden option found is reported.""" job = mock_job(slurm_extra="--job-name=test --account=myaccount") # Should raise error for job-name (first one encountered) with pytest.raises(WorkflowError, match=r"job-name.*not allowed"): validate_slurm_extra(job) + + def test_forbidden_partition_long_and_short_form(self, mock_job): + """Both --partition and -p must be rejected.""" + job = mock_job(slurm_extra="--partition=debug") + with pytest.raises(WorkflowError, match=r"partition.*not allowed"): + validate_slurm_extra(job) + job = mock_job(slurm_extra="-p debug") + with pytest.raises(WorkflowError, match=r"partition.*not allowed"): + validate_slurm_extra(job) + + def test_missing_slurm_extra_is_noop(self, mock_job): + """No slurm_extra set should not raise.""" + job = mock_job() + validate_slurm_extra(job)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/tests.py(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: cmeesters
PR: snakemake/snakemake-executor-plugin-slurm#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
PR: snakemake/snakemake-executor-plugin-slurm#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.
📚 Learning: 2025-03-31T11:52:05.373Z
Learnt from: cmeesters
PR: snakemake/snakemake-executor-plugin-slurm#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:
tests/tests.py
🧬 Code graph analysis (1)
tests/tests.py (1)
snakemake_executor_plugin_slurm/validation.py (1)
validate_slurm_extra(48-75)
⏰ 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)
tests/tests.py (3)
15-15: Centralized validator import — LGTM.Importing
validate_slurm_extradirectly into tests is appropriate and keeps coverage close to the source behavior.
681-704: Avoid MagicMock attribute auto-creation for resources; use a tiny plain object.
getattr(job.resources, "slurm_extra", None)on a MagicMock returns a child mock when unset, which is truthy and will causere.search(...)to raiseTypeError. Use a simpleResourcesclass so missing attributes are truly missing.Apply this diff to harden the fixture:
- def _create_job(**resources): - mock_resources = MagicMock() - # Configure get method to return values from resources dict - mock_resources.get.side_effect = lambda key, default=None: resources.get( - key, default - ) - # Add direct attribute access for certain resources - for key, value in resources.items(): - setattr(mock_resources, key, value) - - mock_job = MagicMock() - mock_job.resources = mock_resources + def _create_job(**resources): + class Resources: + def __init__(self, data): + self._data = dict(data) + for k, v in data.items(): + setattr(self, k, v) + def get(self, key, default=None): + return self._data.get(key, default) + mock_job = MagicMock() + mock_job.resources = Resources(resources) mock_job.name = "test_job" mock_job.wildcards = {} mock_job.is_group.return_value = False mock_job.jobid = 1 return mock_job
711-746: Good: raw regex in pytest.raises(match=...) avoids RUF043.Using
r"..."for patterns with metacharacters prevents Ruff failures and ensures intended matching.
1 participant
From this PR onwards, overwriting flags already used internally with the
slurm_extraparameter. The plugin is feature rich and gets improved continuously. Overwriting internally used flags (via CLI or resources) already caused a number of issues. Now, we should have a reasonable error message, instead.Summary by CodeRabbit
New Features
Documentation
Tests