Skip to content

fix: correctly handle meta-wrapper tag replacement depending on the used snakemake-wrapper release#3826

Merged
johanneskoester merged 5 commits intomainfrom
fix/wrapper-tag
Nov 4, 2025
Merged

fix: correctly handle meta-wrapper tag replacement depending on the used snakemake-wrapper release#3826
johanneskoester merged 5 commits intomainfrom
fix/wrapper-tag

Conversation

@johanneskoester
Copy link
Copy Markdown
Contributor

@johanneskoester johanneskoester commented Nov 4, 2025

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

    • Stop erroring when a wrapper reference is a URL; such references are now ignored instead of causing failures.
  • Refactor

    • Improved wrapper-tag handling: newer-style wrappers (version 8.0.0+) and certain tag formats are now treated as no-op for tag replacement, improving compatibility with recent wrapper versions.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Nov 4, 2025

📝 Walkthrough

Walkthrough

Modified wrapper tag handling in modules.py to return None for URL-based meta_wrapper and for non-URL meta_wrapper tags >= 8.0.0 instead of raising an error. Adjusted wrapper.py regex to use a named ver capturing group for version matching.

Changes

Cohort / File(s) Summary
Wrapper tag handling
src/snakemake/modules.py
Added local import of Version from packaging.version. get_wrapper_tag() now returns None for URL meta_wrapper values and for non-URL meta_wrapper first path segments that parse as versions >= 8.0.0, replacing prior error-raising behavior.
Version regex named group
src/snakemake/wrapper.py
Replaced ver_regex from r"v?[0-9]+\.[0-9]+\.[0-9]+" to r"v?(?P<ver>[0-9]+\.[0-9]+\.[0-9]+)", introducing a named ver capture while preserving existing match usage.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant Caller
  participant modules_py as modules.get_wrapper_tag
  Note over modules_py: input meta_wrapper (string)

  Caller->>modules_py: call get_wrapper_tag(meta_wrapper)
  alt meta_wrapper is a URL
    modules_py-->>Caller: return None
  else meta_wrapper is not a URL
    modules_py->>modules_py: extract first path segment as tag
    alt tag parses as version >= 8.0.0
      modules_py-->>Caller: return None
    else
      modules_py-->>Caller: proceed with existing tag parsing/return
    end
  end
Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

  • Verify Version import and comparisons behave correctly with edge cases (pre-release/build metadata).
  • Confirm URL detection logic reliably identifies URLs across inputs.
  • Ensure the new named regex group doesn't create expectations elsewhere (no code currently depends on ver group).

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Description check ⚠️ Warning The PR description is largely incomplete with only QC checkboxes marked but no substantive description content provided where the placeholder comment indicates it should be. Add a detailed description of the changes, problem being solved, and implementation approach in the Description section to complete the template.
✅ Passed checks (1 passed)
Check name Status Explanation
Title check ✅ Passed The title correctly describes the main change: fixing meta-wrapper tag replacement handling based on snakemake-wrapper release versions, which is the core objective of the code modifications in modules.py and wrapper.py.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix/wrapper-tag

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 7e95b3c and 480e7e9.

📒 Files selected for processing (1)
  • src/snakemake/modules.py (1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py

⚙️ CodeRabbit configuration file

**/*.py: Do not try to improve formatting.
Do not suggest type annotations for functions that are defined inside of functions or methods.
Do not suggest type annotation of the self argument of methods.
Do not suggest type annotation of the cls argument of classmethods.
Do not suggest return type annotation if a function or method does not contain a return statement.

Files:

  • src/snakemake/modules.py
🧠 Learnings (4)
📓 Common learnings
Learnt from: johanneskoester
Repo: snakemake/snakemake PR: 3117
File: tests/test_wrapper/Snakefile:11-11
Timestamp: 2024-10-06T14:09:54.370Z
Learning: Changes made within test cases, such as in `tests/test_wrapper/Snakefile`, are for testing purposes and do not require updates to the project documentation.
📚 Learning: 2024-10-06T14:09:54.370Z
Learnt from: johanneskoester
Repo: snakemake/snakemake PR: 3117
File: tests/test_wrapper/Snakefile:11-11
Timestamp: 2024-10-06T14:09:54.370Z
Learning: Changes made within test cases, such as in `tests/test_wrapper/Snakefile`, are for testing purposes and do not require updates to the project documentation.

Applied to files:

  • src/snakemake/modules.py
📚 Learning: 2024-10-06T14:09:26.494Z
Learnt from: johanneskoester
Repo: snakemake/snakemake PR: 3117
File: snakemake/deployment/conda.py:0-0
Timestamp: 2024-10-06T14:09:26.494Z
Learning: In `snakemake/deployment/conda.py`, the exception handling in the `_get_version` method is intentional. The method raises a `WorkflowError` when the version cannot be determined, and this behavior is desired. Do not suggest modifying this exception handling.

Applied to files:

  • src/snakemake/modules.py
📚 Learning: 2024-11-12T12:08:20.342Z
Learnt from: johanneskoester
Repo: snakemake/snakemake PR: 3148
File: snakemake/dag.py:1332-1336
Timestamp: 2024-11-12T12:08:20.342Z
Learning: In `snakemake/dag.py`, when code is outdated and will disappear upon resolving merge conflicts, avoid making code review suggestions on that code.

Applied to files:

  • src/snakemake/modules.py
🧬 Code graph analysis (1)
src/snakemake/modules.py (1)
src/snakemake/wrapper.py (1)
  • is_url (50-55)
⏰ 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). (44)
  • GitHub Check: tests (7, ubuntu-latest, py311)
  • GitHub Check: tests (10, ubuntu-latest, py313)
  • GitHub Check: tests (10, windows-2022, py313)
  • GitHub Check: tests (10, ubuntu-latest, py312)
  • GitHub Check: tests (9, ubuntu-latest, py313)
  • GitHub Check: tests (8, windows-2022, py313)
  • GitHub Check: tests (10, ubuntu-latest, py311)
  • GitHub Check: tests (9, ubuntu-latest, py311)
  • GitHub Check: tests (9, ubuntu-latest, py312)
  • GitHub Check: tests (7, macos-latest, py313)
  • GitHub Check: tests (8, ubuntu-latest, py312)
  • GitHub Check: tests (9, windows-2022, py313)
  • GitHub Check: tests (7, windows-2022, py313)
  • GitHub Check: tests (4, ubuntu-latest, py313)
  • GitHub Check: tests (6, ubuntu-latest, py312)
  • GitHub Check: tests (8, ubuntu-latest, py313)
  • GitHub Check: tests (8, ubuntu-latest, py311)
  • GitHub Check: tests (6, windows-2022, py313)
  • GitHub Check: tests (7, ubuntu-latest, py313)
  • GitHub Check: tests (6, ubuntu-latest, py313)
  • GitHub Check: tests (7, ubuntu-latest, py312)
  • GitHub Check: tests (5, ubuntu-latest, py312)
  • GitHub Check: tests (5, ubuntu-latest, py311)
  • GitHub Check: tests (6, ubuntu-latest, py311)
  • GitHub Check: tests (4, windows-2022, py313)
  • GitHub Check: tests (4, ubuntu-latest, py312)
  • GitHub Check: tests (5, windows-2022, py313)
  • GitHub Check: tests (4, ubuntu-latest, py311)
  • GitHub Check: tests (5, ubuntu-latest, py313)
  • GitHub Check: tests (3, ubuntu-latest, py313)
  • GitHub Check: tests (1, windows-2022, py313)
  • GitHub Check: tests (3, windows-2022, py313)
  • GitHub Check: tests (2, macos-latest, py313)
  • GitHub Check: tests (2, ubuntu-latest, py313)
  • GitHub Check: tests (2, windows-2022, py313)
  • GitHub Check: tests (3, ubuntu-latest, py312)
  • GitHub Check: tests (2, ubuntu-latest, py311)
  • GitHub Check: tests (3, ubuntu-latest, py311)
  • GitHub Check: tests (2, ubuntu-latest, py312)
  • GitHub Check: tests (1, ubuntu-latest, py311)
  • GitHub Check: tests (1, macos-latest, py313)
  • GitHub Check: tests (1, ubuntu-latest, py312)
  • GitHub Check: tests (1, ubuntu-latest, py313)
  • GitHub Check: apidocs
🔇 Additional comments (2)
src/snakemake/modules.py (2)

130-131: LGTM: Local import is appropriate.

The local import of Version is well-placed and avoids adding a module-level dependency for a method-specific need.


132-143: Logic fix correctly implemented.

The else block at lines 141-142 properly addresses the concern from the previous review. The logic now correctly handles wrapper tag replacement based on version:

  • URL-based meta-wrappers: No tag replacement (line 135)
  • New-style wrappers (≥ 8.0.0): No tag replacement needed, as they contain concrete versions (line 140)
  • Old-style wrappers (< 8.0.0): Returns tag for URI replacement (line 142)
  • Non-version tags: Falls through to else block and returns tag as a safe default

The short-circuit evaluation at line 138 correctly handles cases where the tag doesn't match the version regex pattern.


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.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 a217e50 and 7e95b3c.

📒 Files selected for processing (2)
  • src/snakemake/modules.py (1 hunks)
  • src/snakemake/wrapper.py (1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py

⚙️ CodeRabbit configuration file

**/*.py: Do not try to improve formatting.
Do not suggest type annotations for functions that are defined inside of functions or methods.
Do not suggest type annotation of the self argument of methods.
Do not suggest type annotation of the cls argument of classmethods.
Do not suggest return type annotation if a function or method does not contain a return statement.

Files:

  • src/snakemake/modules.py
  • src/snakemake/wrapper.py
**/wrapper.py

⚙️ CodeRabbit configuration file

Do not complain about use of undefined variable called snakemake.

Files:

  • src/snakemake/wrapper.py
🧠 Learnings (2)
📚 Learning: 2024-10-06T14:09:54.370Z
Learnt from: johanneskoester
Repo: snakemake/snakemake PR: 3117
File: tests/test_wrapper/Snakefile:11-11
Timestamp: 2024-10-06T14:09:54.370Z
Learning: Changes made within test cases, such as in `tests/test_wrapper/Snakefile`, are for testing purposes and do not require updates to the project documentation.

Applied to files:

  • src/snakemake/modules.py
  • src/snakemake/wrapper.py
📚 Learning: 2024-10-06T14:09:26.494Z
Learnt from: johanneskoester
Repo: snakemake/snakemake PR: 3117
File: snakemake/deployment/conda.py:0-0
Timestamp: 2024-10-06T14:09:26.494Z
Learning: In `snakemake/deployment/conda.py`, the exception handling in the `_get_version` method is intentional. The method raises a `WorkflowError` when the version cannot be determined, and this behavior is desired. Do not suggest modifying this exception handling.

Applied to files:

  • src/snakemake/modules.py
🧬 Code graph analysis (1)
src/snakemake/modules.py (1)
src/snakemake/wrapper.py (1)
  • is_url (50-55)
⏰ 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). (44)
  • GitHub Check: tests (10, ubuntu-latest, py311)
  • GitHub Check: tests (10, ubuntu-latest, py312)
  • GitHub Check: tests (8, macos-latest, py313)
  • GitHub Check: tests (6, ubuntu-latest, py313)
  • GitHub Check: tests (10, ubuntu-latest, py313)
  • GitHub Check: tests (10, windows-2022, py313)
  • GitHub Check: tests (9, windows-2022, py313)
  • GitHub Check: tests (9, ubuntu-latest, py313)
  • GitHub Check: tests (9, ubuntu-latest, py312)
  • GitHub Check: tests (8, ubuntu-latest, py312)
  • GitHub Check: tests (8, ubuntu-latest, py313)
  • GitHub Check: tests (8, windows-2022, py313)
  • GitHub Check: tests (9, ubuntu-latest, py311)
  • GitHub Check: tests (7, ubuntu-latest, py313)
  • GitHub Check: tests (7, ubuntu-latest, py311)
  • GitHub Check: tests (5, ubuntu-latest, py313)
  • GitHub Check: tests (7, windows-2022, py313)
  • GitHub Check: tests (7, ubuntu-latest, py312)
  • GitHub Check: tests (6, windows-2022, py313)
  • GitHub Check: tests (6, ubuntu-latest, py311)
  • GitHub Check: tests (8, ubuntu-latest, py311)
  • GitHub Check: tests (6, ubuntu-latest, py312)
  • GitHub Check: tests (5, windows-2022, py313)
  • GitHub Check: tests (2, ubuntu-latest, py313)
  • GitHub Check: tests (5, ubuntu-latest, py311)
  • GitHub Check: tests (4, ubuntu-latest, py313)
  • GitHub Check: tests (5, ubuntu-latest, py312)
  • GitHub Check: tests (4, windows-2022, py313)
  • GitHub Check: tests (4, macos-latest, py313)
  • GitHub Check: tests (4, ubuntu-latest, py312)
  • GitHub Check: tests (3, ubuntu-latest, py312)
  • GitHub Check: tests (3, windows-2022, py313)
  • GitHub Check: tests (3, ubuntu-latest, py313)
  • GitHub Check: tests (4, ubuntu-latest, py311)
  • GitHub Check: tests (1, ubuntu-latest, py311)
  • GitHub Check: tests (3, ubuntu-latest, py311)
  • GitHub Check: tests (1, ubuntu-latest, py312)
  • GitHub Check: tests (1, macos-latest, py313)
  • GitHub Check: tests (2, windows-2022, py313)
  • GitHub Check: tests (1, ubuntu-latest, py313)
  • GitHub Check: tests (1, windows-2022, py313)
  • GitHub Check: tests (2, ubuntu-latest, py312)
  • GitHub Check: tests (2, ubuntu-latest, py311)
  • GitHub Check: apidocs
🔇 Additional comments (1)
src/snakemake/wrapper.py (1)

17-17: LGTM! Named group enables version extraction in modules.py.

The addition of the named capturing group (?P<ver>...) maintains backward compatibility while enabling modules.py to extract and compare version numbers. The pattern correctly excludes the optional "v" prefix from the captured group, which is appropriate for parsing with packaging.version.Version.

Comment thread src/snakemake/modules.py
@johanneskoester johanneskoester merged commit 8d46a4a into main Nov 4, 2025
60 checks passed
@johanneskoester johanneskoester deleted the fix/wrapper-tag branch November 4, 2025 14:39
johanneskoester pushed a commit that referenced this pull request Nov 4, 2025
🤖 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).
kjohnsen pushed a commit to kjohnsen/snakemake that referenced this pull request Dec 15, 2025
…sed snakemake-wrapper release (snakemake#3826)

### Description

<!--Add a description of your PR here-->

### QC
<!-- Make sure that you can tick the boxes below. -->

* [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).


<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit

* **Bug Fixes**
* Stop erroring when a wrapper reference is a URL; such references are
now ignored instead of causing failures.

* **Refactor**
* Improved wrapper-tag handling: newer-style wrappers (version 8.0.0+)
and certain tag formats are now treated as no-op for tag replacement,
improving compatibility with recent wrapper versions.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
kjohnsen pushed a commit to kjohnsen/snakemake that referenced this pull request Dec 15, 2025
🤖 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).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant