refactor: first step to outsource query command #339
Conversation
WalkthroughAdds SLURM status-query utilities and autodetection; extends ExecutorSettings with status_command, status_attempts and efficiency-report options; adds validation hooks; implements runtime selection/fallback between sacct and squeue; improves log cleanup and shutdown/report generation; tooling/formatting updates. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User
participant Executor
participant QueryUtils as job_status_query
participant SLURM
User->>Executor: instantiate Executor(settings)
Executor->>QueryUtils: is_query_tool_available("sacct")
Executor->>QueryUtils: is_query_tool_available("squeue")
Executor->>QueryUtils: get_min_job_age()
Executor->>Executor: _get_status_command_default() / _validate_status_command_settings()
loop status polling
Executor->>Executor: get_status_command()
alt resolved -> sacct
Executor->>QueryUtils: query_job_status_sacct(runid)
Executor->>SLURM: run sacct cmd
else resolved -> squeue
Executor->>QueryUtils: query_job_status_squeue(runid)
Executor->>SLURM: run squeue cmd
end
SLURM-->>Executor: status results
Executor->>Executor: retry / fallback based on status_attempts and heuristics
end
sequenceDiagram
autonumber
participant Executor
participant FS as Filesystem
Executor->>Executor: clean_old_logs()
Executor->>FS: delete old log files and clean empty dirs
alt efficiency_report enabled
Executor->>FS: write efficiency report to efficiency_report_path (if threshold met)
end
Executor->>Executor: call super().shutdown()
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas needing extra attention:
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (7)
snakemake_executor_plugin_slurm/job_status_query.py (5)
42-49: Defensive: support minute units (if present) instead of silently assuming seconds.If MinJobAge is set like “5min”, we currently misinterpret it as 5 seconds.
Apply this diff:
- if unit in ("h", "hour", "hours"): + if unit in ("h", "hour", "hours"): return value * 3600 - elif unit in ("s", "sec", "secs", "second", "seconds", ""): + elif unit in ("m", "min", "mins", "minute", "minutes"): + return value * 60 + elif unit in ("s", "sec", "secs", "second", "seconds", ""): return value else: # Unknown unit, assume seconds return value
56-67: Use shutil.which instead of spawning a subprocess.This avoids S603 concerns and is faster/portable.
Apply this diff:
+import shutil @@ -def is_query_tool_available(tool_name): +def is_query_tool_available(tool_name): """ - Check if the sacct command is available on the system. - Returns True if sacct is available, False otherwise. + Check if a given command is available on the system. + Returns True if available, False otherwise. """ - cmd = f"which {tool_name}" - cmd = shlex.split(cmd) - try: - subprocess.check_output(cmd, stderr=subprocess.PIPE) - return True - except subprocess.CalledProcessError: - return False + return shutil.which(tool_name) is not None
70-98: Clarify function intent and docstring.Name says “recommend”, docstring says “available”. Keep it consistent: the function returns True when squeue is recommended (MinJobAge >= threshold). No code change needed; just update the docstring.
Apply this diff:
def should_recommend_squeue_status_command(min_threshold_seconds=120): """ - Determine if the status query with squeue should be recommended based on - the MinJobAge configuration (if very low, squeue might not work well) + Determine if squeue should be recommended based on MinJobAge. + If MinJobAge >= min_threshold_seconds, squeue is considered reliable. + Otherwise prefer sacct. @@ - Returns True if the option should be available, False otherwise. + Returns True if squeue is recommended, False otherwise. """
100-111: Fix return type and docstring; build a safe, readable sacct command.The function returns a string, not a list; the docstring mentions unrelated args. Also avoid manual split-join; build tokens then shlex.join for readability.
Apply this diff:
-def query_job_status_sacct(jobuid) -> list: +def query_job_status_sacct(jobuid) -> str: """ - Query job status using sacct command + Build a sacct command to query job status for a given workflow run UUID. @@ - job_ids: List of SLURM job IDs - timeout: Timeout in seconds for subprocess call + jobuid: Workflow run UUID used as SLURM job name @@ - Dictionary mapping job ID to JobStatus object + Command string suitable for subprocess with shell=True """ @@ - # -X: only show main job, no substeps - query_command = f"""sacct -X --parsable2 \ - --clusters all \ - --noheader --format=JobIdRaw,State \ - --starttime {sacct_starttime} \ - --endtime now --name {jobuid}""" - - # for better redability in verbose output - query_command = " ".join(shlex.split(query_command)) - - return query_command + # -X: only show main job, no substeps + cmd = [ + "sacct", + "-X", + "--parsable2", + "--clusters", + "all", + "--noheader", + "--format=JobIdRaw,State", + "--starttime", + sacct_starttime, + "--endtime", + "now", + "--name", + str(jobuid), + ] + # readable one-liner for logs while keeping quoting correct + return shlex.join(cmd)Also applies to: 119-129
132-154: Unify return type with sacct variant; return a string via shlex.join.Keeps consumers consistent and improves logging readability.
Apply this diff:
-def query_job_status_squeue(jobuid) -> list: +def query_job_status_squeue(jobuid) -> str: """ - Query job status using squeue command (newer SLURM functionality) + Build an squeue command to query job status (newer SLURM functionality). @@ - Returns: - Dictionary mapping job ID to JobStatus object + Returns: + Command string suitable for subprocess with shell=True @@ - query_command = [ + cmd = [ "squeue", "--format=%i|%T", "--states=all", "--noheader", "--name", - f"{jobuid}" + str(jobuid), ] - - return query_command + return shlex.join(cmd)snakemake_executor_plugin_slurm/__init__.py (2)
601-610: De‑duplicate: build the sacct command via the new helper.Use query_job_status_sacct(self.run_uuid) to avoid drift between implementations.
Apply this diff (and import the helper):
-from .job_status_query import ( - get_min_job_age, - is_query_tool_available, - should_recommend_squeue_status_command, -) +from .job_status_query import ( + get_min_job_age, + is_query_tool_available, + should_recommend_squeue_status_command, + query_job_status_sacct, +) @@ - # -X: only show main job, no substeps - sacct_command = f"""sacct -X --parsable2 \ - --clusters all \ - --noheader --format=JobIdRaw,State \ - --starttime {sacct_starttime} \ - --endtime now --name {self.run_uuid}""" - - # for better redability in verbose output - sacct_command = " ".join(shlex.split(sacct_command)) + # Build via helper (one-liner string for logging) + sacct_command = query_job_status_sacct(self.run_uuid)
81-84: Nit: extra space and wording in help message.“(MinJobAge is sufficient )” contains a stray space; also prefer “sufficient for reliable usage”.
Apply this diff:
- msg_part1 = "'squeue' recommended (MinJobAge is sufficient )" - msg_part2 = " for reliable usage" + msg_part1 = "'squeue' recommended (MinJobAge is sufficient" + msg_part2 = ") for reliable usage"
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
snakemake_executor_plugin_slurm/__init__.py(8 hunks)snakemake_executor_plugin_slurm/job_status_query.py(1 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.
📚 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/job_status_query.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/job_status_query.py (3)
get_min_job_age(7-53)is_query_tool_available(56-67)should_recommend_squeue_status_command(70-97)
🪛 Ruff (0.12.2)
snakemake_executor_plugin_slurm/job_status_query.py
16-16: subprocess call: check for execution of untrusted input
(S603)
64-64: subprocess call: check for execution of untrusted input
(S603)
65-65: Consider moving this statement to an else block
(TRY300)
snakemake_executor_plugin_slurm/__init__.py
52-55: Avoid specifying long messages outside the exception class
(TRY003)
320-320: String contains ambiguous × (MULTIPLICATION SIGN). Did you mean x (LATIN SMALL LETTER X)?
(RUF001)
329-329: String contains ambiguous × (MULTIPLICATION SIGN). Did you mean x (LATIN SMALL LETTER X)?
(RUF001)
⏰ 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/__init__.py (2)
45-60: Default selection logic looks good.sacct preferred, squeue as fallback, and hard-fail if neither is present makes sense.
583-587: status_command validated but not wired into status checksget_status_command exists and job_status_query implements both sacct and squeue, but active-job checks still appear to use sacct only — either wire get_status_command into check_active_jobs (use squeue when selected) or hide the status_command option behind a feature flag. To confirm, run:
rg -nP -C2 '\bcheck_active_jobs\b' snakemake_executor_plugin_slurm
rg -nP -C2 '\bquery_job_status_sacct\b|\bquery_job_status_squeue\b' snakemake_executor_plugin_slurm
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
snakemake_executor_plugin_slurm/job_status_query.py (1)
13-18: Harden scontrol probe: add timeout and catch FileNotFoundError/TimeoutExpired.Prevents hangs and handles missing binaries gracefully.
Apply:
- output = subprocess.check_output(cmd, text=True, stderr=subprocess.PIPE) - except subprocess.CalledProcessError: + output = subprocess.check_output( + cmd, text=True, stderr=subprocess.PIPE, timeout=10 + ) + except (subprocess.CalledProcessError, FileNotFoundError, subprocess.TimeoutExpired): return Nonesnakemake_executor_plugin_slurm/__init__.py (2)
67-105: Make help static to avoid external calls at import time.Prevents import-time scontrol/sacct probes and potential hangs.
Apply:
def _get_status_command_help(): - """Get help text with computed default.""" - default_cmd = _get_status_command_default() - sacct_available = is_query_tool_available("sacct") - squeue_recommended = should_recommend_squeue_status_command() - - base_help = "Command to query job status. Options: 'sacct', 'squeue'. " - - if default_cmd == "sacct": - if sacct_available and not squeue_recommended: - info = ( - "'sacct' detected and will be used " - "(MinJobAge may be too low for reliable 'squeue' usage)" - ) - else: - info = "'sacct' detected and will be used" - else: # default_cmd == "squeue" - if squeue_recommended: - # cumbersome, due to black and the need to stay below 80 chars - msg_part1 = "'squeue' recommended (MinJobAge is sufficient )" - msg_part2 = " for reliable usage" - info = msg_part1 + msg_part2 - elif not sacct_available: - info = ( - "'sacct' not available, falling back to 'squeue'. " - "WARNING: 'squeue' may not work reliably if MinJobAge is " - "too low" - ) - else: - info = ( - "'squeue' will be used. " - "WARNING: MinJobAge may be too low for reliable 'squeue' usage" - ) - - return ( - f"{base_help}Default: '{default_cmd}' ({info}). " - f"Set explicitly to override auto-detection." - ) + """Help: job status command.""" + return ( + "Command to query job status. Options: 'sacct' (preferred), 'squeue'. " + "Default is chosen dynamically: 'sacct' if available, otherwise 'squeue'. " + "Note: 'squeue' reliability depends on MinJobAge; see plugin docs." + )
302-354: Fix MinJobAge logic and threshold source; replace ambiguous ×.MinJobAge gates squeue visibility, not sacct; derive threshold from init_seconds_before_status_checks.
Apply:
- # Calculate dynamic threshold: 3 times the initial - # status check interval. - # The plugin starts with 40 seconds and increases, - # so we use (3 * 10) + 40 = 70 seconds as minimum - between_status_check_seconds = getattr( - self.workflow.executor_settings, - "seconds_between_status_checks", - 70, - ) - dynamic_check_threshold = 3 * between_status_check_seconds + # Threshold: 3x initial status check interval (default 40s) + initial_interval = getattr( + self.workflow.executor_settings, + "init_seconds_before_status_checks", + 40, + ) + dynamic_check_threshold = 3 * initial_interval @@ - elif sacct_available and min_job_age is not None: - if ( - min_job_age < dynamic_check_threshold - and status_command == "sacct" - ): - self.logger.warning( - f"MinJobAge is {min_job_age} seconds " - f"(< {dynamic_check_threshold}s). " - f"This may cause 'sacct' to report inaccurate " - "job states and the status_command option may " - "be unreliable. " - "(Threshold is 3x status check interval: 3 × " - f"{between_status_check_seconds}s = " - f"{dynamic_check_threshold}s)" - ) - elif ( - min_job_age >= dynamic_check_threshold - and status_command == "squeue" - ): - self.logger.warning( - f"MinJobAge is {min_job_age} seconds (>= " - f"{dynamic_check_threshold}s). " - f"The 'squeue' command should work reliably for " - "status queries. " - "(Threshold is 3x status check interval: 3 × " - f"{between_status_check_seconds}s = " - f"{dynamic_check_threshold}s)" - ) + elif sacct_available and min_job_age is not None: + if min_job_age < dynamic_check_threshold and status_command == "squeue": + self.logger.warning( + f"MinJobAge is {min_job_age} seconds (< {dynamic_check_threshold}s). " + "This may cause 'squeue' to miss recently finished jobs; consider using 'sacct'. " + f"(Threshold is 3x initial interval: 3 x {initial_interval}s = {dynamic_check_threshold}s)" + ) + elif min_job_age >= dynamic_check_threshold and status_command == "sacct": + self.logger.warning( + f"MinJobAge is {min_job_age} seconds (>= {dynamic_check_threshold}s). " + "The 'squeue' command should also work reliably for status queries. " + f"(Threshold is 3x initial interval: 3 x {initial_interval}s = {dynamic_check_threshold}s)" + )Run to confirm no stray uses of the removed attribute:
#!/bin/bash rg -nP -C2 '\bseconds_between_status_checks\b|\binit_seconds_before_status_checks\b'
🧹 Nitpick comments (7)
snakemake_executor_plugin_slurm/job_status_query.py (4)
56-67: Use shutil.which instead of spawning 'which'.Faster, portable, and avoids S603 concerns; also fix docstring.
Apply:
def is_query_tool_available(tool_name): - """ - Check if the sacct command is available on the system. - Returns True if sacct is available, False otherwise. - """ - cmd = f"which {tool_name}" - cmd = shlex.split(cmd) - try: - subprocess.check_output(cmd, stderr=subprocess.PIPE) - return True - except subprocess.CalledProcessError: - return False + """ + Check if a given CLI tool is available on PATH. + """ + return shutil.which(tool_name) is not NoneAdd import:
+import shutil
97-127: Fix return type/docstring of query_job_status_sacct (returns str, not list/dict).Align signature, doc, and behavior.
Apply:
-def query_job_status_sacct(jobuid) -> list: +def query_job_status_sacct(jobuid) -> str: """ - Query job status using sacct command + Build the sacct command used to query job status for a given workflow UUID. Args: - job_ids: List of SLURM job IDs - timeout: Timeout in seconds for subprocess call + jobuid: Workflow UUID used as job name filter. Returns: - Dictionary mapping job ID to JobStatus object + str: command string to pass to subprocess """
129-151: Unify squeue builder to return a command string (consistent with sacct path).Ensures compatibility with job_stati(), which expects a string.
Apply:
-def query_job_status_squeue(jobuid) -> list: +def query_job_status_squeue(jobuid) -> str: """ - Query job status using squeue command (newer SLURM functionality) + Build the squeue command used to query job status for a given workflow UUID. @@ - Returns: - Dictionary mapping job ID to JobStatus object + Returns: + str: command string to pass to subprocess @@ - query_command = [ + query_command = [ "squeue", "--format=%i|%T", "--states=all", "--noheader", "--name", f"{jobuid}", ] - return query_command + return shlex.join(query_command)
123-124: Typo.“redability” → “readability”.
Apply:
- # for better redability in verbose output + # for better readability in verbose outputsnakemake_executor_plugin_slurm/__init__.py (3)
355-363: Return default when status_command is None.Avoids propagating None.
Apply:
def get_status_command(self): """Get the status command to use, with fallback logic.""" if hasattr(self.workflow.executor_settings, "status_command"): - return self.workflow.executor_settings.status_command + value = self.workflow.executor_settings.status_command + return value or _get_status_command_default() else: # Fallback: determine the best command based on # cluster configuration return _get_status_command_default()
386-395: Guard against None for delete_logfiles_older_than.Make cleanup robust to explicit None.
Apply:
- if age_cutoff <= 0 or keep_all: + if age_cutoff is None or age_cutoff <= 0 or keep_all: return
377-384: Don't let efficiency report failures abort shutdown.Wrap to log and continue.
Apply:
- if self.workflow.executor_settings.efficiency_report: - threshold = self.workflow.executor_settings.efficiency_threshold - report_path = self.workflow.executor_settings.efficiency_report_path - create_efficiency_report( - e_threshold=threshold, - run_uuid=self.run_uuid, - e_report_path=report_path, - logger=self.logger, - ) + if self.workflow.executor_settings.efficiency_report: + threshold = self.workflow.executor_settings.efficiency_threshold + report_path = self.workflow.executor_settings.efficiency_report_path + try: + create_efficiency_report( + e_threshold=threshold, + run_uuid=self.run_uuid, + e_report_path=report_path, + logger=self.logger, + ) + except Exception as e: # best-effort + self.logger.warning(f"Efficiency report failed: {e}")
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
pyproject.toml(1 hunks)snakemake_executor_plugin_slurm/__init__.py(19 hunks)snakemake_executor_plugin_slurm/efficiency_report.py(1 hunks)snakemake_executor_plugin_slurm/job_status_query.py(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- pyproject.toml
- snakemake_executor_plugin_slurm/efficiency_report.py
🧰 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.
📚 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/job_status_query.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/job_status_query.py
🧬 Code graph analysis (1)
snakemake_executor_plugin_slurm/__init__.py (2)
snakemake_executor_plugin_slurm/job_status_query.py (3)
get_min_job_age(7-53)is_query_tool_available(56-67)should_recommend_squeue_status_command(70-94)snakemake_executor_plugin_slurm/efficiency_report.py (1)
create_efficiency_report(190-219)
🪛 Ruff (0.12.2)
snakemake_executor_plugin_slurm/__init__.py
56-60: Avoid specifying long messages outside the exception class
(TRY003)
337-337: String contains ambiguous × (MULTIPLICATION SIGN). Did you mean x (LATIN SMALL LETTER X)?
(RUF001)
350-350: String contains ambiguous × (MULTIPLICATION SIGN). Did you mean x (LATIN SMALL LETTER X)?
(RUF001)
566-566: Avoid specifying long messages outside the exception class
(TRY003)
snakemake_executor_plugin_slurm/job_status_query.py
16-16: subprocess call: check for execution of untrusted input
(S603)
64-64: subprocess call: check for execution of untrusted input
(S603)
65-65: Consider moving this statement to an else block
(TRY300)
⏰ 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
There was a problem hiding this comment.
Actionable comments posted: 0
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)
745-747: Fix path handling for aux_logs (Path has no _str).Use str(path) to avoid AttributeError during error reporting.
- 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"])] + )
♻️ Duplicate comments (4)
snakemake_executor_plugin_slurm/__init__.py (3)
66-104: Make help text static; avoid external checks at import time.This prevents scontrol/sacct/squeue probes during import. Prior review asked for this.
-def _get_status_command_help(): - """Get help text with computed default.""" - default_cmd = _get_status_command_default() - sacct_available = is_query_tool_available("sacct") - squeue_recommended = should_recommend_squeue_status_command() - - base_help = "Command to query job status. Options: 'sacct', 'squeue'. " - - if default_cmd == "sacct": - if sacct_available and not squeue_recommended: - info = ( - "'sacct' detected and will be used " - "(MinJobAge may be too low for reliable 'squeue' usage)" - ) - else: - info = "'sacct' detected and will be used" - else: # default_cmd == "squeue" - if squeue_recommended: - # cumbersome, due to black and the need to stay below 80 chars - msg_part1 = "'squeue' recommended (MinJobAge is sufficient )" - msg_part2 = " for reliable usage" - info = msg_part1 + msg_part2 - elif not sacct_available: - info = ( - "'sacct' not available, falling back to 'squeue'. " - "WARNING: 'squeue' may not work reliably if MinJobAge is " - "too low" - ) - else: - info = ( - "'squeue' will be used. " - "WARNING: MinJobAge may be too low for reliable 'squeue' usage" - ) - - return ( - f"{base_help}Default: '{default_cmd}' ({info}). " - f"Set explicitly to override auto-detection." - ) +def _get_status_command_help(): + """Help: job status command.""" + return ( + "Command to query job status. Options: 'sacct' (preferred), 'squeue'. " + "Default is chosen dynamically at runtime: 'sacct' if available, otherwise 'squeue'. " + "Note: 'squeue' reliability depends on MinJobAge; see plugin docs." + )
210-217: Avoid calling a function in dataclass metadata; inline the static help string.Even though the helper is now pure, inlining avoids any import-time call.
- status_command: Optional[str] = field( - default_factory=_get_status_command_default, - metadata={ - "help": _get_status_command_help(), - "env_var": False, - "required": False, - }, - ) + status_command: Optional[str] = field( + default_factory=_get_status_command_default, + metadata={ + "help": ( + "Command to query job status. Options: 'sacct' (preferred), 'squeue'. " + "Default is chosen dynamically at runtime: 'sacct' if available, otherwise 'squeue'. " + "Note: 'squeue' reliability depends on MinJobAge; see plugin docs." + ), + "env_var": False, + "required": False, + }, + )
301-353: Fix threshold source and inverted warnings in _validate_status_command_settings.Use init_seconds_before_status_checks (default 40s) and warn about squeue when MinJobAge is low; replace × with x. This repeats earlier guidance.
- # Calculate dynamic threshold: 3 times the initial - # status check interval. - # The plugin starts with 40 seconds and increases, - # so we use (3 * 10) + 40 = 70 seconds as minimum - between_status_check_seconds = getattr( - self.workflow.executor_settings, - "seconds_between_status_checks", - 70, - ) - dynamic_check_threshold = 3 * between_status_check_seconds + # Threshold: 3x initial status check interval (default 40s) + initial_interval = getattr( + self.workflow.executor_settings, + "init_seconds_before_status_checks", + 40, + ) + dynamic_check_threshold = 3 * initial_interval @@ - if ( - min_job_age < dynamic_check_threshold - and status_command == "sacct" - ): - self.logger.warning( - f"MinJobAge is {min_job_age} seconds " - f"(< {dynamic_check_threshold}s). " - f"This may cause 'sacct' to report inaccurate " - "job states and the status_command option may " - "be unreliable. " - "(Threshold is 3x status check interval: 3 × " - f"{between_status_check_seconds}s = " - f"{dynamic_check_threshold}s)" - ) - elif ( - min_job_age >= dynamic_check_threshold - and status_command == "squeue" - ): - self.logger.warning( - f"MinJobAge is {min_job_age} seconds (>= " - f"{dynamic_check_threshold}s). " - f"The 'squeue' command should work reliably for " - "status queries. " - "(Threshold is 3x status check interval: 3 × " - f"{between_status_check_seconds}s = " - f"{dynamic_check_threshold}s)" - ) + if min_job_age < dynamic_check_threshold and status_command == "squeue": + self.logger.warning( + f"MinJobAge is {min_job_age} seconds (< {dynamic_check_threshold}s). " + "This may cause 'squeue' to miss recently finished jobs; consider using 'sacct'. " + f"(Threshold is 3x initial interval: 3 x {initial_interval}s = {dynamic_check_threshold}s)" + ) + elif min_job_age >= dynamic_check_threshold and status_command == "sacct": + self.logger.warning( + f"MinJobAge is {min_job_age} seconds (>= {dynamic_check_threshold}s). " + "The 'squeue' command should also work reliably for status queries. " + f"(Threshold is 3x initial interval: 3 x {initial_interval}s = {dynamic_check_threshold}s)" + )snakemake_executor_plugin_slurm/job_status_query.py (1)
14-19: Add timeout and catch FileNotFoundError/TimeoutExpired to avoid hangs.If scontrol is missing or stalls, get_min_job_age() can block startup. Add timeout and broaden exceptions. This mirrors a prior review; re-applying here.
Apply this diff:
- output = subprocess.check_output(cmd, text=True, stderr=subprocess.PIPE) - except subprocess.CalledProcessError: + output = subprocess.check_output( + cmd, text=True, stderr=subprocess.PIPE, timeout=10 + ) + except ( + subprocess.CalledProcessError, + FileNotFoundError, + subprocess.TimeoutExpired, + ): return None
🧹 Nitpick comments (7)
snakemake_executor_plugin_slurm/job_status_query.py (4)
1-5: Import shutil (prep for safer tool detection).Follow-up to next comment: we’ll use shutil.which instead of invoking which.
import shlex import subprocess import re from datetime import datetime, timedelta +import shutil
57-68: Use shutil.which; fix docstring; silence S603.Avoid spawning which; simpler, faster, and safer.
-def is_query_tool_available(tool_name): - """ - Check if the sacct command is available on the system. - Returns True if sacct is available, False otherwise. - """ - cmd = f"which {tool_name}" - cmd = shlex.split(cmd) - try: - subprocess.check_output(cmd, stderr=subprocess.PIPE) - return True - except subprocess.CalledProcessError: - return False +def is_query_tool_available(tool_name: str) -> bool: + """Return True if tool_name is on PATH, False otherwise.""" + return shutil.which(tool_name) is not None
98-128: Fix return type and doc for query_job_status_sacct.Annotation and docstring claim a list/dict but the function returns a string command.
-def query_job_status_sacct(jobuid) -> list: - """ - Query job status using sacct command - - Args: - job_ids: List of SLURM job IDs - timeout: Timeout in seconds for subprocess call - - Returns: - Dictionary mapping job ID to JobStatus object - """ +def query_job_status_sacct(jobuid: str) -> str: + """ + Build the sacct command (as a shell string) to query status for the given + workflow run identifier (jobuid). The caller is responsible for execution. + """
130-151: Align squeue helper: clarify return type/doc; add --clusters=all for parity.Keep returning argv list (to allow shell=False later) but document it; include clusters flag like sacct.
-def query_job_status_squeue(jobuid) -> list: - """ - Query job status using squeue command (newer SLURM functionality) - - Args: - job_ids: List of SLURM job IDs - timeout: Timeout in seconds for subprocess call - - Returns: - Dictionary mapping job ID to JobStatus object - """ +def query_job_status_squeue(jobuid: str) -> list[str]: + """ + Build the squeue command (argv list) to query status for the given run id. + The caller is responsible for execution. + """ @@ query_command = [ "squeue", "--format=%i|%T", "--states=all", "--noheader", + "--clusters=all", "--name", f"{jobuid}", ]snakemake_executor_plugin_slurm/__init__.py (3)
39-43: Import the new query builders to avoid duplicate command construction.Use the helpers instead of re-assembling sacct strings locally; this matches the PR goal.
from .job_status_query import ( get_min_job_age, is_query_tool_available, should_recommend_squeue_status_command, + query_job_status_sacct, + query_job_status_squeue, )
619-626: Initialize variables used after the loop (status_attempts could be 0).Prevents UnboundLocalError when status_attempts == 0.
- sacct_query_durations = [] + sacct_query_durations = [] + status_of_jobs = None + sacct_query_duration = None
629-646: Use outsourced query builders and honor status_command.Respect the configured/default status command and remove duplicate sacct assembly.
- # We use this sacct syntax for argument 'starttime' to keep it - # compatible with slurm < 20.11 - sacct_starttime = f"{datetime.now() - timedelta(days=2):%Y-%m-%dT%H:00}" - # previously we had - # f"--starttime now-2days --endtime now --name {self.run_uuid}" - # in line 218 - once v20.11 is definitively not in use any more, - # the more readable version ought to be re-adapted - - # -X: only show main job, no substeps - sacct_command = f"""sacct -X --parsable2 \ - --clusters all \ - --noheader --format=JobIdRaw,State \ - --starttime {sacct_starttime} \ - --endtime now --name {self.run_uuid}""" - - # for better redability in verbose output - sacct_command = " ".join(shlex.split(sacct_command)) + # Build status query command according to selected backend + selected = self.get_status_command() + if selected == "squeue": + argv = query_job_status_squeue(self.run_uuid) + status_query_cmd = " ".join(shlex.quote(a) for a in argv) + else: + status_query_cmd = query_job_status_sacct(self.run_uuid)And below (Line 651) replace sacct_command with status_query_cmd:
- (status_of_jobs, sacct_query_duration) = await self.job_stati( - sacct_command - ) + (status_of_jobs, sacct_query_duration) = await self.job_stati( + status_query_cmd + )
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
snakemake_executor_plugin_slurm/__init__.py(18 hunks)snakemake_executor_plugin_slurm/job_status_query.py(1 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.
📚 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/job_status_query.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/job_status_query.py
🧬 Code graph analysis (1)
snakemake_executor_plugin_slurm/__init__.py (2)
snakemake_executor_plugin_slurm/job_status_query.py (3)
get_min_job_age(7-54)is_query_tool_available(57-68)should_recommend_squeue_status_command(71-95)snakemake_executor_plugin_slurm/efficiency_report.py (1)
create_efficiency_report(190-219)
🪛 Ruff (0.12.2)
snakemake_executor_plugin_slurm/__init__.py
55-59: Avoid specifying long messages outside the exception class
(TRY003)
336-336: String contains ambiguous × (MULTIPLICATION SIGN). Did you mean x (LATIN SMALL LETTER X)?
(RUF001)
349-349: String contains ambiguous × (MULTIPLICATION SIGN). Did you mean x (LATIN SMALL LETTER X)?
(RUF001)
565-565: Avoid specifying long messages outside the exception class
(TRY003)
snakemake_executor_plugin_slurm/job_status_query.py
17-17: subprocess call: check for execution of untrusted input
(S603)
65-65: subprocess call: check for execution of untrusted input
(S603)
66-66: Consider moving this statement to an else block
(TRY300)
⏰ 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/__init__.py (1)
581-606: Confirm squeue output format on your SLURM cluster
squeue isn’t available in this environment. Please run on your target cluster:squeue --format=%i\|%T --states=all --noheader --clusters=all | head -5and ensure each line is exactly
<id>|<state>with no multi-cluster prefixes or extra fields.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
snakemake_executor_plugin_slurm/job_status_query.py (1)
14-21: Good: timeout + broader exceptions in get_min_job_age.This resolves the earlier concern about hangs/missing scontrol. Looks solid.
🧹 Nitpick comments (4)
snakemake_executor_plugin_slurm/job_status_query.py (4)
23-56: Parse minutes and avoid ambiguous units.Support “m/min/minute(s)” and fail closed on unknown units rather than silently treating them as seconds. Also simplifies conversion via a map.
- # Use regex to parse value and optional unit - # Pattern matches: number + optional whitespace + optional unit - match = re.match(r"^(\d+)\s*([a-zA-Z]*)", value_part) + # Parse number + optional unit + match = re.match(r"^(\d+)\s*([a-zA-Z]+)?", value_part) if not match: continue value_str = match.group(1) - unit = match.group(2).lower() if match.group(2) else "" + unit = (match.group(2) or "").lower() try: value = int(value_str) - - # Convert to seconds based on unit - if unit in ("h", "hour", "hours"): - return value * 3600 - elif unit in ("s", "sec", "secs", "second", "seconds", ""): - return value - else: - # Unknown unit, assume seconds - return value + unit_map = { + "": 1, "s": 1, "sec": 1, "secs": 1, "second": 1, "seconds": 1, + "m": 60, "min": 60, "mins": 60, "minute": 60, "minutes": 60, + "h": 3600, "hour": 3600, "hours": 3600, + } + factor = unit_map.get(unit) + if factor is None: + return None # unknown unit → safer to disable recommendation + return value * factor
59-70: Use shutil.which instead of spawning ‘which’.Removes a subprocess, fixes Ruff S603, and is more portable. Also generalize the docstring.
+import shutil @@ -def is_query_tool_available(tool_name): - """ - Check if the sacct command is available on the system. - Returns True if sacct is available, False otherwise. - """ - cmd = f"which {tool_name}" - cmd = shlex.split(cmd) - try: - subprocess.check_output(cmd, stderr=subprocess.PIPE) - return True - except subprocess.CalledProcessError: - return False +def is_query_tool_available(tool_name: str) -> bool: + """ + Check if a CLI tool is available on PATH. + """ + return shutil.which(tool_name) is not None
73-97: Clarify intent in docstring/name.Function returns whether squeue is viable, not a recommendation per se. Consider rewording docstring (or rename to is_squeue_viable) to avoid confusion.
- """ - Determine if the status query with squeue should be recommended based on - the MinJobAge configuration (if very low, squeue might not work well) + """ + Determine if using squeue for status queries is viable based on MinJobAge + (low values make completed jobs disappear quickly). @@ - Returns True if the option should be available, False otherwise. + Returns True if squeue is viable, False otherwise.
132-153: Fix docstring/type-hint drift and clarify jobuid semantics (job-name == run_uuid).query_job_status_squeue returns an argv list; its docstring and signature are stale — jobuid is the run_uuid used as sbatch --job-name (submit_string.py:28; init.py:463–465). Update signature/docstring as shown.
-def query_job_status_squeue(jobuid) -> list: - """ - Query job status using squeue command (newer SLURM functionality) - - Args: - job_ids: List of SLURM job IDs - timeout: Timeout in seconds for subprocess call - - Returns: - Dictionary mapping job ID to JobStatus object - """ +def query_job_status_squeue(jobuid: str) -> List[str]: + """ + Build the squeue argv to query status by job name/UID. + Returns a list of arguments suitable for subprocess without shell=True. + """Also add the necessary typing import (or use list[str] to match the codebase).
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
snakemake_executor_plugin_slurm/job_status_query.py(1 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.
📚 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/job_status_query.py
🪛 Ruff (0.12.2)
snakemake_executor_plugin_slurm/job_status_query.py
17-17: subprocess call: check for execution of untrusted input
(S603)
67-67: subprocess call: check for execution of untrusted input
(S603)
68-68: Consider moving this statement to an else block
(TRY300)
⏰ 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/job_status_query.py (1)
100-131: Return argv List[str] from query_job_status_sacct (fix docstring/type-hint/return mismatch)Change the function to return a List[str] argv suitable for subprocess.run(..., shell=False) and update the signature/docstring. Repo search found no callers outside the definition; verify external callers/tests before merging.
-from datetime import datetime, timedelta +from datetime import datetime, timedelta +from typing import List @@ -def query_job_status_sacct(jobuid) -> list: - """ - Query job status using sacct command - - Args: - job_ids: List of SLURM job IDs - timeout: Timeout in seconds for subprocess call - - Returns: - Dictionary mapping job ID to JobStatus object - """ +def query_job_status_sacct(jobuid: str) -> List[str]: + """ + Build the sacct argv to query job status for a given job name/UID. + Returns a list of arguments suitable for subprocess without shell=True. + """ @@ - # -X: only show main job, no substeps - query_command = f"""sacct -X --parsable2 \ - --clusters all \ - --noheader --format=JobIdRaw,State \ - --starttime {sacct_starttime} \ - --endtime now --name {jobuid}""" - - # for better redability in verbose output - query_command = " ".join(shlex.split(query_command)) - - return query_command + # -X: only show main job, no substeps + query_command = [ + "sacct", + "-X", + "--parsable2", + "--clusters", "all", + "--noheader", + "--format=JobIdRaw,State", + "--starttime", sacct_starttime, + "--endtime", "now", + "--name", str(jobuid), + ] + return query_command
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
snakemake_executor_plugin_slurm/__init__.py (2)
40-44: Avoid running cluster probes in_get_status_command_help/ metadata (import-time subprocess calls).
_get_status_command_help()still calls_get_status_command_default(),is_query_tool_available(), andshould_recommend_squeue_status_command(), which in turn runwhichandscontrol show config. Because the help text is evaluated in themetadata["help"]field, these subprocess calls happen at import time (e.g. onsnakemake --help), can be slow, and can fail on systems without SLURM or with misconfiguredscontrol. This was already flagged earlier and is still present.Consider making the help text static and moving all probing logic to runtime-only paths (
_get_status_command_default,_validate_status_command_settings, and the actual status-query code). For example:-def _get_status_command_help(): - """Get help text with computed default.""" - default_cmd = _get_status_command_default() - sacct_available = is_query_tool_available("sacct") - squeue_recommended = should_recommend_squeue_status_command() - - base_help = "Command to query job status. Options: 'sacct', 'squeue'. " - - if default_cmd == "sacct": - if sacct_available and not squeue_recommended: - info = ( - "'sacct' detected and will be used " - "(MinJobAge may be too low for reliable 'squeue' usage)" - ) - else: - info = "'sacct' detected and will be used" - else: # default_cmd == "squeue" - if squeue_recommended: - # cumbersome, due to black and the need to stay below 80 chars - msg_part1 = "'squeue' recommended (MinJobAge is sufficient )" - msg_part2 = " for reliable usage" - info = msg_part1 + msg_part2 - elif not sacct_available: - info = ( - "'sacct' not available, falling back to 'squeue'. " - "WARNING: 'squeue' may not work reliably if MinJobAge is " - "too low" - ) - else: - info = ( - "'squeue' will be used. " - "WARNING: MinJobAge may be too low for reliable 'squeue' usage" - ) - - return ( - f"{base_help}Default: '{default_cmd}' ({info}). " - f"Set explicitly to override auto-detection." - ) +def _get_status_command_help(): + """Help text for the status_command setting.""" + return ( + "Command to query job status. Options: 'sacct' (preferred), 'squeue'. " + "The actual default is chosen dynamically at runtime based on cluster " + "configuration; see the plugin documentation for details." + )Once this is done, keeping
metadata={"help": _get_status_command_help(), ...}is safe because the function becomes pure and cheap.Also applies to: 51-67, 69-106, 226-233
334-335: Fix MinJobAge logic, threshold source, and ambiguous×in_validate_status_command_settings.The MinJobAge warnings are still inverted and the threshold is derived from a non-existent
seconds_between_status_checkssetting, whileinit_seconds_before_status_checksis the documented knob. Also, the log strings still use the ambiguous×character. This was pointed out earlier and remains unresolved.Suggested adjustment aligning with the documented behaviour (MinJobAge affects
squeue, notsacct) and Ruff’s feedback:- # Calculate dynamic threshold: 3 times the initial - # status check interval. - # The plugin starts with 40 seconds and increases, - # so we use (3 * 10) + 40 = 70 seconds as minimum - between_status_check_seconds = getattr( - self.workflow.executor_settings, - "seconds_between_status_checks", - 70, - ) - dynamic_check_threshold = 3 * between_status_check_seconds + # Threshold: 3x initial status check interval (default 40s) + initial_interval = getattr( + self.workflow.executor_settings, + "init_seconds_before_status_checks", + 40, + ) + dynamic_check_threshold = 3 * initial_interval @@ - elif sacct_available and min_job_age is not None: - if ( - min_job_age < dynamic_check_threshold - and status_command == "sacct" - ): - self.logger.warning( - f"MinJobAge is {min_job_age} seconds " - f"(< {dynamic_check_threshold}s). " - f"This may cause 'sacct' to report inaccurate " - "job states and the status_command option may " - "be unreliable. " - "(Threshold is 3x status check interval: 3 × " - f"{between_status_check_seconds}s = " - f"{dynamic_check_threshold}s)" - ) - elif ( - min_job_age >= dynamic_check_threshold - and status_command == "squeue" - ): - self.logger.warning( - f"MinJobAge is {min_job_age} seconds (>= " - f"{dynamic_check_threshold}s). " - f"The 'squeue' command should work reliably for " - "status queries. " - "(Threshold is 3x status check interval: 3 × " - f"{between_status_check_seconds}s = " - f"{dynamic_check_threshold}s)" - ) + elif sacct_available and min_job_age is not None: + if min_job_age < dynamic_check_threshold and status_command == "squeue": + self.logger.warning( + f"MinJobAge is {min_job_age} seconds (< {dynamic_check_threshold}s). " + "This may cause 'squeue' to miss recently finished jobs; " + "consider using 'sacct'. " + f"(Threshold is 3x initial interval: 3 x {initial_interval}s = " + f"{dynamic_check_threshold}s)" + ) + elif min_job_age >= dynamic_check_threshold and status_command == "sacct": + self.logger.warning( + f"MinJobAge is {min_job_age} seconds (>= {dynamic_check_threshold}s). " + "The 'squeue' command should also work reliably for status queries. " + f"(Threshold is 3x initial interval: 3 x {initial_interval}s = " + f"{dynamic_check_threshold}s)" + )Also note that the warning branch
if not sacct_available and status_command == "sacct": self.logger.warning( "The 'sacct' command is not available on this system. " "Using 'squeue' instead for job status queries." )does not actually switch
status_commandto"squeue"; it only logs. Either adjust the message (“will fall back to 'squeue' once status querying honours this setting”) or implement the actual fallback by mutating the setting.Also applies to: 337-389
🧹 Nitpick comments (2)
snakemake_executor_plugin_slurm/__init__.py (2)
191-224: Efficiency report wiring now runs debug-style directory listing; consider trimming before production.With the new
efficiency_report*settings and the shutdown hook, enablingefficiency_reportwill now callcreate_efficiency_report()on every run. Per the retrieved learnings, that function still logsos.listdir(cwd)at debug level for testing purposes only. This can clutter logs and leak directory contents unnecessarily once this feature is on the main execution path.Based on learnings, I’d suggest removing or guarding that debug line in
snakemake_executor_plugin_slurm/efficiency_report.py(while keeping the summarylogger.info), now that report generation is wired into normal shutdown.Also applies to: 399-419
137-147: Handledelete_logfiles_older_thanbeingNonedespite Optional typing.
delete_logfiles_older_thanis typed asOptional[int]butclean_old_logs()assumes it’s always anint:age_cutoff = self.workflow.executor_settings.delete_logfiles_older_than keep_all = self.workflow.executor_settings.keep_successful_logs if age_cutoff <= 0 or keep_all: returnIf configuration ever sets this to
None, the comparisonage_cutoff <= 0will raise aTypeError.A small defensive tweak would make this robust:
- age_cutoff = self.workflow.executor_settings.delete_logfiles_older_than - keep_all = self.workflow.executor_settings.keep_successful_logs - if age_cutoff <= 0 or keep_all: + age_cutoff = self.workflow.executor_settings.delete_logfiles_older_than + keep_all = self.workflow.executor_settings.keep_successful_logs + if age_cutoff is None or age_cutoff <= 0 or keep_all: returnOptionally, you could also mention in the help text that
None/negative values disable cleanup.Also applies to: 421-428
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
pyproject.toml(1 hunks)snakemake_executor_plugin_slurm/__init__.py(17 hunks)snakemake_executor_plugin_slurm/efficiency_report.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- pyproject.toml
- snakemake_executor_plugin_slurm/efficiency_report.py
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 0
File: :0-0
Timestamp: 2025-01-13T09:54:22.950Z
Learning: PR #173 (adding gres resource specification) depends on PR #28 in snakemake-executor-plugin-slurm-jobstep repository, as changes were required in the cpu-settings function of the jobstep-Executor module.
Learnt from: johanneskoester
Repo: snakemake/snakemake-executor-plugin-slurm PR: 173
File: docs/further.md:96-96
Timestamp: 2025-03-10T15:20:51.829Z
Learning: PR #173 in snakemake-executor-plugin-slurm implements GPU job support by adding resources: `gres` for generic resource specifications (e.g., 'gpu:1'), `gpu`/`gpus` for specifying GPU counts, and `gpu_model`/`gpu_manufacturer` for specifying GPU types, allowing users to request GPU resources directly rather than having to use slurm_extra.
📚 Learning: 2024-12-11T14:17:08.749Z
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 178
File: snakemake_executor_plugin_slurm/__init__.py:0-0
Timestamp: 2024-12-11T14:17:08.749Z
Learning: In the `snakemake-executor-plugin-slurm` project, when handling exceptions in `snakemake_executor_plugin_slurm/__init__.py`, prefer concise error messages and avoid unnecessary verbosity or exception chaining when it's not necessary.
Applied to files:
snakemake_executor_plugin_slurm/__init__.py
📚 Learning: 2025-03-10T15:20:51.829Z
Learnt from: johanneskoester
Repo: snakemake/snakemake-executor-plugin-slurm PR: 173
File: docs/further.md:96-96
Timestamp: 2025-03-10T15:20:51.829Z
Learning: PR #173 in snakemake-executor-plugin-slurm implements GPU job support by adding resources: `gres` for generic resource specifications (e.g., 'gpu:1'), `gpu`/`gpus` for specifying GPU counts, and `gpu_model`/`gpu_manufacturer` for specifying GPU types, allowing users to request GPU resources directly rather than having to use slurm_extra.
Applied to files:
snakemake_executor_plugin_slurm/__init__.py
📚 Learning: 2025-01-20T09:13:26.443Z
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 193
File: .github/workflows/post_to_mastodon.yml:26-26
Timestamp: 2025-01-20T09:13:26.443Z
Learning: In the snakemake-executor-plugin-slurm repository, release PRs follow the naming pattern "chore(main): release X.Y.Z" where X.Y.Z is the version number.
Applied to files:
snakemake_executor_plugin_slurm/__init__.py
📚 Learning: 2025-01-13T09:54:22.950Z
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 0
File: :0-0
Timestamp: 2025-01-13T09:54:22.950Z
Learning: PR #173 (adding gres resource specification) depends on PR #28 in snakemake-executor-plugin-slurm-jobstep repository, as changes were required in the cpu-settings function of the jobstep-Executor module.
Applied to files:
snakemake_executor_plugin_slurm/__init__.py
📚 Learning: 2025-03-31T11:52:05.373Z
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 249
File: snakemake_executor_plugin_slurm/submit_string.py:21-29
Timestamp: 2025-03-31T11:52:05.373Z
Learning: In the snakemake-executor-plugin-slurm project, account and partition parameters are validated in the submit function before reaching the get_submit_command function. By design, these parameters are either empty strings or already properly formatted with SLURM parameter tags, making additional validation in get_submit_command unnecessary.
Applied to files:
snakemake_executor_plugin_slurm/__init__.py
📚 Learning: 2025-09-09T15:55:55.669Z
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 0
File: :0-0
Timestamp: 2025-09-09T15:55:55.669Z
Learning: In the Snakemake SLURM executor plugin, Snakemake submits itself as a job using the --wrap parameter in the sbatch command, rather than creating or submitting a separate job.sh script file. The actual implementation uses `call += f' --wrap="{exec_job}"'` where exec_job is the formatted snakemake execution command.
Applied to files:
snakemake_executor_plugin_slurm/__init__.py
📚 Learning: 2025-06-03T17:10:51.292Z
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 221
File: snakemake_executor_plugin_slurm/__init__.py:0-0
Timestamp: 2025-06-03T17:10:51.292Z
Learning: In the snakemake-executor-plugin-slurm repository, the create_efficiency_report method in snakemake_executor_plugin_slurm/__init__.py contains temporary testing/debugging code that logs directory contents using os.listdir() and directory paths. This code was inserted for testing purposes only and should be removed from the production implementation.
Applied to files:
snakemake_executor_plugin_slurm/__init__.py
🧬 Code graph analysis (1)
snakemake_executor_plugin_slurm/__init__.py (2)
snakemake_executor_plugin_slurm/job_status_query.py (3)
get_min_job_age(7-56)is_query_tool_available(59-70)should_recommend_squeue_status_command(73-97)snakemake_executor_plugin_slurm/efficiency_report.py (1)
create_efficiency_report(233-262)
🪛 Ruff (0.14.5)
snakemake_executor_plugin_slurm/__init__.py
58-62: Avoid specifying long messages outside the exception class
(TRY003)
372-372: String contains ambiguous × (MULTIPLICATION SIGN). Did you mean x (LATIN SMALL LETTER X)?
(RUF001)
385-385: String contains ambiguous × (MULTIPLICATION SIGN). Did you mean x (LATIN SMALL LETTER X)?
(RUF001)
592-592: 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
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
snakemake_executor_plugin_slurm/__init__.py (2)
69-106: Avoid executing external commands at import time for metadata help text.The
_get_status_command_help()function calls_get_status_command_default(),is_query_tool_available(), andshould_recommend_squeue_status_command(), which execute external commands (which,scontrol). When this function is invoked in the field metadata at line 229, these commands run at module import time, potentially causing delays or failures during import.Replace the dynamic help with a static string:
def _get_status_command_help(): - """Get help text with computed default.""" - default_cmd = _get_status_command_default() - sacct_available = is_query_tool_available("sacct") - squeue_recommended = should_recommend_squeue_status_command() - - base_help = "Command to query job status. Options: 'sacct', 'squeue'. " - - if default_cmd == "sacct": - if sacct_available and not squeue_recommended: - info = ( - "'sacct' detected and will be used " - "(MinJobAge may be too low for reliable 'squeue' usage)" - ) - else: - info = "'sacct' detected and will be used" - else: # default_cmd == "squeue" - if squeue_recommended: - # cumbersome, due to black and the need to stay below 80 chars - msg_part1 = "'squeue' recommended (MinJobAge is sufficient )" - msg_part2 = " for reliable usage" - info = msg_part1 + msg_part2 - elif not sacct_available: - info = ( - "'sacct' not available, falling back to 'squeue'. " - "WARNING: 'squeue' may not work reliably if MinJobAge is " - "too low" - ) - else: - info = ( - "'squeue' will be used. " - "WARNING: MinJobAge may be too low for reliable 'squeue' usage" - ) - - return ( - f"{base_help}Default: '{default_cmd}' ({info}). " - f"Set explicitly to override auto-detection." - ) + """Static help text for status_command.""" + return ( + "Command to query job status. Options: 'sacct' (preferred), 'squeue'. " + "Default is chosen dynamically: 'sacct' if available, otherwise 'squeue'. " + "Note: 'squeue' reliability depends on MinJobAge configuration; see plugin documentation." + )
226-246: Complete the integration:status_commandis defined but never used in job status querying.The
status_commandconfiguration field is defined with validation and aget_status_command()method, but the actual status polling incheck_active_jobs()(lines 662-669) hardcodes asacctquery and never invokesget_status_command(). The two query functionsquery_job_status_sacct()andquery_job_status_squeue()fromjob_status_query.pyare imported but never called. This means thestatus_commandsetting currently has no effect on execution.To complete the integration, modify
check_active_jobs()to use the configured command. Here's the approach:# -X: only show main job, no substeps - sacct_command = f"""sacct -X --parsable2 \ - --clusters all \ - --noheader --format=JobIdRaw,State \ - --starttime {sacct_starttime} \ - --endtime now --name {self.run_uuid}""" - - # for better redability in verbose output - sacct_command = " ".join(shlex.split(sacct_command)) + # Import query functions at the top of the file + from .job_status_query import query_job_status_sacct, query_job_status_squeue + + # Get the configured status command + status_cmd = self.get_status_command() + + if status_cmd == "sacct": + query_command = query_job_status_sacct(self.run_uuid) + elif status_cmd == "squeue": + query_command = query_job_status_squeue(self.run_uuid) + # Convert list to string for consistency + query_command = " ".join(query_command) + else: + raise WorkflowError(f"Unknown status_command: {status_cmd}")Alternatively, if you prefer not to implement this now, remove or hide the
status_commandfield to avoid misleading users with a non-functional option.Also applies to: 387-394, 662-669
🧹 Nitpick comments (2)
snakemake_executor_plugin_slurm/job_status_query.py (1)
63-74: Update docstring to match generic function signature.The docstring specifically mentions "Check if the sacct command" but the function accepts a generic
tool_nameparameter. Update the docstring to reflect that this checks for any tool.Apply this diff:
def is_query_tool_available(tool_name): """ - Check if the sacct command is available on the system. - Returns True if sacct is available, False otherwise. + Check if the specified command is available on the system. + Returns True if the tool is available, False otherwise. """snakemake_executor_plugin_slurm/__init__.py (1)
268-271: Remove empty__post_init__or add validation logic.The
__post_init__method is defined but empty with a placeholder comment. Either remove it if no validation is needed, or implement the validation logic referenced in the docstring.If no validation is needed:
- def __post_init__(self): - """Validate settings after initialization.""" - # Add any validation logic here if needed in the future - passOtherwise, add the actual validation logic here.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
snakemake_executor_plugin_slurm/__init__.py(17 hunks)snakemake_executor_plugin_slurm/job_status_query.py(1 hunks)
🧰 Additional context used
🧠 Learnings (11)
📓 Common learnings
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 0
File: :0-0
Timestamp: 2025-01-13T09:54:22.950Z
Learning: PR #173 (adding gres resource specification) depends on PR #28 in snakemake-executor-plugin-slurm-jobstep repository, as changes were required in the cpu-settings function of the jobstep-Executor module.
Learnt from: johanneskoester
Repo: snakemake/snakemake-executor-plugin-slurm PR: 173
File: docs/further.md:96-96
Timestamp: 2025-03-10T15:20:51.829Z
Learning: PR #173 in snakemake-executor-plugin-slurm implements GPU job support by adding resources: `gres` for generic resource specifications (e.g., 'gpu:1'), `gpu`/`gpus` for specifying GPU counts, and `gpu_model`/`gpu_manufacturer` for specifying GPU types, allowing users to request GPU resources directly rather than having to use slurm_extra.
📚 Learning: 2024-12-11T14:17:08.749Z
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 178
File: snakemake_executor_plugin_slurm/__init__.py:0-0
Timestamp: 2024-12-11T14:17:08.749Z
Learning: In the `snakemake-executor-plugin-slurm` project, when handling exceptions in `snakemake_executor_plugin_slurm/__init__.py`, prefer concise error messages and avoid unnecessary verbosity or exception chaining when it's not necessary.
Applied to files:
snakemake_executor_plugin_slurm/__init__.pysnakemake_executor_plugin_slurm/job_status_query.py
📚 Learning: 2025-03-10T15:20:51.829Z
Learnt from: johanneskoester
Repo: snakemake/snakemake-executor-plugin-slurm PR: 173
File: docs/further.md:96-96
Timestamp: 2025-03-10T15:20:51.829Z
Learning: PR #173 in snakemake-executor-plugin-slurm implements GPU job support by adding resources: `gres` for generic resource specifications (e.g., 'gpu:1'), `gpu`/`gpus` for specifying GPU counts, and `gpu_model`/`gpu_manufacturer` for specifying GPU types, allowing users to request GPU resources directly rather than having to use slurm_extra.
Applied to files:
snakemake_executor_plugin_slurm/__init__.pysnakemake_executor_plugin_slurm/job_status_query.py
📚 Learning: 2025-01-13T09:54:22.950Z
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 0
File: :0-0
Timestamp: 2025-01-13T09:54:22.950Z
Learning: PR #173 (adding gres resource specification) depends on PR #28 in snakemake-executor-plugin-slurm-jobstep repository, as changes were required in the cpu-settings function of the jobstep-Executor module.
Applied to files:
snakemake_executor_plugin_slurm/__init__.pysnakemake_executor_plugin_slurm/job_status_query.py
📚 Learning: 2025-01-20T09:13:26.443Z
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 193
File: .github/workflows/post_to_mastodon.yml:26-26
Timestamp: 2025-01-20T09:13:26.443Z
Learning: In the snakemake-executor-plugin-slurm repository, release PRs follow the naming pattern "chore(main): release X.Y.Z" where X.Y.Z is the version number.
Applied to files:
snakemake_executor_plugin_slurm/__init__.py
📚 Learning: 2025-03-31T11:52:05.373Z
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 249
File: snakemake_executor_plugin_slurm/submit_string.py:21-29
Timestamp: 2025-03-31T11:52:05.373Z
Learning: In the snakemake-executor-plugin-slurm project, account and partition parameters are validated in the submit function before reaching the get_submit_command function. By design, these parameters are either empty strings or already properly formatted with SLURM parameter tags, making additional validation in get_submit_command unnecessary.
Applied to files:
snakemake_executor_plugin_slurm/__init__.py
📚 Learning: 2025-07-22T17:31:41.629Z
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 333
File: .github/workflows/announce-release.yml:0-0
Timestamp: 2025-07-22T17:31:41.629Z
Learning: In the snakemake-executor-plugin-slurm repository, cmeesters prefers using `main` references for GitHub Actions to always get the latest version, with security provided through protected branches and mandatory CI checks rather than commit SHA pinning.
Applied to files:
snakemake_executor_plugin_slurm/__init__.py
📚 Learning: 2025-06-24T12:41:31.837Z
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 221
File: tests/tests.py:62-70
Timestamp: 2025-06-24T12:41:31.837Z
Learning: In the snakemake-executor-plugin-slurm efficiency report feature, the executor is responsible for creating the efficiency_report_path directory when generating efficiency reports. Tests should not manually check for directory existence but should rely on the executor to properly create necessary directories as part of its normal operation.
Applied to files:
snakemake_executor_plugin_slurm/__init__.py
📚 Learning: 2025-01-17T17:27:32.446Z
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 187
File: .github/workflows/post_to_mastodon.yml:0-0
Timestamp: 2025-01-17T17:27:32.446Z
Learning: In the mastodon publishing workflow for snakemake-executor-plugin-slurm, the PR_TITLE environment variable is required by the post_to_mastodon.sh script and should be preserved.
Applied to files:
snakemake_executor_plugin_slurm/__init__.py
📚 Learning: 2025-09-16T17:25:38.450Z
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 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/__init__.py
📚 Learning: 2025-06-03T17:10:51.292Z
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 221
File: snakemake_executor_plugin_slurm/__init__.py:0-0
Timestamp: 2025-06-03T17:10:51.292Z
Learning: In the snakemake-executor-plugin-slurm repository, the create_efficiency_report method in snakemake_executor_plugin_slurm/__init__.py contains temporary testing/debugging code that logs directory contents using os.listdir() and directory paths. This code was inserted for testing purposes only and should be removed from the production implementation.
Applied to files:
snakemake_executor_plugin_slurm/__init__.py
🧬 Code graph analysis (1)
snakemake_executor_plugin_slurm/__init__.py (2)
snakemake_executor_plugin_slurm/job_status_query.py (3)
get_min_job_age(7-60)is_query_tool_available(63-74)should_recommend_squeue_status_command(77-101)snakemake_executor_plugin_slurm/efficiency_report.py (1)
create_efficiency_report(233-262)
🪛 Ruff (0.14.5)
snakemake_executor_plugin_slurm/__init__.py
58-62: Avoid specifying long messages outside the exception class
(TRY003)
369-369: String contains ambiguous × (MULTIPLICATION SIGN). Did you mean x (LATIN SMALL LETTER X)?
(RUF001)
382-382: String contains ambiguous × (MULTIPLICATION SIGN). Did you mean x (LATIN SMALL LETTER X)?
(RUF001)
589-589: Avoid specifying long messages outside the exception class
(TRY003)
snakemake_executor_plugin_slurm/job_status_query.py
17-17: subprocess call: check for execution of untrusted input
(S603)
71-71: subprocess call: check for execution of untrusted input
(S603)
72-72: Consider moving this statement to an else block
(TRY300)
⏰ 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
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
snakemake_executor_plugin_slurm/__init__.py (2)
228-235: Import-time execution triggered by metadata help text.The metadata
"help"field at line 231 calls_get_status_command_help()during class definition (import time). Once the function in the previous comment is converted to return a static string, this call becomes safe.Ensure
_get_status_command_help()returns only a static string (no function calls) before this metadata evaluation.
71-108: Critical: Import-time execution still present in help text generation.Despite past review feedback marking this as addressed,
_get_status_command_help()still calls_get_status_command_default(),is_query_tool_available(), andshould_recommend_squeue_status_command()which execute external commands (scontrol,which) at import time. This occurs when the metadata dictionary at line 231 is evaluated during module import, causing:
- Slow/hanging imports if SLURM tools are unavailable
- Potential exceptions during import
- Violation of Python import best practices
Replace with a static help string:
def _get_status_command_help(): - """Get help text with computed default.""" - default_cmd = _get_status_command_default() - sacct_available = is_query_tool_available("sacct") - squeue_recommended = should_recommend_squeue_status_command() - - base_help = "Command to query job status. Options: 'sacct', 'squeue'. " - - if default_cmd == "sacct": - if sacct_available and not squeue_recommended: - info = ( - "'sacct' detected and will be used " - "(MinJobAge may be too low for reliable 'squeue' usage)" - ) - else: - info = "'sacct' detected and will be used" - else: # default_cmd == "squeue" - if squeue_recommended: - # cumbersome, due to black and the need to stay below 80 chars - msg_part1 = "'squeue' recommended (MinJobAge is sufficient )" - msg_part2 = " for reliable usage" - info = msg_part1 + msg_part2 - elif not sacct_available: - info = ( - "'sacct' not available, falling back to 'squeue'. " - "WARNING: 'squeue' may not work reliably if MinJobAge is " - "too low" - ) - else: - info = ( - "'squeue' will be used. " - "WARNING: MinJobAge may be too low for reliable 'squeue' usage" - ) - - return ( - f"{base_help}Default: '{default_cmd}' ({info}). " - f"Set explicitly to override auto-detection." - ) + """Static help text for status_command.""" + return ( + "Command to query job status. Options: 'sacct' (preferred), 'squeue'. " + "Default is chosen dynamically based on tool availability and MinJobAge. " + "Set explicitly to override auto-detection." + )
🧹 Nitpick comments (1)
snakemake_executor_plugin_slurm/__init__.py (1)
667-667: Consider renaming unused loop variable to underscore.The loop variable
iis not used within the loop body. By convention, use_for unused variables.- for i in range(status_attempts): + for _ in range(status_attempts):
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
snakemake_executor_plugin_slurm/__init__.py(17 hunks)snakemake_executor_plugin_slurm/partitions.py(2 hunks)tests/test_partition_selection.py(2 hunks)tests/tests.py(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- snakemake_executor_plugin_slurm/partitions.py
- tests/tests.py
- tests/test_partition_selection.py
🧰 Additional context used
🧠 Learnings (12)
📓 Common learnings
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 0
File: :0-0
Timestamp: 2025-01-13T09:54:22.950Z
Learning: PR #173 (adding gres resource specification) depends on PR #28 in snakemake-executor-plugin-slurm-jobstep repository, as changes were required in the cpu-settings function of the jobstep-Executor module.
Learnt from: johanneskoester
Repo: snakemake/snakemake-executor-plugin-slurm PR: 173
File: docs/further.md:96-96
Timestamp: 2025-03-10T15:20:51.829Z
Learning: PR #173 in snakemake-executor-plugin-slurm implements GPU job support by adding resources: `gres` for generic resource specifications (e.g., 'gpu:1'), `gpu`/`gpus` for specifying GPU counts, and `gpu_model`/`gpu_manufacturer` for specifying GPU types, allowing users to request GPU resources directly rather than having to use slurm_extra.
📚 Learning: 2024-12-11T14:17:08.749Z
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 178
File: snakemake_executor_plugin_slurm/__init__.py:0-0
Timestamp: 2024-12-11T14:17:08.749Z
Learning: In the `snakemake-executor-plugin-slurm` project, when handling exceptions in `snakemake_executor_plugin_slurm/__init__.py`, prefer concise error messages and avoid unnecessary verbosity or exception chaining when it's not necessary.
Applied to files:
snakemake_executor_plugin_slurm/__init__.py
📚 Learning: 2025-03-10T15:20:51.829Z
Learnt from: johanneskoester
Repo: snakemake/snakemake-executor-plugin-slurm PR: 173
File: docs/further.md:96-96
Timestamp: 2025-03-10T15:20:51.829Z
Learning: PR #173 in snakemake-executor-plugin-slurm implements GPU job support by adding resources: `gres` for generic resource specifications (e.g., 'gpu:1'), `gpu`/`gpus` for specifying GPU counts, and `gpu_model`/`gpu_manufacturer` for specifying GPU types, allowing users to request GPU resources directly rather than having to use slurm_extra.
Applied to files:
snakemake_executor_plugin_slurm/__init__.py
📚 Learning: 2025-01-20T09:13:26.443Z
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 193
File: .github/workflows/post_to_mastodon.yml:26-26
Timestamp: 2025-01-20T09:13:26.443Z
Learning: In the snakemake-executor-plugin-slurm repository, release PRs follow the naming pattern "chore(main): release X.Y.Z" where X.Y.Z is the version number.
Applied to files:
snakemake_executor_plugin_slurm/__init__.py
📚 Learning: 2025-01-13T09:54:22.950Z
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 0
File: :0-0
Timestamp: 2025-01-13T09:54:22.950Z
Learning: PR #173 (adding gres resource specification) depends on PR #28 in snakemake-executor-plugin-slurm-jobstep repository, as changes were required in the cpu-settings function of the jobstep-Executor module.
Applied to files:
snakemake_executor_plugin_slurm/__init__.py
📚 Learning: 2025-03-31T11:52:05.373Z
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 249
File: snakemake_executor_plugin_slurm/submit_string.py:21-29
Timestamp: 2025-03-31T11:52:05.373Z
Learning: In the snakemake-executor-plugin-slurm project, account and partition parameters are validated in the submit function before reaching the get_submit_command function. By design, these parameters are either empty strings or already properly formatted with SLURM parameter tags, making additional validation in get_submit_command unnecessary.
Applied to files:
snakemake_executor_plugin_slurm/__init__.py
📚 Learning: 2025-01-17T17:27:32.446Z
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 187
File: .github/workflows/post_to_mastodon.yml:0-0
Timestamp: 2025-01-17T17:27:32.446Z
Learning: In the mastodon publishing workflow for snakemake-executor-plugin-slurm, the PR_TITLE environment variable is required by the post_to_mastodon.sh script and should be preserved.
Applied to files:
snakemake_executor_plugin_slurm/__init__.py
📚 Learning: 2025-07-22T17:31:41.629Z
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 333
File: .github/workflows/announce-release.yml:0-0
Timestamp: 2025-07-22T17:31:41.629Z
Learning: In the snakemake-executor-plugin-slurm repository, cmeesters prefers using `main` references for GitHub Actions to always get the latest version, with security provided through protected branches and mandatory CI checks rather than commit SHA pinning.
Applied to files:
snakemake_executor_plugin_slurm/__init__.py
📚 Learning: 2025-06-24T12:41:31.837Z
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 221
File: tests/tests.py:62-70
Timestamp: 2025-06-24T12:41:31.837Z
Learning: In the snakemake-executor-plugin-slurm efficiency report feature, the executor is responsible for creating the efficiency_report_path directory when generating efficiency reports. Tests should not manually check for directory existence but should rely on the executor to properly create necessary directories as part of its normal operation.
Applied to files:
snakemake_executor_plugin_slurm/__init__.py
📚 Learning: 2025-09-16T17:25:38.450Z
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 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/__init__.py
📚 Learning: 2025-05-26T11:50:44.708Z
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 221
File: snakemake_executor_plugin_slurm/__init__.py:800-859
Timestamp: 2025-05-26T11:50:44.708Z
Learning: When using sacct with a specific --format parameter in the snakemake SLURM executor plugin, column validation is unnecessary because the query format is controlled and the specified columns will always be present in the output.
Applied to files:
snakemake_executor_plugin_slurm/__init__.py
📚 Learning: 2025-06-03T17:10:51.292Z
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 221
File: snakemake_executor_plugin_slurm/__init__.py:0-0
Timestamp: 2025-06-03T17:10:51.292Z
Learning: In the snakemake-executor-plugin-slurm repository, the create_efficiency_report method in snakemake_executor_plugin_slurm/__init__.py contains temporary testing/debugging code that logs directory contents using os.listdir() and directory paths. This code was inserted for testing purposes only and should be removed from the production implementation.
Applied to files:
snakemake_executor_plugin_slurm/__init__.py
🧬 Code graph analysis (1)
snakemake_executor_plugin_slurm/__init__.py (2)
snakemake_executor_plugin_slurm/job_status_query.py (5)
get_min_job_age(7-60)is_query_tool_available(63-74)should_recommend_squeue_status_command(77-101)query_job_status_squeue(136-157)query_job_status_sacct(104-133)snakemake_executor_plugin_slurm/efficiency_report.py (1)
create_efficiency_report(233-262)
🪛 Ruff (0.14.5)
snakemake_executor_plugin_slurm/__init__.py
60-64: Avoid specifying long messages outside the exception class
(TRY003)
369-369: String contains ambiguous × (MULTIPLICATION SIGN). Did you mean x (LATIN SMALL LETTER X)?
(RUF001)
382-382: String contains ambiguous × (MULTIPLICATION SIGN). Did you mean x (LATIN SMALL LETTER X)?
(RUF001)
591-591: Avoid specifying long messages outside the exception class
(TRY003)
667-667: Loop control variable i not used within loop body
Rename unused i to _i
(B007)
⏰ 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/__init__.py (1)
655-661: Status command integration looks good.The
status_commandoption is now properly integrated. The code callsget_status_command()and conditionally invokes eitherquery_job_status_sacct()orquery_job_status_squeue()based on the configuration, addressing the concern from past reviews that the option was non-functional.
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)
836-851: Fix error message formatting and clarify exception handling.Line 839: Missing proper punctuation/spacing between the quote and the next sentence.
Line 851: Barepassafter logging an error swallows the exception. Clarify whether this is intentional (graceful degradation) or should propagate the error.Apply this diff:
self.logger.warning( "The SLURM database might not be available ... " - f"Error message: '{error_message}'" + f"Error message: '{error_message}'. " "This error message indicates that the SLURM database is " "currently not available. This is not an error of the " "Snakemake plugin, but some kind of server issue. " "Please consult with your HPC provider." ) else: self.logger.error( - f"The job status query failed with command '{command}'" - f"Error message: '{error_message}'" + f"The job status query failed with command '{command}'. " + f"Error message: '{error_message}'. " "This error message is not expected, please report it back to us." ) - pass + # Intentionally swallow exception to allow retry on next status check
♻️ Duplicate comments (1)
snakemake_executor_plugin_slurm/__init__.py (1)
70-107: Import-time execution still occurs in _get_status_command_help().Despite the past review marking this as addressed, the current code still calls
_get_status_command_default()(line 72),is_query_tool_available()(line 73), andshould_recommend_squeue_status_command()(line 74) within_get_status_command_help(), which is invoked in field metadata at line 230 during module import. These functions execute external commands (which,scontrol), causing side effects at import time.Replace with a static help string:
def _get_status_command_help(): - """Get help text with computed default.""" - default_cmd = _get_status_command_default() - sacct_available = is_query_tool_available("sacct") - squeue_recommended = should_recommend_squeue_status_command() - - base_help = "Command to query job status. Options: 'sacct', 'squeue'. " - - if default_cmd == "sacct": - if sacct_available and not squeue_recommended: - info = ( - "'sacct' detected and will be used " - "(MinJobAge may be too low for reliable 'squeue' usage)" - ) - else: - info = "'sacct' detected and will be used" - else: # default_cmd == "squeue" - if squeue_recommended: - # cumbersome, due to black and the need to stay below 80 chars - msg_part1 = "'squeue' recommended (MinJobAge is sufficient )" - msg_part2 = " for reliable usage" - info = msg_part1 + msg_part2 - elif not sacct_available: - info = ( - "'sacct' not available, falling back to 'squeue'. " - "WARNING: 'squeue' may not work reliably if MinJobAge is " - "too low" - ) - else: - info = ( - "'squeue' will be used. " - "WARNING: MinJobAge may be too low for reliable 'squeue' usage" - ) - - return ( - f"{base_help}Default: '{default_cmd}' ({info}). " - f"Set explicitly to override auto-detection." - ) + """Help text for status_command.""" + return ( + "Command to query job status. Options: 'sacct' (preferred), 'squeue'. " + "Default is auto-detected: 'sacct' if available, otherwise 'squeue'. " + "Note: 'squeue' reliability depends on MinJobAge configuration; " + "see plugin documentation for details." + )
🧹 Nitpick comments (8)
snakemake_executor_plugin_slurm/__init__.py (8)
642-642: Rename variable to reflect support for both status commands.The variable
sacct_query_durationsis now used for bothsacctandsqueuequeries. Rename to a command-agnostic name.Apply this diff:
- sacct_query_durations = [] + status_query_durations = []And update references at lines 674, etc.
650-652: Rename variables to be command-agnostic.Variables
active_jobs_seen_by_sacctandmissing_sacct_statusare now used for bothsacctandsqueuequeries.Apply this diff:
- active_jobs_ids = {job_info.external_jobid for job_info in active_jobs} - active_jobs_seen_by_sacct = set() - missing_sacct_status = set() + active_jobs_ids = {job_info.external_jobid for job_info in active_jobs} + active_jobs_seen_by_status_query = set() + missing_status = set()Update all references throughout the method (lines 684, 689, 691, 695, 699, 703, 724, 754, 765).
666-666: Use loop variable or rename to '_'.The loop variable
iis not used within the loop body. Either use it for logging/debugging or rename to_to indicate intentional discard.Apply this diff:
- for i in range(status_attempts): + for _ in range(status_attempts):Based on static analysis hints.
675-696: Update comments and log messages to be command-agnostic.Comments and debug messages still reference "sacct" specifically but should reflect that either
sacctorsqueuemay be used.Apply this diff:
self.logger.debug(f"could not check status of job {self.run_uuid}") continue - sacct_query_durations.append(sacct_query_duration) - self.logger.debug(f"status_of_jobs after sacct is: {status_of_jobs}") + status_query_durations.append(status_query_duration) + self.logger.debug(f"status_of_jobs after query is: {status_of_jobs}") # only take jobs that are still active - active_jobs_ids_with_current_sacct_status = ( + active_jobs_ids_with_current_status = ( set(status_of_jobs.keys()) & active_jobs_ids ) self.logger.debug( - f"active_jobs_ids_with_current_sacct_status are: " - f"{active_jobs_ids_with_current_sacct_status}" + f"active_jobs_ids_with_current_status are: " + f"{active_jobs_ids_with_current_status}" ) - active_jobs_seen_by_sacct = ( - active_jobs_seen_by_sacct - | active_jobs_ids_with_current_sacct_status + active_jobs_seen_by_status_query = ( + active_jobs_seen_by_status_query + | active_jobs_ids_with_current_status ) self.logger.debug( - "active_jobs_seen_by_sacct are: " f"{active_jobs_seen_by_sacct}" + "active_jobs_seen_by_status_query are: " f"{active_jobs_seen_by_status_query}" ) - missing_sacct_status = ( - active_jobs_seen_by_sacct - - active_jobs_ids_with_current_sacct_status + missing_status = ( + active_jobs_seen_by_status_query + - active_jobs_ids_with_current_status ) - self.logger.debug(f"missing_sacct_status are: {missing_sacct_status}") - if not missing_sacct_status: + self.logger.debug(f"missing_status are: {missing_status}") + if not missing_status: break
699-708: Update warning message to be command-agnostic.The warning message references "slurmdbd" and "sacct" specifically, but this warning applies to either status command when the accounting system is experiencing issues.
Apply this diff:
if missing_status: self.logger.warning( - "Unable to get the status of all active jobs that should be " - f"in slurmdbd, even after {status_attempts} attempts.\n" - "The jobs with the following slurm job ids were previously " - " seen by sacct, but sacct doesn't report them any more:\n" - f"{missing_status}\n" - "Please double-check with your slurm cluster administrator, " - "that slurmdbd job accounting is properly set up.\n" + f"Unable to get the status of all active jobs, even after " + f"{status_attempts} attempts.\n" + f"The jobs with the following SLURM job IDs were previously seen " + f"but are no longer reported:\n" + f"{missing_status}\n" + f"Please verify with your SLURM cluster administrator that the " + f"accounting/status system is properly configured.\n" )
710-768: Update comments referencing sacct/slurmdbd.Comments at lines 713-717 still reference "slurmdbd" and "sacct" specifically. These should be more generic since squeue is also supported.
Apply this diff:
for j in active_jobs: - # the job probably didn't make it into slurmdbd yet, so - # `sacct` doesn't return it + # The job probably hasn't been recorded yet or is too new, + # so the status query doesn't return it if j.external_jobid not in status_of_jobs: - # but the job should still be queueing or running and - # appear in slurmdbd (and thus `sacct` output) later + # The job should still be queueing or running and + # appear in subsequent status queries yield j continue
722-765: Update references to active_jobs_seen_by_sacct.After renaming the variable, update the remove() calls to use the new name.
Apply this diff (if the earlier rename is applied):
if status == "COMPLETED": self.report_job_success(j) any_finished = True - active_jobs_seen_by_sacct.remove(j.external_jobid) + active_jobs_seen_by_status_query.remove(j.external_jobid) if not self.workflow.executor_settings.keep_successful_logs:And similarly at lines 754, 765.
749-751: Consider logging the UNKNOWN status case.When status is "UNKNOWN," the job is assumed finished successfully without confirmation. This silent assumption might mask issues.
Add a debug log:
elif status == "UNKNOWN": - # the job probably does not exist anymore, but 'sacct' did not work - # so we assume it is finished + # The job probably does not exist anymore, but the status query + # returned UNKNOWN, so we assume it finished successfully + self.logger.debug( + f"Job {j.external_jobid} has UNKNOWN status; assuming success" + ) self.report_job_success(j)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
snakemake_executor_plugin_slurm/__init__.py(17 hunks)
🧰 Additional context used
🧠 Learnings (11)
📓 Common learnings
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 0
File: :0-0
Timestamp: 2025-01-13T09:54:22.950Z
Learning: PR #173 (adding gres resource specification) depends on PR #28 in snakemake-executor-plugin-slurm-jobstep repository, as changes were required in the cpu-settings function of the jobstep-Executor module.
Learnt from: johanneskoester
Repo: snakemake/snakemake-executor-plugin-slurm PR: 173
File: docs/further.md:96-96
Timestamp: 2025-03-10T15:20:51.829Z
Learning: PR #173 in snakemake-executor-plugin-slurm implements GPU job support by adding resources: `gres` for generic resource specifications (e.g., 'gpu:1'), `gpu`/`gpus` for specifying GPU counts, and `gpu_model`/`gpu_manufacturer` for specifying GPU types, allowing users to request GPU resources directly rather than having to use slurm_extra.
📚 Learning: 2024-12-11T14:17:08.749Z
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 178
File: snakemake_executor_plugin_slurm/__init__.py:0-0
Timestamp: 2024-12-11T14:17:08.749Z
Learning: In the `snakemake-executor-plugin-slurm` project, when handling exceptions in `snakemake_executor_plugin_slurm/__init__.py`, prefer concise error messages and avoid unnecessary verbosity or exception chaining when it's not necessary.
Applied to files:
snakemake_executor_plugin_slurm/__init__.py
📚 Learning: 2025-03-10T15:20:51.829Z
Learnt from: johanneskoester
Repo: snakemake/snakemake-executor-plugin-slurm PR: 173
File: docs/further.md:96-96
Timestamp: 2025-03-10T15:20:51.829Z
Learning: PR #173 in snakemake-executor-plugin-slurm implements GPU job support by adding resources: `gres` for generic resource specifications (e.g., 'gpu:1'), `gpu`/`gpus` for specifying GPU counts, and `gpu_model`/`gpu_manufacturer` for specifying GPU types, allowing users to request GPU resources directly rather than having to use slurm_extra.
Applied to files:
snakemake_executor_plugin_slurm/__init__.py
📚 Learning: 2025-01-13T09:54:22.950Z
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 0
File: :0-0
Timestamp: 2025-01-13T09:54:22.950Z
Learning: PR #173 (adding gres resource specification) depends on PR #28 in snakemake-executor-plugin-slurm-jobstep repository, as changes were required in the cpu-settings function of the jobstep-Executor module.
Applied to files:
snakemake_executor_plugin_slurm/__init__.py
📚 Learning: 2025-01-20T09:13:26.443Z
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 193
File: .github/workflows/post_to_mastodon.yml:26-26
Timestamp: 2025-01-20T09:13:26.443Z
Learning: In the snakemake-executor-plugin-slurm repository, release PRs follow the naming pattern "chore(main): release X.Y.Z" where X.Y.Z is the version number.
Applied to files:
snakemake_executor_plugin_slurm/__init__.py
📚 Learning: 2025-03-31T11:52:05.373Z
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 249
File: snakemake_executor_plugin_slurm/submit_string.py:21-29
Timestamp: 2025-03-31T11:52:05.373Z
Learning: In the snakemake-executor-plugin-slurm project, account and partition parameters are validated in the submit function before reaching the get_submit_command function. By design, these parameters are either empty strings or already properly formatted with SLURM parameter tags, making additional validation in get_submit_command unnecessary.
Applied to files:
snakemake_executor_plugin_slurm/__init__.py
📚 Learning: 2025-01-17T17:27:32.446Z
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 187
File: .github/workflows/post_to_mastodon.yml:0-0
Timestamp: 2025-01-17T17:27:32.446Z
Learning: In the mastodon publishing workflow for snakemake-executor-plugin-slurm, the PR_TITLE environment variable is required by the post_to_mastodon.sh script and should be preserved.
Applied to files:
snakemake_executor_plugin_slurm/__init__.py
📚 Learning: 2025-09-16T17:25:38.450Z
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 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/__init__.py
📚 Learning: 2025-07-22T17:31:41.629Z
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 333
File: .github/workflows/announce-release.yml:0-0
Timestamp: 2025-07-22T17:31:41.629Z
Learning: In the snakemake-executor-plugin-slurm repository, cmeesters prefers using `main` references for GitHub Actions to always get the latest version, with security provided through protected branches and mandatory CI checks rather than commit SHA pinning.
Applied to files:
snakemake_executor_plugin_slurm/__init__.py
📚 Learning: 2025-06-16T08:54:07.957Z
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 221
File: snakemake_executor_plugin_slurm/efficiency_report.py:0-0
Timestamp: 2025-06-16T08:54:07.957Z
Learning: In the context of SLURM executor plugin development, user cmeesters (experienced with SLURM) noted they are not aware of SLURM typically reporting memory values with decimals (like 1.5G), but accepted defensive programming to handle such cases anyway.
Applied to files:
snakemake_executor_plugin_slurm/__init__.py
📚 Learning: 2025-06-03T17:10:51.292Z
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 221
File: snakemake_executor_plugin_slurm/__init__.py:0-0
Timestamp: 2025-06-03T17:10:51.292Z
Learning: In the snakemake-executor-plugin-slurm repository, the create_efficiency_report method in snakemake_executor_plugin_slurm/__init__.py contains temporary testing/debugging code that logs directory contents using os.listdir() and directory paths. This code was inserted for testing purposes only and should be removed from the production implementation.
Applied to files:
snakemake_executor_plugin_slurm/__init__.py
🧬 Code graph analysis (1)
snakemake_executor_plugin_slurm/__init__.py (1)
snakemake_executor_plugin_slurm/job_status_query.py (5)
get_min_job_age(7-60)is_query_tool_available(63-74)should_recommend_squeue_status_command(77-101)query_job_status_squeue(136-157)query_job_status_sacct(104-133)
🪛 Ruff (0.14.5)
snakemake_executor_plugin_slurm/__init__.py
59-63: Avoid specifying long messages outside the exception class
(TRY003)
590-590: Avoid specifying long messages outside the exception class
(TRY003)
666-666: Loop control variable i not used within loop body
Rename unused i to _i
(B007)
⏰ 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/__init__.py (3)
395-416: LGTM!The shutdown flow is well-structured: invokes parent cleanup first, then removes old logs, and finally generates an optional efficiency report. The conditional logic for efficiency reporting is clear and correct.
417-447: LGTM!The log cleanup implementation is robust: early returns for skip conditions, proper time calculations, appropriate error handling, and cleanup of empty directories. The logic correctly handles the age cutoff and the keep_successful_logs flag.
486-604: Improved documentation and messaging.The updated comments (lines 486-494, 497-499) and reformatted warning messages (lines 537-546) enhance code clarity without changing behavior. The assertion message clearly explains the %j placeholder constraint, and the run_uuid usage comment now correctly reflects support for both sacct and squeue.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
snakemake_executor_plugin_slurm/__init__.py (1)
70-107: Remove dynamic logic from _get_status_command_help() to prevent import-time subprocess execution.Past review comments referencing commits 7220326 and 8d9624a are incorrect—those commits only fixed formatting and did not address the underlying issue. The function still executes
_get_status_command_default(),is_query_tool_available(), andshould_recommend_squeue_status_command()at import time (via field metadata at line 230), causing subprocess calls during module initialization. This must be refactored to static help text:def _get_status_command_help(): - """Get help text with computed default.""" - default_cmd = _get_status_command_default() - sacct_available = is_query_tool_available("sacct") - squeue_recommended = should_recommend_squeue_status_command() - - base_help = "Command to query job status. Options: 'sacct', 'squeue'. " - - if default_cmd == "sacct": - if sacct_available and not squeue_recommended: - info = ( - "'sacct' detected and will be used " - "(MinJobAge may be too low for reliable 'squeue' usage)" - ) - else: - info = "'sacct' detected and will be used" - else: # default_cmd == "squeue" - if squeue_recommended: - # cumbersome, due to black and the need to stay below 80 chars - msg_part1 = "'squeue' recommended (MinJobAge is sufficient )" - msg_part2 = " for reliable usage" - info = msg_part1 + msg_part2 - elif not sacct_available: - info = ( - "'sacct' not available, falling back to 'squeue'. " - "WARNING: 'squeue' may not work reliably if MinJobAge is " - "too low" - ) - else: - info = ( - "'squeue' will be used. " - "WARNING: MinJobAge may be too low for reliable 'squeue' usage" - ) - - return ( - f"{base_help}Default: '{default_cmd}' ({info}). " - f"Set explicitly to override auto-detection." - ) + """Help text for status_command field.""" + return ( + "Command to query job status. Options: 'sacct' (preferred), 'squeue'. " + "Default is chosen dynamically: 'sacct' if available, otherwise 'squeue'. " + "Note: 'squeue' reliability depends on MinJobAge; see plugin documentation." + )
🧹 Nitpick comments (2)
snakemake_executor_plugin_slurm/__init__.py (2)
362-373: Fix awkward word hyphenation in warning message.The word "administrator" is split with a hyphen across lines 367-368 ("admini-" / "strator"), which is unusual for Python string literals. Consider rewording for better readability.
Apply this diff:
self.logger.warning( f"MinJobAge is {min_job_age} seconds " f"(< {dynamic_check_threshold}s). " f"This may cause 'squeue' to miss recently finished jobs " - "that have been purged from slurmctld, leading to job " - "status queries being impossible with 'squeue'. " - "Consider using 'sacct' instead or let your admini- " - "strator increase MinJobAge. " + "that have been purged from slurmctld, leading to job " + "status queries being impossible with 'squeue'. " + "Consider using 'sacct' instead or ask your " + "administrator to increase MinJobAge. " "(Threshold is 3x status check interval: 3 x " f"{initial_interval}s = " f"{dynamic_check_threshold}s)" )
668-668: Use_for intentionally unused loop variable.The loop variable
iis not used within the loop body. Use_to indicate it's intentionally unused.Apply this diff:
- for i in range(status_attempts): + for _ in range(status_attempts):Based on static analysis hints.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
snakemake_executor_plugin_slurm/__init__.py(17 hunks)
🧰 Additional context used
🧠 Learnings (13)
📓 Common learnings
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 0
File: :0-0
Timestamp: 2025-01-13T09:54:22.950Z
Learning: PR #173 (adding gres resource specification) depends on PR #28 in snakemake-executor-plugin-slurm-jobstep repository, as changes were required in the cpu-settings function of the jobstep-Executor module.
Learnt from: johanneskoester
Repo: snakemake/snakemake-executor-plugin-slurm PR: 173
File: docs/further.md:96-96
Timestamp: 2025-03-10T15:20:51.829Z
Learning: PR #173 in snakemake-executor-plugin-slurm implements GPU job support by adding resources: `gres` for generic resource specifications (e.g., 'gpu:1'), `gpu`/`gpus` for specifying GPU counts, and `gpu_model`/`gpu_manufacturer` for specifying GPU types, allowing users to request GPU resources directly rather than having to use slurm_extra.
📚 Learning: 2024-12-11T14:17:08.749Z
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 178
File: snakemake_executor_plugin_slurm/__init__.py:0-0
Timestamp: 2024-12-11T14:17:08.749Z
Learning: In the `snakemake-executor-plugin-slurm` project, when handling exceptions in `snakemake_executor_plugin_slurm/__init__.py`, prefer concise error messages and avoid unnecessary verbosity or exception chaining when it's not necessary.
Applied to files:
snakemake_executor_plugin_slurm/__init__.py
📚 Learning: 2025-03-10T15:20:51.829Z
Learnt from: johanneskoester
Repo: snakemake/snakemake-executor-plugin-slurm PR: 173
File: docs/further.md:96-96
Timestamp: 2025-03-10T15:20:51.829Z
Learning: PR #173 in snakemake-executor-plugin-slurm implements GPU job support by adding resources: `gres` for generic resource specifications (e.g., 'gpu:1'), `gpu`/`gpus` for specifying GPU counts, and `gpu_model`/`gpu_manufacturer` for specifying GPU types, allowing users to request GPU resources directly rather than having to use slurm_extra.
Applied to files:
snakemake_executor_plugin_slurm/__init__.py
📚 Learning: 2025-01-13T09:54:22.950Z
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 0
File: :0-0
Timestamp: 2025-01-13T09:54:22.950Z
Learning: PR #173 (adding gres resource specification) depends on PR #28 in snakemake-executor-plugin-slurm-jobstep repository, as changes were required in the cpu-settings function of the jobstep-Executor module.
Applied to files:
snakemake_executor_plugin_slurm/__init__.py
📚 Learning: 2025-01-20T09:13:26.443Z
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 193
File: .github/workflows/post_to_mastodon.yml:26-26
Timestamp: 2025-01-20T09:13:26.443Z
Learning: In the snakemake-executor-plugin-slurm repository, release PRs follow the naming pattern "chore(main): release X.Y.Z" where X.Y.Z is the version number.
Applied to files:
snakemake_executor_plugin_slurm/__init__.py
📚 Learning: 2025-03-31T11:52:05.373Z
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 249
File: snakemake_executor_plugin_slurm/submit_string.py:21-29
Timestamp: 2025-03-31T11:52:05.373Z
Learning: In the snakemake-executor-plugin-slurm project, account and partition parameters are validated in the submit function before reaching the get_submit_command function. By design, these parameters are either empty strings or already properly formatted with SLURM parameter tags, making additional validation in get_submit_command unnecessary.
Applied to files:
snakemake_executor_plugin_slurm/__init__.py
📚 Learning: 2025-06-24T12:41:31.837Z
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 221
File: tests/tests.py:62-70
Timestamp: 2025-06-24T12:41:31.837Z
Learning: In the snakemake-executor-plugin-slurm efficiency report feature, the executor is responsible for creating the efficiency_report_path directory when generating efficiency reports. Tests should not manually check for directory existence but should rely on the executor to properly create necessary directories as part of its normal operation.
Applied to files:
snakemake_executor_plugin_slurm/__init__.py
📚 Learning: 2025-01-17T17:27:32.446Z
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 187
File: .github/workflows/post_to_mastodon.yml:0-0
Timestamp: 2025-01-17T17:27:32.446Z
Learning: In the mastodon publishing workflow for snakemake-executor-plugin-slurm, the PR_TITLE environment variable is required by the post_to_mastodon.sh script and should be preserved.
Applied to files:
snakemake_executor_plugin_slurm/__init__.py
📚 Learning: 2025-07-22T17:31:41.629Z
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 333
File: .github/workflows/announce-release.yml:0-0
Timestamp: 2025-07-22T17:31:41.629Z
Learning: In the snakemake-executor-plugin-slurm repository, cmeesters prefers using `main` references for GitHub Actions to always get the latest version, with security provided through protected branches and mandatory CI checks rather than commit SHA pinning.
Applied to files:
snakemake_executor_plugin_slurm/__init__.py
📚 Learning: 2025-09-09T15:55:55.669Z
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 0
File: :0-0
Timestamp: 2025-09-09T15:55:55.669Z
Learning: In the Snakemake SLURM executor plugin, Snakemake submits itself as a job using the --wrap parameter in the sbatch command, rather than creating or submitting a separate job.sh script file. The actual implementation uses `call += f' --wrap="{exec_job}"'` where exec_job is the formatted snakemake execution command.
Applied to files:
snakemake_executor_plugin_slurm/__init__.py
📚 Learning: 2025-09-16T17:25:38.450Z
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 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/__init__.py
📚 Learning: 2025-06-16T08:54:07.957Z
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 221
File: snakemake_executor_plugin_slurm/efficiency_report.py:0-0
Timestamp: 2025-06-16T08:54:07.957Z
Learning: In the context of SLURM executor plugin development, user cmeesters (experienced with SLURM) noted they are not aware of SLURM typically reporting memory values with decimals (like 1.5G), but accepted defensive programming to handle such cases anyway.
Applied to files:
snakemake_executor_plugin_slurm/__init__.py
📚 Learning: 2025-06-03T17:10:51.292Z
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 221
File: snakemake_executor_plugin_slurm/__init__.py:0-0
Timestamp: 2025-06-03T17:10:51.292Z
Learning: In the snakemake-executor-plugin-slurm repository, the create_efficiency_report method in snakemake_executor_plugin_slurm/__init__.py contains temporary testing/debugging code that logs directory contents using os.listdir() and directory paths. This code was inserted for testing purposes only and should be removed from the production implementation.
Applied to files:
snakemake_executor_plugin_slurm/__init__.py
🧬 Code graph analysis (1)
snakemake_executor_plugin_slurm/__init__.py (1)
snakemake_executor_plugin_slurm/job_status_query.py (5)
get_min_job_age(7-60)is_query_tool_available(63-74)should_recommend_squeue_status_command(77-101)query_job_status_squeue(136-157)query_job_status_sacct(104-133)
🪛 Ruff (0.14.5)
snakemake_executor_plugin_slurm/__init__.py
59-63: Avoid specifying long messages outside the exception class
(TRY003)
592-592: Avoid specifying long messages outside the exception class
(TRY003)
668-668: Loop control variable i not used within loop body
Rename unused i to _i
(B007)
⏰ 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
Corrected a typo in the comment regarding command generation.
There was a problem hiding this comment.
Actionable comments posted: 2
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)
810-816: Update docstring to reflect both query tools and fix missing spaces in error messages.The review comment is accurate. The code at lines 661-664 shows that
status_commandcan be either asacctorsqueuecommand before being passed tojob_stati. The docstring at line 811 should be updated from "with sacct" to "via sacct or squeue" for clarity.Additionally, the error message at lines 848-850 (and similar warning at lines 843-846) lacks spacing between concatenated strings:
self.logger.error( f"The job status query failed with command '{command}'" f"Error message: '{error_message}'" "This error message is not expected, please report it back to us." )This produces output like
"...'{command}'Error message:..."without proper spacing. The suggested fix with periods and spaces is appropriate.
♻️ Duplicate comments (3)
snakemake_executor_plugin_slurm/__init__.py (2)
70-107: Avoid running external probes at import time in_get_status_command_help.
_get_status_command_help()currently calls_get_status_command_default(),is_query_tool_available(), andshould_recommend_squeue_status_command(), and its result is evaluated at import time in thestatus_commandfield metadata. That meanswhich,scontrol show config, etc. are all invoked just to render help text, which can slow or even fail imports on misconfigured clusters.Given you already perform dynamic checks when actually validating/using
status_command, making the help string static keeps imports cheap and side-effect-free. For example:-def _get_status_command_help(): - """Get help text with computed default.""" - default_cmd = _get_status_command_default() - sacct_available = is_query_tool_available("sacct") - squeue_recommended = should_recommend_squeue_status_command() - - base_help = "Command to query job status. Options: 'sacct', 'squeue'. " - - ... - - return ( - f"{base_help}Default: '{default_cmd}' ({info}). " - f"Set explicitly to override auto-detection." - ) +def _get_status_command_help(): + """Help: job status command.""" + return ( + "Command to query job status. Options: 'sacct' (preferred), 'squeue'. " + "Default is chosen dynamically: 'sacct' if available, otherwise 'squeue'. " + "Note: 'squeue' reliability depends on MinJobAge; see plugin docs." + )This keeps the CLI description accurate without any import-time subprocess calls.
Also applies to: 227-234
656-664: Update comment to reflect that the command is a string, not a list (and fix typo).After the refactor to have
query_job_status_sacct/query_job_status_squeuereturn strings, the comment here is now outdated and still says “commad will be a list generated with shlex.split()”, with a typo.Consider simplifying it:
- # Getting the actual command with parameters. - # Here, the commad will be a list generated with - # shlex.split(). + # Build the concrete status-query command string for the selected tool.This matches the current helpers and avoids confusion.
snakemake_executor_plugin_slurm/job_status_query.py (1)
104-132: Fix type hints and docstrings forquery_job_status_sacct/query_job_status_squeue.Both functions now correctly return strings (command lines), but they are still annotated as
-> listand their docstrings say they return a dict of job IDs toJobStatus. This contradicts the implementation and earlier review feedback.Consider aligning signature and docs like this:
-def query_job_status_sacct(runid) -> list: - """ - Query job status using sacct command - - Args: - runid: workflow run ID - - Returns: - Dictionary mapping job ID to JobStatus object - """ +def query_job_status_sacct(runid: str) -> str: + """ + Build a `sacct` command string to query job status for a workflow run. + + Args: + runid: Workflow run ID (used as SLURM job name filter). + + Returns: + Shell command string for the `sacct` query (to be passed to subprocess). + """ @@ -def query_job_status_squeue(runid) -> list: - """ - Query job status using squeue command (newer SLURM functionality) - - Args: - runid: workflow run ID - - Returns: - Dictionary mapping job ID to JobStatus object - """ +def query_job_status_squeue(runid: str) -> str: + """ + Build an `squeue` command string to query job status for a workflow run. + + Args: + runid: Workflow run ID (used as SLURM job name filter). + + Returns: + Shell command string for the `squeue` query (to be passed to subprocess). + """This keeps type-checkers and future readers in sync with how
job_stati()actually uses these helpers.Also applies to: 135-154
🧹 Nitpick comments (1)
snakemake_executor_plugin_slurm/job_status_query.py (1)
63-75: Generalizeis_query_tool_availableand avoid spawningwhich.Functionally this works, but two small improvements would help:
- The docstring is hard-coded to
sacct, while the function is generic overtool_name.- You can avoid an external
whichsubprocess (and the S603 warning) by usingshutil.which, which is simpler and more portable.Example refactor:
-import subprocess +import subprocess +import shutil @@ -def is_query_tool_available(tool_name): - """ - Check if the sacct command is available on the system. - Returns True if sacct is available, False otherwise. - """ - cmd = f"which {tool_name}" - cmd = shlex.split(cmd) - try: - subprocess.check_output(cmd, stderr=subprocess.PIPE) - return True - except subprocess.CalledProcessError: - return False +def is_query_tool_available(tool_name: str) -> bool: + """ + Check if a given SLURM command-line tool is available on the system. + Returns True if the tool is found on PATH, False otherwise. + """ + return shutil.which(tool_name) is not None
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
pyproject.toml(1 hunks)snakemake_executor_plugin_slurm/__init__.py(17 hunks)snakemake_executor_plugin_slurm/job_status_query.py(1 hunks)snakemake_executor_plugin_slurm/partitions.py(2 hunks)
🧰 Additional context used
🧠 Learnings (18)
📓 Common learnings
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 0
File: :0-0
Timestamp: 2025-01-13T09:54:22.950Z
Learning: PR #173 (adding gres resource specification) depends on PR #28 in snakemake-executor-plugin-slurm-jobstep repository, as changes were required in the cpu-settings function of the jobstep-Executor module.
Learnt from: johanneskoester
Repo: snakemake/snakemake-executor-plugin-slurm PR: 173
File: docs/further.md:96-96
Timestamp: 2025-03-10T15:20:51.829Z
Learning: PR #173 in snakemake-executor-plugin-slurm implements GPU job support by adding resources: `gres` for generic resource specifications (e.g., 'gpu:1'), `gpu`/`gpus` for specifying GPU counts, and `gpu_model`/`gpu_manufacturer` for specifying GPU types, allowing users to request GPU resources directly rather than having to use slurm_extra.
📚 Learning: 2025-03-10T15:20:51.829Z
Learnt from: johanneskoester
Repo: snakemake/snakemake-executor-plugin-slurm PR: 173
File: docs/further.md:96-96
Timestamp: 2025-03-10T15:20:51.829Z
Learning: PR #173 in snakemake-executor-plugin-slurm implements GPU job support by adding resources: `gres` for generic resource specifications (e.g., 'gpu:1'), `gpu`/`gpus` for specifying GPU counts, and `gpu_model`/`gpu_manufacturer` for specifying GPU types, allowing users to request GPU resources directly rather than having to use slurm_extra.
Applied to files:
snakemake_executor_plugin_slurm/partitions.pysnakemake_executor_plugin_slurm/job_status_query.pysnakemake_executor_plugin_slurm/__init__.py
📚 Learning: 2025-03-10T13:34:59.270Z
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 173
File: tests/tests.py:79-86
Timestamp: 2025-03-10T13:34:59.270Z
Learning: In the Snakemake executor plugin for SLURM, all GPU resources (both simple numeric values and model:number specifications) should be translated to the `--gpus` flag (plural form) to match SLURM's command line interface expectations.
Applied to files:
snakemake_executor_plugin_slurm/partitions.py
📚 Learning: 2025-03-10T13:34:59.270Z
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 173
File: tests/tests.py:79-86
Timestamp: 2025-03-10T13:34:59.270Z
Learning: In the Snakemake executor plugin for SLURM, all GPU resources (even simple numeric ones) should be translated to the `--gpus` flag (plural) to match SLURM's expectations, not `--gpu` (singular).
Applied to files:
snakemake_executor_plugin_slurm/partitions.py
📚 Learning: 2025-05-26T12:22:07.005Z
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 221
File: snakemake_executor_plugin_slurm/efficiency_report.py:34-49
Timestamp: 2025-05-26T12:22:07.005Z
Learning: In the parse_reqmem function in snakemake_executor_plugin_slurm/efficiency_report.py, the conditional check for per_unit == "/node" is necessary for proper conversion of memory values in multi-node jobs, not redundant as it might initially appear.
Applied to files:
snakemake_executor_plugin_slurm/partitions.py
📚 Learning: 2025-03-31T11:52:05.373Z
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 249
File: snakemake_executor_plugin_slurm/submit_string.py:21-29
Timestamp: 2025-03-31T11:52:05.373Z
Learning: In the snakemake-executor-plugin-slurm project, account and partition parameters are validated in the submit function before reaching the get_submit_command function. By design, these parameters are either empty strings or already properly formatted with SLURM parameter tags, making additional validation in get_submit_command unnecessary.
Applied to files:
snakemake_executor_plugin_slurm/partitions.pysnakemake_executor_plugin_slurm/__init__.py
📚 Learning: 2025-01-13T09:54:22.950Z
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 0
File: :0-0
Timestamp: 2025-01-13T09:54:22.950Z
Learning: PR #173 (adding gres resource specification) depends on PR #28 in snakemake-executor-plugin-slurm-jobstep repository, as changes were required in the cpu-settings function of the jobstep-Executor module.
Applied to files:
snakemake_executor_plugin_slurm/partitions.pysnakemake_executor_plugin_slurm/job_status_query.pysnakemake_executor_plugin_slurm/__init__.py
📚 Learning: 2024-12-11T14:17:08.749Z
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 178
File: snakemake_executor_plugin_slurm/__init__.py:0-0
Timestamp: 2024-12-11T14:17:08.749Z
Learning: In the `snakemake-executor-plugin-slurm` project, when handling exceptions in `snakemake_executor_plugin_slurm/__init__.py`, prefer concise error messages and avoid unnecessary verbosity or exception chaining when it's not necessary.
Applied to files:
snakemake_executor_plugin_slurm/partitions.pysnakemake_executor_plugin_slurm/job_status_query.pysnakemake_executor_plugin_slurm/__init__.py
📚 Learning: 2025-03-10T13:22:54.081Z
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 173
File: tests/tests.py:79-86
Timestamp: 2025-03-10T13:22:54.081Z
Learning: In the Snakemake executor plugin for SLURM, when using GPU resources, the function returns `--gpu=n` (singular) when only a number is provided, but `--gpus=model:n` (plural) when a model is specified in the implementation.
Applied to files:
snakemake_executor_plugin_slurm/partitions.py
📚 Learning: 2025-02-18T14:40:27.064Z
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 173
File: snakemake_executor_plugin_slurm/__init__.py:0-0
Timestamp: 2025-02-18T14:40:27.064Z
Learning: In the Snakemake executor plugin for SLURM, the GPU resource is specified using the "gpus" (plural) resource name, not "gpu" (singular).
Applied to files:
snakemake_executor_plugin_slurm/partitions.py
📚 Learning: 2025-01-20T09:13:26.443Z
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 193
File: .github/workflows/post_to_mastodon.yml:26-26
Timestamp: 2025-01-20T09:13:26.443Z
Learning: In the snakemake-executor-plugin-slurm repository, release PRs follow the naming pattern "chore(main): release X.Y.Z" where X.Y.Z is the version number.
Applied to files:
snakemake_executor_plugin_slurm/partitions.pysnakemake_executor_plugin_slurm/job_status_query.pysnakemake_executor_plugin_slurm/__init__.py
📚 Learning: 2025-02-18T14:49:42.624Z
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 173
File: snakemake_executor_plugin_slurm/utils.py:75-100
Timestamp: 2025-02-18T14:49:42.624Z
Learning: In the Snakemake SLURM executor plugin, users must specify either `gres` or `gpus` for GPU resources, but not both simultaneously, as these are mutually exclusive ways to request GPU resources.
Applied to files:
snakemake_executor_plugin_slurm/partitions.py
📚 Learning: 2025-01-17T17:27:32.446Z
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 187
File: .github/workflows/post_to_mastodon.yml:0-0
Timestamp: 2025-01-17T17:27:32.446Z
Learning: In the mastodon publishing workflow for snakemake-executor-plugin-slurm, the PR_TITLE environment variable is required by the post_to_mastodon.sh script and should be preserved.
Applied to files:
snakemake_executor_plugin_slurm/job_status_query.pysnakemake_executor_plugin_slurm/__init__.py
📚 Learning: 2025-06-24T12:41:31.837Z
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 221
File: tests/tests.py:62-70
Timestamp: 2025-06-24T12:41:31.837Z
Learning: In the snakemake-executor-plugin-slurm efficiency report feature, the executor is responsible for creating the efficiency_report_path directory when generating efficiency reports. Tests should not manually check for directory existence but should rely on the executor to properly create necessary directories as part of its normal operation.
Applied to files:
snakemake_executor_plugin_slurm/__init__.py
📚 Learning: 2025-09-16T17:25:38.450Z
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 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/__init__.py
📚 Learning: 2025-07-22T17:31:41.629Z
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 333
File: .github/workflows/announce-release.yml:0-0
Timestamp: 2025-07-22T17:31:41.629Z
Learning: In the snakemake-executor-plugin-slurm repository, cmeesters prefers using `main` references for GitHub Actions to always get the latest version, with security provided through protected branches and mandatory CI checks rather than commit SHA pinning.
Applied to files:
snakemake_executor_plugin_slurm/__init__.py
📚 Learning: 2025-06-16T08:54:07.957Z
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 221
File: snakemake_executor_plugin_slurm/efficiency_report.py:0-0
Timestamp: 2025-06-16T08:54:07.957Z
Learning: In the context of SLURM executor plugin development, user cmeesters (experienced with SLURM) noted they are not aware of SLURM typically reporting memory values with decimals (like 1.5G), but accepted defensive programming to handle such cases anyway.
Applied to files:
snakemake_executor_plugin_slurm/__init__.py
📚 Learning: 2025-06-03T17:10:51.292Z
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 221
File: snakemake_executor_plugin_slurm/__init__.py:0-0
Timestamp: 2025-06-03T17:10:51.292Z
Learning: In the snakemake-executor-plugin-slurm repository, the create_efficiency_report method in snakemake_executor_plugin_slurm/__init__.py contains temporary testing/debugging code that logs directory contents using os.listdir() and directory paths. This code was inserted for testing purposes only and should be removed from the production implementation.
Applied to files:
snakemake_executor_plugin_slurm/__init__.py
🧬 Code graph analysis (1)
snakemake_executor_plugin_slurm/__init__.py (4)
snakemake_executor_plugin_slurm/job_status_query.py (5)
get_min_job_age(7-60)is_query_tool_available(63-74)should_recommend_squeue_status_command(77-101)query_job_status_squeue(135-154)query_job_status_sacct(104-132)snakemake_executor_plugin_slurm/efficiency_report.py (1)
create_efficiency_report(233-262)snakemake_executor_plugin_slurm/submit_string.py (1)
get_submit_command(19-130)snakemake_executor_plugin_slurm/validation.py (1)
validate_slurm_extra(48-75)
🪛 Ruff (0.14.6)
snakemake_executor_plugin_slurm/job_status_query.py
17-17: subprocess call: check for execution of untrusted input
(S603)
71-71: subprocess call: check for execution of untrusted input
(S603)
72-72: Consider moving this statement to an else block
(TRY300)
snakemake_executor_plugin_slurm/__init__.py
59-63: Avoid specifying long messages outside the exception class
(TRY003)
592-592: Avoid specifying long messages outside the exception class
(TRY003)
668-668: Loop control variable i not used within loop body
Rename unused i to _i
(B007)
⏰ 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 (5)
pyproject.toml (1)
36-38: Black configuration properly configured to support consistent formatting.The Black configuration aligns with the project's Python 3.11 target and supports the formatting changes across the refactored modules in this PR.
snakemake_executor_plugin_slurm/partitions.py (2)
107-109: LGTM: Formatting improvement for readability.The multi-line formatting of the list comprehension improves readability while preserving the original logic.
359-362: LGTM: Formatting improvement for readability.The multi-line formatting of the
all()predicate improves readability while preserving the original logic.snakemake_executor_plugin_slurm/job_status_query.py (1)
7-25: MinJobAge parsing and timeout handling look good.You now bound
scontrol show configwith a timeout and catchCalledProcessError,FileNotFoundError, andTimeoutExpired, and the regex/unit handling covers the documented cases (s,h, or bare seconds). Ruff’s S603 here is effectively a false positive since the command is a fixed literal, so I would keep this as-is.Also applies to: 27-60
snakemake_executor_plugin_slurm/__init__.py (1)
192-225: Executor settings, efficiency report, and log cleanup wiring look consistent.The new
efficiency_report*fields, hook inshutdown(), andclean_old_logs()behavior match the expectations fromefficiency_report.create_efficiency_report()(directory creation is handled there) and keep log handling localized to the executor. I don’t see functional issues here; if you ever add more settings interdependencies,ExecutorSettings.__post_init__is a good central place for validation.Based on learnings, this matches the earlier efficiency-report design.
Also applies to: 397-417, 419-448
| status_attempts: Optional[int] = field( | ||
| default=5, | ||
| metadata={ | ||
| "help": "Defines the number of attempts to query the status of " | ||
| "all active jobs. If the status query fails, the next attempt " | ||
| "will be performed after the next status check interval. " | ||
| "The default is 5 status attempts before giving up. The maximum " | ||
| "time between status checks is 180 seconds.", | ||
| "env_var": False, | ||
| "required": False, | ||
| }, | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
wc -l snakemake_executor_plugin_slurm/__init__.pyRepository: snakemake/snakemake-executor-plugin-slurm
Length of output: 133
🏁 Script executed:
# Get the size and then read around the problematic lines
head -n 750 snakemake_executor_plugin_slurm/__init__.py | tail -n 150 > /tmp/context.txt
cat /tmp/context.txt | head -100Repository: snakemake/snakemake-executor-plugin-slurm
Length of output: 4357
🏁 Script executed:
sed -n '680,730p' snakemake_executor_plugin_slurm/__init__.pyRepository: snakemake/snakemake-executor-plugin-slurm
Length of output: 2576
🏁 Script executed:
sed -n '620,660p' snakemake_executor_plugin_slurm/__init__.pyRepository: snakemake/snakemake-executor-plugin-slurm
Length of output: 1689
🏁 Script executed:
sed -n '236,247p' snakemake_executor_plugin_slurm/__init__.pyRepository: snakemake/snakemake-executor-plugin-slurm
Length of output: 604
🏁 Script executed:
sed -n '710,715p' snakemake_executor_plugin_slurm/__init__.pyRepository: snakemake/snakemake-executor-plugin-slurm
Length of output: 277
Perfect. I have verified the bug claim comprehensively. The review comment is accurate and the issue is real. Let me provide the final rewritten review comment:
Guard against invalid status_attempts values and prevent UnboundLocalError.
The status_attempts field is Optional[int] and lacks input validation. If a user sets it to 0, a negative value, or None, the for i in range(status_attempts) loop never executes, leaving status_of_jobs and sacct_query_duration uninitialized. This causes an UnboundLocalError at line 712 when the code tries to check if status_of_jobs is not None:.
Initialize these variables before the loop and add validation to ensure at least one attempt occurs:
sacct_query_durations = []
-
- status_attempts = self.workflow.executor_settings.status_attempts
+ # Ensure we always perform at least one status query and have
+ # well-defined locals even if misconfigured.
+ status_of_jobs = None
+ sacct_query_duration = None
+ raw_status_attempts = self.workflow.executor_settings.status_attempts
+ try:
+ status_attempts = int(raw_status_attempts)
+ except (TypeError, ValueError):
+ self.logger.warning(
+ f"Invalid status_attempts={raw_status_attempts!r}; falling back to 5."
+ )
+ status_attempts = 5
+ status_attempts = max(1, status_attempts)
- for i in range(status_attempts):
+ for _attempt in range(status_attempts):This prevents crashes from misconfiguration and addresses Ruff's B007 warning about the unused loop variable.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In snakemake_executor_plugin_slurm/__init__.py around lines 236-247 (and the
loop usage later around line 712), the Optional[int] status_attempts lacks
validation and the loop can never run (0/None/negative), leaving status_of_jobs
and sacct_query_duration uninitialized and causing UnboundLocalError; fix by
validating/coercing status_attempts to an integer >= 1 before use (e.g., if None
or <1 set to 1), initialize status_of_jobs and sacct_query_duration to safe
default values before the loop, and ensure the loop variable is either used or
replaced with a convention (e.g., _ ) to address the unused-variable warning.
There was a problem hiding this comment.
Actionable comments posted: 0
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)
810-852: Fix spacing in job-stati error logs and update docstring for squeue support.Two minor but user-visible issues here:
- The docstring still says “with sacct”, although
commandcan now be either sacct or squeue.- The concatenated log strings in both exception branches are missing spaces between the
"Error message: ..."part and the following sentence, producing slightly mangled log output.Given prior guidance to keep error messages clear but not overly verbose, tightening these up is worthwhile.
Suggested adjustments:
- async def job_stati(self, command): - """Obtain SLURM job status of all submitted jobs with sacct + async def job_stati(self, command): + """Obtain SLURM job status of all submitted jobs via sacct/squeue. @@ - self.logger.warning( - "The SLURM database might not be available ... " - f"Error message: '{error_message}'" - "This error message indicates that the SLURM database is " - "currently not available. This is not an error of the " - "Snakemake plugin, but some kind of server issue. " - "Please consult with your HPC provider." - ) + self.logger.warning( + "The SLURM database might not be available ... " + f"Error message: '{error_message}'. " + "This error message indicates that the SLURM database is " + "currently not available. This is not an error of the " + "Snakemake plugin, but some kind of server issue. " + "Please consult with your HPC provider." + ) @@ - self.logger.error( - f"The job status query failed with command '{command}'" - f"Error message: '{error_message}'" - "This error message is not expected, please report it back to us." - ) + self.logger.error( + f"The job status query failed with command '{command}'. " + f"Error message: '{error_message}'. " + "This error message is not expected, please report it back to us." + )This keeps the messages concise but more readable.
Based on learnings, this aligns with the preference for clear, non-verbose error handling.
♻️ Duplicate comments (3)
snakemake_executor_plugin_slurm/__init__.py (3)
52-68: Avoid running external SLURM probes at import time for help text.
_get_status_command_help()currently calls_get_status_command_default(),is_query_tool_available(), andshould_recommend_squeue_status_command(). Because it is invoked inExecutorSettings.status_command.metadata["help"], these subprocess-based probes (which,scontrol show config) all run at import time, and can:
- Slow down any process that merely imports the plugin.
- Fail on systems without SLURM tools, raising
WorkflowErrorvia_get_status_command_default().Help text does not need live cluster inspection; a static description is safer and matches earlier review guidance.
A minimal, import-safe change is:
-def _get_status_command_help(): - """Get help text with computed default.""" - default_cmd = _get_status_command_default() - sacct_available = is_query_tool_available("sacct") - squeue_recommended = should_recommend_squeue_status_command() - - base_help = "Command to query job status. Options: 'sacct', 'squeue'. " - - if default_cmd == "sacct": - if sacct_available and not squeue_recommended: - info = ( - "'sacct' detected and will be used " - "(MinJobAge may be too low for reliable 'squeue' usage)" - ) - else: - info = "'sacct' detected and will be used" - else: # default_cmd == "squeue" - if squeue_recommended: - # cumbersome, due to black and the need to stay below 80 chars - msg_part1 = "'squeue' recommended (MinJobAge is sufficient )" - msg_part2 = " for reliable usage" - info = msg_part1 + msg_part2 - elif not sacct_available: - info = ( - "'sacct' not available, falling back to 'squeue'. " - "WARNING: 'squeue' may not work reliably if MinJobAge is " - "too low" - ) - else: - info = ( - "'squeue' will be used. " - "WARNING: MinJobAge may be too low for reliable 'squeue' usage" - ) - - return ( - f"{base_help}Default: '{default_cmd}' ({info}). " - f"Set explicitly to override auto-detection." - ) +def _get_status_command_help(): + """Help: job status query command.""" + return ( + "Command to query job status. Options: 'sacct' (preferred), 'squeue'. " + "The default is chosen dynamically at runtime: 'sacct' if available, " + "otherwise 'squeue'. Note: 'squeue' reliability depends on MinJobAge; " + "see the plugin documentation for details." + )With this change, the
metadata["help"]call at lines 227–234 remains safe while default selection continues to be handled by_get_status_command_default()at runtime.Also applies to: 70-107, 227-234
236-247: Validatestatus_attempts, initialize locals, and fix unused loop variable.As written, misconfigured
status_attemptscan crashcheck_active_jobs:
- If
status_attemptsis0, negative, orNone, thefor i in range(status_attempts)loop never executes, leavingstatus_of_jobsandsacct_query_durationuninitialized.- Line 712 then does
if status_of_jobs is not None:, which will raiseUnboundLocalError.- Ruff also rightfully flags the unused
iloop variable.Given
status_attemptsis anOptional[int]user setting, this is a realistic misconfiguration path.You can harden this block and address B007 in one go:
- sacct_query_durations = [] - - status_attempts = self.workflow.executor_settings.status_attempts - self.logger.debug( - f"Checking the status of {len(active_jobs)} active jobs " - f"with {status_attempts} attempts." - ) + sacct_query_durations = [] + + # Ensure robust defaults even if status_attempts is misconfigured. + status_of_jobs = None + sacct_query_duration = None + raw_status_attempts = self.workflow.executor_settings.status_attempts + try: + status_attempts = int(raw_status_attempts) + except (TypeError, ValueError): + self.logger.warning( + f"Invalid status_attempts={raw_status_attempts!r}; falling back to 5." + ) + status_attempts = 5 + status_attempts = max(1, status_attempts) + + self.logger.debug( + f"Checking the status of {len(active_jobs)} active jobs " + f"with {status_attempts} attempts." + ) @@ - # decide which status command to use + # decide which status command to use status_command = self.get_status_command() - # Getting the actual command with parameters. - # Here, the command will be a list generated with - # shlex.split(). + # Resolve the actual command with parameters as a shell string. @@ - for i in range(status_attempts): + for _attempt in range(status_attempts): @@ - if status_of_jobs is not None: + if status_of_jobs is not None:This guarantees at least one attempt, keeps the log message honest, prevents
UnboundLocalError, and removes the unused loop variable.Also applies to: 644-650, 668-701, 712-712
333-357: Clarify warning whenstatus_command='sacct'butsacctis unavailable.In
_validate_status_command_settings, the branchif not sacct_available and status_command == "sacct": self.logger.warning( "The 'sacct' command is not available on this system. " "Using 'squeue' instead for job status queries." )only logs but does not actually switch to
squeue; later,get_status_command()still returns"sacct", so status checks will continue to use a non-existent command. The message is therefore misleading.Either implement a real fallback (and ensure
check_active_jobsuses it), or reword the log:- self.logger.warning( - "The 'sacct' command is not available on this system. " - "Using 'squeue' instead for job status queries." - ) + self.logger.warning( + "The 'sacct' command is not available on this system, " + "but status_command is set to 'sacct'. " + "Status queries will fail unless you install 'sacct' or " + "set status_command='squeue'." + )This keeps behavior and user expectations aligned.
🧹 Nitpick comments (1)
snakemake_executor_plugin_slurm/__init__.py (1)
656-665: Status-command selection wiring looks correct; small comment mismatch only.The updated
check_active_jobs()now:
- Resolves
status_command = self.get_status_command().- Expands it via
query_job_status_sacct()/query_job_status_squeue()based on that value.- Passes the resulting shell command string into
job_stati().This properly hooks the new
status_commandsetting into actual status polling. The only nit is that the comment still mentions “command will be a list generated with shlex.split()”, while both query helpers currently return strings.If you touch this block again, consider updating the comment to avoid confusion:
- # Getting the actual command with parameters. - # Here, the command will be a list generated with - # shlex.split(). + # Getting the actual command with parameters as a shell string.Also applies to: 668-672
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
snakemake_executor_plugin_slurm/__init__.py(17 hunks)
🧰 Additional context used
🧠 Learnings (12)
📓 Common learnings
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 0
File: :0-0
Timestamp: 2025-01-13T09:54:22.950Z
Learning: PR #173 (adding gres resource specification) depends on PR #28 in snakemake-executor-plugin-slurm-jobstep repository, as changes were required in the cpu-settings function of the jobstep-Executor module.
Learnt from: johanneskoester
Repo: snakemake/snakemake-executor-plugin-slurm PR: 173
File: docs/further.md:96-96
Timestamp: 2025-03-10T15:20:51.829Z
Learning: PR #173 in snakemake-executor-plugin-slurm implements GPU job support by adding resources: `gres` for generic resource specifications (e.g., 'gpu:1'), `gpu`/`gpus` for specifying GPU counts, and `gpu_model`/`gpu_manufacturer` for specifying GPU types, allowing users to request GPU resources directly rather than having to use slurm_extra.
📚 Learning: 2024-12-11T14:17:08.749Z
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 178
File: snakemake_executor_plugin_slurm/__init__.py:0-0
Timestamp: 2024-12-11T14:17:08.749Z
Learning: In the `snakemake-executor-plugin-slurm` project, when handling exceptions in `snakemake_executor_plugin_slurm/__init__.py`, prefer concise error messages and avoid unnecessary verbosity or exception chaining when it's not necessary.
Applied to files:
snakemake_executor_plugin_slurm/__init__.py
📚 Learning: 2025-03-10T15:20:51.829Z
Learnt from: johanneskoester
Repo: snakemake/snakemake-executor-plugin-slurm PR: 173
File: docs/further.md:96-96
Timestamp: 2025-03-10T15:20:51.829Z
Learning: PR #173 in snakemake-executor-plugin-slurm implements GPU job support by adding resources: `gres` for generic resource specifications (e.g., 'gpu:1'), `gpu`/`gpus` for specifying GPU counts, and `gpu_model`/`gpu_manufacturer` for specifying GPU types, allowing users to request GPU resources directly rather than having to use slurm_extra.
Applied to files:
snakemake_executor_plugin_slurm/__init__.py
📚 Learning: 2025-01-13T09:54:22.950Z
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 0
File: :0-0
Timestamp: 2025-01-13T09:54:22.950Z
Learning: PR #173 (adding gres resource specification) depends on PR #28 in snakemake-executor-plugin-slurm-jobstep repository, as changes were required in the cpu-settings function of the jobstep-Executor module.
Applied to files:
snakemake_executor_plugin_slurm/__init__.py
📚 Learning: 2025-01-20T09:13:26.443Z
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 193
File: .github/workflows/post_to_mastodon.yml:26-26
Timestamp: 2025-01-20T09:13:26.443Z
Learning: In the snakemake-executor-plugin-slurm repository, release PRs follow the naming pattern "chore(main): release X.Y.Z" where X.Y.Z is the version number.
Applied to files:
snakemake_executor_plugin_slurm/__init__.py
📚 Learning: 2025-03-31T11:52:05.373Z
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 249
File: snakemake_executor_plugin_slurm/submit_string.py:21-29
Timestamp: 2025-03-31T11:52:05.373Z
Learning: In the snakemake-executor-plugin-slurm project, account and partition parameters are validated in the submit function before reaching the get_submit_command function. By design, these parameters are either empty strings or already properly formatted with SLURM parameter tags, making additional validation in get_submit_command unnecessary.
Applied to files:
snakemake_executor_plugin_slurm/__init__.py
📚 Learning: 2025-09-16T17:25:38.450Z
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 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/__init__.py
📚 Learning: 2025-01-17T17:27:32.446Z
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 187
File: .github/workflows/post_to_mastodon.yml:0-0
Timestamp: 2025-01-17T17:27:32.446Z
Learning: In the mastodon publishing workflow for snakemake-executor-plugin-slurm, the PR_TITLE environment variable is required by the post_to_mastodon.sh script and should be preserved.
Applied to files:
snakemake_executor_plugin_slurm/__init__.py
📚 Learning: 2025-07-22T17:31:41.629Z
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 333
File: .github/workflows/announce-release.yml:0-0
Timestamp: 2025-07-22T17:31:41.629Z
Learning: In the snakemake-executor-plugin-slurm repository, cmeesters prefers using `main` references for GitHub Actions to always get the latest version, with security provided through protected branches and mandatory CI checks rather than commit SHA pinning.
Applied to files:
snakemake_executor_plugin_slurm/__init__.py
📚 Learning: 2025-06-16T08:54:07.957Z
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 221
File: snakemake_executor_plugin_slurm/efficiency_report.py:0-0
Timestamp: 2025-06-16T08:54:07.957Z
Learning: In the context of SLURM executor plugin development, user cmeesters (experienced with SLURM) noted they are not aware of SLURM typically reporting memory values with decimals (like 1.5G), but accepted defensive programming to handle such cases anyway.
Applied to files:
snakemake_executor_plugin_slurm/__init__.py
📚 Learning: 2025-09-15T12:00:31.977Z
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 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/__init__.py
📚 Learning: 2025-06-03T17:10:51.292Z
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 221
File: snakemake_executor_plugin_slurm/__init__.py:0-0
Timestamp: 2025-06-03T17:10:51.292Z
Learning: In the snakemake-executor-plugin-slurm repository, the create_efficiency_report method in snakemake_executor_plugin_slurm/__init__.py contains temporary testing/debugging code that logs directory contents using os.listdir() and directory paths. This code was inserted for testing purposes only and should be removed from the production implementation.
Applied to files:
snakemake_executor_plugin_slurm/__init__.py
🧬 Code graph analysis (1)
snakemake_executor_plugin_slurm/__init__.py (1)
snakemake_executor_plugin_slurm/job_status_query.py (5)
get_min_job_age(7-60)is_query_tool_available(63-74)should_recommend_squeue_status_command(77-101)query_job_status_squeue(135-154)query_job_status_sacct(104-132)
🪛 Ruff (0.14.6)
snakemake_executor_plugin_slurm/__init__.py
59-63: Avoid specifying long messages outside the exception class
(TRY003)
592-592: Avoid specifying long messages outside the exception class
(TRY003)
668-668: Loop control variable i not used within loop body
Rename unused i to _i
(B007)
⏰ 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 (7)
snakemake_executor_plugin_slurm/__init__.py (7)
19-24: Imports and new job-status utilities are wired correctly.The split imports for
SubmittedJobInfo/RemoteExecutorand the new.job_status_queryhelpers are clean and match their later usage (_get_status_command_default,_validate_status_command_settings,check_active_jobs,job_stati). No issues here.Also applies to: 39-45
178-191: Partition configuration option and wiring look consistent.The new
partition_config: Optional[Path]setting plus theSNAKEMAKE_SLURM_PARTITIONSoverride handling inExecutor.__post_init__are coherent: CLI-configuredpartition_configtakes precedence over the env var, and both paths end up inread_partition_file(). No functional issues spotted.Also applies to: 315-330
192-225: Efficiency report options are correctly integrated; just verify helper signature.
efficiency_report,efficiency_report_path, andefficiency_thresholdare cleanly threaded intoshutdown()and passed tocreate_efficiency_report(e_threshold=…, run_uuid=…, e_report_path=…, logger=…). This matches the intended behavior and is a nice integration point.Please just double-check that
create_efficiency_reportstill has the exact parameter names(e_threshold, run_uuid, e_report_path, logger)and that any temporary debugos.listdir()logging in that helper has been removed as planned.
Based on learnings, this keeps the implementation production‑ready.Also applies to: 406-417
420-432: Log cleanup behavior is clear and bounded.The new docstring and info log in
clean_old_logs()make the deletion policy explicit and keep the behavior unchanged (guarded byage_cutoff <= 0andkeep_successful_logs). The iteration over*.logplusdelete_empty_dirs()is reasonable.
455-458: Job-context warning text is clearer and user-friendly.The expanded message in
warn_on_jobcontext()better explains why running inside a SLURM job is discouraged and guides users toward the login node. The brief 5-second pause plus environment cleanup remain sensible.
488-502: Job submission comments and warnings align with behavior.
- The comment around
%jinslurm_logfilecorrectly documents the pre‑creation requirement and the silent-fail behavior of older SLURM.- The explanation that
run_uuidis used as the job name to enable--name-based filtering is consistent with the sacct/squeue query helpers.- The warnings for missing
runtimeand memory resources clearly communicate potential cluster-side failures without changing behavior.- The log for the submitted job (
Job ... submitted with SLURM jobid ... (log: ...)) matches howslurm_jobidand log paths are computed.No functional issues here.
Also applies to: 539-547, 588-599
701-710: Warning about missing statuses is clear and helpful.The warning when
missing_sacct_statusis non-empty clearly states that some jobs were previously seen but no longer reported, and points users toward checking SLURM accounting setup. This is good operational guidance.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
snakemake_executor_plugin_slurm/__init__.py (2)
52-107: Avoid running external probes in _get_status_command_help at import time.
_get_status_command_help()calls_get_status_command_default(),is_query_tool_available(), andshould_recommend_squeue_status_command(). Because it’s invoked in thestatus_commandfield metadata ("help": _get_status_command_help()), all those subprocess-based checks (which,scontrol show config, etc.) run at import time whenever the plugin is loaded. That can be slow, brittle, and was previously flagged in earlier review as something to avoid.Given that the actual selection and validation of the status command already happens at runtime (
_get_status_command_default(),_validate_status_command_settings()and the query helpers), I’d strongly recommend:
- Making
_get_status_command_help()a static, descriptive string (no probes), or- Inlining that static help text directly into the
metadata["help"]value and using_get_status_command_helponly (if at all) in non-import-time contexts.This keeps import side effects minimal while preserving smart runtime behavior.
Also applies to: 227-234
336-357: Make the sacct-unavailable warning match actual behavior (no automatic fallback).When
sacctis unavailable andstatus_command == "sacct",_validate_status_command_settings()logs:"Using 'squeue' instead for job status queries."
However, no fallback is implemented:
get_status_command()still returns the configuredstatus_command, andcheck_active_jobs()will continue to construct and run a sacct-based command. This warning is therefore misleading.Either:
- Implement the actual fallback (
status_command = "squeue"and ensure subsequent logic respects that), or- Adjust the warning text to state that sacct is not available and the user must switch to
status_command='squeue'or install sacct.For example:
- self.logger.warning( - "The 'sacct' command is not available on this system. " - "Using 'squeue' instead for job status queries." - ) + self.logger.warning( + "The 'sacct' command is not available on this system, " + "but status_command is set to 'sacct'. " + "Status queries will fail unless you install 'sacct' or " + "set status_command='squeue'." + )
🧹 Nitpick comments (2)
snakemake_executor_plugin_slurm/validation.py (1)
79-141: Polish validate_executor_settings docstring and range error message.The validation logic looks good, but there are a couple of small text issues that could confuse users:
- Line 85:
ExecutorSettins→ExecutorSettingsin the docstring.- Lines 111–113: the check allows
0(< 0is rejected), but the message says “must be a positive integer.” Either change the check to<= 0or (simpler) reword the message to “must be a non-negative integer.”Example diff:
- settings: ExecutorSettins instance to validate + settings: ExecutorSettings instance to validate @@ - "init-seconds-before-status-checks must be a positive integer." + "init-seconds-before-status-checks must be a non-negative integer."snakemake_executor_plugin_slurm/__init__.py (1)
646-651: Tie status_attempts invariants to validation and clean up loop usage.The status-check loop in
check_active_jobs()assumesstatus_attemptsis a positive integer:status_attempts = self.workflow.executor_settings.status_attempts ... for i in range(status_attempts): ...and later relies on
status_of_jobsbeing set in the loop before checking it after the loop. OnceExecutorSettings.__post_init__correctly callsvalidate_executor_settings(), misconfigurations likestatus_attempts=0or non-int values will be rejected early, so this loop is safe.Given that:
- After fixing the typo in
__post_init__, the invariants are enforced centrally invalidate_executor_settings.- The only remaining issues here are cosmetic / robustness:
You may want to:
- Rename the unused loop variable to satisfy Ruff B007 and signal intent:
- for i in range(status_attempts): + for _attempt in range(status_attempts):
- (Optional) Initialize
status_of_jobsandsacct_query_durationtoNonebefore the loop as a defensive guard if future changes ever relax validation guarantees.Overall, once the validation hook is fixed, the status-command resolution and dispatch through
query_job_status_sacct/query_job_status_squeuelook consistent with the configuration.Also applies to: 656-665, 668-700, 772-775
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
snakemake_executor_plugin_slurm/__init__.py(17 hunks)snakemake_executor_plugin_slurm/validation.py(2 hunks)
🧰 Additional context used
🧠 Learnings (13)
📓 Common learnings
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 0
File: :0-0
Timestamp: 2025-01-13T09:54:22.950Z
Learning: PR #173 (adding gres resource specification) depends on PR #28 in snakemake-executor-plugin-slurm-jobstep repository, as changes were required in the cpu-settings function of the jobstep-Executor module.
Learnt from: johanneskoester
Repo: snakemake/snakemake-executor-plugin-slurm PR: 173
File: docs/further.md:96-96
Timestamp: 2025-03-10T15:20:51.829Z
Learning: PR #173 in snakemake-executor-plugin-slurm implements GPU job support by adding resources: `gres` for generic resource specifications (e.g., 'gpu:1'), `gpu`/`gpus` for specifying GPU counts, and `gpu_model`/`gpu_manufacturer` for specifying GPU types, allowing users to request GPU resources directly rather than having to use slurm_extra.
📚 Learning: 2024-12-11T14:17:08.749Z
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 178
File: snakemake_executor_plugin_slurm/__init__.py:0-0
Timestamp: 2024-12-11T14:17:08.749Z
Learning: In the `snakemake-executor-plugin-slurm` project, when handling exceptions in `snakemake_executor_plugin_slurm/__init__.py`, prefer concise error messages and avoid unnecessary verbosity or exception chaining when it's not necessary.
Applied to files:
snakemake_executor_plugin_slurm/validation.pysnakemake_executor_plugin_slurm/__init__.py
📚 Learning: 2025-03-31T11:52:05.373Z
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 249
File: snakemake_executor_plugin_slurm/submit_string.py:21-29
Timestamp: 2025-03-31T11:52:05.373Z
Learning: In the snakemake-executor-plugin-slurm project, account and partition parameters are validated in the submit function before reaching the get_submit_command function. By design, these parameters are either empty strings or already properly formatted with SLURM parameter tags, making additional validation in get_submit_command unnecessary.
Applied to files:
snakemake_executor_plugin_slurm/validation.pysnakemake_executor_plugin_slurm/__init__.py
📚 Learning: 2025-06-24T12:41:31.837Z
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 221
File: tests/tests.py:62-70
Timestamp: 2025-06-24T12:41:31.837Z
Learning: In the snakemake-executor-plugin-slurm efficiency report feature, the executor is responsible for creating the efficiency_report_path directory when generating efficiency reports. Tests should not manually check for directory existence but should rely on the executor to properly create necessary directories as part of its normal operation.
Applied to files:
snakemake_executor_plugin_slurm/validation.pysnakemake_executor_plugin_slurm/__init__.py
📚 Learning: 2025-09-15T12:00:31.977Z
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 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/validation.pysnakemake_executor_plugin_slurm/__init__.py
📚 Learning: 2025-03-10T15:20:51.829Z
Learnt from: johanneskoester
Repo: snakemake/snakemake-executor-plugin-slurm PR: 173
File: docs/further.md:96-96
Timestamp: 2025-03-10T15:20:51.829Z
Learning: PR #173 in snakemake-executor-plugin-slurm implements GPU job support by adding resources: `gres` for generic resource specifications (e.g., 'gpu:1'), `gpu`/`gpus` for specifying GPU counts, and `gpu_model`/`gpu_manufacturer` for specifying GPU types, allowing users to request GPU resources directly rather than having to use slurm_extra.
Applied to files:
snakemake_executor_plugin_slurm/validation.pysnakemake_executor_plugin_slurm/__init__.py
📚 Learning: 2025-01-13T09:54:22.950Z
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 0
File: :0-0
Timestamp: 2025-01-13T09:54:22.950Z
Learning: PR #173 (adding gres resource specification) depends on PR #28 in snakemake-executor-plugin-slurm-jobstep repository, as changes were required in the cpu-settings function of the jobstep-Executor module.
Applied to files:
snakemake_executor_plugin_slurm/__init__.py
📚 Learning: 2025-01-20T09:13:26.443Z
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 193
File: .github/workflows/post_to_mastodon.yml:26-26
Timestamp: 2025-01-20T09:13:26.443Z
Learning: In the snakemake-executor-plugin-slurm repository, release PRs follow the naming pattern "chore(main): release X.Y.Z" where X.Y.Z is the version number.
Applied to files:
snakemake_executor_plugin_slurm/__init__.py
📚 Learning: 2025-09-16T17:25:38.450Z
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 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/__init__.py
📚 Learning: 2025-01-17T17:27:32.446Z
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 187
File: .github/workflows/post_to_mastodon.yml:0-0
Timestamp: 2025-01-17T17:27:32.446Z
Learning: In the mastodon publishing workflow for snakemake-executor-plugin-slurm, the PR_TITLE environment variable is required by the post_to_mastodon.sh script and should be preserved.
Applied to files:
snakemake_executor_plugin_slurm/__init__.py
📚 Learning: 2025-07-22T17:31:41.629Z
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 333
File: .github/workflows/announce-release.yml:0-0
Timestamp: 2025-07-22T17:31:41.629Z
Learning: In the snakemake-executor-plugin-slurm repository, cmeesters prefers using `main` references for GitHub Actions to always get the latest version, with security provided through protected branches and mandatory CI checks rather than commit SHA pinning.
Applied to files:
snakemake_executor_plugin_slurm/__init__.py
📚 Learning: 2025-06-16T08:54:07.957Z
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 221
File: snakemake_executor_plugin_slurm/efficiency_report.py:0-0
Timestamp: 2025-06-16T08:54:07.957Z
Learning: In the context of SLURM executor plugin development, user cmeesters (experienced with SLURM) noted they are not aware of SLURM typically reporting memory values with decimals (like 1.5G), but accepted defensive programming to handle such cases anyway.
Applied to files:
snakemake_executor_plugin_slurm/__init__.py
📚 Learning: 2025-06-03T17:10:51.292Z
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 221
File: snakemake_executor_plugin_slurm/__init__.py:0-0
Timestamp: 2025-06-03T17:10:51.292Z
Learning: In the snakemake-executor-plugin-slurm repository, the create_efficiency_report method in snakemake_executor_plugin_slurm/__init__.py contains temporary testing/debugging code that logs directory contents using os.listdir() and directory paths. This code was inserted for testing purposes only and should be removed from the production implementation.
Applied to files:
snakemake_executor_plugin_slurm/__init__.py
🧬 Code graph analysis (1)
snakemake_executor_plugin_slurm/__init__.py (2)
snakemake_executor_plugin_slurm/job_status_query.py (5)
get_min_job_age(7-60)is_query_tool_available(63-74)should_recommend_squeue_status_command(77-101)query_job_status_squeue(135-154)query_job_status_sacct(104-132)snakemake_executor_plugin_slurm/validation.py (2)
validate_slurm_extra(49-76)validate_executor_settings(79-141)
🪛 Ruff (0.14.6)
snakemake_executor_plugin_slurm/validation.py
93-95: Avoid specifying long messages outside the exception class
(TRY003)
103-103: Avoid specifying long messages outside the exception class
(TRY003)
111-113: Avoid specifying long messages outside the exception class
(TRY003)
120-122: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
120-122: Avoid specifying long messages outside the exception class
(TRY003)
124-126: Avoid specifying long messages outside the exception class
(TRY003)
132-134: Avoid specifying long messages outside the exception class
(TRY003)
139-141: Avoid specifying long messages outside the exception class
(TRY003)
snakemake_executor_plugin_slurm/__init__.py
59-63: Avoid specifying long messages outside the exception class
(TRY003)
270-270: Undefined name validate_executor_settins
(F821)
592-592: Avoid specifying long messages outside the exception class
(TRY003)
668-668: Loop control variable i not used within loop body
Rename unused i to _i
(B007)
⏰ 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/__init__.py (1)
410-416: Efficiency report integration and log cleanup behavior look consistent.The new shutdown flow (
efficiency_report/efficiency_report_path/efficiency_threshold) andclean_old_logs()logic align well with the ExecutorSettings semantics and the documented “<=0 disables cleanup” behavior fordelete_logfiles_older_than. No functional issues from this change set on these paths.Also applies to: 419-432
There was a problem hiding this comment.
Actionable comments posted: 2
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)
596-644: Guard against None or invalid status_attempts to prevent UnboundLocalError.
status_attemptsisOptional[int]with default 5, but validation only checks positive values "if not None" (validation.py:125-130). If explicitly set toNone, line 639'srange(status_attempts)will crash withTypeError. If set to 0 or negative (bypassing validation), the loop won't execute, leavingstatus_of_jobsandsacct_query_durationuninitialized, causingUnboundLocalErrorat line 644.Initialize variables before the loop and coerce to a valid value:
sacct_query_durations = [] - + # Initialize to safe defaults + status_of_jobs = None + sacct_query_duration = None + - status_attempts = self.workflow.executor_settings.status_attempts + raw_attempts = self.workflow.executor_settings.status_attempts + try: + status_attempts = max(1, int(raw_attempts)) + except (TypeError, ValueError): + self.logger.warning( + f"Invalid status_attempts={raw_attempts!r}; using default of 5." + ) + status_attempts = 5 + self.logger.debug( f"Checking the status of {len(active_jobs)} active jobs " f"with {status_attempts} attempts." )Also rename the unused loop variable:
- for i in range(status_attempts): + for _attempt in range(status_attempts):
♻️ Duplicate comments (1)
snakemake_executor_plugin_slurm/__init__.py (1)
632-635: I'll verify the return types of the query functions and trace them through to the subprocess call.
<function_calls>
shell
#!/bin/bashFind and examine the query_job_status functions
fd job_status_query.py
</function_calls>Looking for the query functions in the codebase...
<function_calls>
shell
#!/bin/bashSearch for query_job_status_sacct and query_job_status_squeue definitions
rg "def query_job_status_sacct|def query_job_status_squeue" -A 30 --type py
</function_calls>Now let me check the exact context around line 791 where subprocess is called:
<function_calls>
shell
#!/bin/bashCheck the check_active_jobs method and subprocess call
rg "def check_active_jobs" -A 50 --type py | head -80
</function_calls>Let me also search for subprocess.check_output with shell=True:
<function_calls>
shell
#!/bin/bashFind subprocess.check_output calls
rg "subprocess.check_output.shell\s=\s*True" -B 5 -A 2 --type py
</function_calls><function_calls>
shell
#!/bin/bashGet the file structure to understand layout
wc -l snakemake_executor_plugin_slurm/init.py
</function_calls><function_calls>
shell
#!/bin/bashCheck around lines 630-650 for the code context
sed -n '625,645p' snakemake_executor_plugin_slurm/init.py
</function_calls><function_calls>
shell
#!/bin/bashCheck around line 791 area
sed -n '785,800p' snakemake_executor_plugin_slurm/init.py
</function_calls><function_calls>
shell
#!/bin/bashFind where query functions are defined
find . -name "*.py" -type f | xargs rg "def query_job_status_sacct|def query_job_status_squeue" -l
</function_calls><function_calls>
shell
#!/bin/bashList Python files in the snakemake_executor_plugin_slurm directory
ls -la snakemake_executor_plugin_slurm/*.py
</function_calls><function_calls>
shell
#!/bin/bashSearch for query_job_status functions in init.py
rg "def query_job_status" snakemake_executor_plugin_slurm/init.py -A 25
</function_calls><function_calls>
shell
#!/bin/bashGet the entire function definitions for both query functions
First, let's find their line numbers
rg "def query_job_status_sacct|def query_job_status_squeue" snakemake_executor_plugin_slurm/init.py -n
</function_calls><function_calls>
shell
#!/bin/bashExamine query_job_status_sacct (should be around line 659-670)
sed -n '659,680p' snakemake_executor_plugin_slurm/init.py
</function_calls><function_calls>
shell
#!/bin/bashExamine query_job_status_squeue (should be around line 681-700)
sed -n '681,710p' snakemake_executor_plugin_slurm/init.py
</function_calls><function_calls>
shell
#!/bin/bashCheck around line 641 for job_stati usage
sed -n '635,655p' snakemake_executor_plugin_slurm/init.py
</function_calls><function_calls>
shell
#!/bin/bashFind the actual job_stati function to see how it processes the command
rg "def job_stati" snakemake_executor_plugin_slurm/init.py -A 20
</function_calls><function_calls>
shell
#!/bin/bashSearch for any list/string conversions or handling in job_stati
sed -n '770,810p' snakemake_executor_plugin_slurm/init.py
</function_calls>Perfect. I've found the issue. Let me verify the exact return types one more time with a focused look:
<function_calls>
shell
#!/bin/bashGet exact return statements for both functions
echo "=== query_job_status_sacct return ==="
sed -n '659,676p' snakemake_executor_plugin_slurm/init.pyecho -e "\n=== query_job_status_squeue return ==="
sed -n '681,710p' snakemake_executor_plugin_slurm/init.pyecho -e "\n=== subprocess.check_output call in job_stati ==="
sed -n '774,780p' snakemake_executor_plugin_slurm/init.py
</function_calls>Confirmed type mismatch:
query_job_status_sacct()returns a string whilequery_job_status_squeue()returns a list, but both are passed tosubprocess.check_output(..., shell=True)which requires a string.At line 632-635, the code conditionally assigns
status_commandfrom either function. However:
query_job_status_sacct()(line 676) returns a string:f"sacct -o {output_fields}..."query_job_status_squeue()(line 708) returns a list:["squeue", "-o", format_string, ...]Both are passed to
job_stati()which callssubprocess.check_output(command, text=True, shell=True, ...)at line 779. Withshell=True, a string is required; passing a list will cause a TypeError.Fix: Either (1) ensure both functions return strings (convert
query_job_status_squeue()to return a joined string), or (2) change toshell=Falseand always use lists.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
snakemake_executor_plugin_slurm/__init__.py(17 hunks)snakemake_executor_plugin_slurm/validation.py(2 hunks)
🧰 Additional context used
🧠 Learnings (13)
📓 Common learnings
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 0
File: :0-0
Timestamp: 2025-01-13T09:54:22.950Z
Learning: PR #173 (adding gres resource specification) depends on PR #28 in snakemake-executor-plugin-slurm-jobstep repository, as changes were required in the cpu-settings function of the jobstep-Executor module.
Learnt from: johanneskoester
Repo: snakemake/snakemake-executor-plugin-slurm PR: 173
File: docs/further.md:96-96
Timestamp: 2025-03-10T15:20:51.829Z
Learning: PR #173 in snakemake-executor-plugin-slurm implements GPU job support by adding resources: `gres` for generic resource specifications (e.g., 'gpu:1'), `gpu`/`gpus` for specifying GPU counts, and `gpu_model`/`gpu_manufacturer` for specifying GPU types, allowing users to request GPU resources directly rather than having to use slurm_extra.
📚 Learning: 2025-03-31T11:52:05.373Z
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 249
File: snakemake_executor_plugin_slurm/submit_string.py:21-29
Timestamp: 2025-03-31T11:52:05.373Z
Learning: In the snakemake-executor-plugin-slurm project, account and partition parameters are validated in the submit function before reaching the get_submit_command function. By design, these parameters are either empty strings or already properly formatted with SLURM parameter tags, making additional validation in get_submit_command unnecessary.
Applied to files:
snakemake_executor_plugin_slurm/validation.pysnakemake_executor_plugin_slurm/__init__.py
📚 Learning: 2024-12-11T14:17:08.749Z
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 178
File: snakemake_executor_plugin_slurm/__init__.py:0-0
Timestamp: 2024-12-11T14:17:08.749Z
Learning: In the `snakemake-executor-plugin-slurm` project, when handling exceptions in `snakemake_executor_plugin_slurm/__init__.py`, prefer concise error messages and avoid unnecessary verbosity or exception chaining when it's not necessary.
Applied to files:
snakemake_executor_plugin_slurm/validation.pysnakemake_executor_plugin_slurm/__init__.py
📚 Learning: 2025-09-15T12:00:31.977Z
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 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/validation.pysnakemake_executor_plugin_slurm/__init__.py
📚 Learning: 2025-03-10T15:20:51.829Z
Learnt from: johanneskoester
Repo: snakemake/snakemake-executor-plugin-slurm PR: 173
File: docs/further.md:96-96
Timestamp: 2025-03-10T15:20:51.829Z
Learning: PR #173 in snakemake-executor-plugin-slurm implements GPU job support by adding resources: `gres` for generic resource specifications (e.g., 'gpu:1'), `gpu`/`gpus` for specifying GPU counts, and `gpu_model`/`gpu_manufacturer` for specifying GPU types, allowing users to request GPU resources directly rather than having to use slurm_extra.
Applied to files:
snakemake_executor_plugin_slurm/__init__.py
📚 Learning: 2025-01-13T09:54:22.950Z
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 0
File: :0-0
Timestamp: 2025-01-13T09:54:22.950Z
Learning: PR #173 (adding gres resource specification) depends on PR #28 in snakemake-executor-plugin-slurm-jobstep repository, as changes were required in the cpu-settings function of the jobstep-Executor module.
Applied to files:
snakemake_executor_plugin_slurm/__init__.py
📚 Learning: 2025-01-20T09:13:26.443Z
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 193
File: .github/workflows/post_to_mastodon.yml:26-26
Timestamp: 2025-01-20T09:13:26.443Z
Learning: In the snakemake-executor-plugin-slurm repository, release PRs follow the naming pattern "chore(main): release X.Y.Z" where X.Y.Z is the version number.
Applied to files:
snakemake_executor_plugin_slurm/__init__.py
📚 Learning: 2025-09-16T17:25:38.450Z
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 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/__init__.py
📚 Learning: 2025-01-17T17:27:32.446Z
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 187
File: .github/workflows/post_to_mastodon.yml:0-0
Timestamp: 2025-01-17T17:27:32.446Z
Learning: In the mastodon publishing workflow for snakemake-executor-plugin-slurm, the PR_TITLE environment variable is required by the post_to_mastodon.sh script and should be preserved.
Applied to files:
snakemake_executor_plugin_slurm/__init__.py
📚 Learning: 2025-07-22T17:31:41.629Z
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 333
File: .github/workflows/announce-release.yml:0-0
Timestamp: 2025-07-22T17:31:41.629Z
Learning: In the snakemake-executor-plugin-slurm repository, cmeesters prefers using `main` references for GitHub Actions to always get the latest version, with security provided through protected branches and mandatory CI checks rather than commit SHA pinning.
Applied to files:
snakemake_executor_plugin_slurm/__init__.py
📚 Learning: 2025-06-16T08:54:07.957Z
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 221
File: snakemake_executor_plugin_slurm/efficiency_report.py:0-0
Timestamp: 2025-06-16T08:54:07.957Z
Learning: In the context of SLURM executor plugin development, user cmeesters (experienced with SLURM) noted they are not aware of SLURM typically reporting memory values with decimals (like 1.5G), but accepted defensive programming to handle such cases anyway.
Applied to files:
snakemake_executor_plugin_slurm/__init__.py
📚 Learning: 2025-06-24T12:41:31.837Z
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 221
File: tests/tests.py:62-70
Timestamp: 2025-06-24T12:41:31.837Z
Learning: In the snakemake-executor-plugin-slurm efficiency report feature, the executor is responsible for creating the efficiency_report_path directory when generating efficiency reports. Tests should not manually check for directory existence but should rely on the executor to properly create necessary directories as part of its normal operation.
Applied to files:
snakemake_executor_plugin_slurm/__init__.py
📚 Learning: 2025-06-03T17:10:51.292Z
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 221
File: snakemake_executor_plugin_slurm/__init__.py:0-0
Timestamp: 2025-06-03T17:10:51.292Z
Learning: In the snakemake-executor-plugin-slurm repository, the create_efficiency_report method in snakemake_executor_plugin_slurm/__init__.py contains temporary testing/debugging code that logs directory contents using os.listdir() and directory paths. This code was inserted for testing purposes only and should be removed from the production implementation.
Applied to files:
snakemake_executor_plugin_slurm/__init__.py
🧬 Code graph analysis (1)
snakemake_executor_plugin_slurm/__init__.py (5)
snakemake_executor_plugin_slurm/job_status_query.py (3)
get_min_job_age(7-60)is_query_tool_available(63-74)should_recommend_squeue_status_command(77-101)snakemake_executor_plugin_slurm/efficiency_report.py (1)
create_efficiency_report(233-262)snakemake_executor_plugin_slurm/submit_string.py (1)
get_submit_command(19-130)snakemake_executor_plugin_slurm/partitions.py (2)
read_partition_file(14-51)get_best_partition(54-79)snakemake_executor_plugin_slurm/validation.py (2)
validate_slurm_extra(50-77)validate_status_command_settings(80-110)
🪛 Ruff (0.14.6)
snakemake_executor_plugin_slurm/validation.py
121-123: Avoid specifying long messages outside the exception class
(TRY003)
130-130: Avoid specifying long messages outside the exception class
(TRY003)
137-139: Avoid specifying long messages outside the exception class
(TRY003)
145-147: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
145-147: Avoid specifying long messages outside the exception class
(TRY003)
149-151: Avoid specifying long messages outside the exception class
(TRY003)
156-158: Avoid specifying long messages outside the exception class
(TRY003)
162-164: Avoid specifying long messages outside the exception class
(TRY003)
snakemake_executor_plugin_slurm/__init__.py
63-67: Avoid specifying long messages outside the exception class
(TRY003)
544-544: Avoid specifying long messages outside the exception class
(TRY003)
639-639: Loop control variable i not used within loop body
Rename unused i to _i
(B007)
⏰ 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
| if settings.init_seconds_before_status_checks is not None: | ||
| if ( | ||
| not isinstance(settings.init_seconds_before_status_checks, int) | ||
| or settings.init_seconds_before_status_checks < 0 | ||
| ): | ||
| raise WorkflowError( | ||
| "init-seconds-before-status-checks must be a positive integer." | ||
| ) |
There was a problem hiding this comment.
Fix inconsistent error message for init_seconds_before_status_checks validation.
The validation checks >= 0 (non-negative), but the error message says "must be a positive integer" (which would be > 0). Either change the check to < 1 or update the message to say "non-negative integer".
Apply this diff to align the message with the validation:
if (
not isinstance(settings.init_seconds_before_status_checks, int)
or settings.init_seconds_before_status_checks < 0
):
raise WorkflowError(
- "init-seconds-before-status-checks must be a positive integer."
+ "init-seconds-before-status-checks must be a non-negative integer."
)📝 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.
| if settings.init_seconds_before_status_checks is not None: | |
| if ( | |
| not isinstance(settings.init_seconds_before_status_checks, int) | |
| or settings.init_seconds_before_status_checks < 0 | |
| ): | |
| raise WorkflowError( | |
| "init-seconds-before-status-checks must be a positive integer." | |
| ) | |
| if settings.init_seconds_before_status_checks is not None: | |
| if ( | |
| not isinstance(settings.init_seconds_before_status_checks, int) | |
| or settings.init_seconds_before_status_checks < 0 | |
| ): | |
| raise WorkflowError( | |
| "init-seconds-before-status-checks must be a non-negative integer." | |
| ) |
🧰 Tools
🪛 Ruff (0.14.6)
137-139: Avoid specifying long messages outside the exception class
(TRY003)
🤖 Prompt for AI Agents
In snakemake_executor_plugin_slurm/validation.py around lines 132 to 139, the
validation allows zero (non-negative) but the error message says "positive
integer"; update the raise WorkflowError message to state
"init-seconds-before-status-checks must be a non-negative integer." so the text
matches the existing check (or alternatively change the condition to < 1 if you
prefer to require strictly positive values).
…djusted help message and validator
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
snakemake_executor_plugin_slurm/__init__.py (1)
56-71: Avoid running SLURM detection and raisingWorkflowErrorat import time forstatus_command.
_get_status_command_help()calls_get_status_command_default(),is_query_tool_available(), andshould_recommend_squeue_status_command(). Because it is invoked in thestatus_commandfield metadata"help": _get_status_command_help(),all of this runs at module import time. That means:
- Importing this plugin executes several subprocesses (
which sacct,which squeue,scontrol show config), which can be slow or flaky on non-SLURM or misconfigured systems.- If neither
sacctnorsqueueis onPATH,_get_status_command_default()raisesWorkflowError, so merely importing the plugin fails even if the executor is never used.The environment-sensitive detection is appropriate when we actually run the executor, but it’s risky at import time.
I suggest:
- Make the help text static (describe the behavior generically), and
- Keep
_get_status_command_default()for runtime defaulting only.For example:
- status_command: Optional[str] = field( - default_factory=_get_status_command_default, - metadata={ - "help": _get_status_command_help(), - "env_var": False, - "required": False, - }, - ) + status_command: Optional[str] = field( + default_factory=_get_status_command_default, + metadata={ + "help": ( + "Command to query job status. Options: 'sacct', 'squeue'. " + "By default, the executor prefers 'sacct' if available and " + "falls back to 'squeue' otherwise." + ), + "env_var": False, + "required": False, + }, + )Optionally, keep
_get_status_command_help()as a pure helper returning the same static string if you still want a function form. This avoids subprocess work andWorkflowErrorat import while preserving the smart runtime default in_get_status_command_default().Also applies to: 74-111, 231-238
🧹 Nitpick comments (1)
snakemake_executor_plugin_slurm/__init__.py (1)
596-600: Status-command selection logic is sound; consider renaming unused loop variable.The new
status_command_nameselection with fallbacks betweensacctandsqueuebased on MinJobAge and tool availability is coherent and matches the validation semantics. One small cleanup:
- Ruff correctly flags
ias unused infor i in range(status_attempts):.You can silence that and clarify intent with:
- for i in range(status_attempts): + for _ in range(status_attempts):No behavioral change, just cleaner and lint-friendly.
Also applies to: 606-631, 634-667, 739-741
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
snakemake_executor_plugin_slurm/__init__.py(17 hunks)
🧰 Additional context used
🧠 Learnings (13)
📓 Common learnings
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 0
File: :0-0
Timestamp: 2025-01-13T09:54:22.950Z
Learning: PR #173 (adding gres resource specification) depends on PR #28 in snakemake-executor-plugin-slurm-jobstep repository, as changes were required in the cpu-settings function of the jobstep-Executor module.
Learnt from: johanneskoester
Repo: snakemake/snakemake-executor-plugin-slurm PR: 173
File: docs/further.md:96-96
Timestamp: 2025-03-10T15:20:51.829Z
Learning: PR #173 in snakemake-executor-plugin-slurm implements GPU job support by adding resources: `gres` for generic resource specifications (e.g., 'gpu:1'), `gpu`/`gpus` for specifying GPU counts, and `gpu_model`/`gpu_manufacturer` for specifying GPU types, allowing users to request GPU resources directly rather than having to use slurm_extra.
📚 Learning: 2024-12-11T14:17:08.749Z
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 178
File: snakemake_executor_plugin_slurm/__init__.py:0-0
Timestamp: 2024-12-11T14:17:08.749Z
Learning: In the `snakemake-executor-plugin-slurm` project, when handling exceptions in `snakemake_executor_plugin_slurm/__init__.py`, prefer concise error messages and avoid unnecessary verbosity or exception chaining when it's not necessary.
Applied to files:
snakemake_executor_plugin_slurm/__init__.py
📚 Learning: 2025-03-10T15:20:51.829Z
Learnt from: johanneskoester
Repo: snakemake/snakemake-executor-plugin-slurm PR: 173
File: docs/further.md:96-96
Timestamp: 2025-03-10T15:20:51.829Z
Learning: PR #173 in snakemake-executor-plugin-slurm implements GPU job support by adding resources: `gres` for generic resource specifications (e.g., 'gpu:1'), `gpu`/`gpus` for specifying GPU counts, and `gpu_model`/`gpu_manufacturer` for specifying GPU types, allowing users to request GPU resources directly rather than having to use slurm_extra.
Applied to files:
snakemake_executor_plugin_slurm/__init__.py
📚 Learning: 2025-01-20T09:13:26.443Z
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 193
File: .github/workflows/post_to_mastodon.yml:26-26
Timestamp: 2025-01-20T09:13:26.443Z
Learning: In the snakemake-executor-plugin-slurm repository, release PRs follow the naming pattern "chore(main): release X.Y.Z" where X.Y.Z is the version number.
Applied to files:
snakemake_executor_plugin_slurm/__init__.py
📚 Learning: 2025-03-31T11:52:05.373Z
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 249
File: snakemake_executor_plugin_slurm/submit_string.py:21-29
Timestamp: 2025-03-31T11:52:05.373Z
Learning: In the snakemake-executor-plugin-slurm project, account and partition parameters are validated in the submit function before reaching the get_submit_command function. By design, these parameters are either empty strings or already properly formatted with SLURM parameter tags, making additional validation in get_submit_command unnecessary.
Applied to files:
snakemake_executor_plugin_slurm/__init__.py
📚 Learning: 2025-01-13T09:54:22.950Z
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 0
File: :0-0
Timestamp: 2025-01-13T09:54:22.950Z
Learning: PR #173 (adding gres resource specification) depends on PR #28 in snakemake-executor-plugin-slurm-jobstep repository, as changes were required in the cpu-settings function of the jobstep-Executor module.
Applied to files:
snakemake_executor_plugin_slurm/__init__.py
📚 Learning: 2025-09-16T17:25:38.450Z
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 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/__init__.py
📚 Learning: 2025-01-17T17:27:32.446Z
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 187
File: .github/workflows/post_to_mastodon.yml:0-0
Timestamp: 2025-01-17T17:27:32.446Z
Learning: In the mastodon publishing workflow for snakemake-executor-plugin-slurm, the PR_TITLE environment variable is required by the post_to_mastodon.sh script and should be preserved.
Applied to files:
snakemake_executor_plugin_slurm/__init__.py
📚 Learning: 2025-07-22T17:31:41.629Z
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 333
File: .github/workflows/announce-release.yml:0-0
Timestamp: 2025-07-22T17:31:41.629Z
Learning: In the snakemake-executor-plugin-slurm repository, cmeesters prefers using `main` references for GitHub Actions to always get the latest version, with security provided through protected branches and mandatory CI checks rather than commit SHA pinning.
Applied to files:
snakemake_executor_plugin_slurm/__init__.py
📚 Learning: 2025-06-16T08:54:07.957Z
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 221
File: snakemake_executor_plugin_slurm/efficiency_report.py:0-0
Timestamp: 2025-06-16T08:54:07.957Z
Learning: In the context of SLURM executor plugin development, user cmeesters (experienced with SLURM) noted they are not aware of SLURM typically reporting memory values with decimals (like 1.5G), but accepted defensive programming to handle such cases anyway.
Applied to files:
snakemake_executor_plugin_slurm/__init__.py
📚 Learning: 2025-09-15T12:00:31.977Z
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 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/__init__.py
📚 Learning: 2025-06-03T17:10:51.292Z
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 221
File: snakemake_executor_plugin_slurm/__init__.py:0-0
Timestamp: 2025-06-03T17:10:51.292Z
Learning: In the snakemake-executor-plugin-slurm repository, the create_efficiency_report method in snakemake_executor_plugin_slurm/__init__.py contains temporary testing/debugging code that logs directory contents using os.listdir() and directory paths. This code was inserted for testing purposes only and should be removed from the production implementation.
Applied to files:
snakemake_executor_plugin_slurm/__init__.py
📚 Learning: 2025-06-24T12:41:31.837Z
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 221
File: tests/tests.py:62-70
Timestamp: 2025-06-24T12:41:31.837Z
Learning: In the snakemake-executor-plugin-slurm efficiency report feature, the executor is responsible for creating the efficiency_report_path directory when generating efficiency reports. Tests should not manually check for directory existence but should rely on the executor to properly create necessary directories as part of its normal operation.
Applied to files:
snakemake_executor_plugin_slurm/__init__.py
🧬 Code graph analysis (1)
snakemake_executor_plugin_slurm/__init__.py (5)
snakemake_executor_plugin_slurm/job_status_query.py (5)
get_min_job_age(7-60)is_query_tool_available(63-74)should_recommend_squeue_status_command(77-101)query_job_status_squeue(135-154)query_job_status_sacct(104-132)snakemake_executor_plugin_slurm/efficiency_report.py (1)
create_efficiency_report(233-262)snakemake_executor_plugin_slurm/submit_string.py (1)
get_submit_command(19-130)snakemake_executor_plugin_slurm/partitions.py (2)
read_partition_file(14-51)get_best_partition(54-79)snakemake_executor_plugin_slurm/validation.py (3)
validate_slurm_extra(50-77)validate_executor_settings(113-167)validate_status_command_settings(80-110)
🪛 Ruff (0.14.6)
snakemake_executor_plugin_slurm/__init__.py
63-67: Avoid specifying long messages outside the exception class
(TRY003)
542-542: Avoid specifying long messages outside the exception class
(TRY003)
634-634: Loop control variable i not used within loop body
Rename unused i to _i
(B007)
⏰ 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/__init__.py (2)
19-24: Status-query helper imports and validation wiring look consistent.Importing the query/validation helpers here and invoking
validate_executor_settingsinExecutorSettings.__post_init__plusvalidate_status_command_settingsinExecutor.__post_init__gives a clear, centralized validation flow and keeps status-command warnings in one place. No issues from this integration.Also applies to: 39-45, 49-53, 271-274, 335-337
196-229: Efficiency report wiring, log cleanup, and submission logging look good.
shutdown()now callingsuper().shutdown(), thenclean_old_logs(), and optionallycreate_efficiency_report()withefficiency_threshold/efficiency_report_pathis a clean, predictable sequence.clean_old_logs()correctly respectsdelete_logfiles_older_than(including the<= 0“disable” case) andkeep_successful_logs, and the error handling around file deletion / empty-dir cleanup is robust.- The SLURM logdir handling in
run_job()(ensuring parent dirs exist, guarding against%jin the parent, and logging jobid + logfile path) is solid and should make debugging submissions easier.I don’t see functional or reliability issues in these additions.
Also applies to: 347-398, 400-412, 435-499, 539-549, 552-555
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
snakemake_executor_plugin_slurm/validation.py (1)
132-139: Critical: Fix inverted validation logic for init_seconds_before_status_checks.Line 135 checks
or settings.init_seconds_before_status_checks > 0, which raises an error when the value is positive. This is backwards—the validation should reject negative values, not positive ones.Apply this diff to fix the logic:
if settings.init_seconds_before_status_checks is not None: if ( not isinstance(settings.init_seconds_before_status_checks, int) - or settings.init_seconds_before_status_checks > 0 + or settings.init_seconds_before_status_checks < 0 ): raise WorkflowError( - "init-seconds-before-status-checks must be a positive integer." + "init-seconds-before-status-checks must be a non-negative integer." )snakemake_executor_plugin_slurm/__init__.py (1)
74-111: Defer subprocess execution to avoid import-time overhead.The
_get_status_command_help()function executes multiple subprocess commands (which sacct,which squeue,scontrol show config) to build the help text. When this function is called at line 235 during field metadata construction, these subprocesses run at module import time, slowing imports and potentially failing on systems where SLURM tools aren't available or PATH isn't configured.Recommendation: Replace the dynamic help text with a static string that describes the auto-detection behavior without executing it:
def _get_status_command_help(): """Get help text for status_command.""" return ( "Command to query job status. Options: 'sacct', 'squeue'. " "Default is auto-detected: 'sacct' if available, otherwise 'squeue'. " "The auto-detection considers MinJobAge configuration to recommend " "the most reliable option for your cluster. " "Set explicitly to override auto-detection." )This preserves the informative help text while eliminating import-time subprocess calls.
🧹 Nitpick comments (1)
snakemake_executor_plugin_slurm/__init__.py (1)
597-601: Add defensive check for status_attempts and fix unused loop variable.While
status_attemptshasdefault=5, it's typed asOptional[int]and could theoretically beNone. Line 635'srange(status_attempts)would raiseTypeErrorifstatus_attemptsisNone.Additionally, Ruff flags the loop variable
iat line 635 as unused (B007).Apply this diff:
- status_attempts = self.workflow.executor_settings.status_attempts + status_attempts = self.workflow.executor_settings.status_attempts or 5 self.logger.debug( f"Checking the status of {len(active_jobs)} active jobs " f"with {status_attempts} attempts." )- for i in range(status_attempts): + for _ in range(status_attempts):This ensures at least one attempt and addresses the linter warning.
Also applies to: 635-635
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
snakemake_executor_plugin_slurm/__init__.py(17 hunks)snakemake_executor_plugin_slurm/validation.py(2 hunks)
🧰 Additional context used
🧠 Learnings (13)
📓 Common learnings
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 0
File: :0-0
Timestamp: 2025-01-13T09:54:22.950Z
Learning: PR #173 (adding gres resource specification) depends on PR #28 in snakemake-executor-plugin-slurm-jobstep repository, as changes were required in the cpu-settings function of the jobstep-Executor module.
Learnt from: johanneskoester
Repo: snakemake/snakemake-executor-plugin-slurm PR: 173
File: docs/further.md:96-96
Timestamp: 2025-03-10T15:20:51.829Z
Learning: PR #173 in snakemake-executor-plugin-slurm implements GPU job support by adding resources: `gres` for generic resource specifications (e.g., 'gpu:1'), `gpu`/`gpus` for specifying GPU counts, and `gpu_model`/`gpu_manufacturer` for specifying GPU types, allowing users to request GPU resources directly rather than having to use slurm_extra.
📚 Learning: 2025-03-31T11:52:05.373Z
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 249
File: snakemake_executor_plugin_slurm/submit_string.py:21-29
Timestamp: 2025-03-31T11:52:05.373Z
Learning: In the snakemake-executor-plugin-slurm project, account and partition parameters are validated in the submit function before reaching the get_submit_command function. By design, these parameters are either empty strings or already properly formatted with SLURM parameter tags, making additional validation in get_submit_command unnecessary.
Applied to files:
snakemake_executor_plugin_slurm/validation.pysnakemake_executor_plugin_slurm/__init__.py
📚 Learning: 2024-12-11T14:17:08.749Z
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 178
File: snakemake_executor_plugin_slurm/__init__.py:0-0
Timestamp: 2024-12-11T14:17:08.749Z
Learning: In the `snakemake-executor-plugin-slurm` project, when handling exceptions in `snakemake_executor_plugin_slurm/__init__.py`, prefer concise error messages and avoid unnecessary verbosity or exception chaining when it's not necessary.
Applied to files:
snakemake_executor_plugin_slurm/validation.pysnakemake_executor_plugin_slurm/__init__.py
📚 Learning: 2025-09-15T12:00:31.977Z
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 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/validation.pysnakemake_executor_plugin_slurm/__init__.py
📚 Learning: 2025-03-10T15:20:51.829Z
Learnt from: johanneskoester
Repo: snakemake/snakemake-executor-plugin-slurm PR: 173
File: docs/further.md:96-96
Timestamp: 2025-03-10T15:20:51.829Z
Learning: PR #173 in snakemake-executor-plugin-slurm implements GPU job support by adding resources: `gres` for generic resource specifications (e.g., 'gpu:1'), `gpu`/`gpus` for specifying GPU counts, and `gpu_model`/`gpu_manufacturer` for specifying GPU types, allowing users to request GPU resources directly rather than having to use slurm_extra.
Applied to files:
snakemake_executor_plugin_slurm/__init__.py
📚 Learning: 2025-01-20T09:13:26.443Z
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 193
File: .github/workflows/post_to_mastodon.yml:26-26
Timestamp: 2025-01-20T09:13:26.443Z
Learning: In the snakemake-executor-plugin-slurm repository, release PRs follow the naming pattern "chore(main): release X.Y.Z" where X.Y.Z is the version number.
Applied to files:
snakemake_executor_plugin_slurm/__init__.py
📚 Learning: 2025-01-13T09:54:22.950Z
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 0
File: :0-0
Timestamp: 2025-01-13T09:54:22.950Z
Learning: PR #173 (adding gres resource specification) depends on PR #28 in snakemake-executor-plugin-slurm-jobstep repository, as changes were required in the cpu-settings function of the jobstep-Executor module.
Applied to files:
snakemake_executor_plugin_slurm/__init__.py
📚 Learning: 2025-09-16T17:25:38.450Z
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 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/__init__.py
📚 Learning: 2025-01-17T17:27:32.446Z
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 187
File: .github/workflows/post_to_mastodon.yml:0-0
Timestamp: 2025-01-17T17:27:32.446Z
Learning: In the mastodon publishing workflow for snakemake-executor-plugin-slurm, the PR_TITLE environment variable is required by the post_to_mastodon.sh script and should be preserved.
Applied to files:
snakemake_executor_plugin_slurm/__init__.py
📚 Learning: 2025-07-22T17:31:41.629Z
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 333
File: .github/workflows/announce-release.yml:0-0
Timestamp: 2025-07-22T17:31:41.629Z
Learning: In the snakemake-executor-plugin-slurm repository, cmeesters prefers using `main` references for GitHub Actions to always get the latest version, with security provided through protected branches and mandatory CI checks rather than commit SHA pinning.
Applied to files:
snakemake_executor_plugin_slurm/__init__.py
📚 Learning: 2025-06-16T08:54:07.957Z
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 221
File: snakemake_executor_plugin_slurm/efficiency_report.py:0-0
Timestamp: 2025-06-16T08:54:07.957Z
Learning: In the context of SLURM executor plugin development, user cmeesters (experienced with SLURM) noted they are not aware of SLURM typically reporting memory values with decimals (like 1.5G), but accepted defensive programming to handle such cases anyway.
Applied to files:
snakemake_executor_plugin_slurm/__init__.py
📚 Learning: 2025-06-03T17:10:51.292Z
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 221
File: snakemake_executor_plugin_slurm/__init__.py:0-0
Timestamp: 2025-06-03T17:10:51.292Z
Learning: In the snakemake-executor-plugin-slurm repository, the create_efficiency_report method in snakemake_executor_plugin_slurm/__init__.py contains temporary testing/debugging code that logs directory contents using os.listdir() and directory paths. This code was inserted for testing purposes only and should be removed from the production implementation.
Applied to files:
snakemake_executor_plugin_slurm/__init__.py
📚 Learning: 2025-06-24T12:41:31.837Z
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 221
File: tests/tests.py:62-70
Timestamp: 2025-06-24T12:41:31.837Z
Learning: In the snakemake-executor-plugin-slurm efficiency report feature, the executor is responsible for creating the efficiency_report_path directory when generating efficiency reports. Tests should not manually check for directory existence but should rely on the executor to properly create necessary directories as part of its normal operation.
Applied to files:
snakemake_executor_plugin_slurm/__init__.py
🧬 Code graph analysis (2)
snakemake_executor_plugin_slurm/validation.py (1)
snakemake_executor_plugin_slurm/job_status_query.py (2)
get_min_job_age(7-60)is_query_tool_available(63-74)
snakemake_executor_plugin_slurm/__init__.py (2)
snakemake_executor_plugin_slurm/job_status_query.py (5)
get_min_job_age(7-60)is_query_tool_available(63-74)should_recommend_squeue_status_command(77-101)query_job_status_squeue(135-154)query_job_status_sacct(104-132)snakemake_executor_plugin_slurm/validation.py (3)
validate_slurm_extra(50-77)validate_executor_settings(113-167)validate_status_command_settings(80-110)
🪛 Ruff (0.14.6)
snakemake_executor_plugin_slurm/validation.py
121-123: Avoid specifying long messages outside the exception class
(TRY003)
130-130: Avoid specifying long messages outside the exception class
(TRY003)
137-139: Avoid specifying long messages outside the exception class
(TRY003)
145-147: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
145-147: Avoid specifying long messages outside the exception class
(TRY003)
149-151: Avoid specifying long messages outside the exception class
(TRY003)
156-158: Avoid specifying long messages outside the exception class
(TRY003)
162-164: Avoid specifying long messages outside the exception class
(TRY003)
snakemake_executor_plugin_slurm/__init__.py
63-67: Avoid specifying long messages outside the exception class
(TRY003)
543-543: Avoid specifying long messages outside the exception class
(TRY003)
635-635: Loop control variable i not used within loop body
Rename unused i to _i
(B007)
⏰ 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)
snakemake_executor_plugin_slurm/validation.py (2)
80-111: LGTM!The
validate_status_command_settingsfunction provides sensible warnings to guide users toward optimal status command configuration based on cluster setup (MinJobAge, tool availability).
119-164: LGTM on remaining validations.The validation logic for
status_command,status_attempts,efficiency_threshold,partition_config, anddelete_logfiles_older_thanis correct and provides clear error messages.snakemake_executor_plugin_slurm/__init__.py (4)
56-71: LGTM!The
_get_status_command_defaultfunction provides a sensible default selection with appropriate error handling when neither SLURM tool is available.
272-274: LGTM!The
__post_init__hook correctly callsvalidate_executor_settingsto validate configuration after initialization.
339-346: LGTM!The
get_status_commandmethod provides a clean interface with appropriate fallback to the default when the setting isn't configured.
607-631: LGTM!The status command selection logic implements intelligent runtime fallback between
sacctandsqueuebased on MinJobAge and tool availability. The query functions both return strings compatible withshell=Trueexecution at line 788.
This is an attempt to fulfil the request outlined in PR #336 .
Summary by CodeRabbit
New Features
Improvements
Chores
✏️ Tip: You can customize this high-level summary in your review settings.