fix: account lookup / test in multicluster environment #350
Conversation
Walkthroughget_account_arg now accepts multiple comma/whitespace-separated SLURM accounts and yields one or more safely quoted Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User
participant Snakemake
participant Plugin as SLURM Plugin
participant sacctmgr
participant sshare
User->>Snakemake: specify account(s)
Snakemake->>Plugin: get_account_arg(account_str)
Note over Plugin: split input on commas/whitespace\nfor each token: call test_account
Plugin->>sacctmgr: query accounts (primary)
alt sacctmgr returns accounts
sacctmgr-->>Plugin: accounts (captured in sacctmgr_report)
else sacctmgr empty or error
Plugin->>sshare: query accounts (fallback)
sshare-->>Plugin: accounts or error (captured in sshare_report)
end
Note over Plugin: dedupe discovered accounts\nverify requested tokens
alt token(s) valid
Plugin-->>Snakemake: yield one or more `-A` args (each shlex.quoted)
else none valid
Plugin-->>Snakemake: raise WorkflowError with available accounts + sacctmgr/sshare reports
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 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)
756-771: Normalize case for validation to avoid false negatives; stabilize error output.You compare account.lower() against raw entries, which can fail if the cluster returns mixed-case accounts. Normalize both sides and sort the list when displaying.
- accounts = set(_.strip() for _ in accounts.split("\n") if _) + accounts = {a.strip().lower() for a in accounts.splitlines() if a.strip()} @@ - if account.lower() not in accounts: + if account.strip().lower() not in accounts: raise WorkflowError( - f"The given account {account} appears to be invalid. Available " - f"accounts:\n{', '.join(accounts)}" + f"The given account {account} appears to be invalid. Available " + f"accounts:\n{', '.join(sorted(accounts))}" )
🧹 Nitpick comments (1)
snakemake_executor_plugin_slurm/__init__.py (1)
730-733: Prefer shell=False for subprocess calls (ruff S602).Avoid shell=True where not needed; pass argv list. This reduces injection risk and aligns with best practices. Using shlex.split here is a minimal-change approach.
- accounts = subprocess.check_output( - cmd, shell=True, text=True, stderr=subprocess.PIPE - ) + accounts = subprocess.check_output( + shlex.split(cmd), text=True, stderr=subprocess.PIPE + ) @@ - accounts = subprocess.check_output( - cmd, shell=True, text=True, stderr=subprocess.PIPE - ) + accounts = subprocess.check_output( + shlex.split(cmd), text=True, stderr=subprocess.PIPE + )Also applies to: 743-745
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
snakemake_executor_plugin_slurm/__init__.py(3 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-03-31T11:52:05.373Z
Learnt from: cmeesters
PR: snakemake/snakemake-executor-plugin-slurm#249
File: snakemake_executor_plugin_slurm/submit_string.py:21-29
Timestamp: 2025-03-31T11:52:05.373Z
Learning: In the snakemake-executor-plugin-slurm project, account and partition parameters are validated in the submit function before reaching the get_submit_command function. By design, these parameters are either empty strings or already properly formatted with SLURM parameter tags, making additional validation in get_submit_command unnecessary.
Applied to files:
snakemake_executor_plugin_slurm/__init__.py
📚 Learning: 2024-12-11T14:17:08.749Z
Learnt from: cmeesters
PR: snakemake/snakemake-executor-plugin-slurm#178
File: snakemake_executor_plugin_slurm/__init__.py:0-0
Timestamp: 2024-12-11T14:17:08.749Z
Learning: In the `snakemake-executor-plugin-slurm` project, when handling exceptions in `snakemake_executor_plugin_slurm/__init__.py`, prefer concise error messages and avoid unnecessary verbosity or exception chaining when it's not necessary.
Applied to files:
snakemake_executor_plugin_slurm/__init__.py
🪛 Ruff (0.12.2)
snakemake_executor_plugin_slurm/__init__.py
730-730: subprocess call with shell=True identified, security issue
(S602)
743-743: subprocess call with shell=True identified, security issue
(S602)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: testing
🔇 Additional comments (1)
snakemake_executor_plugin_slurm/__init__.py (1)
660-667: Enforce single Slurm account, filter empties, and safely quoteIn get_submit_command (snakemake_executor_plugin_slurm/init.py around line 660):
- Split and strip
job.resources.slurm_account, filter out any empty tokens.- If the resulting list’s length is not exactly one, raise a
WorkflowError.- Call
self.test_accounton that single account and return-A {shlex.quote(account)}.⛔ Skipped due to learnings
Learnt from: cmeesters PR: snakemake/snakemake-executor-plugin-slurm#249 File: snakemake_executor_plugin_slurm/submit_string.py:21-29 Timestamp: 2025-03-31T11:52:05.373Z Learning: In the snakemake-executor-plugin-slurm project, account and partition parameters are validated in the submit function before reaching the get_submit_command function. By design, these parameters are either empty strings or already properly formatted with SLURM parameter tags, making additional validation in get_submit_command unnecessary.Learnt from: cmeesters PR: snakemake/snakemake-executor-plugin-slurm#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.
|
@johanneskoester this is rel. urgent, as I cannot proceed well without this fix. |
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)
757-772: Case-normalization mismatch can cause false negativesaccount.lower() is compared against a case-sensitive set. Normalize both sides; also sort for stable messaging.
- accounts = set(_.strip() for _ in accounts.split("\n") if _) + accounts = {_.strip() for _ in accounts.splitlines() if _.strip()} + accounts_lower = {a.lower() for a in accounts} @@ - if account.lower() not in accounts: + if account.lower() not in accounts_lower: raise WorkflowError( - f"The given account {account} appears to be invalid. Available " - f"accounts:\n{', '.join(accounts)}" + f"The given account {account} appears to be invalid. Available " + f"accounts:\n{', '.join(sorted(accounts))}" )
♻️ Duplicate comments (1)
snakemake_executor_plugin_slurm/__init__.py (1)
729-729: Nice: Initialization prevents UnboundLocalError on error pathsInitializing sacctmgr_report/sshare_report resolves the earlier “referenced before assignment” issue.
🧹 Nitpick comments (4)
snakemake_executor_plugin_slurm/__init__.py (4)
726-734: Avoid shell=True; pass argv list and quote USER safely (sacctmgr call)Eliminates S602 and quoting pitfalls if $USER contains unexpected characters.
- cmd = f'sacctmgr -n -s list user "{os.environ["USER"]}" format=account%256' - sacctmgr_report = sshare_report = "" - try: - accounts = subprocess.check_output( - cmd, shell=True, text=True, stderr=subprocess.PIPE - ) + user = os.environ.get("USER", "") + cmd = ["sacctmgr", "-n", "-s", "list", "user", user, "format=account%256"] + sacctmgr_report = sshare_report = "" + try: + accounts = subprocess.check_output(cmd, text=True, stderr=subprocess.PIPE)
741-755: Record sacctmgr ‘empty output’ and drop shell=True in sshare fallbackImproves diagnostics and security; ensures WorkflowError always includes informative sacctmgr_report.
- if not accounts.strip(): - cmd = "sshare -U --format Account%256 --noheader" + if not accounts.strip(): + if not sacctmgr_report: + sacctmgr_report = ( + f"sacctmgr returned an empty account list for user " + f"'{os.environ.get('USER','')}'." + ) + cmd = ["sshare", "-U", "--format", "Account%256", "--noheader"] try: - accounts = subprocess.check_output( - cmd, shell=True, text=True, stderr=subprocess.PIPE - ) + accounts = subprocess.check_output(cmd, text=True, stderr=subprocess.PIPE) except subprocess.CalledProcessError as e: sshare_report = ( "Unable to test the validity of the given or guessed " f"SLURM account '{account}' with sshare: {e.stderr}." ) raise WorkflowError( f"The 'sacctmgr' reported: '{sacctmgr_report}' " f"and likewise 'sshare' reported: '{sshare_report}'." )
763-765: Clarify warning: tools returned empty list for the user, not “for account ”Avoids confusion; the commands list user accounts, not a specific account.
- self.logger.warning( - f"Both 'sacctmgr' and 'sshare' returned empty results for account " - f"'{account}'. Proceeding without account validation." - ) + self.logger.warning( + "Both 'sacctmgr' and 'sshare' returned an empty account list for " + f"user '{os.environ.get('USER','')}'. Proceeding without account validation." + )
660-667: Reduce repeated cluster queries when multiple accounts are providedCurrent approach invokes sacctmgr/sshare for each token via test_account. Consider fetching the user’s account set once and validating all tokens against it to cut latency and load.
Happy to propose a small helper (e.g., _fetch_user_accounts()) and refactor test_account/get_account_arg accordingly.
Also applies to: 726-755
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
snakemake_executor_plugin_slurm/__init__.py(3 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-03-31T11:52:05.373Z
Learnt from: cmeesters
PR: snakemake/snakemake-executor-plugin-slurm#249
File: snakemake_executor_plugin_slurm/submit_string.py:21-29
Timestamp: 2025-03-31T11:52:05.373Z
Learning: In the snakemake-executor-plugin-slurm project, account and partition parameters are validated in the submit function before reaching the get_submit_command function. By design, these parameters are either empty strings or already properly formatted with SLURM parameter tags, making additional validation in get_submit_command unnecessary.
Applied to files:
snakemake_executor_plugin_slurm/__init__.py
📚 Learning: 2024-12-11T14:17:08.749Z
Learnt from: cmeesters
PR: snakemake/snakemake-executor-plugin-slurm#178
File: snakemake_executor_plugin_slurm/__init__.py:0-0
Timestamp: 2024-12-11T14:17:08.749Z
Learning: In the `snakemake-executor-plugin-slurm` project, when handling exceptions in `snakemake_executor_plugin_slurm/__init__.py`, prefer concise error messages and avoid unnecessary verbosity or exception chaining when it's not necessary.
Applied to files:
snakemake_executor_plugin_slurm/__init__.py
🪛 Ruff (0.12.2)
snakemake_executor_plugin_slurm/__init__.py
731-731: subprocess call with shell=True identified, security issue
(S602)
744-744: subprocess call with shell=True identified, security issue
(S602)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: testing
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
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 (2)
snakemake_executor_plugin_slurm/__init__.py (2)
724-731: get_account never returns a guessed account (missing return).As written, a non-"none" value is discarded, disabling the fallback guess.
- possible_account = sacct_out.replace("(null)", "").strip() - if possible_account == "none": # some clusters may not use an account - return None + possible_account = sacct_out.replace("(null)", "").strip() + if not possible_account or possible_account == "none": # some clusters may not use an account + return None + return possible_account
774-789: Fix case-insensitive comparison; avoid rejecting valid accounts.You lower() the requested account but not the fetched set. Normalize both sides.
- accounts = set(_.strip() for _ in accounts.split("\n") if _) + accounts_raw = [ln.strip() for ln in accounts.splitlines() if ln.strip()] + accounts_norm = {a.lower() for a in accounts_raw} - if not accounts: + if not accounts_norm: self.logger.warning( - f"Both 'sacctmgr' and 'sshare' returned empty results for account " - f"'{account}'. Proceeding without account validation." + f"Both 'sacctmgr' and 'sshare' returned empty results for account " + f"'{account}'. Proceeding without account validation." ) return "" - if account.lower() not in accounts: + if account.lower() not in accounts_norm: raise WorkflowError( - f"The given account {account} appears to be invalid. Available " - f"accounts:\n{', '.join(accounts)}" + f"The given account '{account}' appears to be invalid. Available " + f"accounts:\n{', '.join(sorted(accounts_raw))}" )
♻️ Duplicate comments (2)
snakemake_executor_plugin_slurm/__init__.py (2)
671-684: Single-account submission with tokenization and quoting: solid.Splitting, validating each token, picking the first, and shlex-quoting the sbatch -A arg is correct for Slurm. Matches prior review guidance.
743-746: Populate sacctmgr_report when output is empty to improve diagnostics.You initialized the variable (good), but the combined error shows an empty sacctmgr_report when sacctmgr returns no rows. Set a message on the empty-output path.
if not accounts.strip(): + if not sacctmgr_report: + sacctmgr_report = ( + f"sacctmgr returned an empty account list for user " + f"'{os.environ.get('USER','')}'." + ) cmd = "sshare -U --format Account%256 --noheader"Also applies to: 758-772
🧹 Nitpick comments (5)
snakemake_executor_plugin_slurm/__init__.py (5)
671-684: Guard against empty token list to avoid IndexError.If slurm_account is only commas/whitespace, accounts becomes empty and accounts[0] raises. Fail fast with a clear error.
- accounts = [a for a in re.split(r"[,\s]+", job.resources.slurm_account) if a] + accounts = [a for a in re.split(r"[,\s]+", job.resources.slurm_account) if a] + if not accounts: + raise WorkflowError( + "No valid account name found in resource 'slurm_account'. " + "Provide a single account or a comma/space-separated list." + )
690-693: Quote the guessed account for consistency/safety.The fallback uses the unquoted account. Quote it like the explicit path.
- self._fallback_account_arg = f" -A {account}" + self._fallback_account_arg = f" -A {shlex.quote(account)}"
779-783: Clarify empty-results warning (it’s about the user’s account list, not the specific account).- self.logger.warning( - f"Both 'sacctmgr' and 'sshare' returned empty results for account " - f"'{account}'. Proceeding without account validation." - ) + self.logger.warning( + "Both 'sacctmgr' and 'sshare' returned an empty account list " + f"for user '{os.environ.get('USER','')}'. Proceeding without validation of " + f"'{account}'." + )
745-763: Prefer shell=False and argv lists for sacctmgr/sshare invocations.Reduces injection risk and quoting pitfalls; also avoids relying on the shell.
Example:
- cmd = f'sacctmgr -n -s list user "{os.environ["USER"]}" format=account%256' + user = os.environ.get("USER") or getpass.getuser() + cmd = ["sacctmgr", "-n", "-s", "list", "user", user, "format=account%256"] ... - accounts = subprocess.check_output( - cmd, shell=True, text=True, stderr=subprocess.PIPE - ) + accounts = subprocess.check_output( + cmd, text=True, stderr=subprocess.PIPE + ) ... - cmd = "sshare -U --format Account%256 --noheader" + cmd = ["sshare", "-U", "--format", "Account%256", "--noheader"] ... - accounts = subprocess.check_output( - cmd, shell=True, text=True, stderr=subprocess.PIPE - ) + accounts = subprocess.check_output( + cmd, text=True, stderr=subprocess.PIPE + )Add once at top-level:
import getpass
724-724: Don’t assume USER is set in env; fall back to getpass.getuser().More robust across non-login contexts and CI.
- cmd = f'sacct -nu "{os.environ["USER"]}" -o Account%256 | tail -1' + user = os.environ.get("USER") or getpass.getuser() + cmd = f'sacct -nu "{user}" -o Account%256 | tail -1'- cmd = f'sacctmgr -n -s list user "{os.environ["USER"]}" format=account%256' + user = os.environ.get("USER") or getpass.getuser() + cmd = f'sacctmgr -n -s list user "{user}" format=account%256'Add once:
import getpassAlso applies to: 745-745
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
snakemake_executor_plugin_slurm/__init__.py(3 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-31T11:52:05.373Z
Learnt from: cmeesters
PR: snakemake/snakemake-executor-plugin-slurm#249
File: snakemake_executor_plugin_slurm/submit_string.py:21-29
Timestamp: 2025-03-31T11:52:05.373Z
Learning: In the snakemake-executor-plugin-slurm project, account and partition parameters are validated in the submit function before reaching the get_submit_command function. By design, these parameters are either empty strings or already properly formatted with SLURM parameter tags, making additional validation in get_submit_command unnecessary.
Applied to files:
snakemake_executor_plugin_slurm/__init__.py
📚 Learning: 2024-12-11T14:17:08.749Z
Learnt from: cmeesters
PR: snakemake/snakemake-executor-plugin-slurm#178
File: snakemake_executor_plugin_slurm/__init__.py:0-0
Timestamp: 2024-12-11T14:17:08.749Z
Learning: In the `snakemake-executor-plugin-slurm` project, when handling exceptions in `snakemake_executor_plugin_slurm/__init__.py`, prefer concise error messages and avoid unnecessary verbosity or exception chaining when it's not necessary.
Applied to files:
snakemake_executor_plugin_slurm/__init__.py
🪛 Ruff (0.12.2)
snakemake_executor_plugin_slurm/__init__.py
748-748: subprocess call with shell=True identified, security issue
(S602)
761-761: subprocess call with shell=True identified, security issue
(S602)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: testing
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)
784-788: Case-insensitive check is incorrect; normalize the discovered accounts too.You compare account.lower() to a set of unnormalized tokens, causing valid accounts to be rejected if cases differ.
Apply:
- if account.lower() not in accounts: + accounts_lc = {a.lower() for a in accounts} + if account.lower() not in accounts_lc: raise WorkflowError( - f"The given account {account} appears to be invalid. Available " - f"accounts:\n{', '.join(accounts)}" + f"The given account {account} appears to be invalid. Available " + f"accounts:\n{', '.join(sorted(accounts))}" )
♻️ Duplicate comments (2)
snakemake_executor_plugin_slurm/__init__.py (2)
745-746: Ensure sacctmgr_report is informative when sacctmgr returns empty output.You initialize sacctmgr_report, but if sacctmgr succeeds with empty output and sshare fails, the WorkflowError will include an empty sacctmgr_report string.
Apply:
if not accounts.strip(): + if not sacctmgr_report: + sacctmgr_report = ( + f"sacctmgr returned an empty account list for user '{getuser()}'." + ) cmd = "sshare -U --format Account%256 --noheader"Also applies to: 757-771
671-683: Do not convert get_account_arg into a generator; return a single -A string and avoid mixed return types.
- Presence of yield makes this a generator in all code paths; elsewhere (Lines 685-699) you return a string. This API inconsistency is a likely runtime breakage when get_submit_command formats job_params["account"].
- sbatch accepts exactly one --account; emitting multiple -A flags will either fail or only honor the last one. Validate all tokens, but submit with a single selected account.
Apply:
- accounts = [ - a for a in re.split(r"[,\s]+", job.resources.slurm_account) if a - ] - for account in accounts: - # here, we check whether the given or guessed account is valid - # if not, a WorkflowError is raised - self.test_account(account) - # sbatch only allows one account per submission - # yield one after the other, if multiple were given - for account in accounts: - yield f" -A {shlex.quote(account)}" + accounts = [a for a in re.split(r"[,\s]+", job.resources.slurm_account) if a] + for acc in accounts: + # validate each provided/guessed account (raises on invalid) + self.test_account(acc) + # sbatch allows a single account; submit with the first token + selected = accounts[0] + if len(accounts) > 1: + self.logger.info( + f"Multiple accounts provided ({accounts}); submitting with '{selected}'." + ) + return f" -A {shlex.quote(selected)}"To ensure no fallout, please verify how submit_string.get_submit_command consumes job_params["account"]:
#!/bin/bash fd -a submit_string.py | xargs -I{} sed -n '1,240p' {} rg -nP -C3 "get_submit_command\(|job_params\[\s*[\"']account[\"']\s*\]" --type=py
🧹 Nitpick comments (2)
snakemake_executor_plugin_slurm/__init__.py (2)
744-744: Avoid KeyError on USER; use getpass.getuser().os.environ["USER"] can be unset in non-login contexts. Prefer getpass.getuser().
Example change (outside this hunk):
- Add near imports:
from getpass import getuser- Here:
- cmd = f'sacctmgr -n -s list user "{os.environ["USER"]}" format=account%256' + cmd = f'sacctmgr -n -s list user "{getuser()}" format=account%256'
747-749: Reduce shell=True usage for subprocess calls.Where feasible, pass argv lists and shell=False to mitigate injection risks and improve quoting guarantees.
Example:
- accounts = subprocess.check_output( - cmd, shell=True, text=True, stderr=subprocess.PIPE - ) + accounts = subprocess.check_output( + ["sacctmgr", "-n", "-s", "list", "user", getuser(), "format=account%256"], + text=True, stderr=subprocess.PIPE + )And similarly for sshare.
Also applies to: 760-762
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
snakemake_executor_plugin_slurm/__init__.py(3 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-31T11:52:05.373Z
Learnt from: cmeesters
PR: snakemake/snakemake-executor-plugin-slurm#249
File: snakemake_executor_plugin_slurm/submit_string.py:21-29
Timestamp: 2025-03-31T11:52:05.373Z
Learning: In the snakemake-executor-plugin-slurm project, account and partition parameters are validated in the submit function before reaching the get_submit_command function. By design, these parameters are either empty strings or already properly formatted with SLURM parameter tags, making additional validation in get_submit_command unnecessary.
Applied to files:
snakemake_executor_plugin_slurm/__init__.py
📚 Learning: 2024-12-11T14:17:08.749Z
Learnt from: cmeesters
PR: snakemake/snakemake-executor-plugin-slurm#178
File: snakemake_executor_plugin_slurm/__init__.py:0-0
Timestamp: 2024-12-11T14:17:08.749Z
Learning: In the `snakemake-executor-plugin-slurm` project, when handling exceptions in `snakemake_executor_plugin_slurm/__init__.py`, prefer concise error messages and avoid unnecessary verbosity or exception chaining when it's not necessary.
Applied to files:
snakemake_executor_plugin_slurm/__init__.py
🪛 Ruff (0.12.2)
snakemake_executor_plugin_slurm/__init__.py
747-747: subprocess call with shell=True identified, security issue
(S602)
760-760: subprocess call with shell=True identified, security issue
(S602)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: testing
🔇 Additional comments (2)
snakemake_executor_plugin_slurm/__init__.py (2)
742-744: Good change: prefer sacctmgr before sshare for multi-cluster correctness.Switching the lookup order matches SLURM’s data sources (slurmdbd vs single slurmctld).
773-776: Deduplicating accounts is good.Converting to a set removes distracting duplicates from sacctmgr/sshare output.
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)
777-792: Fix case-sensitivity and produce stable error messages.Compare case-insensitively but report the original account names in sorted order.
- accounts = set(_.strip() for _ in accounts.split("\n") if _) - - if not accounts: + accounts_raw = {_.strip() for _ in accounts.splitlines() if _} + if not accounts_raw: self.logger.warning( f"Both 'sacctmgr' and 'sshare' returned empty results for account " f"'{account}'. Proceeding without account validation." ) return "" - if account.lower() not in accounts: + accounts_lc = {a.lower() for a in accounts_raw} + if account.strip().lower() not in accounts_lc: raise WorkflowError( f"The given account {account} appears to be invalid. Available " - f"accounts:\n{', '.join(accounts)}" + f"accounts:\n{', '.join(sorted(accounts_raw))}" )
♻️ Duplicate comments (1)
snakemake_executor_plugin_slurm/__init__.py (1)
671-685: Do not turn get_account_arg into a generator — return a single-Astring.submit_string.py concatenates params.account as a preformatted string (call += f" {params.account}"), so yielding will break get_submit_command. Fix init.py to return one quoted account (log if multiple).
File: snakemake_executor_plugin_slurm/init.py (lines 671-685)
- # split the account upon ',' and whitespace, to allow - # multiple accounts being given - accounts = [ - a for a in re.split(r"[,\s]+", job.resources.slurm_account) if a - ] - for account in accounts: - # here, we check whether the given or guessed account is valid - # if not, a WorkflowError is raised - self.test_account(account) - # sbatch only allows one account per submission - # yield one after the other, if multiple were given - # we have to quote the account, because it might - # contain build-in shell commands - see issue #354 - for account in accounts: - yield f" -A {shlex.quote(account)}" + # Split on comma/whitespace and filter empties + accounts = [a for a in re.split(r"[,\s]+", job.resources.slurm_account) if a] + for a in accounts: + # Validate each token; WorkflowError will be raised if invalid + self.test_account(a) + # sbatch accepts a single --account; submit with the first token + selected = accounts[0] + if len(accounts) > 1: + self.logger.info( + f"Multiple accounts provided ({accounts}); submitting with '{selected}'." + ) + return f" -A {shlex.quote(selected)}"
🧹 Nitpick comments (2)
snakemake_executor_plugin_slurm/__init__.py (2)
716-718: Typo in comment; keep the quoting change.Comment says “build-in”; should be “built-in”. Quoting itself is correct.
- # we have to quote the partition, because it might - # contain build-in shell commands + # we have to quote the partition, because it might + # contain built-in shell commands
746-775: Harden subprocess usage and always populate sacctmgr_report on empty result.Avoid shell=True where not needed and ensure diagnostics are always set when sacctmgr returns no rows.
- cmd = f'sacctmgr -n -s list user "{os.environ["USER"]}" format=account%256' - sacctmgr_report = sshare_report = "" + cmd = [ + "sacctmgr", + "-n", + "-s", + "list", + "user", + os.environ.get("USER", ""), + "format=account%256", + ] + sacctmgr_report = sshare_report = "" try: - accounts = subprocess.check_output( - cmd, shell=True, text=True, stderr=subprocess.PIPE - ) + accounts = subprocess.check_output(cmd, text=True, stderr=subprocess.PIPE) except subprocess.CalledProcessError as e: sacctmgr_report = ( "Unable to test the validity of the given or guessed" f" SLURM account '{account}' with sacctmgr: {e.stderr}." ) accounts = "" if not accounts.strip(): - cmd = "sshare -U --format Account%256 --noheader" + if not sacctmgr_report: + sacctmgr_report = ( + f"sacctmgr returned an empty account list for user " + f"'{os.environ.get('USER','')}'." + ) + cmd = ["sshare", "-U", "--format", "Account%256", "--noheader"] try: - accounts = subprocess.check_output( - cmd, shell=True, text=True, stderr=subprocess.PIPE - ) + accounts = subprocess.check_output(cmd, text=True, stderr=subprocess.PIPE) except subprocess.CalledProcessError as e: sshare_report = ( "Unable to test the validity of the given or guessed " f"SLURM account '{account}' with sshare: {e.stderr}." ) raise WorkflowError( f"The 'sacctmgr' reported: '{sacctmgr_report}' " f"and likewise 'sshare' reported: '{sshare_report}'." )
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
snakemake_executor_plugin_slurm/__init__.py(4 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: cmeesters
PR: snakemake/snakemake-executor-plugin-slurm#0
File: :0-0
Timestamp: 2025-01-13T09:54:22.950Z
Learning: PR #173 (adding gres resource specification) depends on PR #28 in snakemake-executor-plugin-slurm-jobstep repository, as changes were required in the cpu-settings function of the jobstep-Executor module.
📚 Learning: 2025-03-31T11:52:05.373Z
Learnt from: cmeesters
PR: snakemake/snakemake-executor-plugin-slurm#249
File: snakemake_executor_plugin_slurm/submit_string.py:21-29
Timestamp: 2025-03-31T11:52:05.373Z
Learning: In the snakemake-executor-plugin-slurm project, account and partition parameters are validated in the submit function before reaching the get_submit_command function. By design, these parameters are either empty strings or already properly formatted with SLURM parameter tags, making additional validation in get_submit_command unnecessary.
Applied to files:
snakemake_executor_plugin_slurm/__init__.py
📚 Learning: 2024-12-11T14:17:08.749Z
Learnt from: cmeesters
PR: snakemake/snakemake-executor-plugin-slurm#178
File: snakemake_executor_plugin_slurm/__init__.py:0-0
Timestamp: 2024-12-11T14:17:08.749Z
Learning: In the `snakemake-executor-plugin-slurm` project, when handling exceptions in `snakemake_executor_plugin_slurm/__init__.py`, prefer concise error messages and avoid unnecessary verbosity or exception chaining when it's not necessary.
Applied to files:
snakemake_executor_plugin_slurm/__init__.py
🪛 Ruff (0.12.2)
snakemake_executor_plugin_slurm/__init__.py
751-751: subprocess call with shell=True identified, security issue
(S602)
764-764: subprocess call with shell=True identified, security issue
(S602)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: testing
🔇 Additional comments (1)
snakemake_executor_plugin_slurm/__init__.py (1)
693-693: Good: quote guessed/fallback account.Prevents shell injection; change looks correct.
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 (2)
snakemake_executor_plugin_slurm/__init__.py (2)
788-792: Fix case check; sort accounts for deterministic error messageComparing
account.lower()to the un-normalized set can reject valid accounts. Compare exactly (or normalize both sides). Also sort for stable output.- if account.lower() not in accounts: + if account not in accounts: raise WorkflowError( - f"The given account {account} appears to be invalid. Available " - f"accounts:\n{', '.join(accounts)}" + f"The given account {account} appears to be invalid. Available " + f"accounts:\n{', '.join(sorted(accounts))}" )
727-735: Bug: get_account never returns the inferred accountOn success (and not "none"), the function falls through and returns None. Return the inferred value.
possible_account = sacct_out.replace("(null)", "").strip() if possible_account == "none": # some clusters may not use an account return None + return possible_account
♻️ Duplicate comments (2)
snakemake_executor_plugin_slurm/__init__.py (2)
761-767: Always set sacctmgr_report when sacctmgr returns empty output (not just on exceptions)If sacctmgr succeeds but returns empty and sshare then fails, the combined error shows an empty sacctmgr report. Initialize it on the “empty output” path.
@@ - if not accounts.strip(): + if not accounts.strip(): + if not sacctmgr_report: + sacctmgr_report = ( + f"sacctmgr returned an empty account list for user " + f"'{os.environ.get('USER','')}'." + ) cmd = "sshare -U --format Account%256 --noheader"
310-313: Use single-account semantics; avoid generator/next() and StopIteration risk
- sbatch accepts exactly one account. Yielding many and calling
next(...)is awkward and will raiseStopIterationwhen the provided string splits into zero tokens (e.g., only commas/spaces).- Validate all tokens but submit with the first; log if extras; return a string, not a generator.
@@ - "account": next(self.get_account_arg(job)), + "account": self.get_account_arg(job), @@ def get_account_arg(self, job: JobExecutorInterface): - # split the account upon ',' and whitespace, to allow - # multiple accounts being given - accounts = [ - a for a in re.split(r"[,\s]+", job.resources.slurm_account) if a - ] - for account in accounts: - # here, we check whether the given or guessed account is valid - # if not, a WorkflowError is raised - self.test_account(account) - # sbatch only allows one account per submission - # yield one after the other, if multiple were given - # we have to quote the account, because it might - # contain build-in shell commands - see issue #354 - for account in accounts: - yield f" -A {shlex.quote(account)}" + accounts = [a for a in re.split(r"[,\s]+", job.resources.slurm_account) if a] + if not accounts: + raise WorkflowError("No usable account token found in 'slurm_account'.") + # validate all tokens but submit with the first + for a in accounts: + self.test_account(a) + selected = accounts[0] + if len(accounts) > 1: + self.logger.info( + f"Multiple accounts provided ({accounts}); submitting with '{selected}'." + ) + return f" -A {shlex.quote(selected)}"Also applies to: 671-686
🧹 Nitpick comments (1)
snakemake_executor_plugin_slurm/__init__.py (1)
748-766: Optional: avoid shell=True for sacctmgr/sshare callsMinor hardening: pass argv lists and
shell=Falseto reduce injection surface (Ruff S602). You already quote values, but this further improves posture.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
snakemake_executor_plugin_slurm/__init__.py(5 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: cmeesters
PR: snakemake/snakemake-executor-plugin-slurm#0
File: :0-0
Timestamp: 2025-01-13T09:54:22.950Z
Learning: PR #173 (adding gres resource specification) depends on PR #28 in snakemake-executor-plugin-slurm-jobstep repository, as changes were required in the cpu-settings function of the jobstep-Executor module.
📚 Learning: 2025-03-31T11:52:05.373Z
Learnt from: cmeesters
PR: snakemake/snakemake-executor-plugin-slurm#249
File: snakemake_executor_plugin_slurm/submit_string.py:21-29
Timestamp: 2025-03-31T11:52:05.373Z
Learning: In the snakemake-executor-plugin-slurm project, account and partition parameters are validated in the submit function before reaching the get_submit_command function. By design, these parameters are either empty strings or already properly formatted with SLURM parameter tags, making additional validation in get_submit_command unnecessary.
Applied to files:
snakemake_executor_plugin_slurm/__init__.py
📚 Learning: 2024-12-11T14:17:08.749Z
Learnt from: cmeesters
PR: snakemake/snakemake-executor-plugin-slurm#178
File: snakemake_executor_plugin_slurm/__init__.py:0-0
Timestamp: 2024-12-11T14:17:08.749Z
Learning: In the `snakemake-executor-plugin-slurm` project, when handling exceptions in `snakemake_executor_plugin_slurm/__init__.py`, prefer concise error messages and avoid unnecessary verbosity or exception chaining when it's not necessary.
Applied to files:
snakemake_executor_plugin_slurm/__init__.py
🪛 Ruff (0.12.2)
snakemake_executor_plugin_slurm/__init__.py
751-751: subprocess call with shell=True identified, security issue
(S602)
764-764: subprocess call with shell=True identified, security issue
(S602)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: testing
🔇 Additional comments (3)
snakemake_executor_plugin_slurm/__init__.py (3)
693-694: Good: quote guessed accountQuoting the fallback account prevents shell injection and aligns with how you handle explicit accounts.
716-718: Good: quote partitionSafer construction of the sbatch argument.
781-786: OK: clearer empty-result warningThe message now reflects both tools and the multicluster-first approach.
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)
777-785: Case handling bug: membership check mixes cases.
if account.lower() not in accounts:compares lowercase candidate to unnormalized set -> false negatives. Normalize both sides.- accounts = set(_.strip() for _ in accounts.split("\n") if _) + accounts = {_.strip() for _ in accounts.splitlines() if _.strip()} + accounts_lower = {a.lower() for a in accounts} ... - if not accounts: + if not accounts: self.logger.warning( f"Both 'sacctmgr' and 'sshare' returned empty results for account " f"'{account}'. Proceeding without account validation." ) return "" - if account.lower() not in accounts: + if account.lower() not in accounts_lower: raise WorkflowError( f"The given account {account} appears to be invalid. Available " f"accounts:\n{', '.join(accounts)}" )Also applies to: 788-792
♻️ Duplicate comments (3)
snakemake_executor_plugin_slurm/__init__.py (3)
310-312: Only the first provided account is used; surface this explicitly.
next(self.get_account_arg(job))consumes just one-A .... If multiple accounts are provided, log which one will be used to avoid confusion and ease debugging.
671-686: Validate tokens but submit with the first valid account; ignore invalid extras with a log.Currently, any invalid token raises, even if an earlier token is valid. Prefer selecting the first valid token (what sbatch accepts) and log others. Keep yielding a single arg to preserve the generator shape.
- accounts = [ - a for a in re.split(r"[,\s]+", job.resources.slurm_account) if a - ] - for account in accounts: - # here, we check whether the given or guessed account is valid - # if not, a WorkflowError is raised - self.test_account(account) - # sbatch only allows one account per submission - # yield one after the other, if multiple were given - # we have to quote the account, because it might - # contain build-in shell commands - see issue #354 - for account in accounts: - yield f" -A {shlex.quote(account)}" + accounts = [a for a in re.split(r"[,\s]+", job.resources.slurm_account) if a] + invalid = [] + selected = None + for a in accounts: + try: + self.test_account(a) + selected = a + break + except WorkflowError: + invalid.append(a) + if selected is None: + raise WorkflowError( + f"No valid SLURM account among provided values: {accounts}" + ) + if len(accounts) > 1: + self.logger.info( + f"Multiple accounts provided ({accounts}); submitting with '{selected}'." + ) + # sbatch allows one account; quote to avoid shell injection + yield f" -A {shlex.quote(selected)}"
761-775: Preserve diagnostics when sacctmgr returns empty (no exception).If sacctmgr succeeds but is empty, and sshare then fails,
sacctmgr_reportremains empty, degrading the combined error message. Record the “empty result” case.- if not accounts.strip(): + if not accounts.strip(): + if not sacctmgr_report: + sacctmgr_report = ( + f"sacctmgr returned an empty account list for user " + f"'{os.environ.get('USER','')}'." + ) cmd = "sshare -U --format Account%256 --noheader"
🧹 Nitpick comments (2)
snakemake_executor_plugin_slurm/__init__.py (2)
748-748: Avoid KeyError on USER env; be resilient in non-interactive contexts.
os.environ["USER"]can be unset. Use a safe getter.- cmd = f'sacctmgr -n -s list user "{os.environ["USER"]}" format=account%256' + user = os.environ.get("USER") or "" + cmd = f'sacctmgr -n -s list user "{user}" format=account%256'
751-751: Optional: drop shell=True for these checks (Ruff S602).Construct argv lists and call without a shell to shrink attack surface. Non-blocking, but aligns with tooling.
- accounts = subprocess.check_output( - cmd, shell=True, text=True, stderr=subprocess.PIPE - ) + accounts = subprocess.check_output( + ["sacctmgr", "-n", "-s", "list", "user", os.environ.get("USER",""), "format=account%256"], + text=True, + stderr=subprocess.PIPE, + ) @@ - accounts = subprocess.check_output( - cmd, shell=True, text=True, stderr=subprocess.PIPE - ) + accounts = subprocess.check_output( + ["sshare", "-U", "--format", "Account%256", "--noheader"], + text=True, + stderr=subprocess.PIPE, + )Also applies to: 764-764
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
snakemake_executor_plugin_slurm/__init__.py(5 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: cmeesters
PR: snakemake/snakemake-executor-plugin-slurm#0
File: :0-0
Timestamp: 2025-01-13T09:54:22.950Z
Learning: PR #173 (adding gres resource specification) depends on PR #28 in snakemake-executor-plugin-slurm-jobstep repository, as changes were required in the cpu-settings function of the jobstep-Executor module.
📚 Learning: 2025-03-31T11:52:05.373Z
Learnt from: cmeesters
PR: snakemake/snakemake-executor-plugin-slurm#249
File: snakemake_executor_plugin_slurm/submit_string.py:21-29
Timestamp: 2025-03-31T11:52:05.373Z
Learning: In the snakemake-executor-plugin-slurm project, account and partition parameters are validated in the submit function before reaching the get_submit_command function. By design, these parameters are either empty strings or already properly formatted with SLURM parameter tags, making additional validation in get_submit_command unnecessary.
Applied to files:
snakemake_executor_plugin_slurm/__init__.py
📚 Learning: 2024-12-11T14:17:08.749Z
Learnt from: cmeesters
PR: snakemake/snakemake-executor-plugin-slurm#178
File: snakemake_executor_plugin_slurm/__init__.py:0-0
Timestamp: 2024-12-11T14:17:08.749Z
Learning: In the `snakemake-executor-plugin-slurm` project, when handling exceptions in `snakemake_executor_plugin_slurm/__init__.py`, prefer concise error messages and avoid unnecessary verbosity or exception chaining when it's not necessary.
Applied to files:
snakemake_executor_plugin_slurm/__init__.py
📚 Learning: 2025-03-10T15:20:51.829Z
Learnt from: johanneskoester
PR: snakemake/snakemake-executor-plugin-slurm#173
File: docs/further.md:96-96
Timestamp: 2025-03-10T15:20:51.829Z
Learning: PR #173 in snakemake-executor-plugin-slurm implements GPU job support by adding resources: `gres` for generic resource specifications (e.g., 'gpu:1'), `gpu`/`gpus` for specifying GPU counts, and `gpu_model`/`gpu_manufacturer` for specifying GPU types, allowing users to request GPU resources directly rather than having to use slurm_extra.
Applied to files:
snakemake_executor_plugin_slurm/__init__.py
🪛 Ruff (0.12.2)
snakemake_executor_plugin_slurm/__init__.py
751-751: subprocess call with shell=True identified, security issue
(S602)
764-764: subprocess call with shell=True identified, security issue
(S602)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: testing
🔇 Additional comments (3)
snakemake_executor_plugin_slurm/__init__.py (3)
693-694: Good: quote guessed account.Quoting the guessed account closes an injection hole. LGTM.
716-718: Good: quote partition.Safer construction for
-p. LGTM.
746-755: sacctmgr-first is the right call; also good init of error strings.This aligns with multicluster behavior and avoids UnboundLocalError paths previously discussed. LGTM.
We need to perform the account testing with
sacctmgr, first, then withsshare.ssharemight not provide all accounts, as SLURM in a multicluster environment is only queried on one cluster. This is an observation from my cluster, wheressharedid only show accounts for cluster A, whereassacctmgrdisplayed for clustersAandB.The reason is, that
ssharequeries theslurmctldof a given cluster, whereassacctmgrqueries the overlooking SLURM database of all clusters in a multi cluster environment.Summary by CodeRabbit
New Features
Bug Fixes
Documentation