feat: migrate star-arriba meta-wrapper to the new pathvars syntax #4622
Conversation
📝 WalkthroughWalkthroughReplace many in-file Snakemake rule implementations with module-driven Snakefiles (min_version + configfile + module declarations), add pathvars and per-test config.yaml files, introduce multiple meta_wrapper.smk rule sets, update docs rendering and templates, tweak CI linting, and adjust tests and small test-data edits. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor TestSnakefile as "test/Snakefile (top-level)"
participant SM as "Snakemake"
participant Module as "module: ../meta_wrapper.smk"
participant Rule as "pipeline rules (index/align/...)"
TestSnakefile->>SM: invoke (min_version, configfile, module declaration)
SM->>Module: load ../meta_wrapper.smk with config
Module->>Rule: expose rules via "use rule *"
SM->>Rule: schedule/write targets (index -> align -> postproc)
Note right of Rule #D6EAF8: rules implement inputs/outputs, wrappers, logs
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
meta/bio/star_arriba/test/Snakefile (2)
34-37: Fix params.extra: multiline strings are not concatenatedThe bare string lines become separate positional params;
extraonly contains the first f-string, dropping required chimera flags. Wrap in parentheses so literals concatenate.- extra=lambda wc, input: f"--quantMode GeneCounts --sjdbGTFfile {input.annotation}" - " --outSAMtype BAM Unsorted --chimSegmentMin 10 --chimOutType WithinBAM SoftClip" - " --chimJunctionOverhangMin 10 --chimScoreMin 1 --chimScoreDropMax 30 --chimScoreJunctionNonGTAG 0" - " --chimScoreSeparation 1 --alignSJstitchMismatchNmax 5 -1 5 5 --chimSegmentReadGapMax 3", + extra=lambda wc, input: ( + f"--quantMode GeneCounts --sjdbGTFfile {input.annotation}" + " --outSAMtype BAM Unsorted --chimSegmentMin 10 --chimOutType WithinBAM SoftClip" + " --chimJunctionOverhangMin 10 --chimScoreMin 1 --chimScoreDropMax 30 --chimScoreJunctionNonGTAG 0" + " --chimScoreSeparation 1 --alignSJstitchMismatchNmax 5 -1 5 5 --chimSegmentReadGapMax 3" + ),
1-66: Add missingabout.homeandrequirementsto meta.yaml
The filemeta/bio/star_arriba/meta.yamlcurrently only definesname,description, andauthors. Per our wrapper-metadata guidelines, it must also include:
- an
about.homeURL pointing at the official docs (e.g.https://arriba.readthedocs.io/en/latest/workflow/)- a minimal
requirements:block with host/run dependencies- a top-level
channels:list with only the necessary channels (e.g.conda-forge)Tests in
test_wrappers.pyalready importmeta/bio/star_arriba, so CI will exercise this example once the metadata is updated.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
meta/bio/star_arriba/test/Snakefile(3 hunks)
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: tdayris
PR: snakemake/snakemake-wrappers#3496
File: bio/mtnucratio/test/Snakefile:2-6
Timestamp: 2024-11-26T08:31:00.099Z
Learning: In test files for Snakemake wrappers, such as `bio/mtnucratio/test/Snakefile`, hard-coded input and output paths are acceptable as examples and do not need to use wildcards to make paths flexible.
Learnt from: tdayris
PR: snakemake/snakemake-wrappers#3499
File: bio/ngscheckmate/makesnvpattern/test/Snakefile:14-22
Timestamp: 2024-11-26T09:16:39.570Z
Learning: In the `snakemake-wrappers` repository, when writing test `Snakefile`s (e.g., `bio/ngscheckmate/makesnvpattern/test/Snakefile`), hardcoded input/output paths are acceptable because these are examples and the IO can be chosen freely.
Learnt from: tdayris
PR: snakemake/snakemake-wrappers#3501
File: meta/bio/varscan2_snpeff/test/Snakefile:58-71
Timestamp: 2024-11-26T10:49:54.765Z
Learning: In test Snakefiles within the snakemake-wrappers repository, it is acceptable to use simplified paths and logging configurations that may differ from real-life pipelines.
Learnt from: tdayris
PR: snakemake/snakemake-wrappers#3499
File: bio/ngscheckmate/makesnvpattern/test/Snakefile:1-13
Timestamp: 2024-11-26T09:16:24.981Z
Learning: In test `Snakefile`s (e.g., `test/Snakefile`), it's acceptable to use fixed input and output file names instead of wildcards.
📚 Learning: 2024-11-26T10:49:54.765Z
Learnt from: tdayris
PR: snakemake/snakemake-wrappers#3501
File: meta/bio/varscan2_snpeff/test/Snakefile:58-71
Timestamp: 2024-11-26T10:49:54.765Z
Learning: In test Snakefiles within the snakemake-wrappers repository, it is acceptable to use simplified paths and logging configurations that may differ from real-life pipelines.
Applied to files:
meta/bio/star_arriba/test/Snakefile
📚 Learning: 2024-11-26T09:16:24.981Z
Learnt from: tdayris
PR: snakemake/snakemake-wrappers#3499
File: bio/ngscheckmate/makesnvpattern/test/Snakefile:1-13
Timestamp: 2024-11-26T09:16:24.981Z
Learning: In test `Snakefile`s (e.g., `test/Snakefile`), it's acceptable to use fixed input and output file names instead of wildcards.
Applied to files:
meta/bio/star_arriba/test/Snakefile
📚 Learning: 2024-11-26T09:16:39.570Z
Learnt from: tdayris
PR: snakemake/snakemake-wrappers#3499
File: bio/ngscheckmate/makesnvpattern/test/Snakefile:14-22
Timestamp: 2024-11-26T09:16:39.570Z
Learning: In the `snakemake-wrappers` repository, when writing test `Snakefile`s (e.g., `bio/ngscheckmate/makesnvpattern/test/Snakefile`), hardcoded input/output paths are acceptable because these are examples and the IO can be chosen freely.
Applied to files:
meta/bio/star_arriba/test/Snakefile
📚 Learning: 2024-11-26T08:31:00.099Z
Learnt from: tdayris
PR: snakemake/snakemake-wrappers#3496
File: bio/mtnucratio/test/Snakefile:2-6
Timestamp: 2024-11-26T08:31:00.099Z
Learning: In test files for Snakemake wrappers, such as `bio/mtnucratio/test/Snakefile`, hard-coded input and output paths are acceptable as examples and do not need to use wildcards to make paths flexible.
Applied to files:
meta/bio/star_arriba/test/Snakefile
📚 Learning: 2024-11-21T10:50:09.006Z
Learnt from: johanneskoester
PR: snakemake/snakemake-wrappers#3478
File: bio/varlociraptor/call-variants/test/Snakefile:29-47
Timestamp: 2024-11-21T10:50:09.006Z
Learning: In example Snakefiles, using hardcoded sample names is acceptable.
Applied to files:
meta/bio/star_arriba/test/Snakefile
📚 Learning: 2024-11-26T15:01:13.202Z
Learnt from: tdayris
PR: snakemake/snakemake-wrappers#3502
File: bio/ngsbits/sampleancestry/wrapper.py:1-23
Timestamp: 2024-11-26T15:01:13.202Z
Learning: The NGS-bits SampleAncestry wrapper in `bio/ngsbits/sampleancestry/` includes a test Snakefile, sample VCF files, and tests available in the `test/` directory.
Applied to files:
meta/bio/star_arriba/test/Snakefile
⏰ 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). (3)
- GitHub Check: docs
- GitHub Check: testing
- GitHub Check: Summary
🔇 Additional comments (7)
meta/bio/star_arriba/test/Snakefile (7)
3-4: LGTM: pathvars for genome inputs
fastaandgtfmigrated to<genome_sequence>/<genome_annotation>correctly.
6-6: LGTM: cacheable STAR index dir
directory("<resources>/star_genome")matches the index location used downstream.
12-12: LGTM: log path migratedLog now uses
<logs>pathvar.
22-26: LGTM: STAR align inputs on pathvarsReads, index dir, and annotation wired to pathvars consistently.
28-31: LGTM: outputs/logs per samplePer-sample
<per>usage in results and logs looks consistent.
53-54: LGTM: Arriba outputs on<results>with<per>Output paths look correct.
62-62: LGTM: Arriba log pathPer-sample log migrated to
<logs>.
…/log/benchmark paths for the entire workflow, per module, or per rule (#3760) See the changes in the docs to get a feeling for the utility of this: https://snakemake--3760.org.readthedocs.build/en/3760/snakefiles/rules.html#pathvar-definition Also, this will be a great feature for snakemake-wrappers. Especially meta-wrappers can be written in a generic way. For example, the star-arriba meta-wrapper can be used much easier: snakemake/snakemake-wrappers#4622 ### 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 * **New Features** * Introduced pathvars: define and substitute <name> tokens in file paths at global, workflow, module, and rule scopes; APIs to register, merge, and expose pathvars. * **Documentation** * Expanded guides and examples for pathvars, module usage, dynamic modules, and updated code-hosting / private repo sections. * **Tests** * Added tests covering pathvars resolution, module integration, and cycle detection. * **Improvements** * Clearer runtime errors for undefined/invalid path variables and improved precedence behavior. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
meta/bio/star_arriba/test/Snakefile (1)
33-37: Invalid params.extra expression (lambda + stray strings)The extra parameter mixes a lambda with bare string literals, which is invalid. Wrap the concatenated string inside the lambda.
- extra=lambda wc, input: f"--quantMode GeneCounts --sjdbGTFfile {input.annotation}" - " --outSAMtype BAM Unsorted --chimSegmentMin 10 --chimOutType WithinBAM SoftClip" - " --chimJunctionOverhangMin 10 --chimScoreMin 1 --chimScoreDropMax 30 --chimScoreJunctionNonGTAG 0" - " --chimScoreSeparation 1 --alignSJstitchMismatchNmax 5 -1 5 5 --chimSegmentReadGapMax 3", + extra=lambda wc, input: ( + f"--quantMode GeneCounts --sjdbGTFfile {input.annotation}" + " --outSAMtype BAM Unsorted --chimSegmentMin 10 --chimOutType WithinBAM SoftClip" + " --chimJunctionOverhangMin 10 --chimScoreMin 1 --chimScoreDropMax 30 --chimScoreJunctionNonGTAG 0" + " --chimScoreSeparation 1 --alignSJstitchMismatchNmax 5 -1 5 5 --chimSegmentReadGapMax 3" + ),
🧹 Nitpick comments (5)
docs/generate_docs.py (4)
170-170: Use relative path for URIAvoid absolute paths in docs. Make URI relative to WRAPPER_DIR.
- meta["uri"] = f"{tag or 'master'}/{path}" + meta["uri"] = f"{tag or 'master'}/{os.path.relpath(path, WRAPPER_DIR)}"
95-97: Resolve Ruff E741: ambiguous variable nameRename
ltolinein the join for clarity.- snakefile = textwrap.indent( - "\n".join(l.rstrip() for l in lines).strip(), " " - ) + snakefile = textwrap.indent( + "\n".join(line.rstrip() for line in lines).strip(), " " + )
98-100: Limit tag substitution to wrapper pathsAvoid accidental replacements. Scope replacement to wrapper directives.
- if tag is not None: - snakefile = snakefile.replace("master", tag) + if tag is not None: + # only replace wrapper path prefixes like: wrapper: "master/..." + snakefile = snakefile.replace('wrapper: "master/', f'wrapper: "{tag}/')Please verify no other “master” tokens in Snakefiles require substitution.
215-215: Remove unusedargsparameter
setup(*args)doesn’t useargs. Drop it.-def setup(*args): +def setup():meta/bio/bowtie2_sambamba/test/Snakefile (1)
15-15: Use<logs>pathvar for all logsTwo log paths are still literal. Migrate to
<logs>for consistency with pathvars.- "logs/bowtie2_build/build.log", + "<logs>/bowtie2_build/build.log",- "logs/sambamba-view/{sample}.log", + "<logs>/sambamba-view/{sample}.log",Also applies to: 74-74
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
.github/workflows/qc.yml(1 hunks)docs/_static/custom.css(1 hunks)docs/_templates/meta_wrapper.rst(2 hunks)docs/_templates/wrapper.rst(1 hunks)docs/generate_docs.py(9 hunks)meta/bio/bowtie2_sambamba/meta.yaml(1 hunks)meta/bio/bowtie2_sambamba/test/Snakefile(5 hunks)meta/bio/bowtie2_sambamba/test/config.yaml(1 hunks)meta/bio/bwa_mapping/test/Snakefile(1 hunks)meta/bio/bwa_mapping/test/config.yaml(1 hunks)meta/bio/bwa_mapping/test/genome.amb(0 hunks)meta/bio/bwa_mapping/test/genome.ann(0 hunks)meta/bio/calc_consensus_reads/test/Snakefile(4 hunks)meta/bio/calc_consensus_reads/test/config.yaml(1 hunks)meta/bio/calc_consensus_reads/test/resources/genome.fa.amb(0 hunks)meta/bio/calc_consensus_reads/test/resources/genome.fa.ann(0 hunks)meta/bio/star_arriba/meta.yaml(1 hunks)meta/bio/star_arriba/test/Snakefile(3 hunks)meta/bio/star_arriba/test/config.yaml(1 hunks)test_wrappers.py(4 hunks)
💤 Files with no reviewable changes (4)
- meta/bio/bwa_mapping/test/genome.ann
- meta/bio/calc_consensus_reads/test/resources/genome.fa.amb
- meta/bio/bwa_mapping/test/genome.amb
- meta/bio/calc_consensus_reads/test/resources/genome.fa.ann
✅ Files skipped from review due to trivial changes (4)
- meta/bio/star_arriba/test/config.yaml
- meta/bio/bwa_mapping/test/config.yaml
- meta/bio/calc_consensus_reads/test/config.yaml
- meta/bio/bowtie2_sambamba/test/config.yaml
🧰 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 theselfargument of methods.
Do not suggest type annotation of theclsargument of classmethods.
Do not suggest return type annotation if a function or method does not contain areturnstatement.
Files:
test_wrappers.pydocs/generate_docs.py
🧠 Learnings (4)
📚 Learning: 2024-11-26T12:03:25.328Z
Learnt from: tdayris
PR: snakemake/snakemake-wrappers#3501
File: meta/bio/varscan2_snpeff/meta.yaml:1-21
Timestamp: 2024-11-26T12:03:25.328Z
Learning: For meta-wrappers in snakemake-wrappers, the `meta.yaml` does not require keys for parameters (`params`), input, or output specifications, and the documentation requirements differ from regular wrappers.
Applied to files:
docs/_templates/meta_wrapper.rst
📚 Learning: 2024-11-26T10:49:54.765Z
Learnt from: tdayris
PR: snakemake/snakemake-wrappers#3501
File: meta/bio/varscan2_snpeff/test/Snakefile:58-71
Timestamp: 2024-11-26T10:49:54.765Z
Learning: In test Snakefiles within the snakemake-wrappers repository, it is acceptable to use simplified paths and logging configurations that may differ from real-life pipelines.
Applied to files:
meta/bio/star_arriba/test/Snakefile
📚 Learning: 2024-11-26T09:16:24.981Z
Learnt from: tdayris
PR: snakemake/snakemake-wrappers#3499
File: bio/ngscheckmate/makesnvpattern/test/Snakefile:1-13
Timestamp: 2024-11-26T09:16:24.981Z
Learning: In test `Snakefile`s (e.g., `test/Snakefile`), it's acceptable to use fixed input and output file names instead of wildcards.
Applied to files:
meta/bio/star_arriba/test/Snakefile
📚 Learning: 2024-11-26T09:16:39.570Z
Learnt from: tdayris
PR: snakemake/snakemake-wrappers#3499
File: bio/ngscheckmate/makesnvpattern/test/Snakefile:14-22
Timestamp: 2024-11-26T09:16:39.570Z
Learning: In the `snakemake-wrappers` repository, when writing test `Snakefile`s (e.g., `bio/ngscheckmate/makesnvpattern/test/Snakefile`), hardcoded input/output paths are acceptable because these are examples and the IO can be chosen freely.
Applied to files:
meta/bio/star_arriba/test/Snakefile
🧬 Code graph analysis (1)
docs/generate_docs.py (1)
test_wrappers.py (1)
run(41-134)
🪛 Ruff (0.13.3)
docs/generate_docs.py
96-96: Ambiguous variable name: l
(E741)
190-190: subprocess call: check for execution of untrusted input
(S603)
191-191: Starting a process with a partial executable path
(S607)
200-200: subprocess call: check for execution of untrusted input
(S603)
201-201: Starting a process with a partial executable path
(S607)
215-215: Unused function argument: args
(ARG001)
⏰ 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). (1)
- GitHub Check: Summary
🔇 Additional comments (11)
docs/_static/custom.css (1)
1-21: LGTM!The CSS styling changes look good—background fix for images, color variables for links/brand, overflow handling, and monospace font enforcement are all reasonable visual improvements.
.github/workflows/qc.yml (1)
67-76: LGTM!The conditional logic correctly provides
--configfile config.yamlformeta/*Snakefiles during linting, aligning with the broader pathvars migration. The use of$(dirname $f)ensures the config is loaded from the same directory as the Snakefile.meta/bio/bwa_mapping/test/Snakefile (1)
1-6: LGTM!The migration to placeholder-based paths (
<genome_sequence>,<reads_r1>,<reads_r2>,<results>, etc.) is implemented correctly throughout the test Snakefile, providing the flexibility intended by the pathvars feature.Also applies to: 8-10, 12-40
meta/bio/star_arriba/meta.yaml (1)
7-17: LGTM!The pathvars configuration is well-structured with clear descriptions for each custom path variable (genome_sequence, genome_annotation, reads_r1, reads_r2, per). This provides good documentation for users of the meta-wrapper.
docs/_templates/meta_wrapper.rst (1)
1-97: LGTM!The documentation template enhancements effectively communicate the new pathvars functionality. The addition of "Via module" and "Via copy-paste" sections with conditional pathvars guidance provides clear usage examples for users. The template logic correctly handles both pathvars-enabled and non-pathvars wrappers.
docs/_templates/wrapper.rst (1)
10-12: LGTM!The addition of the wrapper_version badge and the updated GitHub issues-pr badge enhance the documentation by displaying version information prominently.
meta/bio/bowtie2_sambamba/meta.yaml (1)
23-32: LGTM!The pathvars configuration mirrors the structure used in other meta-wrappers and provides appropriate path variables for the Bowtie2/Sambamba workflow.
test_wrappers.py (4)
1398-1399: LGTM!The test update correctly adds
--configfile config.yamland switches to--sdm conda(the newer syntax), enabling the pathvars-based configuration for the star_arriba meta-wrapper test.
1499-1502: LGTM!The test correctly updates the output path to include the
results/prefix and adds--configfile config.yaml, aligning with the pathvars migration.
4558-4571: LGTM!The test correctly adds
--configfile config.yamlat the beginning of the argument list, ensuring the pathvars configuration is loaded for the calc_consensus_reads meta-wrapper test.
4574-4587: LGTM!The test updates the output path to include the
results/prefix and adds--configfile config.yaml, properly supporting the pathvars-based configuration for the bowtie2_sambamba meta-wrapper test.
| used_default_pathvars = set(meta.get("pathvars", {}).get("custom", [])) | ||
| custom_pathvars = meta.get("pathvars", {}).get("custom", {}) | ||
| pathvars = { | ||
| var: desc | ||
| for var, desc in DEFAULT_PATHVARS.items() | ||
| if var in used_default_pathvars | ||
| } | ||
| pathvars.update(custom_pathvars) | ||
|
|
There was a problem hiding this comment.
Fix pathvars assembly: use default list, not custom
Currently selects defaults from custom, dropping results/resources/logs/benchmarks. Use default and fall back to all defaults if unset.
- used_default_pathvars = set(meta.get("pathvars", {}).get("custom", []))
- custom_pathvars = meta.get("pathvars", {}).get("custom", {})
+ used_default_pathvars = set(
+ meta.get("pathvars", {}).get("default", DEFAULT_PATHVARS.keys())
+ )
+ custom_pathvars = meta.get("pathvars", {}).get("custom", {})
pathvars = {
var: desc
for var, desc in DEFAULT_PATHVARS.items()
if var in used_default_pathvars
}
pathvars.update(custom_pathvars)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| used_default_pathvars = set(meta.get("pathvars", {}).get("custom", [])) | |
| custom_pathvars = meta.get("pathvars", {}).get("custom", {}) | |
| pathvars = { | |
| var: desc | |
| for var, desc in DEFAULT_PATHVARS.items() | |
| if var in used_default_pathvars | |
| } | |
| pathvars.update(custom_pathvars) | |
| used_default_pathvars = set( | |
| meta.get("pathvars", {}).get("default", DEFAULT_PATHVARS.keys()) | |
| ) | |
| custom_pathvars = meta.get("pathvars", {}).get("custom", {}) | |
| pathvars = { | |
| var: desc | |
| for var, desc in DEFAULT_PATHVARS.items() | |
| if var in used_default_pathvars | |
| } | |
| pathvars.update(custom_pathvars) |
🤖 Prompt for AI Agents
In docs/generate_docs.py around lines 159 to 167, the code is incorrectly
reading the "custom" list to determine which DEFAULT_PATHVARS to include,
causing default entries like results/resources/logs/benchmarks to be dropped;
change it to read the "default" list (meta.get("pathvars", {}).get("default",
[])) and convert that to a set for used_default_pathvars, and if that list is
empty/unspecified fall back to including all DEFAULT_PATHVARS keys (e.g.,
used_default_pathvars = set(meta.get("pathvars", {}).get("default", [])) or
set(DEFAULT_PATHVARS.keys())); keep custom_pathvars as meta.get("pathvars",
{}).get("custom", {}) and then build pathvars by selecting defaults present in
used_default_pathvars and updating with custom_pathvars.
There was a problem hiding this comment.
Actionable comments posted: 11
♻️ Duplicate comments (2)
docs/generate_docs.py (2)
159-166: Restore default pathvars selectionWe’re still reading
customto decide which defaults to include, so the standard entries (results,resources,logs,benchmarks) disappear from docs as soon aspathvarsis present. Use thedefaultlist and fall back to all defaults when it’s missing.- used_default_pathvars = set(meta.get("pathvars", {}).get("custom", [])) - custom_pathvars = meta.get("pathvars", {}).get("custom", {}) + pathvars_cfg = meta.get("pathvars", {}) + defaults = pathvars_cfg.get("default") + if defaults is None: + used_default_pathvars = set(DEFAULT_PATHVARS.keys()) + else: + used_default_pathvars = set(defaults) + custom_pathvars = pathvars_cfg.get("custom", {})
209-212: Return the newest tag
tags[0]is the oldest bycreatordate; the helper should hand back the most recent tag.- else: - return tags[0] + else: + return tags[-1]
🧹 Nitpick comments (3)
meta/bio/gatk_mutect2_calling/meta_wrapper.smk (1)
14-158: Consider pinning wrapper versions for reproducibility.All wrapper references use
"master/..."which points to the latest code on the master branch. For better reproducibility and stability, consider pinning to specific wrapper versions (e.g.,"v1.2.3/bio/picard/createsequencedictionary").This is a common best practice in workflow management to ensure consistent behavior across runs and environments.
meta/bio/dada2_se/test/Snakefile (1)
1-1: Unnecessary import of min_versionYou can call min_version without importing from snakemake.utils; drop the import for brevity.
-from snakemake.utils import min_versionmeta/bio/dada2_se/meta_wrapper.smk (1)
21-24: Adopt pathvars for outputs/logs/resourcesHardcoded "results/" and "logs/" reduce flexibility. Switch to "", "", and "" to align with pathvars.
Example changes:
- "reports/dada2/quality-profile/{sample}.{orientation}-quality-profile.png" + "<results>/reports/dada2/quality-profile/{sample}.{orientation}-quality-profile.png"- "logs/dada2/quality-profile/{sample}.{orientation}-quality-profile-se.log" + "<logs>/dada2/quality-profile/{sample}.{orientation}-quality-profile-se.log"- refFasta="resources/example_train_set.fa.gz" + refFasta="<resources>/example_train_set.fa.gz"Apply similarly across rules for outputs and logs.
Also applies to: 40-41, 55-56, 68-70, 80-81, 94-97, 104-108, 115-119, 127-131
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
docs/generate_docs.py(9 hunks)meta/bio/bowtie2_sambamba/meta_wrapper.smk(1 hunks)meta/bio/bowtie2_sambamba/test/Snakefile(1 hunks)meta/bio/bwa_mapping/meta_wrapper.smk(1 hunks)meta/bio/bwa_mapping/test/Snakefile(1 hunks)meta/bio/calc_consensus_reads/meta_wrapper.smk(1 hunks)meta/bio/calc_consensus_reads/test/Snakefile(1 hunks)meta/bio/dada2_pe/meta_wrapper.smk(1 hunks)meta/bio/dada2_pe/test/Snakefile(1 hunks)meta/bio/dada2_se/meta_wrapper.smk(1 hunks)meta/bio/dada2_se/test/Snakefile(1 hunks)meta/bio/gatk_mutect2_calling/meta_wrapper.smk(1 hunks)meta/bio/gatk_mutect2_calling/test/Snakefile(1 hunks)meta/bio/salmon_tximport/meta_wrapper.smk(1 hunks)meta/bio/salmon_tximport/test/Snakefile(1 hunks)meta/bio/star_arriba/meta_wrapper.smk(1 hunks)meta/bio/star_arriba/test/Snakefile(1 hunks)test_wrappers.py(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- meta/bio/calc_consensus_reads/meta_wrapper.smk
🚧 Files skipped from review as they are similar to previous changes (1)
- test_wrappers.py
🧰 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 theselfargument of methods.
Do not suggest type annotation of theclsargument of classmethods.
Do not suggest return type annotation if a function or method does not contain areturnstatement.
Files:
docs/generate_docs.py
🪛 GitHub Actions: Code quality
meta/bio/bowtie2_sambamba/test/Snakefile
[error] 3-3: WorkflowError: Expecting Snakemake version 9.13.1 or higher (you are currently using 9.12.0).
🪛 Ruff (0.13.3)
docs/generate_docs.py
96-96: Ambiguous variable name: l
(E741)
190-190: subprocess call: check for execution of untrusted input
(S603)
191-191: Starting a process with a partial executable path
(S607)
200-200: subprocess call: check for execution of untrusted input
(S603)
201-201: Starting a process with a partial executable path
(S607)
215-215: Unused function argument: args
(ARG001)
⏰ 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). (3)
- GitHub Check: testing
- GitHub Check: docs
- GitHub Check: Summary
🔇 Additional comments (4)
meta/bio/gatk_mutect2_calling/test/Snakefile (1)
1-11: LGTM! Module-based structure is clean.The modular approach with version gating and config-driven setup aligns well with the described architecture pattern. This provides good separation of concerns between test orchestration and rule definitions.
meta/bio/gatk_mutect2_calling/meta_wrapper.smk (2)
1-158: Inconsistent with PR objectives: No pathvars syntax present.The PR title and objectives explicitly mention migrating to "the new pathvars syntax" for star-arriba (though these files are for gatk_mutect2_calling). However, this meta_wrapper.smk file uses hardcoded paths throughout (e.g., "genome.fasta", "picard/{sample}.bam", "variant/{sample}.vcf").
According to the PR description, pathvars should provide "more flexible and convenient path configuration and reduced need to overwrite rule inputs/outputs via with-statements." The current implementation doesn't demonstrate this pattern.
Based on PR objectives.
92-124: LGTM! Appropriate use of temporary files.The use of
temp()for intermediate outputs (pileup summaries, contamination tables, and artifacts prior) is appropriate and helps manage disk space efficiently.meta/bio/dada2_se/test/Snakefile (1)
7-11: Module wiring LGTMModule declaration, config injection, and use rule * are correct.
| ".rev.2.bt2", | ||
| ), | ||
| log: | ||
| "logs/bowtie2_build/build.log", |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Standardize logs to pathvars
Hardcoded "logs/..." bypasses the new pathvar; use "/..." for consistency and flexibility.
- "logs/bowtie2_build/build.log",
+ "<logs>/bowtie2_build/build.log",- "logs/sambamba-view/{sample}.log",
+ "<logs>/sambamba-view/{sample}.log",Also applies to: 74-74
🤖 Prompt for AI Agents
In meta/bio/bowtie2_sambamba/meta_wrapper.smk around lines 15 and 74, hardcoded
"logs/..." paths are used which bypass the new <logs> pathvar; replace
occurrences of "logs/..." with "<logs>/..." (e.g.,
"logs/bowtie2_build/build.log" -> "<logs>/bowtie2_build/build.log") so the rule
uses the pathvar consistently and supports configurable log locations.
|
|
||
| rule bowtie2_alignment: | ||
| input: | ||
| sample=["<reads_r1>", "<reads_r2>"], |
There was a problem hiding this comment.
Use input key 'reads' for compatibility
Wrappers typically expect input.reads (list) for paired reads. Using 'sample' may break at runtime.
- sample=["<reads_r1>", "<reads_r2>"],
+ reads=["<reads_r1>", "<reads_r2>"],📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| sample=["<reads_r1>", "<reads_r2>"], | |
| reads=["<reads_r1>", "<reads_r2>"], |
🤖 Prompt for AI Agents
In meta/bio/bowtie2_sambamba/meta_wrapper.smk around line 25, the wrapper uses
the key 'sample' for paired reads which should be 'reads' for compatibility;
replace the 'sample' key with 'reads' and ensure it remains a list of two
entries (reads_r1, reads_r2), then update any downstream references in this
wrapper or its consumers to use input.reads instead of input.sample so runtime
expectations align.
| "", | ||
| log: |
There was a problem hiding this comment.
params should be a mapping with extra
Bare string assigns params to a string; wrappers usually read params.extra.
- params:
- "",
+ params:
+ extra="",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "", | |
| log: | |
| params: | |
| extra="", | |
| log: |
🤖 Prompt for AI Agents
In meta/bio/bowtie2_sambamba/meta_wrapper.smk around lines 55 to 56, params is
currently assigned a bare string which makes params a string and breaks wrappers
expecting params.extra; change params to be a mapping with an extra key (e.g.,
params: {"extra": ""}) so wrappers can read params.extra (use an empty string or
appropriate default for extra).
| from snakemake.utils import min_version | ||
|
|
||
| min_version("9.13.1") | ||
|
|
||
| rule bowtie2_alignment: | ||
| input: | ||
| sample=["{sample}.R1.fastq", "{sample}.R2.fastq"], | ||
| idx=multiext( | ||
| "genome", | ||
| ".1.bt2", | ||
| ".2.bt2", | ||
| ".3.bt2", | ||
| ".4.bt2", | ||
| ".rev.1.bt2", | ||
| ".rev.2.bt2", | ||
| ), | ||
| output: | ||
| temp("mapped/{sample}.bam"), | ||
| log: | ||
| "logs/bowtie2/{sample}.log", | ||
| params: | ||
| extra=( | ||
| " --rg-id {sample} " | ||
| "--rg 'SM:{sample} LB:FakeLib PU:FakePU.1.{sample} PL:ILLUMINA' " | ||
| ), | ||
| threads: 8 | ||
| wrapper: | ||
| "master/bio/bowtie2/align" | ||
| configfile: "config.yaml" | ||
|
|
||
| module bowtie2_sambamba: | ||
| snakefile: "../meta_wrapper.smk" | ||
| config: config | ||
|
|
||
| rule sambamba_sort: | ||
| input: | ||
| "mapped/{sample}.bam", | ||
| output: | ||
| temp("mapped/{sample}.sorted.bam"), | ||
| params: | ||
| "", | ||
| log: | ||
| "logs/sambamba-sort/{sample}.log", | ||
| threads: 8 | ||
| wrapper: | ||
| "master/bio/sambamba/sort" | ||
|
|
||
|
|
||
| rule sambamba_view: | ||
| input: | ||
| "mapped/{sample}.sorted.bam", | ||
| output: | ||
| temp("mapped/{sample}.filtered.bam"), | ||
| params: | ||
| extra=( | ||
| " --format 'bam' " | ||
| "--filter 'mapping_quality >= 30 and not (unmapped or mate_is_unmapped)' " | ||
| ), | ||
| log: | ||
| "logs/sambamba-view/{sample}.log", | ||
| threads: 8 | ||
| wrapper: | ||
| "master/bio/sambamba/view" | ||
|
|
||
|
|
||
| rule sambamba_markdup: | ||
| input: | ||
| "mapped/{sample}.filtered.bam", | ||
| output: | ||
| "mapped/{sample}.rmdup.bam", | ||
| params: | ||
| extra=" --remove-duplicates ", # optional parameters | ||
| log: | ||
| "logs/sambamba-markdup/{sample}.log", | ||
| threads: 8 | ||
| wrapper: | ||
| "master/bio/sambamba/markdup" | ||
|
|
||
|
|
||
| rule sambamba_index: | ||
| input: | ||
| "mapped/{sample}.rmdup.bam", | ||
| output: | ||
| "mapped/{sample}.rmdup.bam.bai", | ||
| params: | ||
| extra="", | ||
| log: | ||
| "logs/sambamba-index/{sample}.log", | ||
| threads: 8 | ||
| wrapper: | ||
| "master/bio/sambamba/index" | ||
| use rule * from bowtie2_sambamba |
There was a problem hiding this comment.
Align CI with the new Snakemake requirement
The added min_version("9.13.1") is tripping CI (WorkflowError: Expecting Snakemake version 9.13.1 or higher (you are currently using 9.12.0)). Please bump the test environment (e.g., the pinned Snakemake version used by GitHub Actions) or otherwise ensure the pipeline runs with ≥9.13.1 before merging.
🧰 Tools
🪛 GitHub Actions: Code quality
[error] 3-3: WorkflowError: Expecting Snakemake version 9.13.1 or higher (you are currently using 9.12.0).
🤖 Prompt for AI Agents
In meta/bio/bowtie2_sambamba/test/Snakefile lines 1-11 the added
min_version("9.13.1") causes CI to fail because the test environment is pinned
to Snakemake 9.12.0; update the CI/test environment to use Snakemake >= 9.13.1
(e.g., bump the pinned Snakemake version in the GitHub Actions workflow or test
matrix, update the conda/pip environment file used by tests, or adjust the test
container image) so the pipeline runs with the required Snakemake version before
merging.
| params: | ||
| "" # optional params string |
There was a problem hiding this comment.
params must be a mapping (not a bare string)
This sets snakemake.params to a string, likely breaking wrappers expecting params.extra.
- params:
- "" # optional params string
+ params:
+ extra="" # optional params string📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| params: | |
| "" # optional params string | |
| params: | |
| extra="" # optional params string |
🤖 Prompt for AI Agents
In meta/bio/bwa_mapping/meta_wrapper.smk around lines 37 to 38, params is
currently set to a bare string which makes snakemake.params a string and will
break wrappers expecting params.extra; change params to be a mapping (for
example params: {"extra": ""} or params: {} with a documented optional key
"extra") so params is a dict and callers can safely reference params.extra;
update the comment to reflect that params is a mapping and add a default empty
value for any commonly used key like "extra".
| resources: | ||
| mem_mb=1024, | ||
| params: | ||
| extra=" --tumor-sample {sample} ", |
There was a problem hiding this comment.
🧩 Analysis chain
Verify wildcard formatting in wrapper.
The params include {sample} wildcard formatting. Ensure the gatk/mutect wrapper properly handles this wildcard expansion in the --tumor-sample argument.
🏁 Script executed:
#!/bin/bash
# Check if the mutect wrapper supports wildcard formatting in params
fd -t f wrapper.py bio/gatk/mutect --exec cat {} | head -50Length of output: 1379
🏁 Script executed:
#!/bin/bash
# Locate shell invocation in the mutect wrapper and check params.extra usage
file=$(fd -t f wrapper.py bio/gatk/mutect)
grep -n "shell(" "$file"
echo "---- context around shell call ----"
grep -n "shell(" "$file" | cut -d: -f1 | while read l; do start=$((l-5)); end=$((l+5)); sed -n "${start},${end}p" "$file"; done
grep -n "params.extra" "$file"Length of output: 1006
Include snakemake.params.extra in Mutect2 wrapper’s shell call
The wrapper’s shell() invocation (around line 67 in wrapper.py) never references snakemake.params.extra, so the --tumor-sample {sample} flag is not passed. Append {snakemake.params.extra} to the shell command string.
🤖 Prompt for AI Agents
In meta/bio/gatk_mutect2_calling/meta_wrapper.smk around line 77, the shell()
invocation for Mutect2 does not include snakemake.params.extra so the
--tumor-sample {sample} flag (and any other extra flags) never get passed;
update the shell command string to append a space and {snakemake.params.extra}
(e.g. " ... --tumor-sample {sample} {snakemake.params.extra}") so snakemake
params are forwarded, ensuring you preserve spacing and quoting consistent with
the surrounding command.
| rule gatk_get_pileup_summaries: | ||
| input: | ||
| bam="picard/{sample}.bam", | ||
| bai_bai="picard/{sample}.bam.bai", |
There was a problem hiding this comment.
Fix typo in input field name.
The input field is named bai_bai instead of bai, which appears to be a typo.
Apply this diff:
- bai_bai="picard/{sample}.bam.bai",
+ bai="picard/{sample}.bam.bai",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| bai_bai="picard/{sample}.bam.bai", | |
| bai="picard/{sample}.bam.bai", |
🤖 Prompt for AI Agents
In meta/bio/gatk_mutect2_calling/meta_wrapper.smk around line 87 the input field
is incorrectly named `bai_bai`; change the field name to `bai` (i.e., replace
`bai_bai="picard/{sample}.bam.bai",` with `bai="picard/{sample}.bam.bai",`) so
the input uses the correct key.
| "master/bio/picard/createsequencedictionary" | ||
| from snakemake.utils import min_version | ||
|
|
||
| min_version("9.13.1") |
There was a problem hiding this comment.
🧩 Analysis chain
Verify Snakemake version 9.13.1 exists.
Snakemake 9.13.1 is beyond my knowledge cut-off (March 2025). Please confirm this version exists and is appropriate for this use case.
🌐 Web query:
What is the latest stable version of Snakemake as of October 2025? Does version 9.13.1 exist?
💡 Result:
The latest stable Snakemake release as of October 11, 2025 is v9.12.0. [1][2]
No — there is no Snakemake v9.13.1 available (I found releases up through v9.12.0). [1][2]
Sources:
- PyPI release page for snakemake (v9.12.0 uploaded Oct 4, 2025). [1]
- Snakemake project pages / releases (latest listed v9.12.0, Oct 2025). [2]
meta/bio/gatk_mutect2_calling/test/Snakefile:3 – min_version("9.13.1") references a non-existent Snakemake release; update to a valid version (e.g. "9.12.0").
🤖 Prompt for AI Agents
In meta/bio/gatk_mutect2_calling/test/Snakefile around line 3, the call
min_version("9.13.1") references a Snakemake release that doesn't exist; update
this to a valid released version (for example min_version("9.12.0")) by
replacing the string with a supported version number so the Snakefile enforces a
real Snakemake minimum.
| extra="", # optional params string | ||
| wrapper: | ||
| "master/bio/samtools/faidx" | ||
| configfile: "config.yaml" |
There was a problem hiding this comment.
❓ Verification inconclusive
Verify config.yaml contains pathvars configuration.
The PR objectives mention migrating to pathvars syntax, but the config.yaml file is not provided for review. Ensure it properly defines pathvars for genome files, BAM files, and output paths to align with the PR's stated goals.
🏁 Script executed:
#!/bin/bash
# Verify config.yaml exists and contains pathvars configuration
if [ -f "meta/bio/gatk_mutect2_calling/test/config.yaml" ]; then
cat meta/bio/gatk_mutect2_calling/test/config.yaml
else
echo "config.yaml not found in expected location"
fiLength of output: 0
Populate pathvars in meta/bio/gatk_mutect2_calling/test/config.yaml: the file is empty and must include definitions for reference genome, input BAM, and output paths as outlined in the PR objectives.
🤖 Prompt for AI Agents
In meta/bio/gatk_mutect2_calling/test/Snakefile around line 5, the referenced
config file meta/bio/gatk_mutect2_calling/test/config.yaml is empty and lacks
required path variables; open that config.yaml and populate it with keys for the
reference genome path, input BAM path, and output paths (e.g., reference:
path/to/reference.fasta, input_bam: path/to/sample.bam, output_dir:
path/to/output or specific VCF/metrics paths) matching the names the Snakefile
expects; ensure paths are valid (relative or absolute) and update any config
keys in the Snakefile if names differ.
| # specific parameters to work well with arriba | ||
| extra=lambda wc, input: f"--quantMode GeneCounts --sjdbGTFfile {input.annotation}" | ||
| " --outSAMtype BAM Unsorted --chimSegmentMin 10 --chimOutType WithinBAM SoftClip" | ||
| " --chimJunctionOverhangMin 10 --chimScoreMin 1 --chimScoreDropMax 30 --chimScoreJunctionNonGTAG 0" | ||
| " --chimScoreSeparation 1 --alignSJstitchMismatchNmax 5 -1 5 5 --chimSegmentReadGapMax 3", |
There was a problem hiding this comment.
Fix lambda continuation to avoid SyntaxError.
Line 34’s lambda returns a string, but the subsequent string literals on Lines 35‑37 sit on new lines without parentheses, so Python raises a syntax error before Snakemake can parse the rule. Wrap the concatenated string in parentheses.
- extra=lambda wc, input: f"--quantMode GeneCounts --sjdbGTFfile {input.annotation}"
- " --outSAMtype BAM Unsorted --chimSegmentMin 10 --chimOutType WithinBAM SoftClip"
- " --chimJunctionOverhangMin 10 --chimScoreMin 1 --chimScoreDropMax 30 --chimScoreJunctionNonGTAG 0"
- " --chimScoreSeparation 1 --alignSJstitchMismatchNmax 5 -1 5 5 --chimSegmentReadGapMax 3",
+ extra=lambda wc, input: (
+ f"--quantMode GeneCounts --sjdbGTFfile {input.annotation}"
+ " --outSAMtype BAM Unsorted --chimSegmentMin 10 --chimOutType WithinBAM SoftClip"
+ " --chimJunctionOverhangMin 10 --chimScoreMin 1 --chimScoreDropMax 30 --chimScoreJunctionNonGTAG 0"
+ " --chimScoreSeparation 1 --alignSJstitchMismatchNmax 5 -1 5 5 --chimSegmentReadGapMax 3"
+ ),
<!-- suggestion_start -->
<details>
<summary>📝 Committable suggestion</summary>
> ‼️ **IMPORTANT**
> Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
```suggestion
# specific parameters to work well with arriba
extra=lambda wc, input: (
f"--quantMode GeneCounts --sjdbGTFfile {input.annotation}"
" --outSAMtype BAM Unsorted --chimSegmentMin 10 --chimOutType WithinBAM SoftClip"
" --chimJunctionOverhangMin 10 --chimScoreMin 1 --chimScoreDropMax 30 --chimScoreJunctionNonGTAG 0"
" --chimScoreSeparation 1 --alignSJstitchMismatchNmax 5 -1 5 5 --chimSegmentReadGapMax 3"
),🤖 Prompt for AI Agents
In meta/bio/star_arriba/meta_wrapper.smk around lines 33 to 37, the lambda
assigned to extra returns a string on line 34 but the following string literals
on lines 35–37 are on new lines without parentheses causing a SyntaxError; wrap
the entire returned string expression in parentheses so the f-string and the
adjacent string literals are implicitly concatenated within the lambda and
ensure the trailing comma remains after the closing parenthesis.
1 participant
Depends on snakemake/snakemake#3760.
Usage suddenly becomes much more flexible and convenient:
There is almost no reason anymore to overwrite input/output of certain rules per with-statement.
QC
snakemake-wrappers.While the contributions guidelines are more extensive, please particularly ensure that:
test.pywas updated to call any added or updated example rules in aSnakefileinput:andoutput:file paths in the rules can be chosen arbitrarilyinput:oroutput:)tempfile.gettempdir()points tometa.yamlcontains a link to the documentation of the respective tool or command underurl:Summary by CodeRabbit
New Features
Documentation
Style
Tests
Chores