fix: ensure that flags are properly considered for input files before applying path modifiers (i.e. default storage providers) by johanneskoester · Pull Request #3813 · snakemake/snakemake

johanneskoester · 2025-10-28T16:31:03Z

Description

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 ordering so input path modifiers are applied at the correct stage, ensuring modifiers interact correctly with file flags.
- Prevents misinterpretation of modified inputs during rule execution, improving reliability of input handling.

… applying path modifiers (i.e. default storage providers)

coderabbitai · 2025-10-28T16:31:31Z

📝 Walkthrough

Walkthrough

The input path modifier application for values produced by callables was moved: the code now constructs an IOFile and inherits flags first, then applies the input path modifier to the IOFile once expansion is complete, instead of modifying the raw value before IOFile creation.

Changes

Cohort / File(s)	Summary
Path modifier for callable inputs `src/snakemake/rules.py`	Reordered application of the input path modifier for values returned by callables: construct `IOFile` and inherit flags first, then apply the path modifier to the `IOFile` when expansion is complete; previously the modifier was applied to the raw value before `IOFile` creation.
Test: callable input path `tests/test_pathvars/Snakefile`	Added top-level function `infunc(wildcards)` returning a path and included it in `rule b` inputs to exercise callable-based input handling.

Sequence Diagram(s)

sequenceDiagram
    participant Rule as Rule evaluation
    participant Callable as input callable
    participant Raw as Raw value
    participant IOFile as IOFile ctor
    participant Modifier as Path modifier

    Note over Rule,Callable: Old flow
    Rule->>Callable: invoke
    Callable-->>Raw: returns raw path (possibly AnnotatedString)
    Raw->>Modifier: apply modifier
    Modifier-->>IOFile: pass modified value
    IOFile-->>Rule: created IOFile with modifier applied

    rect rgb(223,240,216)
      Note right of Rule: New flow
      Rule->>Callable: invoke
      Callable-->>Raw: returns raw path
      Raw->>IOFile: construct IOFile (inherit flags)
      IOFile->>Modifier: apply modifier (if expansion complete)
      Modifier-->>Rule: IOFile with modifier applied
    end

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Areas to double-check:
- Conditions gating modifier application (expansion-complete detection).
- That flag inheritance on IOFile occurs correctly before modifier runs.
- Behavior when callables return annotated or already-flagged values; tests cover a callable input but verify edge cases.

Possibly related PRs

fix: inherit flags of input functions #3483: Modifies src/snakemake/rules.py handling of callable inputs and path modifier/flag propagation — strongly related.
fix: fix --consider-ancient for input functions (was not applied in those cases) #3788: Changes callable-input handling and modifier/flag timing in the same area of src/snakemake/rules.py.
fix: skip storage object when cloning flags for shadowed IO #3174: Adjusts IOFile flag cloning/application logic that interacts with modifier/flag ordering.

Pre-merge checks and finishing touches

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Description Check	⚠️ Warning	The PR description is largely incomplete. While the QC checklist is properly filled out with both items checked, indicating that the author confirms test coverage and documentation status, the main "Description" section is empty—it only contains the template placeholder comment without any actual content explaining the changes, their rationale, or implementation details. According to the template requirements, this section should contain a substantive description of the pull request.	The author should add a detailed description explaining the specific changes made, why they were necessary, and what problem they solve. At minimum, they should describe how the fix ensures flags are properly considered before applying path modifiers, what the behavioral change is, and any relevant context around the use of path modifiers with callable inputs. This will help reviewers understand the intent and impact of the changes.
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	You can run `@coderabbitai generate docstrings` to improve docstring coverage.

✅ Passed checks (1 passed)

Check name	Status	Explanation
Title Check	✅ Passed	The PR title clearly and accurately summarizes the main change in the changeset. The title directly describes the fix implemented in `src/snakemake/rules.py`, where the timing of applying input path modifiers has been adjusted to occur after IOFile creation and flag inheritance. This matches the raw summary that shows the modifier is now applied to the IOFile object after flag inheritance. The title is concise, specific, and avoids vague language or noise, making it clear to someone reviewing the repository history what the primary change addresses.

✨ Finishing touches

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment
Commit unit tests in branch fix/path-modifier-func-handling

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

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 56bbe16 and 19d8e60.

📒 Files selected for processing (1)

tests/test_pathvars/Snakefile (1 hunks)

⏰ 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). (47)

GitHub Check: tests (10, windows-2022, py313)
GitHub Check: tests (10, ubuntu-latest, py313)
GitHub Check: tests (10, ubuntu-latest, py312)
GitHub Check: tests (9, ubuntu-latest, py312)
GitHub Check: tests (10, ubuntu-latest, py311)
GitHub Check: tests (9, ubuntu-latest, py313)
GitHub Check: tests (9, windows-2022, py313)
GitHub Check: tests (7, windows-2022, py313)
GitHub Check: tests (8, ubuntu-latest, py313)
GitHub Check: tests (8, ubuntu-latest, py311)
GitHub Check: tests (8, windows-2022, py313)
GitHub Check: tests (9, ubuntu-latest, py311)
GitHub Check: tests (5, windows-2022, py313)
GitHub Check: tests (8, ubuntu-latest, py312)
GitHub Check: tests (7, ubuntu-latest, py312)
GitHub Check: tests (7, ubuntu-latest, py311)
GitHub Check: tests (7, ubuntu-latest, py313)
GitHub Check: tests (6, windows-2022, py313)
GitHub Check: tests (5, ubuntu-latest, py313)
GitHub Check: tests (5, macos-latest, py313)
GitHub Check: tests (6, ubuntu-latest, py312)
GitHub Check: tests (6, ubuntu-latest, py313)
GitHub Check: tests (6, ubuntu-latest, py311)
GitHub Check: tests (5, ubuntu-latest, py311)
GitHub Check: tests (2, macos-latest, py313)
GitHub Check: tests (5, ubuntu-latest, py312)
GitHub Check: tests (4, windows-2022, py313)
GitHub Check: tests (3, ubuntu-latest, py313)
GitHub Check: tests (4, ubuntu-latest, py313)
GitHub Check: tests (4, ubuntu-latest, py311)
GitHub Check: tests (4, macos-latest, py313)
GitHub Check: tests (3, macos-latest, py313)
GitHub Check: tests (3, ubuntu-latest, py311)
GitHub Check: tests (3, windows-2022, py313)
GitHub Check: tests (2, ubuntu-latest, py311)
GitHub Check: tests (2, ubuntu-latest, py312)
GitHub Check: tests (4, ubuntu-latest, py312)
GitHub Check: tests (2, windows-2022, py313)
GitHub Check: tests (2, ubuntu-latest, py313)
GitHub Check: tests (1, ubuntu-latest, py313)
GitHub Check: tests (3, ubuntu-latest, py312)
GitHub Check: tests (1, macos-latest, py313)
GitHub Check: tests (1, ubuntu-latest, py311)
GitHub Check: tests (1, ubuntu-latest, py312)
GitHub Check: tests (1, windows-2022, py313)
GitHub Check: formatting
GitHub Check: apidocs

🔇 Additional comments (1)

tests/test_pathvars/Snakefile (1)
30-30: Consider adding explicit flag testing to verify the fix.

The function-based input addition tests path variable expansion with callables, which aligns with the fix. However, the PR title emphasizes that "flags are properly considered for input files before applying path modifiers," and the AI summary mentions "flag inheritance."

This test doesn't explicitly use IOFile flags (e.g., temp(), ancient(), pipe(), etc.) with the function-based input. Consider whether additional test cases are needed to explicitly verify that flags work correctly with function-based inputs and path modifiers.

For example, does the fix handle scenarios like this correctly?
def infunc_with_flag(wildcards):
    return ancient("<resources>/<resname>.txt")
If such scenarios are part of the fix, ensure they're covered by tests.

🤖 I have created a release *beep* *boop* --- ## [9.13.5](v9.13.4...v9.13.5) (2025-11-04) ### Bug Fixes * cache wrapper files and wait for them in case of shared filesystem for sources ([#3809](#3809)) ([498fff7](498fff7)) * correctly handle meta-wrapper tag replacement depending on the used snakemake-wrapper release ([#3826](#3826)) ([8d46a4a](8d46a4a)) * ensure that flags are properly considered for input files before applying path modifiers (i.e. default storage providers) ([#3813](#3813)) ([d3bfe32](d3bfe32)) * ensure that tokens are not leaked when paths or uris of source files are logged ([#3821](#3821)) ([a217e50](a217e50)) * print secs as numeric in jsonl benchmark format ([#3814](#3814)) ([395a5e6](395a5e6)) * revert breaking change in 9.11.9 disallowing empty input files even when unused ([#3810](#3810)) ([83c05cc](83c05cc)) * shorten report ids (thus dir names) in order to avoid issues with path length on windows ([#3822](#3822)) ([b24d971](b24d971)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).

… applying path modifiers (i.e. default storage providers) (snakemake#3813) ### Description  ### QC  * [x] The PR contains a test case for the changes or the changes are already covered by an existing test case. * [x] 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 ordering so input path modifiers are applied at the correct stage, ensuring modifiers interact correctly with file flags. * Prevents misinterpretation of modified inputs during rule execution, improving reliability of input handling.

🤖 I have created a release *beep* *boop* --- ## [9.13.5](snakemake/snakemake@v9.13.4...v9.13.5) (2025-11-04) ### Bug Fixes * cache wrapper files and wait for them in case of shared filesystem for sources ([snakemake#3809](snakemake#3809)) ([498fff7](snakemake@498fff7)) * correctly handle meta-wrapper tag replacement depending on the used snakemake-wrapper release ([snakemake#3826](snakemake#3826)) ([8d46a4a](snakemake@8d46a4a)) * ensure that flags are properly considered for input files before applying path modifiers (i.e. default storage providers) ([snakemake#3813](snakemake#3813)) ([d3bfe32](snakemake@d3bfe32)) * ensure that tokens are not leaked when paths or uris of source files are logged ([snakemake#3821](snakemake#3821)) ([a217e50](snakemake@a217e50)) * print secs as numeric in jsonl benchmark format ([snakemake#3814](snakemake#3814)) ([395a5e6](snakemake@395a5e6)) * revert breaking change in 9.11.9 disallowing empty input files even when unused ([snakemake#3810](snakemake#3810)) ([83c05cc](snakemake@83c05cc)) * shorten report ids (thus dir names) in order to avoid issues with path length on windows ([snakemake#3822](snakemake#3822)) ([b24d971](snakemake@b24d971)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).

fix: ensure that flags are properly considered for input files before…

221cfa2

… applying path modifiers (i.e. default storage providers)

johanneskoester changed the title ~~Fix/path-modifier-func-handling~~ fix: ensure that flags are properly considered for input files before applying path modifiers (i.e. default storage providers) Oct 28, 2025

johanneskoester added 2 commits October 29, 2025 13:30

fix input file type

56bbe16

update testcase

19d8e60

coderabbitai Bot reviewed Oct 29, 2025

View reviewed changes

Comment thread tests/test_pathvars/Snakefile

johanneskoester merged commit d3bfe32 into main Oct 30, 2025
60 checks passed

johanneskoester deleted the fix/path-modifier-func-handling branch October 30, 2025 15:39

snakemake-bot mentioned this pull request Oct 30, 2025

chore(main): release 9.13.5 #3818

Merged

coderabbitai Bot mentioned this pull request Dec 13, 2025

test: pathvars in modules using input fkt #3877 #3878

Open

2 tasks

coderabbitai Bot mentioned this pull request Feb 12, 2026

fix: resolve pathvars before constructing storage queries #3969

Open

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: ensure that flags are properly considered for input files before applying path modifiers (i.e. default storage providers)#3813

fix: ensure that flags are properly considered for input files before applying path modifiers (i.e. default storage providers)#3813
johanneskoester merged 3 commits intomainfrom
fix/path-modifier-func-handling

johanneskoester commented Oct 28, 2025 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented Oct 28, 2025 •

edited

Loading

Walkthrough

Changes

Sequence Diagram(s)

Estimated code review effort

Possibly related PRs

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

johanneskoester commented Oct 28, 2025 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

QC

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Oct 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Sequence Diagram(s)

Estimated code review effort

Possibly related PRs

Pre-merge checks and finishing touches

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

johanneskoester commented Oct 28, 2025 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Oct 28, 2025 •

edited

Loading