Repository navigation
feat(pipeline): wire dedup --threads onto the declarative chain builder - #876
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Note Reviews pausedUse the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
Included review availability: 0 reviews are currently available. Your included PR review attempts over the past 7 days set your current allowance at 1 review per hour. WalkthroughThreaded deduplication now uses the chain pipeline after shared validation. The chain path records and finalizes duplication ladders, shares filtered-template diagnostics, and adds integration coverage for output, strategy, filtering, CRC, and auxiliary-file parity. ChangesThreaded deduplication chain
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: ⚪ Minimal · up to The PR wires the threaded deduplication path through the declarative chain builder while preserving the existing path and adding parity coverage; no actionable merge-blocking risk remains beyond normal checks and review. Sequence Diagram(s)sequenceDiagram
participant DedupCommand
participant ChainSpec
participant MIAssignStep
participant DedupFinalizeHook
DedupCommand->>ChainSpec: build threaded Dedup stage
ChainSpec->>MIAssignStep: process groups
MIAssignStep->>MIAssignStep: record dedup counts
MIAssignStep->>DedupFinalizeHook: pass processed results
DedupFinalizeHook->>DedupFinalizeHook: finish and write ladder
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
Comment |
|
@coderabbitai review |
|
@coderabbitai pause |
✅ Action performedReview finished.
|
✅ Action performedReviews paused. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #876 +/- ##
==========================================
+ Coverage 92.50% 92.70% +0.19%
==========================================
Files 294 294
Lines 148546 148589 +43
==========================================
+ Hits 137415 137746 +331
+ Misses 11131 10843 -288 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
08a330d to
0a7b4bc
Compare
|
@coderabbitai review |
|
0a7b4bc to
211a43b
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
211a43b to
a5753d0
Compare
|
@coderabbitai review |
|
a5753d0 to
2bf6d6f
Compare
2bf6d6f to
e79bc55
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
…ain dedup MiAssign step The dormant chain dedup builder carried --metrics and --family-size-histogram but not --duplication-ladder: repointing dedup onto the chain would have silently dropped the ladder output. Wire it so the chain path produces a byte-identical ladder. The ladder is order-sensitive (it samples a saturation curve at intervals of cumulative templates), so it must be recorded in the same serial/coordinate-order seam the non-chain path uses -- the serial MiAssign step -- not the parallel serialize step. build_mi_assign_step now takes an optional recorder and, after assigning MI offsets, records each group's per-library counts in coordinate order. MiAssign is Serial + ByItemOrdinal, so batches arrive in input-record order and groups within a batch are coordinate-ordered, reproducing the non-chain per-group stream exactly. DedupFinalizeHook gains the ladder path + recorder and, in finalize, finish()es and writes it through the lock rather than Arc::try_unwrap: the MiAssign step's Arc clone may still be alive when finalize hooks run. Promotes DuplicationLadderRecorder / write_duplication_ladder to pub(crate) and adds Clone to MarkDuplicates (needed so the command struct can populate StageOptionsBag.dedup on the live path in the follow-up).
Replace Dedup::execute's hand-rolled unified-pipeline construction for the --threads N path with the declarative chain builder (ChainSpec::single_stage(Stage::Dedup) -> build_for -> run). The chain opens its own source, validates the template-coordinate sort order, injects @pg, assigns MoleculeIds deterministically, writes the output BAM, and writes the metrics / family-size histogram / duplication ladder via its finalize hook -- all through the same shared helpers as the non-chain path. Unlike group, dedup has no separate single-threaded fast path: its whole execute() is the unified pipeline scaled by thread count. Gate on --threads anyway (mirroring the group pilot) and keep the no-threads path untouched as the in-process parity oracle -- rather than replacing all of execute(), which would remove that oracle and enlarge the diff. The one-engine cleanup is a deliberate follow-up. The dispatch runs after the reader-free pre-flight validations (output collisions, strategy/min-umi combos, index-threshold, input existence), which must hold on both paths, but before the timer/banner/reader, which add_dedup re-emits and which would otherwise double-log and pre-consume stdin (breaking stdin + --threads). Three parity tests pin behaviors the non-chain oracle cannot see: - test_dedup_chain_matches_single_threaded: --threads 1 and 4 output record-for-record identical to the non-chain path. - test_dedup_threaded_crc_policy: --no-check-crc/--check-crc/default CRC policy honored on the chain path; accept case asserts record identity against an intact-file baseline, not merely non-empty. - test_dedup_threaded_duplication_ladder_parity: the order-sensitive --duplication-ladder (plus --metrics, --family-size-histogram) file is byte-identical between the chain and non-chain paths. Runs at --threads 4 with several hundred non-uniform position groups so a reordering regression in the ladder recording actually diverges the file (a --threads 1 or uniform-size test could not catch it).
e79bc55 to
21a8362
Compare
|
@coderabbitai review |
|
This branch was successfully deployed
1 participant
Makes
dedup --threads Na live chain path, the second command (aftergroup) wired onto the declarative chain-builder layer. Stacked on #872 — base isnh/chain-builderand will be retargeted tomain(and rebased) once #872 merges.dedup --threads N's hand-rolled unified-pipeline construction is replaced withChainSpec::single_stage(Stage::Dedup) → build_for → run, reusing the shared helpers #872 introduced. dedup is the closest sibling togroup(same template-coordinate input, same molecule machinery), so this mirrors the group pilot (b01e18eain #872) closely.Key difference from the group pilot
Unlike
group,deduphas no separate single-threaded fast path — its wholeexecute()is the unified pipeline scaled by thread count. This PR still gates on--threadsand leaves the no---threadspath untouched, so that path stays as the in-process parity oracle the new tests diff against. Replacing all ofexecute()(the one-engine cleanup) is a deliberate follow-up, not this PR.What's here (reading order, oldest → newest)
thread the duplication-ladder recorder through the chain dedup MiAssign step— the dormant chain dedup builder carried--metricsand--family-size-histogrambut not--duplication-ladder; repointing dedup would have silently dropped the ladder. The ladder is order-sensitive (it samples a saturation curve at cumulative-template intervals), so it is recorded in the serialMiAssignstep — the same coordinate-order seam the non-chain path uses — not the parallel serialize step.MiAssignisSerial+ByItemOrdinal, so batches arrive in input-record order and groups within a batch are coordinate-ordered, reproducing the non-chain per-group stream exactly. The finalize hook reads the recorder through the lock (notArc::try_unwrap), holds the(path, recorder)pair in oneOptionso the invariant is in the type, and now also emits the"Filtered out N templates before marking"diagnostic the non-chain path emits (it must not be--metrics-only). PromotesDuplicationLadderRecorder/write_duplication_laddertopub(crate)and addsClonetoMarkDuplicates.repoint dedup --threads onto the chain builder— the pilot:execute_chain+ an early--threadsdispatch placed after the reader-free pre-flight validations (which run on both paths) but before the timer/banner/reader (whichadd_dedupre-emits, and pre-opening would consume stdin, breaking stdin +--threads). The--no-umioverride log is gated to the non-chain path so--threadsdoesn't double-log it. Adds the parity tests below.Validation
The pilot adds tests that pin behaviors the non-chain oracle cannot see:
test_dedup_chain_matches_single_threaded—--threads 1and--threads 4output record-for-record identical to the non-chain path.test_dedup_chain_matches_non_chain_across_knobs— the CLI-defaultadjacency,edit,--remove-duplicates,--no-umi, and--min-map-q(filtering the fixture's mapq-10 subfamily) all match the non-chain path, on mixed-UMI input so adjacency/edit cluster non-trivially rather than collapsing to identity; the@HDsort-order header is compared too (the@PGcommand-line field legitimately differs between the two invocations).test_dedup_mixed_umi_fixture_distinguishes_adjacency_from_identity— a non-vacuity guard: adjacency must produce different output than identity on that fixture, else the adjacency parity cases pass vacuously.test_dedup_threaded_crc_policy—--no-check-crc/--check-crc/ default CRC policy honored on the chain path; the accept case asserts record identity against an intact-file baseline, not merely non-empty.test_dedup_threaded_duplication_ladder_parity— the order-sensitive--duplication-ladder(plus--metrics,--family-size-histogram) file is byte-identical between the chain and non-chain paths. Runs at--threads 4with several hundred non-uniform position groups so a reordering regression in the ladder recording actually diverges the file (a--threads 1or uniform-size test could not catch it).Beyond the suite (9,481 tests green), the
dedupoutput was checked at scale against the non-chain engine on a 2.59M-record template-coordinate BAM (1.29M templates, 95% duplicate rate, 558K filtered): single-threaded and--threads 8, identity and default adjacency — output records identical (2,586,791 each, re-sorted to coordinate/mi then content-compared), and the duplication ladder, metrics, and family-size histogram byte-identical in every case; the restored filtered-templates diagnostic was confirmed emitted byte-identically on both paths at scale. The branch also went through two rounds of adversarial multi-lens review plus a scoped CodeRabbit-style pass; the most substantive finding was the dropped filtered-templates diagnostic (restored and extracted into a shared helper both paths call, so they cannot drift), and the second round's findings were all test-coverage gaps, now closed by the cases above.Deferred / follow-ups (not in this PR)
Merge gate
Same release gate as #870/#872: the fgumi-benchmarks AWS run (fgbio equivalency + WES/WGS scale). This PR changes only the
dedup --threadspath's internal construction (proven record-identical at scale) and adds tests.Risk: threaded
dedupoutput changes, but parity and byte-identical tests pin grouping, corrected UMIs, metrics, histograms, duplication ladders, and BAM output;unsafe: none, with no allowlist update; memory, queue capacity, and thread/backpressure policy: none.Fix: route
dedup --threads NthroughChainSpecwhile preserving the serial path.MiAssign.