Multi-sample support via sample sheet - #159
Open
riasc wants to merge 5 commits into
Open
Conversation
Replace the single-sample `data:` block in config.yaml with a wide-format
sample sheet referenced as `samples: config/samples.tsv`. One snakemake
invocation now fans out across all rows; independent per-sample DAGs run
in parallel without any new machinery (the {sample} wildcard was already
threaded through every rule).
config['data'] is replaced by SAMPLES[wildcards.sample] across common.smk
and align.smk. data_structure() becomes a pure per_sample_data(row)
builder; print_run_summary and check_cross_field_consistency iterate the
SAMPLES map. The three parse-time conditionals in align.smk that branched
rule definitions on filetype are dropped; the rules are now unconditional
and the filetype branch lives in get_star_input / get_dna_align_input /
the new get_rnaseq_star_bam helper. merge_alignment_results uses a
distinct output suffix so it can coexist with star_align_fastq when a
batch mixes BAM- and FASTQ-input samples.
Breaking change for v0.5.0: users must convert their `data:` block to a
one-row sheet (migration steps in docs/configuration.md).
Contributor
|
Warning Review limit reached
Next review available in: 25 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThe PR switches workflow input specification from a single ChangesMulti-sample sample sheet refactor
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant Snakefile
participant pandas
participant common_smk as common.smk
participant SAMPLES
participant rule_all as rule all
User->>Snakefile: snakemake with config["samples"]
Snakefile->>pandas: read_csv(samples.tsv, sep="\t", dtype=str).fillna("")
pandas-->>Snakefile: SHEET DataFrame
Snakefile->>Snakefile: validate(SHEET, samples.schema.yaml)
Snakefile->>common_smk: include workflow/rules/common.smk
common_smk->>SAMPLES: build per-sample entries from SHEET
common_smk->>common_smk: validate cross-field consistency
rule_all->>SAMPLES: expand prioritization targets over sample keys
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
workflow/rules/common.smk (1)
160-165:⚠️ Potential issue | 🟠 Major | ⚡ Quick winInvalid SE file extensions are logged but not treated as fatal.
Line 161 reports a config error, but execution continues and can silently drop that replicate. That can produce partial/incorrect sample inputs instead of a fail-fast config error.
Suggested patch
else: print( f"[config error] {mode}.{rpl}: '{files[0]}' is not a valid " f"input file (expected .fq/.fastq/.bam -- this is likely an " f"unfilled placeholder from the default config/config.yaml).", file=sys.stderr, ) + if "--lint" not in sys.argv: + sys.exit(1)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@workflow/rules/common.smk` around lines 160 - 165, The error message for invalid SE file extensions in the common.smk file is printed to stderr but does not halt execution, allowing the workflow to continue with incorrect configuration. After the print statement that logs the config error for invalid file extensions (checking for .fq/.fastq/.bam), add a fail-fast mechanism such as calling sys.exit() or raising an exception to immediately terminate execution and prevent silent data loss from partial/dropped replicates.
🧹 Nitpick comments (1)
workflow/rules/align.smk (1)
67-84: 💤 Low valueOptional: File extension doesn't match content encoding.
The shell pipes output through
gzip -cbut writes to a.fastqfile (line 71). This produces gzip-compressed content with an uncompressed extension. It works because downstreamstar_align_bamfileuses--readFilesCommand zcat, but the mismatch can confuse tooling and developers inspecting intermediate files.Consider renaming the output to
.fastq.gzand updating thestar_align_bamfileinput path accordingly, or removing the gzip step if compression isn't needed.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@workflow/rules/align.smk` around lines 67 - 84, The rule bamfile_RG_to_fastq produces gzip-compressed output (via the gzip -c pipe in the shell command) but the output file path uses the .fastq extension instead of .fastq.gz. Fix this mismatch by either: (1) renaming the output path from .fastq to .fastq.gz and updating any downstream rule (such as star_align_bamfile) that reads from this output to use the new .fastq.gz path, or (2) removing the gzip compression step by deleting the pipe to gzip -c from the shell command if compression is not needed. Choose option 1 to maintain compression or option 2 if compression is unnecessary.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/configuration.md`:
- Around line 18-20: The markdown file contains fenced code blocks without
language identifiers on lines 18 and 39, which triggers the MD040 linting rule.
Add the appropriate language identifier to each code block: replace the opening
triple backticks on line 18 with ```yaml for the YAML configuration example, and
replace the opening triple backticks on line 39 with ```tsv for the TSV table
example. This will satisfy the markdown-lint requirement and prevent CI gate
failures.
- Around line 34-35: The documentation for the `custom_hla_I` and
`custom_hla_II` configuration parameters incorrectly states that these files are
used when the mode "is" custom, but the actual runtime contract is that these
files are used when the mode "contains" custom (to support mixed-mode
configurations). Update the description text in lines 34-35 to change the
wording from "when `hlatyping.MHC-I_mode` is `custom`" to indicate that the mode
should contain or include the custom value, ensuring the documentation aligns
with the actual schema and runtime behavior for mixed-mode scenarios.
In `@workflow/rules/align.smk`:
- Around line 1-6: The snakefmt formatter has identified formatting issues in
the Snakemake rules files that are causing CI pipeline failures. Run the
snakefmt formatter on the workflow/rules/ directory to automatically fix the
formatting violations. The formatter will reorganize directives within rules
(such as input, output, shell) according to the project's formatting standards.
Use the --diff flag first if you want to preview the changes before applying
them, then run the formatter without the --check flag to apply the fixes
in-place.
In `@workflow/rules/common.smk`:
- Line 1443: The code in common.smk file does not follow the snakefmt formatting
standards, as indicated by the failing snakefmt --check CI job. Run the snakefmt
command on the workflow/rules/common.smk file to automatically reformat the code
according to the project's snakefmt configuration, including the line at
position 1443 with the SAMPLES conditional check, and commit the formatted
changes before merging.
- Around line 215-217: The code at line 215 is constructing dictionary keys
using wildcards.readtype to access the SAMPLES data structure, but the SAMPLES
dictionary actually stores fields with seqtype names (like dnaseq_readtype and
rnaseq_readtype), not readtype names. Replace wildcards.readtype with
wildcards.seqtype in the f-string that builds the field name for accessing
SAMPLES[wildcards.sample] in both the SE and PE conditional branches.
- Around line 685-687: The function get_input_filtering_hlatyping_PE derives a
local seqtype variable from wildcards.nartype on line 685, but then attempts to
index the SAMPLES dictionary using wildcards.seqtype on line 686 instead of
using the derived seqtype variable. Since wildcards.seqtype may not be defined
in all rules, this causes failures during input function evaluation. Replace
wildcards.seqtype with the local seqtype variable in the SAMPLES indexing
operation on line 686.
In `@workflow/schemas/config.schema.yaml`:
- Around line 38-40: The samples field in the config schema currently only
specifies type: string, which allows empty strings that would cause a less
actionable error later during pd.read_csv execution. Add a minLength constraint
(minLength: 1) to the samples field definition alongside the existing type and
description properties to enforce that a non-empty path must be provided,
enabling fail-fast validation at the config schema level.
In `@workflow/schemas/samples.schema.yaml`:
- Around line 7-10: The `sample` field in the schema definition allows forward
slash characters, but the workflow Snakefile constrains sample IDs with the
pattern `[^/]+` which prohibits slashes, causing failures during DAG resolution.
Add a `pattern` constraint to the `sample` field in the schema to enforce that
sample names cannot contain forward slashes, matching the regex pattern used in
the Snakefile so validation failures occur at schema validation time rather than
later during workflow execution.
In `@workflow/Snakefile`:
- Around line 15-21: The Snakefile does not meet snakefmt formatting standards
as indicated by the CI check failure. Run snakefmt on the workflow/Snakefile to
automatically reformat the file to comply with the formatting requirements. This
will fix the formatting issues in the section where SHEET is created with
pd.read_csv and the subsequent validate() call.
---
Outside diff comments:
In `@workflow/rules/common.smk`:
- Around line 160-165: The error message for invalid SE file extensions in the
common.smk file is printed to stderr but does not halt execution, allowing the
workflow to continue with incorrect configuration. After the print statement
that logs the config error for invalid file extensions (checking for
.fq/.fastq/.bam), add a fail-fast mechanism such as calling sys.exit() or
raising an exception to immediately terminate execution and prevent silent data
loss from partial/dropped replicates.
---
Nitpick comments:
In `@workflow/rules/align.smk`:
- Around line 67-84: The rule bamfile_RG_to_fastq produces gzip-compressed
output (via the gzip -c pipe in the shell command) but the output file path uses
the .fastq extension instead of .fastq.gz. Fix this mismatch by either: (1)
renaming the output path from .fastq to .fastq.gz and updating any downstream
rule (such as star_align_bamfile) that reads from this output to use the new
.fastq.gz path, or (2) removing the gzip compression step by deleting the pipe
to gzip -c from the shell command if compression is not needed. Choose option 1
to maintain compression or option 2 if compression is unnecessary.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a3baced6-43d9-4751-bacf-fcdc26499742
⛔ Files ignored due to path filters (6)
.tests/integration/config_basic/samples.tsvis excluded by!**/*.tsv.tests/integration/custom-test/config/samples.tsvis excluded by!**/*.tsv.tests/integration/indel-test/config/samples.tsvis excluded by!**/*.tsv.tests/integration/protein-test/config/samples.tsvis excluded by!**/*.tsv.tests/integration/samples.tsvis excluded by!**/*.tsvconfig/samples.tsvis excluded by!**/*.tsv
📒 Files selected for processing (12)
.tests/integration/config_basic/config.yaml.tests/integration/custom-test/config/config.yaml.tests/integration/indel-test/config/config_SE.yml.tests/integration/protein-test/config/config.yamlconfig/config.yamldocs/configuration.mdworkflow/Snakefileworkflow/rules/align.smkworkflow/rules/common.smkworkflow/schemas/config.schema.yamlworkflow/schemas/samples.schema.yamlworkflow/scripts/genotyping/combine_all_alleles.py
Comment on lines
+15
to
+21
| ##### load sample sheet (issue #93) ##### | ||
| # Wide format: one row per sample, columns described in schemas/samples.schema.yaml. | ||
| # .fillna("") gives empty-string sentinels for missing cells so per_sample_data() | ||
| # can treat them as "not provided" without juggling NaN vs None. | ||
| SHEET = pd.read_csv(config["samples"], sep="\t", dtype=str).fillna("") | ||
| validate(SHEET, schema="schemas/samples.schema.yaml") | ||
|
|
Contributor
There was a problem hiding this comment.
snakefmt --check is failing for this file.
CI reports formatting would rewrite files. Please run snakefmt so the formatting job passes.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@workflow/Snakefile` around lines 15 - 21, The Snakefile does not meet
snakefmt formatting standards as indicated by the CI check failure. Run snakefmt
on the workflow/Snakefile to automatically reformat the file to comply with the
formatting requirements. This will fix the formatting issues in the section
where SHEET is created with pd.read_csv and the subsequent validate() call.
Source: Pipeline failures
star_align_fastq and merge_alignment_results both produced
{group}_aligned_STAR.bam, which throws AmbiguousRuleException once both
rules are always-defined. Snakemake matches rules by output pattern
before checking input feasibility, so a producer-side tag is the only
workable resolution that keeps mixed-filetype batches.
star_align_fastq -> rnaseq/align/fq/{group}_aligned_STAR.bam
merge_alignment_results -> rnaseq/align/bam/{group}_aligned_STAR.bam
get_rnaseq_star_bam(wildcards) returns the right sub-path per sample
filetype; rnaseq_postproc_fixmate consumes through it. Final
post-processed output unchanged.
Same pattern still needed for the DNA-side (dnaseq_postproc vs realign
on {group}_final_BWA.bam); follow-up.
Mirror the RNA fix for the DNA side: dnaseq_postproc and realign-for-dnaseq
both produced dnaseq/align/{group}_final_BWA.bam, triggering
AmbiguousRuleException once both rules are always-defined.
Producer changes (no consumer changes):
- dnaseq_postproc output -> dnaseq/align/fq/{group}_final_BWA.bam
- realign constrained to seqtype=rnaseq via wildcard_constraints
- new realign_dnaseq_bam rule -> dnaseq/align/bam/{group}_final_BWA.bam
(same shell as realign's old dnaseq-BAM branch, just split out)
- new dnaseq_final_BWA_stage rule symlinks the tagged path to the canonical
dnaseq/align/{group}_final_BWA.bam so germline.smk, indel.smk, and
samtools_index_BWA_final consume the canonical path unchanged
helper get_dnaseq_final_bam_tagged(wildcards) picks fq/ or bam/ per sample
filetype. Verified with lint + dry-runs across single-sample, mixed-RNA,
and mixed-RNA+DNA-BAM batches (122-job DAG for the mixed batch).
- snakefmt 2.0.0 reflow of align.smk and common.smk (formatting job) - config_basic samples path made .tests-relative so it resolves under the lint action's directory: .tests (was double-prefixed to .tests/.tests/... -> FileNotFoundError at Snakefile:19)
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
workflow/rules/align.smk (1)
218-291: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse
get_readgroups_inputfor DNA BAM source selection. It already returnsSAMPLES[wildcards.sample]["dnaseq"][wildcards.group]forseqtype == "dnaseq", so this lambda just duplicates the same lookup and adds a second source-selection path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@workflow/rules/align.smk` around lines 218 - 291, The dna BAM source selection in rule realign_dnaseq_bam duplicates the same lookup already handled by get_readgroups_input, creating a redundant second path. Update the rule to reuse get_readgroups_input for the bam input instead of the inline lambda, and keep the existing readgroup file input and downstream realignment shell unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@workflow/rules/align.smk`:
- Around line 218-291: The dna BAM source selection in rule realign_dnaseq_bam
duplicates the same lookup already handled by get_readgroups_input, creating a
redundant second path. Update the rule to reuse get_readgroups_input for the bam
input instead of the inline lambda, and keep the existing readgroup file input
and downstream realignment shell unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ce235875-5fcb-4a76-aa01-ce53dfcbc14a
📒 Files selected for processing (3)
.tests/integration/config_basic/config.yamlworkflow/rules/align.smkworkflow/rules/common.smk
🚧 Files skipped from review as they are similar to previous changes (2)
- .tests/integration/config_basic/config.yaml
- workflow/rules/common.smk
- get_input_filtering_hlatyping_PE: index SAMPLES with the local seqtype
(derived from nartype), not wildcards.seqtype which the rule doesn't
define; only reached when preproc.activate is false
- config schema: require non-empty samples path (minLength: 1)
- samples schema: constrain sample to ^[^/]+$ to match the {sample}
wildcard, failing fast on slash-containing IDs
- docs: language identifiers on fenced blocks (MD040); custom_hla wording
'is custom' -> 'contains custom' to match mixed-mode contract
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Added
config/config.yamlnow referencessamples: config/samples.tsv; the wide TSV has one row per sample with columnssample,dnaseq_tumor,dnaseq_normal,rnaseq,custom_variants,custom_proteins,custom_hla_I,custom_hla_II. One `snakemake` invocation fans out across all rows; per-sample DAGs run in parallel without any new machinery (the `{sample}` wildcard was already threaded through every rule).Changed (breaking — v0.5.0)
Closes #93
QC
Summary by CodeRabbit
samples).samplesTSV format, required columns, and custom input options.custom_hla_I/custom_hla_IIsample sheet columns.