Skip to content

fix: resolve pathvars before constructing storage queries#3969

Merged
johanneskoester merged 2 commits into
snakemake:mainfrom
koen-vg:pathvars-storage-fix
May 29, 2026
Merged

fix: resolve pathvars before constructing storage queries#3969
johanneskoester merged 2 commits into
snakemake:mainfrom
koen-vg:pathvars-storage-fix

Conversation

@koen-vg

@koen-vg koen-vg commented Feb 12, 2026 •
edited by coderabbitai Bot
Loading

Copy link
Copy Markdown
Contributor

When using pathvars with a default storage provider (e.g. fs), the storage query was constructed from the unresolved path (still containing literal tokens). This caused rsync failures since the storage plugin tried to sync paths like storage/<processing>/... instead of the resolved storage/processing/... paths.

Apply pathvars resolution to the path before building the storage query in PathModifier.apply_default_storage. The original unresolved path still flows through flag_with_storage_object so IOFile.__new__ continues resolving pathvars on the local file string as before.

I've also added a test covering the pathvars + storage provider case.

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

    • Fixed path variable expansion to occur before storage provider queries, ensuring dynamic path variables are correctly resolved in storage operations.

  • Tests

    • Added test coverage for path variable handling with storage providers.

@koen-vg
fix: resolve pathvars before constructing storage queries
35f9a72 
When using pathvars with a default storage provider (e.g. fs), the
storage query was constructed from the unresolved path (still containing
literal <pathvar> tokens). This caused rsync failures since the storage
plugin tried to sync paths like "storage/<processing>/..." instead of
the resolved "storage/processing/..." paths.

Apply pathvars resolution to the path before building the storage query
in PathModifier.apply_default_storage. The original unresolved path
still flows through flag_with_storage_object so IOFile.__new__ continues
resolving pathvars on the local file string as before.
@coderabbitai

coderabbitai Bot commented Feb 12, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor
📝 Walkthrough

Walkthrough

The PR modifies how default storage paths are resolved by applying path variable expansion through self.workflow.pathvars.apply() before path normalization, and adds comprehensive test coverage including test fixtures and a test case to validate this behavior.

Changes

Cohort / File(s) Summary
Core Logic
src/snakemake/path_modifier.py		 Modified apply_default_storage to resolve path variables via self.workflow.pathvars.apply(str(path)) before normalizing, changing the order of operations in the storage query path construction.
Test Infrastructure
tests/test_pathvars_storage/Snakefile, tests/test_pathvars_storage/storage/input.txt, tests/test_pathvars_storage/expected-results/storage/output/result.txt		 Added test workflow definition with two rules and corresponding test fixture files (input and expected output) to support pathvars storage testing.
Test Function
tests/tests.py		 Added new test function test_pathvars_storage() that exercises path variable expansion with filesystem storage provider using the "storage" prefix on a single core.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

🚥 Pre-merge checks | ✅ 3 | ❌ 1
❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 25.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely summarizes the main change: resolving pathvars before constructing storage queries to fix a bug with storage plugins.
Description check ✅ Passed The PR description is comprehensive and complete, covering the bug explanation, the fix implementation, test addition, and both QC checkboxes marked as complete.
Merge Conflict Detection ✅ Passed ✅ No merge conflicts detected when merging into main

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

✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

No actionable comments were generated in the recent review. 🎉

Tip

Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord.

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.

@johanneskoester johanneskoester merged commit bd15237 into snakemake:main May 29, 2026
110 of 112 checks passed
@snakemake-bot snakemake-bot mentioned this pull request May 29, 2026
Merged
johanneskoester pushed a commit that referenced this pull request May 29, 2026
@snakemake-bot
chore(main): release 9.21.1 (#4200)
5b5482e 
🤖 I have created a release *beep* *boop*
---


##
[9.21.1](v9.21.0...v9.21.1)
(2026-05-29)


### Bug Fixes

* add default json function to benchmarks
([#4128](#4128))
([41fab22](41fab22))
* do not rerun when checkpoint job missing but downstream file exists
([#4124](#4124))
([a060b93](a060b93))
* ensure that error logs contain all available details
([#4183](#4183))
([74a86e9](74a86e9))
* handle missing pss attribute in benchmark on Windows
([#4160](#4160))
([da52080](da52080))
* implement Resources.setdefault
([#3968](#3968))
([2413e99](2413e99))
* reporting remote nodes number
([#3978](#3978))
([8c534f0](8c534f0))
* resolve pathvars before constructing storage queries
([#3969](#3969))
([bd15237](bd15237))
* storage temp() file cleanup with RemoteProvider
([#4189](#4189))
([898bad1](898bad1))
* tolerate FileNotFoundError in drop_iocache
([#4153](#4153))
([#4191](#4191))
([ce26b28](ce26b28))


### Documentation

* Added guide on debugging workflows
([#4029](#4029))
([3d052ae](3d052ae))
* **cli:** Remove broken ref bold markup
([#4204](#4204))
([1200ebf](1200ebf))
* remove duplicated resources attribute in rules.rst
([#4190](#4190))
([6c8ecdd](6c8ecdd))
* **rules:** Update script type hint advice
([#4193](#4193))
([6108712](6108712))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@johanneskoester johanneskoester johanneskoester approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@koen-vg @johanneskoester @fgvieira