Repository navigation
fix(duplex-metrics): drop dense (max_ab × max_ba) grid that OOMs on cfDNA hot spots - #359
Merged
Merged
Conversation
…clones `process_coordinate_group` allocated two fresh `HashMap`s on every iteration of its 20-fraction inner loop — `ss_groups` and `ds_groups`, the latter containing a per-template `Vec<(&str, &str)>` — and at the 100% fraction additionally cloned every `(mi, rx)` pair into an owned `Vec<(String, String)>` before invoking `update_umi_metrics`. On real cfDNA-class inputs with millions of coordinate groups this dominates the allocator pressure inside `fgumi duplex-metrics` and is sufficient to OOM a 30 GiB instance partway through the metrics pass on samples whose simplex-metrics counterpart finishes comfortably. Changes, all confined to `src/lib/commands/duplex_metrics.rs`: * Hoist `downsampled`, `ss_groups`, and `ds_groups` out of the per-fraction loop and reuse them with `.clear()` between iterations, mirroring the pattern already used by `simplex_metrics::process_coordinate_group` (which is why simplex metrics has not exhibited the same OOM behaviour). * Move the `is_full_fraction` flag above the `ds_groups` population so we can skip `entry.2.push(...)` on the 19 downsampled fractions; the per-template `(mi, rx)` pair vec is consumed exclusively by `update_umi_metrics` at the 100% fraction. * Change `update_umi_metrics` to take `&[(&str, &str)]` and drop the ad-hoc `Vec<(String, String)>` clone at the call site — the underlying `String` storage lives in `group`, whose lifetime outlasts the call. No behavioural change is intended: the existing 20 duplex_metrics and 7 simplex_metrics tests all pass unchanged, and the metrics outputs should be byte-identical to the previous implementation.
The previous comment overstated the reuse — `HashMap::clear()` preserves the outer bucket array but still drops each entry's inner `Vec<(&str, &str)>`. Combined with the `is_full_fraction` gate, that inner Vec now allocates exactly once per coordinate group rather than once per (fraction × `or_default()` slot), but the bucket array is the dominant per-group win — not the inner Vec capacity. No code change.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughReuses downsampling scratch buffers and gates DS MI/RX pair collection to the 100% fraction; changes update_umi_metrics to accept borrowed &[(&str,&str)] and uses borrowed comparisons for duplex-U error counting. Replaces flattened-family ideal-duplex calculation with a per-size weighted helper. Replaces dense 2D suffix-sum in duplex-family-size metrics with a sparse snapshot-and-filter approach and adds a regression test for sparse large entries. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #359 +/- ##
=======================================
Coverage 90.97% 90.97%
=======================================
Files 78 78
Lines 51017 51023 +6
=======================================
+ Hits 46412 46418 +6
Misses 4605 4605 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…fDNA hot spots `DuplexMetricsCollector::duplex_family_size_metrics()` computed the 2D cumulative `fraction_gt_or_eq_size` by materializing a dense `(max_ab + 1) × (max_ba + 1)` `Vec<usize>` and running 2D suffix sums across it. The underlying `duplex_family_sizes: HashMap<(usize, usize), usize>` is sparse — well under a thousand distinct `(ab, ba)` pairs even on cfDNA-scale inputs — but the grid is sized by the *maxima* of the two dimensions, which on real cfDNA where the hottest duplex family has tens of thousands of reads per strand explodes to billions of cells. A single sample with `max_ab = max_ba ≈ 50_000` allocates ~20 GiB before any metric is computed, OOM-killing a 30 GiB `c7g.4xlarge` `fgumi duplex-metrics` worker partway through the per-collector yield-metric pass. This is the real cause of the OOM behaviour observed on the `vendor` benchmark profile across all five duplex-capable samples (`idt-cfdna`, `kapa-umi`, `agilent-hs2`, `schmitt-abl1`, `twist-umi`). The previous commit in this branch (the per-coordinate-group HashMap hoist) did not move the needle on this OOM because the kill happens *after* the streaming pass — observably, the same template count (1,252,712 for idt-cfdna) is processed and the kill lands at the same point in the call graph both before and after that change. The hoist is a real micro-perf improvement and stays, but it is not the OOM fix. Changes: * `crates/fgumi-metrics/src/duplex.rs::duplex_family_size_metrics` — replace the dense-grid 2D suffix-sum with a sparse `O(N²)` iteration over the actual `(key, count)` pairs, where `N` is the number of distinct `(ab, ba)` entries in the HashMap (typically <1000, dwarfed by upstream I/O cost). No more `max_ab × max_ba` allocation. * `src/lib/commands/duplex_metrics.rs::generate_yield_metric` and `calculate_ideal_duplex_fraction_per_size` (new) — stop expanding `family_size_metrics` into a flat `Vec<usize>` of `m.family_size` repeated `m.ds_count` times before binomial evaluation. The new helper iterates per-size buckets and weights each size's probability by its `ds_count`, which (a) cuts the flat-Vec allocation, and (b) computes each `Binomial::cdf` exactly once per distinct family size rather than `ds_count` times. Mathematically equivalent. No behavioural change in any output column. The existing 20 `duplex_metrics` tests and 106 `fgumi-metrics` tests all pass on the post-fix tree; in particular `test_generate_accurate_duplex_family_size_counts` and `test_duplex_umi_expected_fraction_calculation` exercise both the 2D cumulative and the ideal-fraction paths end-to-end.
Add `test_duplex_family_size_metrics_sparse_hot_spot` which builds a collector with one duplex family at `(50_000, 50_000)` alongside two small entries. Under the previous dense-grid implementation this test would have allocated `~20 GiB` of scratch memory before any assertion ran; under the sparse implementation it completes in microseconds. The behavioural assertions on `fraction_gt_or_eq_size` for each of the three sparse entries pin the cumulative-fraction logic against any future refactor that might silently reintroduce an `O(max_ab × max_ba)` allocation. Catches the class of bug fixed by the previous commit without itself being implementation-bound.
nh13
changed the title
Member
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
2 tasks done
Merged
This branch was previously deployed
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.
What this fixes
fgumi duplex-metricsOOMs on real cfDNA-class inputs (vendor-profile samples —idt-cfdna,kapa-umi,agilent-hs2,schmitt-abl1,twist-umi) on a 30 GiBc7g.4xlarge. The same machine processes the simplex-metrics counterpart of the same files without issue.The OOM happens after the streaming pass: every affected job logs
Processed N templatesand then gets OOM-killed during the per-collector yield-metric pass, at the exact same template count run after run.Real cause
DuplexMetricsCollector::duplex_family_size_metrics()computed the 2D cumulativefraction_gt_or_eq_sizeby materializing a dense(max_ab + 1) × (max_ba + 1) × usizegrid and running 2D suffix sums over it. The underlyingduplex_family_sizes: HashMap<(usize, usize), usize>is sparse — typically well under a thousand distinct(ab, ba)pairs even on cfDNA-scale inputs — but the grid is sized by the maxima of the two dimensions. On a cfDNA sample whose hottest duplex family lands in the tens of thousands of reads per strand, this allocates billions of cells (max_ab = max_ba = 50_000→ ~20 GiB scratch) before any metric is computed. With 20 collectors andgenerate_yield_metriccallingduplex_family_size_metrics()per collector, you OOM before producing any output.Changes
crates/fgumi-metrics/src/duplex.rs::duplex_family_size_metrics— replace the dense-grid 2D suffix-sum with a sparseO(N²)iteration over the actual(key, count)HashMap entries, whereNis the number of distinct(ab, ba)pairs (typically <1000, dwarfed by upstream I/O cost). No moremax_ab × max_baallocation.src/lib/commands/duplex_metrics.rs::generate_yield_metric+ newcalculate_ideal_duplex_fraction_per_size— stop expandingfamily_size_metricsinto a flatVec<usize>ofm.family_sizerepeatedm.ds_counttimes. Iterate per-size buckets and weight each size's binomial probability by itsds_count. Mathematically equivalent; eliminates the flat-Vec allocation and theO(ds_count)redundantBinomial::cdfcalls per distinct family size.src/lib/commands/duplex_metrics.rs::process_coordinate_group— hoistss_groupsandds_groupsscratch HashMaps out of the 20-fraction loop and reuse via.clear(); skipentry.2.push(...)on non-full fractions; switchupdate_umi_metricsto&[(&str, &str)]to drop the owned-Stringclone at the call site. These are the original commits in this PR — they're real micro-perf improvements (mirroring the existing pattern insimplex_metrics::process_coordinate_group) but did NOT move the needle on the OOM, because the OOM is downstream of the streaming pass. They stay because they reduce allocator churn on the hot path.Behaviour
No behavioural change in any output column. Verified by:
duplex_metrics::testspass on the post-fix tree.fgumi-metrics::testspass (was 106; the new regression test below adds one).simplex_metrics::testspass (unchanged file, sanity check on the shared metrics path).cargo ci-fmt,cargo ci-lintclean.Regression test
test_duplex_family_size_metrics_sparse_hot_spotincrates/fgumi-metrics/src/duplex.rsconstructs a collector with one duplex family at(50_000, 50_000)alongside two small entries. Under the previous dense-grid implementation this would have allocated ~20 GiB before any assertion ran; under the sparse implementation it completes in microseconds. Pins the invariant against any future refactor that might silently reintroduce anO(max_ab × max_ba)allocation.Note on simplex-metrics
Reviewer asked whether simplex-metrics has the analogous bug. No — simplex-metrics has no equivalent 2D
(ab, ba)cumulative structure (itsfamily_size_metricsis 1D over1..=max_size, which is bounded bymax_size × ~80Bper call). The 1D densification pattern is shared between duplex and simplexfamily_size_metrics, but at realisticmax_sizeit does not OOM. Left untouched in this PR.Test plan
cargo test --release --lib duplex_metrics— 20 passedcargo test --release --lib simplex_metrics— 7 passedcargo test --release -p fgumi-metrics— 107 passed (incl. new sparse-hot-spot regression)cargo ci-fmt,cargo ci-lintcleanvendorbenchmark on cfDNA-class inputs against the prior broken build reproduced the OOM at the same template count; re-running with this PR's bundled into2fdcc7breproduced the OOM at the same point (confirming the original hoist-only fix was insufficient) — that's the empirical evidence behind this follow-up fixvendorprofile against this PR head after a fresh submodule bump to confirmduplex_metrics_fgumino longer OOMs on any of the 5 duplex-capable vendor samplesCommit sequence
4649c11perf(duplex-metrics): hoist scratch maps and drop unnecessary string clones— micro-perf; not the OOM fix2fdcc7bdocs(duplex-metrics): clarify what HashMap::clear actually reuses— comment-accuracy nit from initial reviewf6a1174fix(duplex-metrics): drop dense (max_ab × max_ba) grid that OOMs on cfDNA hot spots— the actual OOM fix7460059test(duplex-metrics): pin the sparse-hot-spot regression— guards against re-introducing dense allocation