Repository navigation
fix: dedup --no-umi OOM on production WES data - #231
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #231 +/- ##
==========================================
+ Coverage 88.27% 88.31% +0.03%
==========================================
Files 113 113
Lines 53215 53322 +107
==========================================
+ Hits 46977 47090 +113
+ Misses 6238 6232 -6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 5 minutes and 3 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the 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. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis change refactors dedup metrics collection from lock-free concurrent queues to mutex-guarded aggregation, consolidating per-group metrics directly during pipeline execution. It adds a regression test for large position-group deduplication without UMI tags. Across the unified pipeline (BAM, FASTQ, and base modules), the change removes drain-mode exceptions to memory backpressure enforcement, ensuring consistent backpressure behavior throughout pipeline execution. Documentation is updated to clarify that drain mode represents "input exhausted, completing remaining work" without special memory handling rules. 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/lib/unified_pipeline/base.rs (1)
4723-4734: Tighten theProcessPipelineStatecontract.The trait still advertises drain-specific backpressure semantics, but
shared_try_step_process()no longer consultsis_draining(). That stale contract will mislead the next impl or refactor.✏️ Suggested doc cleanup
- /// Check if backpressure should be applied before processing new work. - /// Returns true if queue is full OR memory is high (unless draining). - /// Default: just checks queue capacity (backwards compatible). + /// Check if backpressure should be applied before processing new work. + /// Returns true if queue is full OR memory is high. + /// Default: just checks queue capacity for backwards compatibility.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/unified_pipeline/base.rs` around lines 4723 - 4734, The trait ProcessPipelineState advertises drain-specific backpressure semantics that are stale because shared_try_step_process() no longer consults is_draining(); update the trait docs and methods to match actual behavior by removing or revising drain-related wording and behavior: either delete the unused is_draining() method from the ProcessPipelineState trait (and any impls), or change its documentation and the should_apply_process_backpressure() docstring to state that backpressure is determined only by process_output_is_full() (and is_draining is not consulted), and update any default implementations/comments in should_apply_process_backpressure() and is_draining() to avoid misleading callers (referencing the ProcessPipelineState trait, should_apply_process_backpressure(), is_draining(), and shared_try_step_process()).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/lib/unified_pipeline/base.rs`:
- Around line 4723-4734: The trait ProcessPipelineState advertises
drain-specific backpressure semantics that are stale because
shared_try_step_process() no longer consults is_draining(); update the trait
docs and methods to match actual behavior by removing or revising drain-related
wording and behavior: either delete the unused is_draining() method from the
ProcessPipelineState trait (and any impls), or change its documentation and the
should_apply_process_backpressure() docstring to state that backpressure is
determined only by process_output_is_full() (and is_draining is not consulted),
and update any default implementations/comments in
should_apply_process_backpressure() and is_draining() to avoid misleading
callers (referencing the ProcessPipelineState trait,
should_apply_process_backpressure(), is_draining(), and
shared_try_step_process()).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b8dd1e28-08be-46a5-9bf3-631393aff8e8
📒 Files selected for processing (4)
src/commands/dedup.rssrc/lib/unified_pipeline/bam.rssrc/lib/unified_pipeline/base.rssrc/lib/unified_pipeline/fastq.rs
…Queue Replace SegQueue<CollectedDedupMetrics> with Mutex<CollectedDedupMetrics> and merge metrics in-place during the serial Serialize step. This eliminates unbounded memory growth from accumulating per-position-group metrics for the entire pipeline run. Also removes unnecessary .clone() calls on dedup_metrics and family_sizes.
Remove the is_draining() bypass on Q5 memory backpressure in the Process step. Previously, once the Read step completed, the Process step could push unlimited data to Q5 with no memory governor, causing OOM on large inputs. The slot-based is_full() check still guarantees forward progress during draining, so removing the memory bypass is deadlock-safe.
Verify that dedup handles a position group with 5000 templates at the same position without unbounded memory growth. Exercises the --no-umi code path that was OOM-ing on production WES data.
This branch was previously deployed
1 participant
Summary
SegQueue<CollectedDedupMetrics>withMutex<CollectedDedupMetrics>and merge metrics incrementally during the serialize step, eliminating unbounded memory growth from per-position-group metric accumulationis_draining()bypass on Q5 memory backpressure in the Process step (bam, fastq, and base pipelines). Previously, once the Read step completed, the Process step could push unlimited data to Q5 with no memory governor, causing OOM on large inputs--no-umimodeContext
fgumi dedup --no-umiwas OOM-killed on a production WES BAM (1000 Genomes HG00100, 219M records, 13 GB) on AWS c7g.4xlarge (32 GB RAM) at ~40M records. Two root causes:The
SegQueue<CollectedDedupMetrics>accumulated one entry per position group for the entire pipeline run, only draining after completion — hundreds of MB on large inputs.The Q5 (processed queue) memory backpressure threshold (256 MB) was completely bypassed during draining mode. WES data has extreme depth pileups at capture targets creating massive position groups. With 128 queue slots and no memory limit, Q5 could accumulate many GB.
Validation
After fix, the same 219M-record BAM completes in 70 seconds with 11.8 GB peak RSS (8 threads), well within the 32 GB instance. Previously it was OOM-killed and never completed.
For comparison on the same data:
samtools markdupcompletes in 170s / 3.4 GB RSS, GATKMarkDuplicatesin 1687s.Test plan
cargo nextest run)cargo ci-fmt && cargo ci-lintclean--no-umi