fix: quoting parameters #355
Conversation
WalkthroughAdds a module-level safe_quote(value) and applies it when assembling SBATCH flags; converts params input to a SimpleNamespace for attribute access; adjusts SBATCH flag construction (constraint, qos, mem, workdir, GPU/task logic) and updates tests to expect unquoted resource specifiers in generated sbatch commands. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller as Caller
participant Builder as submit_string.py
participant SLURM as sbatch
Note over Builder: Introduce safe_quote(...) and params -> SimpleNamespace
Caller->>Builder: get_submit_command(params_dict, job)
activate Builder
Builder->>Builder: params = SimpleNamespace(params_dict)
Builder->>Builder: safe_quote(params.run_uuid) -> --job-name
Builder->>Builder: safe_quote(params.slurm_logfile) -> --output
Builder->>Builder: safe_quote(params.comment_str) -> --comment
Builder->>Builder: handle clusters/runtime/constraint/qos with safe_quote
Builder->>Builder: choose mem-per-cpu or --mem fallback
Builder->>Builder: detect GPUs -> set ntasks-per-gpu or ntasks
Builder->>Builder: if params.workdir truthy -> safe_quote(params.workdir) -> -D
Builder-->>Caller: assembled sbatch command string
deactivate Builder
Caller->>SLURM: sbatch "<command>"
SLURM-->>Caller: submission result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 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: 5
🧹 Nitpick comments (2)
snakemake_executor_plugin_slurm/submit_string.py (2)
42-63: Consistent access pattern for resourcesYou mix attribute and .get access (e.g., check with get("clusters") then read job.resources.clusters). Prefer one style (suggest .get everywhere) to avoid surprises if resources lacks attributes.
32-33: Nit: typo in comment"accout" -> "account".
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
snakemake_executor_plugin_slurm/submit_string.py(4 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 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/submit_string.py
⏰ 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)
snakemake_executor_plugin_slurm/submit_string.py (1)
42-47: LGTM on quoting for clusters/timeGuarded and quoted correctly; equals/space choice is fine here.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/tests.py (1)
391-391: Unquoted constraint expectation is correct; also avoid appending 'None' for account/partition.LGTM on dropping quotes around the constraint. Minor: this test passes account/partition as None; current get_submit_command appends f" {params.account}" and f" {params.partition}", which would inject literal " None". Either set them to "" here or guard in get_submit_command.
Suggested guard in submit_string.py:
- call += f" {params.account}" + if params.account: + call += f" {params.account}" - call += f" {params.partition}" + if params.partition: + call += f" {params.partition}"
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
snakemake_executor_plugin_slurm/submit_string.py(4 hunks)tests/tests.py(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- snakemake_executor_plugin_slurm/submit_string.py
🧰 Additional context used
🧬 Code graph analysis (1)
tests/tests.py (1)
snakemake_executor_plugin_slurm/submit_string.py (1)
get_submit_command(14-91)
⏰ 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 (2)
tests/tests.py (2)
415-415: Unquoted qos expectation is correct.Matches the new quoting behavior.
442-443: Both flags together look good.Expectations for unquoted qos and constraint together are consistent.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
snakemake_executor_plugin_slurm/submit_string.py (1)
55-56: Reuse the fetched qos value to avoid attribute mismatch.
Functional no-op; improves consistency.- if job.resources.get("qos"): - call += f" --qos={safe_quote(job.resources.qos)}" + qos = job.resources.get("qos") + if qos: + call += f" --qos={safe_quote(qos)}"
🧹 Nitpick comments (4)
snakemake_executor_plugin_slurm/submit_string.py (4)
6-15: Clarify None handling in safe_quote’s docstring (behavior: "None" literal).
Current code converts None to the literal string "None". If that’s intended, make it explicit in the docstring to avoid misuse; otherwise, consider returning None and having callers gate emission. Minimal doc fix:def safe_quote(value): """ - Safely quote a parameter value using shlex.quote. - Handles None values and converts to string if needed. - Returns empty quotes for empty strings. + Safely quote a parameter value using shlex.quote. + None is converted to the literal string "None". + Returns "''" for empty strings; callers should gate options if empty/None should be omitted. """
38-42: Fix typos in comments for clarity.
Minor doc nits.- # "-A '{account_name}'" + # already contains "-A '{account_name}'" ... - # "- p '{partition_name}'" + # already contains "-p '{partition_name}'"
44-49: Avoid double access; reuse retrieved values.
Slight cleanup and consistent style with qos/workdir blocks.- if job.resources.get("clusters"): - call += f" --clusters {safe_quote(job.resources.clusters)}" + clusters = job.resources.get("clusters") + if clusters: + call += f" --clusters {safe_quote(clusters)}" - if job.resources.get("runtime"): - call += f" -t {safe_quote(job.resources.runtime)}" + runtime = job.resources.get("runtime") + if runtime: + call += f" -t {safe_quote(runtime)}"
50-53: Remove redundant isinstance() and duplicate get(); keep current empty-string behavior if required.
This preserves emitting-C ''when constraint is empty, but simplifies the predicate.- if job.resources.get("constraint") or isinstance( - job.resources.get("constraint"), str - ): - call += f" -C {safe_quote(job.resources.get('constraint'))}" + constraint = job.resources.get("constraint") + if constraint is not None: + call += f" -C {safe_quote(constraint)}"
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
snakemake_executor_plugin_slurm/submit_string.py(3 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 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/submit_string.py
📚 Learning: 2025-09-15T12:00:31.977Z
Learnt from: cmeesters
PR: snakemake/snakemake-executor-plugin-slurm#355
File: snakemake_executor_plugin_slurm/submit_string.py:23-30
Timestamp: 2025-09-15T12:00:31.977Z
Learning: In the snakemake-executor-plugin-slurm project, the parameters params.run_uuid, params.slurm_logfile, and params.comment_str are always provided and never None, so validation for None values is not necessary for these specific parameters in the get_submit_command function.
Applied to files:
snakemake_executor_plugin_slurm/submit_string.py
📚 Learning: 2025-09-15T12:06:07.556Z
Learnt from: cmeesters
PR: snakemake/snakemake-executor-plugin-slurm#355
File: snakemake_executor_plugin_slurm/submit_string.py:53-55
Timestamp: 2025-09-15T12:06:07.556Z
Learning: In the snakemake-executor-plugin-slurm project, the QoS parameter validation using `or isinstance(job.resources.get("qos"), str)` was identified as problematic because it would pass for empty strings and could cause issues with sbatch when generating "--qos=" with no value. The correct approach is to use truthiness checking with `if qos:` after getting the value with `job.resources.get("qos")`.
Applied to files:
snakemake_executor_plugin_slurm/submit_string.py
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: testing
🔇 Additional comments (3)
snakemake_executor_plugin_slurm/submit_string.py (3)
3-3: Import is correct and scoped.
Required for shell-safe quoting. No concerns.
26-31: LGTM: core sbatch prelude now safely quoted.
Unconditional emission is OK given run_uuid, slurm_logfile, and comment_str are always present (per retrieved learnings).
93-94: LGTM: guard -D to avoid stray “-D ”; quote workdir.
Matches the intended flexibility.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
snakemake_executor_plugin_slurm/submit_string.py (1)
50-61: Do not emit -C/--qos for empty strings (will produce “-C ''”/“--qos=''”)Empty values can confuse sbatch; prior feedback noted this. Gate on truthiness, not “is not None”.
- constraint = job.resources.get("constraint") - if constraint is not None: - call += f" -C {safe_quote(constraint)}" + constraint = job.resources.get("constraint") + if constraint: + call += f" -C {safe_quote(constraint)}" - qos = job.resources.get("qos") - if qos is not None: - call += f" --qos={safe_quote(qos)}" + qos = job.resources.get("qos") + if qos: + call += f" --qos={safe_quote(qos)}"Optional: add tests asserting no “-C ”/“--qos=” appears when values are empty.
🧹 Nitpick comments (3)
snakemake_executor_plugin_slurm/submit_string.py (3)
3-15: safe_quote: docstring and behavior mismatch on NoneCurrently, None becomes the literal string "None". Either special‑case None or clarify the docstring.
Option A (clarify docs):
def safe_quote(value): """ - Safely quote a parameter value using shlex.quote. - Handles None values and converts to string if needed. - Returns empty quotes for empty strings. + Safely quote a parameter value using shlex.quote. + Converts the value to string; None becomes the literal "None". + Returns "''" for empty strings. """Option B (propagate None; requires callers to gate before appending):
def safe_quote(value): - str_value = str(value) + if value is None: + return None + str_value = str(value) if str_value == "": return "''" return shlex.quote(str_value)
38-42: Fix typos in comments (-p and “account”)Minor cleanup to avoid confusion in future edits.
- # "-A '{account_name}'" + # "-A '{account_name}'" ... - # "- p '{partition_name}'" + # "-p '{partition_name}'"
44-49: Use the retrieved value; avoid mixing .get() and attribute accessRead once and reuse to prevent AttributeError on objects that don’t expose attributes for missing keys; also improves consistency.
- if job.resources.get("clusters"): - call += f" --clusters {safe_quote(job.resources.clusters)}" + clusters = job.resources.get("clusters") + if clusters: + call += f" --clusters {safe_quote(clusters)}" - if job.resources.get("runtime"): - call += f" -t {safe_quote(job.resources.runtime)}" + runtime = job.resources.get("runtime") + if runtime: + call += f" -t {safe_quote(runtime)}"
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
snakemake_executor_plugin_slurm/submit_string.py(3 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 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/submit_string.py
📚 Learning: 2025-09-15T12:00:31.977Z
Learnt from: cmeesters
PR: snakemake/snakemake-executor-plugin-slurm#355
File: snakemake_executor_plugin_slurm/submit_string.py:23-30
Timestamp: 2025-09-15T12:00:31.977Z
Learning: In the snakemake-executor-plugin-slurm project, the parameters params.run_uuid, params.slurm_logfile, and params.comment_str are always provided and never None, so validation for None values is not necessary for these specific parameters in the get_submit_command function.
Applied to files:
snakemake_executor_plugin_slurm/submit_string.py
📚 Learning: 2025-09-15T12:06:07.556Z
Learnt from: cmeesters
PR: snakemake/snakemake-executor-plugin-slurm#355
File: snakemake_executor_plugin_slurm/submit_string.py:53-55
Timestamp: 2025-09-15T12:06:07.556Z
Learning: In the snakemake-executor-plugin-slurm project, the QoS parameter validation using `or isinstance(job.resources.get("qos"), str)` was identified as problematic because it would pass for empty strings and could cause issues with sbatch when generating "--qos=" with no value. The correct approach is to use truthiness checking with `if qos:` after getting the value with `job.resources.get("qos")`.
Applied to files:
snakemake_executor_plugin_slurm/submit_string.py
⏰ 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 (2)
snakemake_executor_plugin_slurm/submit_string.py (2)
26-31: Good: centralized quoting for job-name/output/comment
97-98: Good: guard -D workdir and quote
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
snakemake_executor_plugin_slurm/submit_string.py (1)
50-61: Confirm intent: include -C '' / --qos='' when emptyUsing “is not None” emits empty-valued flags (e.g.,
--qos=''). If this is deliberate to represent “explicitly empty,” keep it. If not, switch to truthiness guards.Optional alternative:
-constraint = job.resources.get("constraint") -if constraint is not None: +constraint = job.resources.get("constraint") +if constraint: call += f" -C {safe_quote(constraint)}" -qos = job.resources.get("qos") -if qos is not None: +qos = job.resources.get("qos") +if qos: call += f" --qos={safe_quote(qos)}"
🧹 Nitpick comments (1)
snakemake_executor_plugin_slurm/submit_string.py (1)
38-42: Nit: fix typo in comment (-p)Minor typo in the partition comment.
- # "- p '{partition_name}'" + # "-p '{partition_name}'"
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
snakemake_executor_plugin_slurm/submit_string.py(3 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 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/submit_string.py
📚 Learning: 2025-09-15T12:00:31.977Z
Learnt from: cmeesters
PR: snakemake/snakemake-executor-plugin-slurm#355
File: snakemake_executor_plugin_slurm/submit_string.py:23-30
Timestamp: 2025-09-15T12:00:31.977Z
Learning: In the snakemake-executor-plugin-slurm project, the parameters params.run_uuid, params.slurm_logfile, and params.comment_str are always provided and never None, so validation for None values is not necessary for these specific parameters in the get_submit_command function.
Applied to files:
snakemake_executor_plugin_slurm/submit_string.py
📚 Learning: 2025-09-15T12:06:07.556Z
Learnt from: cmeesters
PR: snakemake/snakemake-executor-plugin-slurm#355
File: snakemake_executor_plugin_slurm/submit_string.py:53-55
Timestamp: 2025-09-15T12:06:07.556Z
Learning: In the snakemake-executor-plugin-slurm project, the QoS parameter validation using `or isinstance(job.resources.get("qos"), str)` was identified as problematic because it would pass for empty strings and could cause issues with sbatch when generating "--qos=" with no value. The correct approach is to use truthiness checking with `if qos:` after getting the value with `job.resources.get("qos")`.
Applied to files:
snakemake_executor_plugin_slurm/submit_string.py
⏰ 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)
snakemake_executor_plugin_slurm/submit_string.py (4)
26-31: LGTM: safer quoting for required flagsQuoting job-name/output/comment is correct and consistent.
45-49: LGTM: clusters/runtime quotingTruthiness gate plus quoting is fine.
97-98: LGTM: guard -D against empty workdirConditional emission avoids stray “-D ”.
3-15: Fix None handling/docstring mismatch in safe_quoteDocstring claims None is handled, but the function does str(None) -> "None", which will leak into sbatch args; rg shows at least one unguarded caller at snakemake_executor_plugin_slurm/submit_string.py:28 (params.run_uuid).
- Non‑breaking: update the docstring to remove "Handles None values" and require callers to gate None.
- Breaking (fail‑fast): make safe_quote raise on None and then audit/fix callers (run
rg -n -C2 'safe_quote\('to confirm).Apply this diff to fail fast:
-def safe_quote(value): +def safe_quote(value: object) -> str: """ - Safely quote a parameter value using shlex.quote. - Handles None values and converts to string if needed. - Returns empty quotes for empty strings. + Safely quote a parameter value using shlex.quote. + Requires non-None; callers must gate None before calling. + Returns empty quotes for empty strings. """ - str_value = str(value) + if value is None: + raise ValueError("safe_quote: value must not be None; gate before calling") + str_value = str(value) if str_value == "": return "''" return shlex.quote(str_value)
|
@johanneskoester ready for review - will merge into #350 , as this needs to play together. |
🤖 I have created a release *beep* *boop* --- ## [1.8.0](v1.7.0...v1.8.0) (2025-09-18) ### Features * adding image for mastodon posts ([#349](#349)) ([b27168c](b27168c)) ### Bug Fixes * account lookup / test in multicluster environment ([#350](#350)) ([d6759d0](d6759d0)) * quoting parameters ([#355](#355)) ([660c800](660c800)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Display images for Mastodon posts. * **Bug Fixes** * Improve account lookup reliability and tests in multi-cluster environments. * Correct handling of quoting parameters. * **Documentation** * Add 1.8.0 release notes to the changelog, summarizing new features and fixes. * **Chores** * Bump application version to 1.8.0. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
This PR should fix issue #354
Summary by CodeRabbit
Bug Fixes
Refactor
Tests