fix(compare): compare a displaced sort-key run by digest instead of aborting - #700
Conversation
|
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 (2)
WalkthroughSort verification replaces the pending-record failure with bounded exact matching and deterministic multiset-digest fallback. It reports digest-compared runs and omits unmatched read names for degraded runs. ChangesSort verification digest fallback
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant BAMInputs
participant sort_verify_compare_opened_with_window
participant run_full_verify
participant RunCanceller
participant SortVerifyOutcome
BAMInputs->>sort_verify_compare_opened_with_window: provide opened inputs
sort_verify_compare_opened_with_window->>run_full_verify: pass exact_window
run_full_verify->>RunCanceller: compare each sort-key run
RunCanceller->>RunCanceller: use exact matching or digest fallback
RunCanceller->>SortVerifyOutcome: return match status and digest count
Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
Comment |
|
@coderabbitai pause |
✅ Action performedReviews paused. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #700 +/- ##
========================================
Coverage 94.06% 94.06%
========================================
Files 178 178
Lines 108601 108835 +234
========================================
+ Hits 102153 102379 +226
- Misses 6448 6456 +8 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Validated this branch against the actual #699 inputs — the 77,270,534-record unmapped tail, not a fixture. It fixes it, and the constant-memory claim holds at 6.4x the scale of the PR's own reproducer. SetupThe two BAMs from the run that produced #699: GIAB HG002 60x (1,327,219,598 records) sorted independently by Built from Result
What this adds over the PR's own numbersThe fixture in the PR is 11,964,096 records (~6M peak pending) and measured 985 MB. This is 77,270,534 records, ~38M peak pending — 6.4x the records, ~6x the peak displacement — and peak RSS came in at 969.7 MB, marginally below the fixture's. Flat across a 6.4x scale-up is the evidence the constant-memory argument needed; the fixture alone couldn't distinguish "constant" from "grows slowly". Steady-state RSS during the run sat around 22 MB, so the ~970 MB is a transient at the fold rather than a sustained footprint. Exactly one run degraded, and it is the right one. The unmapped tail is 5.8% of this file; every mapped run stayed on the exact path and kept its diagnostics. That is The reported degradation line matters as much as the verdict here: an On the exactness trade-offAgree with the framing in the PR. For our use — a benchmark suite checking that two sorters agree — a probabilistic verdict on the unmapped tail specifically is the right trade: those records have no meaningful order to disagree about, so the exact path was paying O(displacement) memory to verify something that isn't part of the contract. The mapped runs, where a real regression would show up, are still exact. Worth noting for anyone sizing a reproducer later: the scale note in the PR is correct and easy to get wrong. Our first attempt at reproducing #699 on a 1.33M-record tail cleared neither cap and showed nothing. |
7585aba to
1399b02
Compare
5862032 to
5483108
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@src/lib/commands/compare/engines/sort_verify.rs`:
- Around line 1610-1629: Add a new test alongside the existing
the_digest_decides_a_mismatched_run case that keeps the same record count and
read name(s) on both sides but changes one record’s content, so the mismatch is
only detectable through compare_via_digest/MultisetDigest rather than counts
alone. Use the existing helpers in sort_verify.rs such as distinct_records and
compare_via_digest, and assert the residual is compared_by_digest and not a
match to prove the digest keys on canonical content rather than the read name.
- Around line 828-839: Update Canceller::degrade_to_digest so an existing
CancellerState::Digest is preserved instead of being replaced with empty
digests. Keep the Exact conversion through exact.into_digests(), and return the
already-degraded state unchanged for the Digest match arm, preserving the
documented no-op behavior.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6d98472d-46f6-46c3-b3f6-c6f1ab9d3b42
📒 Files selected for processing (2)
src/lib/commands/compare/bams.rssrc/lib/commands/compare/engines/sort_verify.rs
1399b02 to
f6a7f76
Compare
5483108 to
3558317
Compare
f6a7f76 to
2b68b78
Compare
3558317 to
f4f648b
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@src/lib/commands/compare/engines/sort_verify.rs`:
- Around line 515-539: Update the documentation above DIGEST_HASHERS to narrow
its determinism claim: describe reproducibility only within the same ahash build
and runtime path, and explicitly state that digest values must not be persisted
or compared across architectures, builds, or releases. Keep the fixed-seed
explanation and distinct-hasher requirement intact.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3c180ac9-1c3f-44b1-b3ed-2d89d275a8f2
📒 Files selected for processing (2)
src/lib/commands/compare/bams.rssrc/lib/commands/compare/engines/sort_verify.rs
…borting `compare bams --command sort` compares each equal-core-sort-key run as a multiset via `RunCanceller`, holding unmatched records keyed by canonical content key until their counterparts arrive. Pending therefore grows with the displacement between the two files' orderings, and exceeding 5M records was a hard error. A BAM's both-ends-unmapped tail is a single run — every such record packs to one constant coordinate key, and to one core template-coordinate key — and two independently-produced sorts order that tail arbitrarily differently. On a whole-genome BAM the tail is tens of millions of records (77.3M of 1.33B in #699), so the comparison aborted. The error's advice, to re-sort both inputs into a common order, is circular in the case that matters: the two inputs are the outputs of the two sorters under comparison, so there is no third order to normalize to, and re-sorting would destroy the property being verified. Rather than abort, `RunCanceller` now degrades. On the observation that pushes pending past the window, both pending sets are folded into per-side `MultisetDigest`es — a commutative fold of count, wrapping sum and xor over a 128-bit hash of each record's canonical content key — the maps are dropped, and the rest of the run accumulates into those digests in constant memory. This is sound because the comparison decides the symmetric difference of the two sides' multisets: each cancellation removed one element from each side, so the digests cover exactly the uncancelled remainders, which are equal iff the full multisets are. Work done before the fallback carries across it rather than being redone. All three digest fields are load-bearing. Count alone catches a missing record; xor alone does not, since it is self-inverse and {A,A,B} and {B,C,C} share both a count and an xor; sum separates those. The result is a probabilistic rather than an exact multiset equality — two different runs agreeing on all three fields would be reported equal, a ~2^-128 event per run — which is the price of comparing a 77M-record run at all rather than aborting on it. The window drops from 5M to 1M. It no longer bounds correctness, only diagnostics: within it a mismatched run still names the reads that went unmatched, and past it the run is reported by count with an explicit note that no reads can be named. Runs finished by digest are counted and reported, so an IDENTICAL verdict that rested on a digest is visible as such. The window is threaded through `VerifyContext` so tests can reach the fallback with a handful of records; it is deliberately not a CLI flag, since exceeding it is no longer something the caller must act on. Validated on the real shape: 11,964,096 both-ends-unmapped records taken from 1000 Genomes HG00096, sorted independently by `fgumi sort` and `samtools sort --template-coordinate` into fully divergent orders (the first record of one file sits 557,077 records into the other). Before, this aborted at 4.6 GB peak RSS; after, it reports IDENTICAL at 985 MB. A dropped record and a single flipped base are both still reported as DIFFER — the latter with equal record counts on both sides, so only the digest could have caught it. Closes #699.
f4f648b to
d44554e
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
This branch was previously deployed
1 participant
Closes #699. Stacked on #696 — review that first; this PR's diff is against its head (
7585aba3).The problem
compare bams --command sortcompares each equal-core-sort-key run as a multiset.RunCancellerholds unmatched records keyed by canonical content key until their counterparts arrive, so pending memory grows with the displacement between the two files' orderings — and past 5M records it was a hard error.A BAM's both-ends-unmapped tail is a single run: every such record packs to one constant coordinate key, and to one core template-coordinate key. Two independently-produced sorts order that tail arbitrarily differently, and on a whole-genome BAM the tail is tens of millions of records — 77.3M of 1.33B in #699. So the comparison aborted.
The error's advice was to re-sort both inputs into a common order. That is circular in exactly the case
--command sortexists for: the two inputs are the outputs of the two sorters under comparison. There is no third order to normalize to, and re-sorting would destroy the property being verified.The fix
RunCancellerdegrades instead of aborting. On the observation that pushes pending past the window, both pending sets are folded into per-sideMultisetDigestes, the maps are dropped, and the rest of the run accumulates into those digests in constant memory.A
MultisetDigestis a commutative fold — count, wrapping sum, and xor over a 128-bit hash of each record's canonical content key. Commutativity is the whole point: it is what makes the accumulated value independent of arrival order, which is what the exact canceller was paying O(displacement) memory to achieve.Why the fallback is sound
The comparison decides the symmetric difference of the two sides' record multisets. Writing
AandBfor the two sides' records andCfor the pairs already cancelled — each of which removed one element fromAand an equal element fromB— the digests cover exactlyA \ CandB \ C, andA == BiffA \ C == B \ C.So the fallback is a change of representation mid-run, not a restart: a partially-displaced run still cancels exactly everything it can before degrading, and that work carries across. Pinned by
cancellations_made_before_degrading_survive_it.The key material is
content_key_exact, unchanged from #696. Its byte-equality already is exact content-equality (there's a proptest pinning that), so hashing it inherits the contract rather than inventing a weaker one. Read names never enter the comparison — they aren't unique across mates and supplementaries, so they'd be unsound as identity.Why all three digest fields
countalone catches a missing or extra record.xoralone does not: it is self-inverse, so{A,A,B}and{B,C,C}share both a count and an xor while being different multisets.sumseparates those, since duplicates add rather than annihilate;xorin turn covers sums that coincide modulo 2¹²⁸.digest_distinguishes_xor_cancelling_multisetspins this, and it is genuinely load-bearing — deleting thesumupdate makes it fail.The guarantee this changes
This trades exactness for constant memory, and that is worth stating plainly. Digest equality is probabilistic: two genuinely different runs agreeing on count, sum and xor would be reported equal. At 128 bits that is ~2⁻¹²⁸ per run and not reachable by construction from non-adversarial BAM data — but it is not zero, and
--command sortis a validation oracle, which is the one place a false MATCH costs most.The alternative on the table was aborting on the input entirely. Exact-at-any-displacement would mean spilling the pending set to disk (#699's option 3), which is a lot of machinery for a feature-gated developer tool; I'd rather add it later if a false MATCH ever plausibly bites than build it speculatively now.
Runs that degrade are counted and reported, so an
IDENTICALverdict that rested on a digest is visible as one rather than silently indistinguishable from an exact match.Window: 5M → 1M
The window no longer bounds correctness, only diagnostics — within it a mismatched run still names the reads that went unmatched; past it the run is reported by count with an explicit note saying why no reads are named. So it is now sized to cover any plausible genuine difference rather than any plausible displacement, and 5M pending content keys was several GB of maps for no remaining benefit.
It is threaded through
VerifyContextso tests can reach the fallback with a handful of records instead of a million. It is deliberately not a CLI flag (#699's option 2): exceeding it is no longer something the caller has to do anything about.Validation on the real shape
11,964,096 both-ends-unmapped records taken from 1000 Genomes HG00096, sorted independently by
fgumi sort --order template-coordinateandsamtools sort --template-coordinate. The two orderings are fully divergent — the first record of one file sits 557,077 records into the other.7585aba3)Error: ... more than 5000000 records held pendingRESULT: BAM files are IDENTICALBoth arms then re-run against a deliberately corrupted copy, and both are still caught:
DIFFER(counts 11964096 vs 11964095)DIFFERwith equal record counts on both sides, so only the digest could have found it — one altered base among 11.96M recordsA note on scale, since it is easy to under-size this reproducer: a fully-scrambled run of N records peaks at only ~N/2 pending, so the 1.33M-record tail I started with cleared neither cap and reproduced nothing. #699's 77.3M tail peaks around 38M. That is why the fixture is ~12M.
Tests
digest_and_exact_verdicts_agree(proptest) — the load-bearing one. For arbitrary multiset pairs, the verdict withwindow = 0(everything through the digest) must equal the verdict withwindow = usize::MAX(never degrades). A digest collision, a record lost across the fold, or a mishandled multiplicity all fail this.a_run_past_the_window_is_compared_by_digest_rather_than_aborting— the compare bams --command sort: cannot compare two template-coordinate sorts with a large unmapped tail #699 regression at the canceller level.a_reordered_unmapped_tail_compares_identical_via_the_digest— the same, end to end through the engine.a_difference_inside_a_degraded_run_is_reported_without_names— a degraded run still catches a real difference, and says why it names nothing.a_degraded_run_does_not_cost_other_runs_their_diagnostics— the fallback is per-run: a huge unmapped tail must not cost the mapped runs their read names.pending_never_exceeds_the_window— bounded memory asserted directly rather than inferred from RSS.the_digest_decides_a_mismatched_run,digest_distinguishes_xor_cancelling_multisets,cancellations_made_before_degrading_survive_it.Existing exact-mode residual-naming tests are unchanged and still pass — that's the regression guard on the common path. Full suite: 6916 passed.
Notes for reviewers
RunResidualis now an enum (Exact { .. }/Digest { matched }), andis_empty()becameis_match()— "empty" stopped describing the digest arm.RunCanceller::observe_bam1/observe_bam2collapsed intoobserve(Side, ..)and became infallible. Removing the error was the point;compare_runloses two?.RandomState::new(). A comparison oracle that could change its mind about the same pair of files between invocations would be worse than no oracle, so the verdict has to be reproducible across runs, hosts and architectures.SortVerifyOutcomegainsruns_compared_by_digest, which deliberately does not feedis_match()— degrading is not an error.Risk: sort comparison output adds deterministic digest-comparison reporting; no grouping, consensus, sort-order, corrected-UMI, or metrics output changes; no
unsafechange and no CLAUDE.md allowlist change; exact pending-record tracking is bounded with constant-memory digest fallback.When the exact window is exceeded,
sort_verifypreserves prior cancellations and switches to a deterministic 128-bit multiset digest. The digest uses a fixed seed and tracks record count, wrapping sum, and XOR of canonical record-content hashes. Digest comparisons preserve multiplicity but cannot identify unmatched records.The change adds degraded-run reporting through
runs_compared_by_digestand retains exact diagnostics for non-degraded runs. Tests cover digest behavior, bounded memory, residual diagnostics, and the large unmapped-tail regression. Full validation passed 6,916 tests.