feat(compare): harden fgumi compare into a sound, faithful fgbio-parity oracle - #530
Merged
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use 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 (10)
WalkthroughReplaces legacy BAM modes with content, grouping, and sort-verification engines. Metrics comparison now uses configurable outer key joins. Header compatibility, typed predicates, structured outcomes, CLI presets, documentation, integration coverage, and determinism checks are expanded. ChangesComparison engine overhaul
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant CompareBams
participant Engine
participant BAMFiles
CLI->>CompareBams: select command preset
CompareBams->>BAMFiles: read headers and records
CompareBams->>Engine: run content, grouping, or sort verification
Engine->>CompareBams: return structured outcome
CompareBams->>CLI: print IDENTICAL or DIFFER
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #530 +/- ##
==========================================
+ Coverage 93.00% 93.38% +0.37%
==========================================
Files 167 175 +8
Lines 103262 104539 +1277
==========================================
+ Hits 96041 97620 +1579
+ Misses 7221 6919 -302 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This was referenced Jul 10, 2026
Member
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
Member
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
✅ 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: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/compare-cli.md (1)
82-98: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDo not present non-presets as
--commandpresets.
clip,downsample, andreviewappear in this preset-settings table but are not accepted by the documented/runtime--commandpreset list. Remove them from the preset table, label them as manual mode guidance, or add corresponding CLI presets.🤖 Prompt for 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. In `@docs/compare-cli.md` around lines 82 - 98, Update the command preset table near the documented Command Presets section so it only lists stages accepted by the documented/runtime --command preset list. Remove clip, downsample, and review from the preset rows, or clearly move them into separate manual-mode guidance; do not present them as --command presets unless corresponding CLI presets are added.
🤖 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 `@docs/compare-cli.md`:
- Line 48: Update the --ignore-order entry in docs/compare-cli.md to describe it
as applying only to grouping modes, replacing the incorrect reference to
parallel consensus output. Keep the existing flag type and default unchanged.
- Line 211: Update the “Comparing fgumi vs fgbio output” documentation to
distinguish tolerated row-order differences from missing or extra rows. State
that row-set differences are known parity gaps reported as DIFFER, while
preserving the existing note about floating-point representation differences.
In `@src/lib/commands/compare/engines/content.rs`:
- Around line 148-166: The tag comparison currently collapses duplicate non-MI
tags via BTreeMap, causing false matches. Replace tag_typed_map_excluding_mi and
the map-based logic in tags_match_excluding_mi with entry-level multiset
matching that filters both MI and tc tags while preserving duplicate
occurrences; add a regression test covering duplicate NM entries versus a single
NM entry.
In `@src/lib/commands/compare/metrics.rs`:
- Around line 225-269: Replace the fully materialized row and key-tree workflow
in parse and the keyed comparison flow with externally sorted temporary row
storage and a streaming merge of both inputs. Keep only bounded buffers in
memory, compare matching keys during the merge, and preserve duplicate-key
detection so repeated UMI keys remain an error rather than being silently
collapsed. Apply the same approach to the related key-joining logic in the
referenced ranges.
- Around line 505-509: Update the public compare_metrics boundary to validate
every tolerance in MetricsCompareConfig, rejecting negative or non-finite values
before comparison begins while preserving the existing Result-based error
handling for invalid configuration.
In `@tests/integration/test_compare_bams.rs`:
- Around line 60-84: Update run_compare_command and the analogous helpers
covering the referenced test ranges to return the subprocess ExitStatus and
captured stderr alongside stdout instead of reducing status to success(). Adjust
negative-test assertions to verify the expected exit code and diagnostic,
distinguishing comparison mismatches from argument-rejection failures.
- Around line 797-851: Strengthen
test_canonicalize_to_queryname_sorts_and_preserves_records and read_all_names
with an independent semantic fingerprint covering each record’s flags, loci,
sequence, qualities, tags, and complete queryname tie-break lanes. Expand the
fixture records to include duplicate QNAMEs spanning segment, secondary, and
supplementary cases, then compare the output fingerprints against the input
multiset and assert non-decreasing order using the full queryname sort key
rather than names alone.
In `@tests/integration/test_compare_metrics_command.rs`:
- Around line 139-152: Strengthen subprocess_quiet_suppresses_output_on_differ
by asserting stderr is empty, not merely that it lacks “Error:”. Update the
--quiet execution path to suppress all informational and warning logging so only
the exit code communicates the comparison result.
---
Outside diff comments:
In `@docs/compare-cli.md`:
- Around line 82-98: Update the command preset table near the documented Command
Presets section so it only lists stages accepted by the documented/runtime
--command preset list. Remove clip, downsample, and review from the preset rows,
or clearly move them into separate manual-mode guidance; do not present them as
--command presets unless corresponding CLI presets are added.
🪄 Autofix (Beta)
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: 84ba94c2-4cb1-4969-a8c9-f23b75846b60
📒 Files selected for processing (20)
docs/compare-cli.mdsrc/lib/commands/compare/bams.rssrc/lib/commands/compare/engines/content.rssrc/lib/commands/compare/engines/header.rssrc/lib/commands/compare/engines/keyjoin.rssrc/lib/commands/compare/engines/mod.rssrc/lib/commands/compare/engines/positional.rssrc/lib/commands/compare/engines/sort_verify.rssrc/lib/commands/compare/metrics.rssrc/lib/commands/compare/mod.rssrc/lib/commands/compare/raw_compare.rssrc/lib/commands/compare/record_key.rssrc/lib/commands/sort.rstests/integration/helpers/bam_generator.rstests/integration/helpers/mod.rstests/integration/main.rstests/integration/test_compare_bams.rstests/integration/test_compare_metrics_command.rstests/integration/test_compare_mutation.rstests/integration/test_e2e_regression.rs
nh13
force-pushed
the
nh/compare-hardening
branch
from
July 16, 2026 15:59
63d7d30 to
13ae4ac
Compare
Member
Author
|
Addressed this review round (pushed to Fixed (threads resolved):
Outside-diff comment (preset table, docs Deferred (thread left open): the metrics external-sort/memory-bound suggestion — QC metric TSVs are bounded and small (largest is All green locally: full test suite (2443 passed), |
Member
Author
|
@coderabbitai resume |
Member
Author
|
@coderabbitai review |
✅ Action performedReviews resumed. |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/commands/compare/raw_compare.rs (1)
190-220: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMatch duplicate tags by both name and value.
The first unused same-name entry is selected before value comparison. Thus
[NM=1, NM=2]falsely differs from reordered[NM=2, NM=1]. Search all unmatched same-name entries for a semantically equal value, then mark that candidate; add this reordered-duplicate regression.🤖 Prompt for 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. In `@src/lib/commands/compare/raw_compare.rs` around lines 190 - 220, Update the matching loop around entries1, entries2, and matched so it searches every unmatched entry with the same tag name and selects one whose value matches byte-for-byte or via integer semantic decoding, instead of returning on the first same-name mismatch. Mark only the successfully matched candidate and return false if no semantically equal unmatched candidate exists; add a regression test covering reordered duplicate values such as [NM=1, NM=2] versus [NM=2, NM=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 `@docs/compare-cli.md`:
- Around line 52-53: Update the scope descriptions for --sort-memory and
--sort-tmp-dir in the CLI documentation to include the explicit --mode grouping
--ignore-order path alongside --command group, making clear these settings
configure the key-join engine in both modes while preserving the existing
behavior details.
In `@src/lib/commands/compare/bams.rs`:
- Around line 605-607: Gate OperationTimer creation/completion and informational
logging in the compare command’s dispatch paths behind !self.quiet, including
execute_sort_verify and the additional referenced branches. Preserve all command
execution and exit-code behavior while ensuring quiet mode emits no timers or
info logs, matching the metrics command.
- Around line 645-660: Update the predicate selection around self.command and
mode so an explicit CompareMode::Content always uses ContentPredicate::Exact,
overriding any command preset such as group; retain preset predicates only when
content mode was not explicitly selected, and add a regression test covering
--command group --mode content with an MI-only difference that must not match.
In `@src/lib/commands/compare/engines/keyjoin.rs`:
- Around line 50-58: Replace the unconditional DEFAULT_DISK_BACKED_TMP_DIR
fallback with a platform-aware default that is valid on Windows and preserves
the disk-backed behavior on Unix-like systems. Update the related
temporary-directory resolution in the command-group comparison flow to verify
actual scratch-directory creation, not merely inspect the resolved path, before
passing it to RawExternalSorter. Add or update tests covering Windows-compatible
fallback selection and creation failure.
In `@src/lib/commands/compare/engines/sort_verify.rs`:
- Around line 122-158: Update detect_sort_order so SS validation checks the
complete value before selecting a comparator: accept only documented bare
subsort aliases or exact SO-prefixed forms matching the current SO, and reject
mismatched prefixes such as coordinate:natural under SO:queryname. Preserve the
existing SortOrder mappings and error behavior for unsupported SS values.
- Around line 187-257: The equal-key comparison currently permits unbounded
memory and quadratic CPU usage. Update pull_run and multiset_equal to compare
canonical record encodings using linear-time counting, and introduce a bounded
threshold that spills or externally sorts records when a run exceeds it;
preserve exact multiset semantics and avoid buffering an entire large run in
memory.
In `@src/lib/commands/compare/metrics.rs`:
- Around line 355-368: Update canonicalize_key_field and the precision-handling
logic in parse_value so non-finite multipliers or scaled float results never
canonicalize distinct finite keys to inf/NaN; preserve the original finite value
or reject unsafe rounding and leave the raw key representation unchanged. Apply
the same safeguard to the related call sites, and add a regression covering
high-precision keys with float-rounding overflow.
In `@src/lib/commands/compare/record_key.rs`:
- Around line 74-79: Update segment_of to handle the (true, true) FIRST|LAST
state explicitly instead of routing it through Segment::Fragment. Represent all
four flag combinations with distinct Segment variants, or reject the invalid
combined state, while preserving the existing First, Last, and Fragment behavior
for the other combinations.
In `@tests/integration/test_compare_mutation.rs`:
- Around line 524-546: Split
positional_consensus_cm_and_ce_differences_are_flagged into independent tests
that vary only cD, only cM, or only cE while keeping the other tags identical,
and assert each mutation produces DIFFER. Add a corresponding catalog entry for
each tag-specific test, including the related cases around the referenced
additional section, and strengthen any assertions that do not explicitly verify
the stated difference contract.
- Around line 1034-1046: Replace the self-maintained
REQUIRED_SUBSTRINGS/MUTATION_CATALOG completeness check with an oracle owned by
production comparison behavior. Export or expose the compared-dimension
inventory from positional_compare, compare_headers, keyjoin_compare,
sort_verify_compare, and CompareMetrics::execute, then have
mutation_catalog_covers_every_compared_dimension validate catalog coverage
against that inventory rather than only catalog fields or test-local closures.
---
Outside diff comments:
In `@src/lib/commands/compare/raw_compare.rs`:
- Around line 190-220: Update the matching loop around entries1, entries2, and
matched so it searches every unmatched entry with the same tag name and selects
one whose value matches byte-for-byte or via integer semantic decoding, instead
of returning on the first same-name mismatch. Mark only the successfully matched
candidate and return false if no semantically equal unmatched candidate exists;
add a regression test covering reordered duplicate values such as [NM=1, NM=2]
versus [NM=2, NM=1].
🪄 Autofix (Beta)
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: 5cc602f9-9027-484a-86c3-7c40f09ba3ae
📒 Files selected for processing (20)
docs/compare-cli.mdsrc/lib/commands/compare/bams.rssrc/lib/commands/compare/engines/content.rssrc/lib/commands/compare/engines/header.rssrc/lib/commands/compare/engines/keyjoin.rssrc/lib/commands/compare/engines/mod.rssrc/lib/commands/compare/engines/positional.rssrc/lib/commands/compare/engines/sort_verify.rssrc/lib/commands/compare/metrics.rssrc/lib/commands/compare/mod.rssrc/lib/commands/compare/raw_compare.rssrc/lib/commands/compare/record_key.rssrc/lib/commands/sort.rstests/integration/helpers/bam_generator.rstests/integration/helpers/mod.rstests/integration/main.rstests/integration/test_compare_bams.rstests/integration/test_compare_metrics_command.rstests/integration/test_compare_mutation.rstests/integration/test_e2e_regression.rs
nh13
force-pushed
the
nh/compare-hardening
branch
from
July 16, 2026 19:41
13ae4ac to
fe79633
Compare
✅ Action performedReview finished.
|
Member
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
Member
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/compare-cli.md (1)
146-148: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDocument tolerance-based equality, not only representation differences.
The implementation accepts any finite float difference within
--precision,--rel-tol, and--abs-tol; formatting differences are only the motivating use case. Reword this to avoid promising exact comparison for other numeric differences.🤖 Prompt for 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. In `@docs/compare-cli.md` around lines 146 - 148, Revise the comparison-behavior statement in docs/compare-cli.md to explain that finite float differences within --precision, --rel-tol, or --abs-tol are tolerated, rather than limiting tolerance to formatting differences. Preserve the statement that row sets and non-key values are compared, but avoid promising exact equality for numeric values covered by these tolerances.
🤖 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 `@docs/compare-cli.md`:
- Around line 184-193: Update the Float Comparison documentation in
docs/compare-cli.md to scope tolerance formulas to finite float values, and
explicitly document that NaN compares equal to NaN and positive or negative
infinity compares equal only to the same-sign infinity. Keep the existing mixed
integer/float special-value behavior unchanged.
In `@tests/integration/test_e2e_regression.rs`:
- Around line 278-300: Move the BAM byte-identical assertion documentation block
from above read_bam_header to directly above assert_bams_record_byte_identical,
leaving the read_bam_header and hd_ss documentation attached to their respective
helpers.
---
Outside diff comments:
In `@docs/compare-cli.md`:
- Around line 146-148: Revise the comparison-behavior statement in
docs/compare-cli.md to explain that finite float differences within --precision,
--rel-tol, or --abs-tol are tolerated, rather than limiting tolerance to
formatting differences. Preserve the statement that row sets and non-key values
are compared, but avoid promising exact equality for numeric values covered by
these tolerances.
🪄 Autofix (Beta)
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: a4d2cc52-8c5f-41bd-a492-c5a172b4b3db
📒 Files selected for processing (10)
docs/compare-cli.mdsrc/lib/commands/compare/bams.rssrc/lib/commands/compare/engines/header.rssrc/lib/commands/compare/engines/molecule_join.rssrc/lib/commands/compare/engines/sort_verify.rssrc/lib/commands/compare/metrics.rssrc/lib/commands/compare/molecule.rstests/integration/test_compare_bams.rstests/integration/test_compare_mutation.rstests/integration/test_e2e_regression.rs
values_equal compares Int/Int exactly but mixed Int/Float with the same rel/abs tolerance as Float/Float. The module doc, CLI long_about, and docs/compare-cli.md all previously said only "integers are compared exactly", which reads as covering the mixed case too. Spell out the three cases (int/int exact, mixed int/float tolerant, string exact) so the docs match values_equal's actual behavior. No implementation change.
sort_order_from_header's SO:coordinate arm ignored the @hd SS tag entirely, so a declared coordinate sub-order (e.g. a hypothetical coordinate:foo) would falsely validate as plain coordinate even though run_compare's equal-core-sort-key grouping only ever verifies (tid, pos, reverse) -- weaker than whatever the sub-sort would promise. Follow the existing SO:queryname arm's pattern: reuse ss_subsort to validate the whole SS value (not just its suffix) against SO:coordinate. No SS tag is still accepted (-> SortOrder::Coordinate); any sub-sort, recognized or not, now bails, since this engine implements none. SortOrder::header_ss_tag confirms fgumi itself never emits a coordinate SS, so this cannot reject fgumi's own output.
mutation_catalog_covers_every_compared_dimension's REQUIRED_SUBSTRINGS completeness list named cD but not cM/cE, even though the catalog has "cM difference flagged" and "cE difference flagged" entries -- so deleting those two rows would have silently passed the guard. Add both substrings alongside the existing cD entries. Verified empirically: temporarily deleting the cM/cE CatalogEntry blocks makes this test fail with "no mutation catalog entry covers required dimension \"cM difference flagged\"", confirming the guard actually catches the deletion.
… diffs compare_sq rendered BOTH entire @sq dictionaries into one diff string whenever they diverged at all, regardless of --max-diffs (which can't cap it since it's a single push_diff entry) -- a large reference dictionary (whole-genome assemblies routinely carry thousands of contigs/decoys/ALTs) allocated unbounded diagnostic text for even a single differing entry. Walk both dictionaries position-by-position instead: any divergence anywhere (name/length/order/M5/etc., or a one-sided length difference) is still counted and reported, but the rendered string shows only the first MAX_SQ_DIFF_ENTRIES (5) differing positions plus an "and N more differences" count. Existing differing_sq_* tests (which only assert `starts_with("@sq")`) stay green; added a regression test building a 5000-entry dictionary with one differing entry and asserting the diff string stays under 2KB, proving it's no longer O(dictionary size).
The same-seed determinism tests in test_e2e_regression.rs asserted identity via CompareBams "content" mode, which normalizes tag order, integer tag width, the tc tag, and @PG/@co (see raw_compare::content_key_exact) -- so a same-seed writer nondeterminism confined to exactly those dimensions would be masked and go undetected. Add assert_bams_record_byte_identical, modeled on the strip_bin/ read_all_records helpers from the pre-redesign key-join canonicalization test (removed by c156ba5 along with the key-join engine): it reads every record's raw on-disk bytes via fgumi_sort::RawBamRecordReader after skip_header(), zeroing only the non-semantic bin field (bytes 10-11). Header is excluded -- @pg carries per-run temp paths that legitimately differ run-to-run even for deterministic record output. Applied to all five same-seed determinism tests (sibling audit): test_simulate_grouped_reads_deterministic, test_simplex_pipeline_ deterministic, test_simplex_filter_pipeline_deterministic, test_full_pipeline_extract_to_filter, test_dedup_pipeline_deterministic. Added alongside (not replacing) the existing content-mode assertion, since content mode also exercises the full compare engine's header/ sort-order preconditions via an independent code path. Verified empirically before wiring this up broadly: ran the new assertion against all five tests repeatedly (4 full runs) and same-seed output was record-byte-identical every time -- no real nondeterminism found in tag order, integer tag width, or the tc tag for any of these pipelines.
require_compatible_headers previously collapsed both branches of the (Err, Err) sort_order_from_header case into a benign Ok(None), which was correct for genuinely orderless headers (extract/fastq/zipper output) but also silently swallowed headers that declare a recognized orderable SO (coordinate or queryname) with an unrecognized SS sub-sort. That let a header claiming a verifiable sort order skip order verification entirely instead of being rejected. Distinguish the two cases by reading the raw @hd SO tag directly: if either header's SO names coordinate or queryname, propagate the underlying error as a hard incompatibility; only fall back to the SO/GO byte comparison when neither side claims an orderable SO.
…conditions Close four soundness gaps in the streaming grouping comparison surfaced by review: - molecule_runs assumed same-MI-consecutive input but never checked it; a scattered MI base (records for one molecule split into two runs) now returns an Err instead of silently mis-grouping. - require_compatible_headers' (Ok, Ok) arm only compared the normalized SortOrder, so two headers agreeing on sort order but declaring different @hd GO tags passed the gate; it now applies the same compare_hd check the (Err, Err) fallback already did. - molecule_join_compare relied on its CLI caller having already run require_compatible_headers; it now enforces that precondition itself so the public API is sound for any caller. - The fully-MI-less guard only fired when *both* inputs were MI-less, so a partial-MI-less pair (one side grouped, the other not) whose single spanning run happened to canonical-id-match a real molecule on the other side reported a false MATCH. The guard now rejects either non-empty MI-less side unconditionally. Also updates the comment in bams.rs that claimed grouping mode needs no order validation at all -- it is order-independent across molecules, but molecule_runs now enforces same-MI-consecutive contiguity within each one.
…agnostics index_by_key collapsed same-RecordKey members of one molecule into a last-wins BTreeMap entry, so a duplicated or dropped record sharing a key with another (e.g. two same-name/same-segment primaries at different positions) could go unnoticed -- a genuine 3-vs-2 multiplicity difference reported MATCH. It now maps each RecordKey to all of its member records and compare_molecule compares per-key multiplicity, closing that false-MATCH avenue. The duplex strand-partition sets are now multisets for the same reason. compare_molecule used to build its full Vec<String> of diffs before the caller applied the max-diffs cap, so a hugely-divergent molecule allocated unbounded diagnostic strings even under --max-diffs 0. It now takes the cap and a diff sink directly, stops allocating past the cap, and returns an uncapped bool so the matched-molecule verdict stays cap-independent. The EOF residual drain iterated pending1/pending2 (AHashMap) via .drain(), so which residual ids survived the max-diffs cap and their reported order varied run to run. Residual canonical ids are now sorted before reporting.
… check The five same-seed determinism tests in test_e2e_regression.rs asserted both assert_bams_identical(..., "content", ...) and assert_bams_record_byte_identical(...). The byte-exact assertion is strictly stronger (content mode normalizes tag order, integer tag width, and the tc tag), so the content-mode call added nothing; drop it and keep determinism asserted byte-exact only, with content-mode compare reserved for semantic-parity checks. assert_bams_identical becomes unused as a result and is removed. molecule_join_does_not_spill_to_disk counted directory entries beside the BAM fixtures, which can't catch a spill routed through tempfile/std::env::temp_dir() to the actual system temp directory, or one written into a subdirectory. It now points TMPDIR/TEMP/TMP at an isolated, empty directory for the compare (run via the real CLI subprocess, so the env vars are scoped to the child process) and asserts that directory's recursive contents stay empty.
… set) Both fgumi group and fgbio GroupReadsByUmi assign the MI as a monotonically increasing counter in template-coordinate emission order, so a valid grouped file's base MI is strictly increasing across runs. Track only the largest base seen (O(1)) instead of a HashSet of all closed bases (O(molecules)): a new run whose base is not greater than one already seen is a reappearance or a non-monotonic id -- i.e. not grouped -- and is rejected. This also catches a base reappearing across an intervening MI-less run, which the set-membership form still handled but the adjacent-only form would miss. Reorder test fixtures are renumbered so each file's MI is monotonic in emission order (the molecule emitted first gets the lower MI), matching real tool output.
nh13
force-pushed
the
nh/compare-hardening
branch
from
July 19, 2026 13:26
772097e to
c00345c
Compare
Member
Author
|
Addressed all three findings from the latest review (2 inline + 1 outside-diff), amended into their originating commits and force-pushed.
A pre-push self-review pass on the reworded prose caught two follow-ons in my own edit, both fixed before pushing: the overview still promised faithful comparison for "any non-key value" (which would have re-introduced the same over-promise, since non-key floats are exactly what the tolerance absorbs), and it conflated Full suite green: 5480 passed, 26 skipped; lint and fmt clean. |
Member
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
Merged
This was referenced Jul 20, 2026
This was referenced Aug 5, 2026
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
Hardens
fgumi compare(the feature-gated developer oracle both fgbio-parity audits lean on) into a sound and faithful fgbio-parity comparator: it should never report MATCH when files genuinely differ (no false negative in the oracle), and should compare everything by default so a real difference is surfaced (no silent blind spot). Both prior parity audits explained every missed divergence via acompareblind spot (BS1–BS7) yetcompareitself was never audited — this is that "audit the auditor" pass.Fully self-contained: everything is under the
comparefeature (newengines/module + rewritten dispatch). No production command behavior changes.Latest revision — streaming molecule-join + semantic header gate
The grouping comparator was reworked from an external-sort key-join into an order-independent streaming molecule-join, and a hard header precondition was added. This supersedes the earlier key-join engine and closes the review findings against it (details under Resolved review findings below). For reviewers who saw the prior revision, the net changes are:
keyjoin.rs), itsMiBijectionTracker, the whole-file external queryname sort, and the--sort-memory/--sort-tmp-dirflags.molecule.rs+engines/molecule_join.rsand theengines/header.rsprecondition.Architecture
Comparison decomposes into two orthogonal axes — how records are paired, and what makes a pair equal — composed per command by an explicit preset table, with no resync (any key/position mismatch is an immediate DIFFER — avoids swap≡drop+add masking):
require_compatible_headers) — runs once, up front, and hard-exits the program on an incompatible header rather than cascading into record diffs.@SQ/@RGmust match; sort-order identity is decided semantically by mapping both@HDheaders to theSortOrderenum, so fgumi's bareSS:template-coordinateand fgbio's SO-prefixedSS:unsorted:template-coordinatecollapse to one order (the byte-for-byteSScompare that would false-fail every cross-tool run is gone).@PG/@COare per-invocation metadata and never compared. Genuinely orderless output (extract/zipper,SO:unsortednoSS) compares fine; a header that claims a recognized order this engine can't verify is rejected.--command sort.RecordKey— collision-resistant record identity (name + segment + secondary/supplementary + multimap locus).group/--mode grouping) — exploits that grouped output keeps same-MI reads consecutive: each input is cut into per-molecule runs, each molecule identified by its MI-invariant canonical id (the lexicographically smallest read name in the molecule), and molecules are matched across the two files by a never-bounded two-sided streaming hash-join (no external sort, no spill). Each matched molecule is compared by three local checks — membership (equalRecordKeyset), content underExactMinusMi, and duplex strand-partition (the/A//Bsplit, accepted modulo a global A↔B relabel). A molecule present on only one side is a DIFFER. This replaces the key-join's global MI-bijection tracker with per-molecule membership equivalence, and the greedy equal-key-run pairing disappears entirely.@HD, verifies each file is independently correctly ordered, and compares as a multiset grouped by maximal equal-sort-key run.Content predicates:
Exact/ExactMinusMi, each a narrow carve-out over exact equality.Accepted divergences (the only tolerances — each individually signed off)
groupMI numbering (value + within-group order) — the molecule-join matches molecules by MI-invariant canonical id, so any MI renumbering that preserves the grouping is tolerated.tctag —zipperwrites it anddedupconsumes it; fgbio never persists it. Ignored in every predicate (narrow — onlytc).sorttemplate-coordinate tie residue (name-hash vs lexical within equal-sort-key runs).Consensus/duplex depth-tag saturation is no longer a tolerance: fgumi now clamps every scalar depth tag to fgbio's
Shortceiling at the source, so those tags compare exactly (see the paired consensus-clamp work).Misuse is rejected, not tolerated: a grouping comparison of two BAMs with no MI tags at all is a hard error (
"grouping comparison requires MI-tagged (grouped) input") — consistent with fgbio's own consensus callers, which throwIllegalStateExceptionon a read missing its MI tag. Empty-vs-empty is a vacuous MATCH.Validation
molecule_join_compare/positional/sort-verify/metrics) and asserts its verdict, so a dimension that stops being compared fails the suite; a completeness guard requires every compared dimension to be named by a case.GroupReadsByUmiandfgumi group(adjacency,--edits 1) on 20k molecules / 119k records, and compared with the new molecule-join and cross-checked the old key-join engine on the same pair: both EQUIVALENT (exit 0), in agreement. Identical family-size distribution, consistent MI bijection, no crash and no gate false-fail on real fgbio output (SS:unsorted:template-coordinateheader, integerMI:Z:tags).--max-diffsindependence — the grouping verdict is keyed off the matched-molecule counter, so it is correct even at--max-diffs 0(a diff-cap can no longer mask a real difference in the--quietCI path).Resolved review findings
All CodeRabbit findings on this PR are resolved: the three heavy-lift grouping findings (F1 secondary/supplementary locus guard, greedy MI-unaware equal-key pairing, unbounded equal-key run memory) are superseded — the code they flag is deleted; the
--sort-memory/--sort-tmp-dirdoc finding is moot (flags removed); and the remaining items are fixed — mixed int/float doc contract, rejecting unsupportedSO:coordinateSSsub-sorts, requiringcM/cEin the mutation-catalog completeness guard, bounding the@SQdiagnostic allocation, and record-byte-exact same-seed determinism assertions.Quality
Built via a task-by-task TDD process with an independent review after each task and a final whole-branch review (which caught and fixed a
--max-diffs 0verdict soundness bug).cargo nextest(2,700+ tests),clippy -D warnings, andrustfmtall clean.Related
@HD SO:unsortedparity → fix(consensus): emit @HD SO:unsorted to match fgbio #526.SSsub-sort-tag prefix (R2-HDR-01) → fix(sort): write SS sub-sort tag as <sort-order>:<sub-sort> (R2-HDR-01) #514.Summary by CodeRabbit
fgumi compare bamspresets via--commandwith a dedicated--command sortmode.fgumi compare metrics --key-columnswith outer-join style comparison, multi-column keys, and row reordering tolerance.--key-columnsand tightened numeric-key/tolerance behavior in metrics comparisons (including float representation handling).compare bams/compare metricsdocs, examples, and CI guidance for--commandpresets and--key-columns.--quietbehavior, and new failure-mode assertions.