feat: add prefix to job uuid #330
Conversation
WalkthroughA new Changes
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
snakemake_executor_plugin_slurm/__init__.py(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
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#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.
snakemake_executor_plugin_slurm/__init__.py (9)
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#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.
Learnt from: cmeesters
PR: snakemake/snakemake-executor-plugin-slurm#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: 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#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
PR: snakemake/snakemake-executor-plugin-slurm#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.
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.
Learnt from: cmeesters
PR: snakemake/snakemake-executor-plugin-slurm#173
File: snakemake_executor_plugin_slurm/__init__.py:29-31
Timestamp: 2025-02-18T14:53:09.504Z
Learning: The snakemake-executor-plugin-slurm tests require system dependencies (gcc, python3-dev) to build the datrie package, which is a dependency of snakemake.
Learnt from: cmeesters
PR: snakemake/snakemake-executor-plugin-slurm#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.
⏰ 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: title-format
| prefix: str = field( | ||
| default="", | ||
| metadata={ | ||
| "help": "Prefix that is added to the job names", | ||
| "env_var": False, | ||
| "required": False | ||
| } | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add input validation for the prefix field.
The prefix field lacks validation for SLURM-safe characters and length limits. Since this prefix will be used in job names and shell commands (e.g., sacct --name), invalid characters could cause command failures.
Consider adding validation in the field definition or in the __post_init__ method:
prefix: str = field(
default="",
metadata={
- "help": "Prefix that is added to the job names",
+ "help": "Prefix that is added to the job names. Must contain only alphanumeric characters, hyphens, and underscores. Maximum length recommended is 50 characters.",
"env_var": False,
"required": False
}
)Additionally, implement validation logic to ensure the prefix contains only SLURM-safe characters (alphanumeric, hyphens, underscores) and doesn't exceed reasonable length limits.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| prefix: str = field( | |
| default="", | |
| metadata={ | |
| "help": "Prefix that is added to the job names", | |
| "env_var": False, | |
| "required": False | |
| } | |
| ) | |
| prefix: str = field( | |
| default="", | |
| metadata={ | |
| "help": "Prefix that is added to the job names. Must contain only alphanumeric characters, hyphens, and underscores. Maximum length recommended is 50 characters.", | |
| "env_var": False, | |
| "required": False | |
| } | |
| ) |
🤖 Prompt for AI Agents
In snakemake_executor_plugin_slurm/__init__.py around lines 151 to 158, the
prefix field lacks validation for allowed SLURM-safe characters and length
constraints. Add validation logic either in the field definition or in the
class's __post_init__ method to check that the prefix contains only alphanumeric
characters, hyphens, and underscores, and that its length does not exceed a
reasonable limit. Raise a ValueError if the validation fails to prevent invalid
prefixes from being used in job names and shell commands.
There was a problem hiding this comment.
@cmeesters Thank you for confirming! You're absolutely right that input validation for the prefix field is important to prevent potential issues with SLURM job submission and status checking.
Would you like me to implement the validation logic for the prefix field? I can add proper validation that:
- Ensures the prefix contains only alphanumeric characters, hyphens, and underscores
- Enforces a reasonable length limit (e.g., 50 characters)
- Raises a
ValueErrorwith a clear message if validation fails
This validation could be added either in the ExecutorSettings.__post_init__() method or in the Executor.__post_init__() method. Let me know if you'd like me to proceed with implementing this validation logic!
🧠 Learnings used
Learnt from: cmeesters
PR: snakemake/snakemake-executor-plugin-slurm#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
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#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.
Learnt from: cmeesters
PR: snakemake/snakemake-executor-plugin-slurm#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
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.
Learnt from: cmeesters
PR: snakemake/snakemake-executor-plugin-slurm#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
PR: snakemake/snakemake-executor-plugin-slurm#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.
| @@ -181,7 +189,7 @@ def __post_init__(self, test_mode: bool = False): | |||
| # run check whether we are running in a SLURM job context | |||
| self.warn_on_jobcontext() | |||
| self.test_mode = test_mode | |||
| self.run_uuid = str(uuid.uuid4()) | |||
| self.run_uuid = self.workflow.executor_settings.prefix + str(uuid.uuid4()) | |||
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Validate prefix before concatenation to prevent command injection.
The direct concatenation of prefix and UUID could introduce security and functionality issues if the prefix contains special characters that break shell commands or exceed SLURM naming constraints.
The prefixed UUID is used in shell commands (line 467: sacct --name {self.run_uuid}). Verify this doesn't break with special characters:
Apply this diff to add basic validation:
def __post_init__(self, test_mode: bool = False):
# run check whether we are running in a SLURM job context
self.warn_on_jobcontext()
self.test_mode = test_mode
+ # Validate prefix for SLURM-safe characters
+ if self.workflow.executor_settings.prefix:
+ import re
+ if not re.match(r'^[a-zA-Z0-9_-]*$', self.workflow.executor_settings.prefix):
+ raise WorkflowError(
+ f"Invalid prefix '{self.workflow.executor_settings.prefix}'. "
+ "Prefix must contain only alphanumeric characters, hyphens, and underscores."
+ )
+ if len(self.workflow.executor_settings.prefix) > 50:
+ raise WorkflowError(
+ f"Prefix '{self.workflow.executor_settings.prefix}' is too long. "
+ "Maximum length is 50 characters."
+ )
self.run_uuid = self.workflow.executor_settings.prefix + str(uuid.uuid4())🏁 Script executed:
#!/bin/bash
# Check for potential issues with special characters in SLURM job names
# Test common problematic characters that could break shell commands
echo "Testing potential problematic characters in SLURM job names:"
echo "Characters that could cause issues: \$, \`, \", ', |, ;, &, >, <, (, ), {, }, [, ], *, ?, \\, space"
# Check SLURM documentation for job name constraints
echo "Checking SLURM job name constraints..."
scontrol show config | grep -i name || echo "Could not retrieve SLURM config"
# Test if there are any length limits
echo "Checking typical SLURM job name length limits..."
man sbatch | grep -A 5 -B 5 "job-name" || echo "Could not retrieve sbatch manual"Length of output: 948
🏁 Script executed:
#!/bin/bash
set -e
echo "== Searching for ‘sacct’ usage =="
rg -n "sacct" .
echo -e "\n== Searching for ‘run_uuid’ usage =="
rg -n "run_uuid" .Length of output: 9846
🏁 Script executed:
#!/bin/bash
set -e
echo "== Context around `--job-name` usage in submit_string.py =="
rg -n "--job-name" -A3 -B3 snakemake_executor_plugin_slurm/submit_string.pyLength of output: 717
🏁 Script executed:
#!/bin/bash
set -e
echo "== Searching for `executor_settings` definition =="
rg -n "class .*Settings" -g "snakemake_executor_plugin_slurm/*.py"
echo -e "\n== Searching for prefix usage in executor_settings =="
rg -n "prefix" -g "snakemake_executor_plugin_slurm/*.py"
echo -e "\n== Searching for where executor_settings.prefix is set =="
rg -n "prefix" -n "executor_settings" -g "snakemake_executor_plugin_slurm/*.py"Length of output: 1023
🏁 Script executed:
#!/bin/bash
set -e
echo "== Searching for subprocess usage =="
rg -n "subprocess\." -n 5 snakemake_executor_plugin_slurm/__init__.pyLength of output: 2623
🏁 Script executed:
#!/bin/bash
set -e
echo "== First 200 lines of submit_string.py =="
sed -n '1,200p' snakemake_executor_plugin_slurm/submit_string.pyLength of output: 2649
🏁 Script executed:
#!/bin/bash
set -e
echo "== Displaying lines 350-430 of __init__.py for sbatch invocation context =="
sed -n '350,430p' snakemake_executor_plugin_slurm/__init__.pyLength of output: 3396
Enforce SLURM-safe prefix in ExecutorSettings to prevent shell injection
The run_uuid (built as prefix + UUID) is interpolated unquoted into shell commands—both in the sbatch --job-name call (submit_string.py) and in sacct --name={run_uuid} (efficiency_report.py and status checks). If a malicious or malformed prefix contains characters like `;|&`` it can break the command or lead to injection.
Please add validation in ExecutorSettings.__post_init__ (snakemake_executor_plugin_slurm/init.py around line 192):
def __post_init__(self, test_mode: bool = False):
# run check whether we are running in a SLURM job context
self.warn_on_jobcontext()
self.test_mode = test_mode
+ # Validate prefix: only allow alphanumeric, hyphens, underscores; cap length
+ prefix = self.workflow.executor_settings.prefix
+ if prefix:
+ import re
+ if not re.fullmatch(r'[A-Za-z0-9_-]+', prefix):
+ raise WorkflowError(
+ f"Invalid prefix '{prefix}'. "
+ "Allowed characters are letters, digits, hyphens, and underscores."
+ )
+ if len(prefix) > 50:
+ raise WorkflowError(
+ f"Prefix '{prefix}' is too long ({len(prefix)} > 50)."
+ )
self.run_uuid = prefix + str(uuid.uuid4())- File:
snakemake_executor_plugin_slurm/__init__.py - Lines: ~190–195 (around
__post_init__)
This ensures run_uuid is safe wherever it’s passed to the shell.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| self.run_uuid = self.workflow.executor_settings.prefix + str(uuid.uuid4()) | |
| def __post_init__(self, test_mode: bool = False): | |
| # run check whether we are running in a SLURM job context | |
| self.warn_on_jobcontext() | |
| self.test_mode = test_mode | |
| # Validate prefix: only allow alphanumeric, hyphens, underscores; cap length | |
| prefix = self.workflow.executor_settings.prefix | |
| if prefix: | |
| import re | |
| if not re.fullmatch(r'[A-Za-z0-9_-]+', prefix): | |
| raise WorkflowError( | |
| f"Invalid prefix '{prefix}'. " | |
| "Allowed characters are letters, digits, hyphens, and underscores." | |
| ) | |
| if len(prefix) > 50: | |
| raise WorkflowError( | |
| f"Prefix '{prefix}' is too long ({len(prefix)} > 50)." | |
| ) | |
| self.run_uuid = prefix + str(uuid.uuid4()) |
🤖 Prompt for AI Agents
In snakemake_executor_plugin_slurm/__init__.py around lines 190 to 195, add
validation in the ExecutorSettings.__post_init__ method to ensure the prefix
used for run_uuid contains only safe characters (e.g., alphanumeric,
underscores, and hyphens) to prevent shell injection. Implement a check that
raises an error or sanitizes the prefix if it contains any unsafe characters
like ;|&` and ensure this validation runs before run_uuid is constructed and
used in shell commands.
|
@sebhoerl thank you for your input! I have been on holidays - please have a look at my comments. |
| @@ -148,6 +148,14 @@ class ExecutorSettings(ExecutorSettingsBase): | |||
| "required": False, | |||
| }, | |||
| ) | |||
| prefix: str = field( | |||
There was a problem hiding this comment.
"prefix" is a bit too generic. How about naming the purpose, e.g. "prefix_uuid"?
| prefix: str = field( | ||
| default="", | ||
| metadata={ | ||
| "help": "Prefix that is added to the job names", | ||
| "env_var": False, | ||
| "required": False | ||
| } | ||
| ) |
| @@ -181,7 +189,7 @@ def __post_init__(self, test_mode: bool = False): | |||
| # run check whether we are running in a SLURM job context | |||
| self.warn_on_jobcontext() | |||
| self.test_mode = test_mode | |||
| self.run_uuid = str(uuid.uuid4()) | |||
| self.run_uuid = self.workflow.executor_settings.prefix + str(uuid.uuid4()) | |||
There was a problem hiding this comment.
there is no separator - even if the default is "", we might want to see a separator.
There was a problem hiding this comment.
also, there is no validation.
| @@ -148,6 +148,14 @@ class ExecutorSettings(ExecutorSettingsBase): | |||
| "required": False, | |||
| }, | |||
| ) | |||
| prefix: str = field( | |||
There was a problem hiding this comment.
the type should be Optional[str], right?
|
I'll maybe find some time to look into the fixes this week, thanks! About the motivation: We have a SLURM cluster in our research institute. People use either the command line interface or a web interface to look at the list of queued and ongoing stops. They are wondering what these dozens of jobs are that simply have some hash as name :) - so we would like to add a prefix to the names to make it clear at one look what project those jobs belong to. And we now have multiple projects using snakemake pipelines, so it will also make it easier for us to distinguish which job belongs to which project, and it also makes it easier to kill all jobs with the same prefix at once. Let me know if this is of general interest, then I can work on the fixes. Otherwise, I'm not even sure if this is the best approach, because I have the impression that the names are not really important for the plugin, since it mainly acts on the job IDs. So another option could be to have a simple |
|
Ah, so the rule name is stored in the SLURM comment section. So, instead of
A workflow, when cancelled, ought to cancel its jobs. So it is sufficient to cancel the master process. If desired, a Actually, in the aftermath of the Hackathon this spring, we reworked the documentation. Perhaps it still requires some clarification in this regard? Pointers or PRs are welcome!
If you look at the number of PRs, you will realize that I/we have some kind of backlog already. Holiday season, stress, teaching obligations and cluster out of operation are taking some toll. So, there is no pressure at all. |
This PR allows to add a prefix to the job UUID that is shown on SLURM.
--slurm-prefix my_prefix_slurm-prefix: my_prefix_