Skip to content

Commit 263f3a8

Browse files
authored
fix: handle integer slurm_account values from YAML parsing (#448)
## Summary Snakemake's YAML parser automatically converts numeric-looking strings (e.g., `"123456789"`) to integers when populating `job.resources`. This caused `account.lower()` in `test_account()` to fail since `int` has no `lower()` method. ## Changes Convert `slurm_account` values to strings before use in `re.split()` to handle both string and integer values. ## Tests Added 3 new unit tests covering: - String account values - Integer account values (from YAML parsing) - Multiple string accounts <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Normalize SLURM account inputs (including numeric values) to strings, split multiple accounts, and ensure submission arguments are consistently formatted and quoted. * **Tests** * Added unit tests validating single, numeric, and multi-account scenarios to ensure correct account argument generation and invocation. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
1 parent 7fa975f commit 263f3a8

2 files changed

Lines changed: 85 additions & 2 deletions

File tree

snakemake_executor_plugin_slurm/__init__.py

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1293,7 +1293,7 @@ async def check_active_jobs(
12931293
# slurmdbd/accounting hiccups where job states are briefly incomplete.
12941294
for i in range(status_attempts):
12951295
async with self.status_rate_limiter:
1296-
(status_of_jobs, sacct_query_duration) = await query_job_status(
1296+
status_of_jobs, sacct_query_duration = await query_job_status(
12971297
status_command, self.logger
12981298
)
12991299
if status_of_jobs is None and sacct_query_duration is None:
@@ -1589,8 +1589,10 @@ def get_account_arg(self, job: JobExecutorInterface):
15891589
if job.resources.get("slurm_account"):
15901590
# split the account upon ',' and whitespace, to allow
15911591
# multiple accounts being given
1592+
# Note: YAML parsing may convert numeric strings (e.g. "123456") to int.
1593+
# Ensure we always work with strings for re.split and shlex.quote.
15921594
accounts = [
1593-
a for a in re.split(r"[,\s]+", job.resources.slurm_account) if a
1595+
a for a in re.split(r"[,\s]+", str(job.resources.slurm_account)) if a
15941596
]
15951597
for account in accounts:
15961598
# here, we check whether the given or guessed account is valid

tests/test_account.py

Lines changed: 81 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,81 @@
1+
"""Unit tests for get_account_arg() method."""
2+
3+
from unittest.mock import MagicMock, patch
4+
5+
from snakemake_executor_plugin_slurm import Executor
6+
7+
8+
class _Resources(dict):
9+
"""Dict-like resources with attribute access for known keys only."""
10+
11+
def __getattr__(self, name):
12+
try:
13+
return self[name]
14+
except KeyError as e:
15+
raise AttributeError(name) from e
16+
17+
18+
def _make_mock_job(
19+
rule_name="myrule", name=None, wildcards=None, jobid=1, is_group=False, **resources
20+
):
21+
"""Return a minimal mock job compatible with get_account_arg."""
22+
mock_resources = _Resources(resources)
23+
24+
mock_rule = MagicMock()
25+
mock_rule.name = rule_name
26+
27+
job = MagicMock()
28+
job.resources = mock_resources
29+
job.rule = mock_rule
30+
job.name = name if name is not None else rule_name
31+
job.wildcards = wildcards if wildcards is not None else {}
32+
job.is_group.return_value = is_group
33+
job.threads = resources.get("threads", 1)
34+
job.jobid = jobid
35+
return job
36+
37+
38+
class TestGetAccountArg:
39+
"""Tests for get_account_arg() method handling string and integer accounts."""
40+
41+
def _make_executor_stub(self):
42+
"""Return a minimal Executor stub."""
43+
executor = Executor.__new__(Executor)
44+
executor.logger = MagicMock()
45+
executor._fallback_account_arg = None
46+
return executor
47+
48+
# Patches prevent actual SLURM command execution (sacct/sacctmgr).
49+
# - get_account() would call 'sacct' to guess account from recent jobs.
50+
# - test_account() would call 'sacctmgr' or 'sshare' to validate account.
51+
# Mocks allow testing without a live SLURM cluster.
52+
@patch("snakemake_executor_plugin_slurm.get_account")
53+
@patch("snakemake_executor_plugin_slurm.test_account")
54+
def test_string_account(self, mock_test_account, mock_get_account, tmp_path):
55+
"""String account values work correctly."""
56+
executor = self._make_executor_stub()
57+
job = _make_mock_job(slurm_account="123456789")
58+
result = next(executor.get_account_arg(job))
59+
assert result == " -A 123456789"
60+
mock_test_account.assert_called_with("123456789", executor.logger)
61+
62+
@patch("snakemake_executor_plugin_slurm.get_account")
63+
@patch("snakemake_executor_plugin_slurm.test_account")
64+
def test_integer_account(self, mock_test_account, mock_get_account, tmp_path):
65+
"""Integer account values (from YAML parsing) work correctly."""
66+
executor = self._make_executor_stub()
67+
job = _make_mock_job(slurm_account=123456789)
68+
result = next(executor.get_account_arg(job))
69+
assert result == " -A 123456789"
70+
mock_test_account.assert_called_with("123456789", executor.logger)
71+
72+
@patch("snakemake_executor_plugin_slurm.get_account")
73+
@patch("snakemake_executor_plugin_slurm.test_account")
74+
def test_multiple_accounts_string(
75+
self, mock_test_account, mock_get_account, tmp_path
76+
):
77+
"""Multiple string accounts work correctly."""
78+
executor = self._make_executor_stub()
79+
job = _make_mock_job(slurm_account="acc1,acc2 acc3")
80+
results = list(executor.get_account_arg(job))
81+
assert results == [" -A acc1", " -A acc2", " -A acc3"]

0 commit comments

Comments
 (0)