fix(markduplicates): avoid duplicate alignment emission under -stub - #2277
Open
gburnett-nvidia wants to merge 4 commits into
Open
fix(markduplicates): avoid duplicate alignment emission under -stub#2277gburnett-nvidia wants to merge 4 commits into
gburnett-nvidia wants to merge 4 commits into
Conversation
BAM_MARKDUPLICATES mixed the bam and cram output channels together, trusting that GATK4_MARKDUPLICATES only ever populates one of them. That holds for real runs, but the module's stub block touches both bam and cram (+ indices) unconditionally, so under -stub the mixed `alignment` channel emits twice per sample. The duplication silently propagates through baserecalibration/applybqsr and only surfaces downstream at the tumor-normal pairing join in workflows/sarek.nf, which has failOnDuplicate: true and throws "Detected join operation duplicate emission". Select the bam or cram branch explicitly by params.save_output_as_bam instead, matching the pattern already used in bam_sentieon_dedup. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
gburnett-nvidia
requested review from
FriederikeHanssen and
maxulysse
as code owners
August 25, 2026 15:08
|
maxulysse
reviewed
Aug 25, 2026
| // Select explicitly rather than mixing both optional channels together: the module's stub | ||
| // block touches both bam and cram outputs unconditionally, so relying on "only one is ever | ||
| // populated" doubles every downstream emission under -stub. | ||
| alignment = params.save_output_as_bam |
Member
There was a problem hiding this comment.
Looking good, but let's not use params directly, but get them from input of the subworkflow.
Or fix the stub so that only 1 of these files is produced
Contributor
Author
There was a problem hiding this comment.
Good call, thank you! Just updated in the latest commit.
gburnett-nvidia
added a commit
that referenced
this pull request
Aug 25, 2026
…uctured scenarios Updates snapshot entries for the 9 currently-passing scenarios to match the renamed/restructured test_scenario list (dropped "somatic" suffix, removed real cram-input scenarios, added real+stub pon scenario on fastq input). The 3 fastq-default-aligner -stub scenarios still crash before reaching their snapshot assertion (Detected join operation duplicate emission), blocked on a separate, unrelated bam_markduplicates bug being fixed upstream in #2277 — no entries for those yet. Stale entries from the pre-rename scenario names are left in place; nf-test only prunes obsolete snapshots when every test in the file passes, which requires that fix to land first.
❌ nf-test failed with latest Nextflow versionNote Tests with Nextflow's latest version failed but it will not cause a CI workflow failure.
See the full run for details. |
Contributor
|
thanks, @maxulysse this is likley affecting many places in the code base where we wanted to be elegant. |
FriederikeHanssen
requested changes
Aug 26, 2026
FriederikeHanssen
left a comment
FriederikeHanssen
left a comment
Contributor
There was a problem hiding this comment.
Please update the changelog
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
3 participants
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.
Fixes #2280
Summary
BAM_MARKDUPLICATESmixes thebamandcramoutput channels fromGATK4_MARKDUPLICATEStogether, trusting that only one is ever populated per sample (real runs: true). Under-stub, the module's stub block touches bothbamandcram(+ indices) unconditionally, so the mixedalignmentchannel emits twice per sample.workflows/sarek.nf(failOnDuplicate: true), throwingDetected join operation duplicate emission on right channel.bamorcrambranch explicitly byparams.save_output_as_bam, instead of.mix()-ing both — same pattern already used inbam_sentieon_dedup..mix()-of-optional-channels pattern (bam_applybqsr,bam_markduplicates_spark,bam_sentieon_dedup) — none of them have this bug:bam_applybqsr:GATK4_APPLYBQSRtakes an explicitoutput_suffixparam and its stub branches on that, matching the real script.bam_markduplicates_spark:GATK4SPARK_MARKDUPLICATEShas a single unified output emit (one glob, not separate bam/cram declarations) — can't double-emit.bam_sentieon_dedup: already uses the explicitparams.save_output_as_bam ? ... : ...branch pattern this PR applies to markduplicates.Test plan
nf-test test tests/default.nf.test --tag cpu_conda_stub --profile test,docker—-profile test -stubscenario: pipeline completes successfully (GATK4_MARKDUPLICATES,GATK4_BASERECALIBRATOR,GATK4_APPLYBQSR,INDEX_CRAMeach run once per sample, no join-duplicate error). Snapshot mismatch on this run is a pre-existing, unrelated warning-log-format drift (also present on the non-stub scenarios in the same file, before this change).nf-test test tests/default.nf.test --tag cpu,cpu_conda --profile +docker— non-stub scenarios unaffected (same pre-existing snapshot drift, pipeline succeeds).🤖 Generated with Claude Code