Skip to content

Conversation

@tdayris
Copy link
Contributor

@tdayris tdayris commented Oct 20, 2025

This PR tries to make salmon-tximport meta-wrapper accessible through pathvars. Yet, I'd like to get some advice on how to proceed smoothly with the expand keyword and the pathvars. But the meta-wrapper wouldn't work if user does not modify the last rule : for now, I still list samples within the expand function.

QC

While the contributions guidelines are more extensive, please particularly ensure that:

  • test.py was updated to call any added or updated example rules in a Snakefile
  • input: and output: file paths in the rules can be chosen arbitrarily
  • wherever possible, command line arguments are inferred and set automatically (e.g. based on file extensions in input: or output:)
  • temporary files are either written to a unique hidden folder in the working directory, or (better) stored where the Python function tempfile.gettempdir() points to
  • the meta.yaml contains a link to the documentation of the respective tool or command under url:
  • conda environments use a minimal amount of channels and packages, in recommended ordering

Summary by CodeRabbit

  • New Features

    • Introduces configurable path variables so users can declare locations for inputs (transcriptome, genome, annotation, reads, tx->gene) and outputs (results, resources, logs).
    • Per-sample outputs and logs are now organized under user-configurable result/log directories.
  • Tests

    • Test fixtures updated to reflect new paths and sample data; test expectations adjusted to the new results path.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 20, 2025

Warning

Rate limit exceeded

@tdayris has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 17 minutes and 8 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

📥 Commits

Reviewing files that changed from the base of the PR and between c249f3a and a529cf5.

📒 Files selected for processing (3)
  • meta/bio/salmon_tximport/meta_wrapper.smk (5 hunks)
  • meta/bio/salmon_tximport/test/Snakefile (1 hunks)
  • meta/bio/salmon_tximport/test/config.yaml (1 hunks)
📝 Walkthrough

Walkthrough

Adds a declarative pathvars configuration to the salmon_tximport module and replaces hardcoded resource/read/output paths in workflow rules and tests with parameterized placeholders, relocating outputs and logs under the new results/logs/resources mappings and updating test resources accordingly.

Changes

Cohort / File(s) Summary
Configuration system
meta/bio/salmon_tximport/meta.yaml, meta/bio/salmon_tximport/test/config.yaml
Adds a pathvars section: default list (results, resources, logs) and custom mappings for transcriptome_sequence, genome_sequence, genome_annotation, reads_r1, reads_r2, tx_to_gene, and per with descriptive comments.
Workflow rule parameterization
meta/bio/salmon_tximport/meta_wrapper.smk
Replaces hardcoded paths with placeholders across rules: salmon_decoy_sequences, salmon_index_gentrome, salmon_quant_reads, and tximport. Inputs/outputs/logs now reference "<resources>", "<results>", "<logs>", and specific custom pathvars (e.g., "<transcriptome_sequence>", "<reads_r1>", "<tx_to_gene>"). Wrapper versions mostly unchanged except tximport updated.
Test infrastructure
meta/bio/salmon_tximport/test/Snakefile, test_wrappers.py
test/Snakefile adds configfile: "config.yaml" and config block for module; test_wrappers.py updates expected tximport output path to results/tximport/SummarizedExperimentObject.RDS.
Test resources
meta/bio/salmon_tximport/test/resources/*
Adds genome.fasta.fai, updates transcriptome.fasta contents, and adds/updates transcriptome.fasta.fai entries to reflect modified sequences.

Sequence Diagram(s)

sequenceDiagram
    participant User
    participant Snakemake as SMK
    participant MetaWrapper as MW
    participant Tools

    rect rgba(200,230,255,0.5)
    note right of SMK: config + meta.pathvars resolved
    User->>SMK: run workflow (module salmon_tximport)
    SMK->>MW: expand rules with placeholders
    end

    rect rgba(220,255,220,0.5)
    SMK->>Tools: salmon_decoy_sequences(inputs: <transcriptome_sequence>,<genome_sequence>) 
    Tools-->>SMK: gentrome, decoys -> saved under <resources>
    SMK->>Tools: salmon_index_gentrome(inputs: <resources>/gentrome.fasta,decoys)
    Tools-->>SMK: index files -> <resources>/salmon_gentrome_index/
    SMK->>Tools: salmon_quant_reads(inputs: <reads_r1>,<reads_r2>, index:<resources>/salmon_gentrome_index/, gtf:<genome_annotation>)
    Tools-->>SMK: per-sample quant outputs -> <results>/pseudo_mapping/{per}/...
    SMK->>Tools: tximport(inputs: per-sample quant from <results>, tx2gene:<tx_to_gene>)
    Tools-->>SMK: txi -> <results>/tximport/SummarizedExperimentObject.RDS
    end

    note over SMK: Logs written under <logs> paths
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Areas to review closely:

  • meta/bio/salmon_tximport/meta_wrapper.smk — ensure all placeholder substitutions and path expressions (temp/expand/multiext usage) are syntactically correct and consistent.
  • meta/bio/salmon_tximport/meta.yaml and test/config.yaml — verify pathvars keys/names match those used in rules.
  • Tests (test_wrappers.py and test resources) — confirm updated test artifact paths and FASTA/FAI contents remain coherent for the wrappers.

Possibly related PRs

Suggested reviewers

  • johanneskoester

Pre-merge checks and finishing touches

❌ Failed checks (1 warning, 1 inconclusive)
Check name Status Explanation Resolution
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.
Description check ❓ Inconclusive The description explains the PR objectives and acknowledges the pathvars limitation, but one QC checkbox (meta.yaml documentation URL) is unchecked. Consider adding the tool documentation URL to meta.yaml as per the contribution guidelines, or clarify why it cannot be added at this time.
✅ Passed checks (1 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and specifically describes the main change: adding pathvars support to the salmon-tximport meta-wrapper.

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
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: 2

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 0bcf3ab and b679f61.

⛔ Files ignored due to path filters (8)
  • meta/bio/salmon_tximport/test/reads/S1.fastq.gz is excluded by !**/*.gz
  • meta/bio/salmon_tximport/test/reads/S1_R1.fq.gz is excluded by !**/*.gz
  • meta/bio/salmon_tximport/test/reads/S1_R2.fq.gz is excluded by !**/*.gz
  • meta/bio/salmon_tximport/test/reads/S2.fastq.gz is excluded by !**/*.gz
  • meta/bio/salmon_tximport/test/reads/S2_R1.fq.gz is excluded by !**/*.gz
  • meta/bio/salmon_tximport/test/reads/S2_R2.fq.gz is excluded by !**/*.gz
  • meta/bio/salmon_tximport/test/reads/S3.fastq.gz is excluded by !**/*.gz
  • meta/bio/salmon_tximport/test/reads/S4.fastq.gz is excluded by !**/*.gz
📒 Files selected for processing (8)
  • meta/bio/salmon_tximport/meta.yaml (1 hunks)
  • meta/bio/salmon_tximport/meta_wrapper.smk (5 hunks)
  • meta/bio/salmon_tximport/test/Snakefile (1 hunks)
  • meta/bio/salmon_tximport/test/config.yaml (1 hunks)
  • meta/bio/salmon_tximport/test/resources/genome.fasta.fai (1 hunks)
  • meta/bio/salmon_tximport/test/resources/transcriptome.fasta (1 hunks)
  • meta/bio/salmon_tximport/test/resources/transcriptome.fasta.fai (1 hunks)
  • test_wrappers.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:

  • test_wrappers.py
⏰ 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/salmon_tximport/test/resources/transcriptome.fasta.fai (1)

1-2: LGTM: Test resource file is correctly formatted.

The FASTA index file follows the standard .fai format and provides appropriate test data for the pathvars-based configuration system.

meta/bio/salmon_tximport/test/resources/genome.fasta.fai (1)

1-1: LGTM: Genome index file is correctly formatted.

The FASTA index file is properly structured and aligns with the new pathvars configuration for test resources.

meta/bio/salmon_tximport/test/config.yaml (1)

1-9: LGTM: Test configuration properly defines pathvars mappings.

The pathvars configuration aligns with the schema defined in meta.yaml and provides appropriate test resource paths.

test_wrappers.py (1)

1104-1104: LGTM: Test expectation updated to match new pathvars structure.

The path correctly reflects the new results-prefixed output location defined in the meta-wrapper.

meta/bio/salmon_tximport/test/resources/transcriptome.fasta (1)

1-5: LGTM: Test sequences updated appropriately.

The FASTA sequences have been updated and their lengths match the corresponding .fai index file entries (transcript1: 49bp, transcript2: 81bp).

meta/bio/salmon_tximport/test/Snakefile (1)

6-13: LGTM: Configuration properly integrated into test workflow.

The addition of configfile and the block-style meta_wrapper with explicit config section correctly enables the pathvars-based configuration system.

meta/bio/salmon_tximport/meta_wrapper.smk (1)

1-89: LGTM: Placeholder-based parameterization for salmon rules is well-implemented.

The salmon_decoy_sequences, salmon_index_gentrome, and salmon_quant_reads rules correctly use pathvars placeholders (<transcriptome_sequence>, <genome_sequence>, <resources>, <logs>, <reads_r1>, <reads_r2>, <genome_annotation>, <per>) to enable user configuration without code modification.

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