fix(slurm): handle integer slurm_account values from YAML parsing#448
fix(slurm): handle integer slurm_account values from YAML parsing#448andchamorro wants to merge 1 commit intosnakemake:mainfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughCoerces Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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.
🧹 Nitpick comments (2)
tests/test_array_jobs.py (1)
685-692: Test name is misleading: tests single integer, not multiple accounts.The test name
test_multiple_accounts_integersuggests it tests multiple integer accounts, but it only tests a single integer value123. Consider renaming totest_single_integer_accountfor clarity, or update the docstring to reflect the actual test purpose.♻️ Proposed fix for naming clarity
`@patch`("snakemake_executor_plugin_slurm.get_account") `@patch`("snakemake_executor_plugin_slurm.test_account") - def test_multiple_accounts_integer(self, mock_test_account, mock_get_account, tmp_path): - """Multiple integer accounts work correctly.""" + def test_single_integer_account(self, mock_test_account, mock_get_account, tmp_path): + """Single integer account is converted to string correctly.""" executor = self._make_executor_stub() job = _make_mock_job(slurm_account=123) results = list(executor.get_account_arg(job)) assert results == [" -A 123"]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_array_jobs.py` around lines 685 - 692, The test function name and docstring are misleading—rename the test function test_multiple_accounts_integer to test_single_integer_account and update its docstring from "Multiple integer accounts work correctly." to "Single integer account works correctly." to match the actual behavior; locate the test by the function name test_multiple_accounts_integer and references to _make_mock_job(slurm_account=123) and executor.get_account_arg to make the change.snakemake_executor_plugin_slurm/__init__.py (1)
1595-1605: Consider removing duplicatetest_accountcalls.Each account is validated twice: once in the first loop (lines 1595-1598) and again in the second loop (line 1604). The first validation pass is sufficient.
♻️ Proposed fix to remove duplicate validation
accounts = [ a for a in re.split(r"[,\s]+", str(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 test_account(account, self.logger) # 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: - test_account(account, self.logger) yield f" -A {shlex.quote(str(account))}"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@snakemake_executor_plugin_slurm/__init__.py` around lines 1595 - 1605, The code calls test_account twice for each entry in accounts — once in the initial for loop and again inside the yield loop; remove the first validation loop so each account is validated only once, keeping the validation call in the yield loop (test_account(account, self.logger)) before yielding f" -A {shlex.quote(str(account))}" to preserve the behavior and logging; ensure no other code relies on the removed loop.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@snakemake_executor_plugin_slurm/__init__.py`:
- Around line 1595-1605: The code calls test_account twice for each entry in
accounts — once in the initial for loop and again inside the yield loop; remove
the first validation loop so each account is validated only once, keeping the
validation call in the yield loop (test_account(account, self.logger)) before
yielding f" -A {shlex.quote(str(account))}" to preserve the behavior and
logging; ensure no other code relies on the removed loop.
In `@tests/test_array_jobs.py`:
- Around line 685-692: The test function name and docstring are
misleading—rename the test function test_multiple_accounts_integer to
test_single_integer_account and update its docstring from "Multiple integer
accounts work correctly." to "Single integer account works correctly." to match
the actual behavior; locate the test by the function name
test_multiple_accounts_integer and references to
_make_mock_job(slurm_account=123) and executor.get_account_arg to make the
change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 69d92cfd-ca73-4e22-96ec-16e0dc53400b
📒 Files selected for processing (2)
snakemake_executor_plugin_slurm/__init__.pytests/test_array_jobs.py
cmeesters
left a comment
cmeesters
left a comment
There was a problem hiding this comment.
Thank you for your contribution! I left a couple of comments to address. Generally speaking, I am happy to accept.
| for account in accounts: | ||
| test_account(account, self.logger) | ||
| yield f" -A {shlex.quote(account)}" | ||
| yield f" -A {shlex.quote(str(account))}" |
There was a problem hiding this comment.
is this conversion necessary, if you convert an account to str before?
There was a problem hiding this comment.
It isn't specifically necessary for the situation I encountered; it's a preventive measure suggested by Sonnet.
| self.logger.warning(f"Guessed SLURM account: {account}") | ||
| test_account(f"{account}", self.logger) | ||
| self._fallback_account_arg = f" -A {shlex.quote(account)}" | ||
| self._fallback_account_arg = f" -A {shlex.quote(str(account))}" |
There was a problem hiding this comment.
is this conversion necessary, after the conversion which happened already?
There was a problem hiding this comment.
It isn't specifically necessary for the situation I encountered; it's a preventive measure suggested by Sonnet.
There was a problem hiding this comment.
I understand the reason, but would rather have a comment about the type (I will introduce more and more type annotations to functions) and where it has been set than redundant conversions.
There was a problem hiding this comment.
I agree, I will remove the edits and leave some comments.
tests/test_array_jobs.py
Outdated
| executor.report_job_error.assert_not_called() | ||
|
|
||
|
|
||
| class TestGetAccountArg: |
There was a problem hiding this comment.
account testing ought not to go into the array test script
There was a problem hiding this comment.
My mistake, I just associate array with slurm arrays. I will move to test/test_account.py
tests/test_array_jobs.py
Outdated
| executor._fallback_account_arg = None | ||
| return executor | ||
|
|
||
| @patch("snakemake_executor_plugin_slurm.get_account") |
There was a problem hiding this comment.
unittest mock patch decorator that replace get_account() and test_account(). Replace the real function with MagicMock objects.
There was a problem hiding this comment.
ah yes. Mind leaving a comment? Or I would stumble across this and accidentally remove it sometime into the future.
There was a problem hiding this comment.
I will leave a comment in the new test location tests/test_account.py.
andchamorro
force-pushed
the
fix/integer-account-handling
branch
from
b9346af to
7b06a22
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/test_account.py (1)
47-83: Consider asserting fallback path is not used in explicit-account tests.Since these tests provide
slurm_account, addingmock_get_account.assert_not_called()would make regressions in control flow easier to catch.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_account.py` around lines 47 - 83, For each test that supplies an explicit slurm_account (test_string_account, test_integer_account, test_multiple_accounts_string, test_multiple_accounts_integer), assert that the fallback get_account path was not used by adding mock_get_account.assert_not_called() after the existing assertions; locate the checks in the respective test_* functions that call executor.get_account_arg(job) and insert the mock_get_account.assert_not_called() immediately after the result assertions to ensure regressions in control flow are caught.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/test_account.py`:
- Around line 78-83: Replace the single numeric account in
test_multiple_accounts_integer with a comma-delimited numeric string (e.g.,
slurm_account="123,456") so the test actually covers splitting numeric-looking
multi-accounts; then update the assertion to verify get_account_arg(job)
produces entries for both account IDs (check for both "123" and "456" in the
produced argument(s) or match the same multi-account output format used by other
tests), referencing test_multiple_accounts_integer, _make_mock_job, and
get_account_arg to locate the change.
---
Nitpick comments:
In `@tests/test_account.py`:
- Around line 47-83: For each test that supplies an explicit slurm_account
(test_string_account, test_integer_account, test_multiple_accounts_string,
test_multiple_accounts_integer), assert that the fallback get_account path was
not used by adding mock_get_account.assert_not_called() after the existing
assertions; locate the checks in the respective test_* functions that call
executor.get_account_arg(job) and insert the
mock_get_account.assert_not_called() immediately after the result assertions to
ensure regressions in control flow are caught.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 78910a52-6b68-4be9-8650-8f4ce4aa8972
📒 Files selected for processing (2)
snakemake_executor_plugin_slurm/__init__.pytests/test_account.py
✅ Files skipped from review due to trivial changes (1)
- snakemake_executor_plugin_slurm/init.py
tests/test_account.py
Outdated
| def test_multiple_accounts_integer(self, mock_test_account, mock_get_account, tmp_path): | ||
| """Multiple integer accounts work correctly.""" | ||
| executor = self._make_executor_stub() | ||
| job = _make_mock_job(slurm_account=123) | ||
| results = list(executor.get_account_arg(job)) | ||
| assert results == [" -A 123"] No newline at end of file |
There was a problem hiding this comment.
test_multiple_accounts_integer does not actually test multiple accounts.
On Line 81, slurm_account=123 yields only one account, so this case doesn’t validate multi-account splitting for numeric-looking values as the test name/docstring suggests.
Proposed test correction
- def test_multiple_accounts_integer(self, mock_test_account, mock_get_account, tmp_path):
- """Multiple integer accounts work correctly."""
+ def test_multiple_numeric_accounts(self, mock_test_account, mock_get_account, tmp_path):
+ """Multiple numeric account tokens are split and emitted correctly."""
executor = self._make_executor_stub()
- job = _make_mock_job(slurm_account=123)
+ job = _make_mock_job(slurm_account="123,456 789")
results = list(executor.get_account_arg(job))
- assert results == [" -A 123"]
+ assert results == [" -A 123", " -A 456", " -A 789"]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/test_account.py` around lines 78 - 83, Replace the single numeric
account in test_multiple_accounts_integer with a comma-delimited numeric string
(e.g., slurm_account="123,456") so the test actually covers splitting
numeric-looking multi-accounts; then update the assertion to verify
get_account_arg(job) produces entries for both account IDs (check for both "123"
and "456" in the produced argument(s) or match the same multi-account output
format used by other tests), referencing test_multiple_accounts_integer,
_make_mock_job, and get_account_arg to locate the change.
7b06a22 to
ae009be
Compare
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. Convert slurm_account and account values to strings before use in re.split() and shlex.quote() calls. Fixes the issue where users specifying slurm_account as a numeric string in their YAML profile would get an AttributeError.
andchamorro
force-pushed
the
fix/integer-account-handling
branch
from
ae009be to
67cd465
Compare
2 participants
Summary
Snakemake's YAML parser automatically converts numeric-looking strings (e.g.,
"123456789") to integers when populatingjob.resources. This causedaccount.lower()intest_account()to fail sinceinthas nolower()method.Changes
Convert
slurm_accountandaccountvalues to strings before use inre.split()andshlex.quote()calls to handle both string and integer values.Tests
Added 4 new unit tests covering:
Summary by CodeRabbit
Bug Fixes
Tests