fix: regression in dynamic resource handling by cademirch · Pull Request #4038 · snakemake/snakemake

cademirch · 2026-03-10T15:12:43Z

Fixes dynamic resource parsing regression introduced in #3421. Closes #3970

QC

The PR contains a test case for the changes or the changes are already covered by an existing test case.
The documentation (docs/) is updated to reflect the changes or this is not necessary (e.g. if the change does neither modify the language nor the behavior or functionalities of Snakemake).

Summary by CodeRabbit

Bug Fixes
- Preserve unresolved placeholder resource values during evaluation so pending resource expressions remain intact and are not incorrectly converted.
Tests
- Added a test validating a minimal sequential workflow to confirm resource handling.
- Updated an existing test to adjust embedded shell invocation structure for more robust execution.

Expressions referencing input.size_mb in --set-resources or --default-resources crash with ResourceConversionError when input files don't exist yet. The wrapped evaluator now passes TBDString through instead of requiring a numeric result, matching the convention in Resource.evaluate().

coderabbitai · 2026-03-10T15:13:07Z

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

@coderabbitai resume to resume automatic reviews.
@coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

▶️ Resume reviews
🔍 Trigger review

📝 Walkthrough

Walkthrough

Short-circuits Resource._wrap_evaluator: when the underlying evaluator returns a TBDString, the value is returned immediately without type-checking or unit conversion, preserving unevaluated placeholder values. Also adds a test case and a minimal Snakefile to cover dynamic resource evaluation.

Changes

Cohort / File(s)	Summary
Core Fix `src/snakemake/resources.py`	Add short-circuit in `Resource._wrap_evaluator` to return `TBDString` results directly, avoiding subsequent type checks and unit conversion for unevaluated placeholders.
Tests — new case `tests/test_issue3970/Snakefile`, `tests/tests.py`	Add `test_issue_3970` and a minimal Snakefile (`one` → `done.txt`); update `test_github_issue1384` to wrap a multiline shell invocation with a nested `shell(...)` call.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title 'fix: regression in dynamic resource handling' clearly and concisely summarizes the main change—fixing a regression in how dynamic resources are handled.
Description check	✅ Passed	The PR description follows the template, includes a clear explanation of the fix and references to issues, and both QC checkboxes are marked as completed indicating test coverage and documentation review.
Linked Issues check	✅ Passed	The PR addresses the core objective from issue `#3970`: allowing dynamic resource evaluators to return TBDString without raising ResourceConversionError, restoring expected behavior for expressions like 'mem_mb: 3*1024 + input.size_mb'.
Out of Scope Changes check	✅ Passed	All changes are directly related to fixing the dynamic resource handling regression: the core fix in Resource._wrap_evaluator, a test case, and a test file update.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

coderabbitai

Actionable comments posted: 1

🤖 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/tests.py`:
- Around line 2295-2300: The test test_issue_3970 passes a shellcmd string with
POSIX single quotes that break on Windows; update the shellcmd argument in the
run(...) call so the outer Python string uses single quotes and the inner CLI
expression uses double quotes (i.e., shellcmd="snakemake --default-resources
\"mem_mb=3*1024+input.size_mb\"") so both POSIX shells and cmd.exe receive the
expression correctly; locate the test by the function name test_issue_3970 and
the run(...) invocation that uses dpath("test_issue3970") to make the change.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 0e5cd367-b4f0-4134-8cf9-b3fccce56067

📥 Commits

Reviewing files that changed from the base of the PR and between 9474afc and e918cf8.

📒 Files selected for processing (1)

tests/tests.py

coderabbitai · 2026-03-10T15:45:14Z

tests/tests.py

+def test_issue_3970():
+    run(
+        dpath("test_issue3970"),
+        shellcmd="snakemake --default-resources 'mem_mb=3*1024+input.size_mb'",
+        check_results=False,
+    )


⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

# First, let's find and verify the file location find . -name "tests.py" -type f | head -20

Repository: snakemake/snakemake

Length of output: 79

🏁 Script executed:

# Read the specific lines mentioned in the review if [ -f "tests/tests.py" ]; then sed -n '2290,2305p' tests/tests.py fi

Repository: snakemake/snakemake

Length of output: 435

🏁 Script executed:

# Look for similar test patterns and `@skip_on_windows` decorators rg "@skip_on_windows" tests/tests.py -B 5 -A 5 | head -100

Repository: snakemake/snakemake

Length of output: 2287

🏁 Script executed:

# Search for other shellcmd patterns in the test file to understand quoting conventions rg 'shellcmd=' tests/tests.py | head -30

Repository: snakemake/snakemake

Length of output: 2252

🏁 Script executed:

# Find test_issue3970 directory find . -type d -name "*issue3970*" -o -name "*issue_3970*"

Repository: snakemake/snakemake

Length of output: 85

🏁 Script executed:

# Search for any comments or documentation about issue 3970 rg "3970" --type-list | head -5 rg "3970" . --type py -B 2 -A 2 | head -50

Repository: snakemake/snakemake

Length of output: 764

🏁 Script executed:

# Check if there are any platform-specific considerations in the test configuration rg "test_issue3970" . -B 5 -A 5

Repository: snakemake/snakemake

Length of output: 463

🏁 Script executed:

# List contents of test_issue3970 directory ls -la tests/test_issue3970/ cat tests/test_issue3970/Snakefile 2>/dev/null || echo "No Snakefile found" find tests/test_issue3970 -type f | xargs head -50

Repository: snakemake/snakemake

Length of output: 707

🏁 Script executed:

# Check the git history or any PR that introduced this test git log --oneline --all -- "*issue3970*" 2>/dev/null | head -20

Repository: snakemake/snakemake

Length of output: 111

🏁 Script executed:

# Search for related issue documentation in the repository find . -name "*.md" -o -name "*.rst" | xargs grep -l "3970" 2>/dev/null

Repository: snakemake/snakemake

Length of output: 47

Use cross-platform quoting for the CLI expression.

Line 2298 uses POSIX-style single quotes, which cmd.exe does not treat as special characters and will pass literally to the invoked command. This causes the test to fail on Windows before it exercises the dynamic default resources feature. Swap the quote styles: use single quotes for the Python string and double quotes for the shell expression, so the quotes are properly removed on both POSIX shells and Windows cmd.exe.

Suggested fix

def test_issue_3970(): run( dpath("test_issue3970"), - shellcmd="snakemake --default-resources 'mem_mb=3*1024+input.size_mb'", + shellcmd='snakemake --default-resources "mem_mb=3*1024+input.size_mb"', check_results=False, )

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed. In `@tests/tests.py` around lines 2295 - 2300, The test test_issue_3970 passes a shellcmd string with POSIX single quotes that break on Windows; update the shellcmd argument in the run(...) call so the outer Python string uses single quotes and the inner CLI expression uses double quotes (i.e., shellcmd="snakemake --default-resources \"mem_mb=3*1024+input.size_mb\"") so both POSIX shells and cmd.exe receive the expression correctly; locate the test by the function name test_issue_3970 and the run(...) invocation that uses dpath("test_issue3970") to make the change.

johanneskoester · 2026-03-11T09:27:56Z

Needs a pixi run format

🤖 I have created a release *beep* *boop* --- ## [9.17.0](v9.16.3...v9.17.0) (2026-03-13) ### Features * Allow storing snakemake metadata in files or databases ([#4012](#4012)) ([dd75f31](dd75f31)) * Allow to specify comparison command per-unit test ([#3956](#3956)) ([b88171c](b88171c)) * job table orderd topological when run is started ([#4018](#4018)) ([75cf506](75cf506)) * lambda functions for priority in rules ([#3253](#3253)) ([d2aa226](d2aa226)) * Make on... directive of modules accessible ([#4050](#4050)) ([e9f2e1c](e9f2e1c)) ### Bug Fixes * adjust conda tests to not fail on apple silicon; fix [#4040](#4040) ([#4049](#4049)) ([f5b0142](f5b0142)) * allow "--containerize apptainer" to output apptainer format instead of dockerfile ([#4030](#4030)) ([f5cac30](f5cac30)) * apptainer command not recognized when singularity is absent ([#4010](#4010)) ([b8162e2](b8162e2)) * capture stderr when tests fail ([#3995](#3995)) ([97d74ba](97d74ba)) * **docs:** make Data-dependent conditional execution a complete example ([#4043](#4043)) ([3a1d7f2](3a1d7f2)) * don't build the DAG when running unlock. Fixes [#4000](#4000) and [#198](#198) ([#4007](#4007)) ([acf79fd](acf79fd)) * Ensure pixi tasks may be run as advertised ([#4046](#4046)) ([88253c2](88253c2)) * fix checkpoint handling corner cases ([#3870](#3870) and [#3559](#3559)) ([#4015](#4015)) ([63f4257](63f4257)) * issue 3642 ([#4054](#4054)) ([76e6fc2](76e6fc2)) * issue 3815 ([#4026](#4026)) ([b0eec96](b0eec96)) * logging None in shellcmd context causes error ([#4064](#4064)) ([d0652cd](d0652cd)) * lookup function returns default value for empty DataFrame queries ([#4056](#4056)) ([f71de97](f71de97)) * make `cache: omit-software` a rule specific property ([#4085](#4085)) ([034a9e7](034a9e7)) * reduce number of tests leaving temporary files behind ([#4033](#4033)) ([a3a1c97](a3a1c97)) * regression in dynamic resource handling ([#4038](#4038)) ([f2c554a](f2c554a)) * somewhat shorter announce message ([#4080](#4080)) ([57efc71](57efc71)) ### Performance Improvements * switch reretry with tenacity; decouple container classes (with Python 3.7 compat for old scripts) from rest of the codebase (enabling moving to newer python versions) ([#4032](#4032)) ([ffb19e7](ffb19e7)) ### Documentation * Add AI-assisted contributions policy to contributing guidelines ([#4051](#4051)) ([dd70526](dd70526)) * **codebase:** Update & simplify plugin architecture section ([#4052](#4052)) ([176cf63](176cf63)) * Correct workflow.source_path() description in documentation ([#4036](#4036)) ([45883c5](45883c5)) * fixed wrong code example for collect() function ([#4037](#4037)) ([5c85ed8](5c85ed8)) * Minor docs improvements ([#4089](#4089)) ([29ea226](29ea226)) * switch to sphinx_design for tabs ([#3976](#3976)) ([9674614](9674614)) * typo in the migration table breaking a pip install command ([#4024](#4024)) ([66f9dda](66f9dda)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).

cademirch added 3 commits March 10, 2026 15:48

add test; make sure it fails

e4c833c

update test to use cli resource parsing

ffd7e7b

cademirch marked this pull request as ready for review March 10, 2026 15:12

cademirch requested a review from johanneskoester as a code owner March 10, 2026 15:12

cademirch added 2 commits March 10, 2026 16:25

formatting

9474afc

whitespace causing issue on windows test

e918cf8

coderabbitai bot reviewed Mar 10, 2026

View reviewed changes

cademirch added 2 commits March 10, 2026 18:51

fix test shell cmd

342fe4b

Merge branch 'main' into fix-3970

c741537

johanneskoester approved these changes Mar 11, 2026

View reviewed changes

johanneskoester enabled auto-merge (squash) March 11, 2026 09:28

fmt

f3028f9

johanneskoester disabled auto-merge March 11, 2026 09:52

johanneskoester merged commit f2c554a into snakemake:main Mar 11, 2026
5 of 57 checks passed

snakemake-bot mentioned this pull request Mar 11, 2026

chore(main): release 9.17.0 #3977

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: regression in dynamic resource handling#4038

fix: regression in dynamic resource handling#4038
johanneskoester merged 8 commits intosnakemake:mainfrom
cademirch:fix-3970

cademirch commented Mar 10, 2026 •

edited by coderabbitai bot

Loading

Uh oh!

coderabbitai bot commented Mar 10, 2026 •

edited

Loading

Reviews paused

Walkthrough

Changes

Estimated code review effort

❌ Failed checks (1 warning)

Uh oh!

coderabbitai bot left a comment

Uh oh!

coderabbitai bot Mar 10, 2026

Uh oh!

johanneskoester commented Mar 11, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

cademirch commented Mar 10, 2026 • edited by coderabbitai bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

QC

Summary by CodeRabbit

Uh oh!

coderabbitai bot commented Mar 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews paused

Walkthrough

Changes

Estimated code review effort

❌ Failed checks (1 warning)

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

coderabbitai bot Mar 10, 2026

Choose a reason for hiding this comment

Uh oh!

johanneskoester commented Mar 11, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

cademirch commented Mar 10, 2026 •

edited by coderabbitai bot

Loading

coderabbitai bot commented Mar 10, 2026 •

edited

Loading