Repository navigation
fix(audit): extract/zipper/group parity, validator guard, memory accounting (3/3) - #536
Conversation
|
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 (6)
WalkthroughThe changes centralize template-count validation, reject incompatible fused pipeline stages earlier, correct failed-group metrics, accept missing mapped reads in query-grouped zipper input, and calculate template memory from allocated capacities. ChangesPipeline behavior updates
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested labels: 🚥 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 @@
## feat-runall #536 +/- ##
==============================================
Coverage ? 94.23%
==============================================
Files ? 111
Lines ? 50988
Branches ? 0
==============================================
Hits ? 48047
Misses ? 2941
Partials ? 0 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
8102324 to
83de353
Compare
83de353 to
3caea5f
Compare
3caea5f to
1ff6787
Compare
1ff6787 to
1e2abc2
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@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: 4
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/group.rs (1)
1310-1328: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd a regression test for the UMI-assignment failure path.
src/lib/commands/group.rs:1310-1328needs coverage that forcesassign_umi_groups_implto returnErrand assertsaccepted_templatesis rolled back while discard counters stay unchanged.🤖 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/group.rs` around lines 1310 - 1328, Add a regression test covering the error branch around assign_umi_groups_impl: arrange inputs or a test double that makes it return Err, then assert the group’s accepted_templates contribution is subtracted from total_filter_metrics while discard counters remain unchanged. Keep the existing successful assignment behavior and failure return path intact.
🤖 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/zipper.rs`:
- Around line 2782-2813: Strengthen
grouped_out_of_natural_order_missing_mapped_read_is_accepted by asserting the
expected total record count and validating each read’s mapping status,
specifically that q10 is emitted unmapped-only while q2 is merged with its
mapped records. Follow the flags().is_unmapped() assertion pattern used by
test_unmapped_only_reads, replacing the name-only any checks with assertions
that detect duplication or accidental merging.
In `@src/lib/pipeline/chains/builder.rs`:
- Around line 3773-3776: Update the comments around the guards near add_group
and the corresponding locations to document that the intermediate Group stage
uses ChainTailKind::BatchedProcessedPositionGroups, replacing the incorrect
BamTemplateBatch reference. Keep the existing validation logic unchanged.
In `@src/lib/template.rs`:
- Around line 1030-1053: Update
heap_size_counts_capacity_and_matches_memory_estimate to include a RawRecord
with deliberately reserved spare capacity in the template’s Vec<RawRecord>.
Assert heap_size() against an independent lower bound that includes
RawRecord::capacity() and size_of::<RawRecord>(), rather than relying only on
the tautological estimate_heap_size comparison; retain the queryname capacity
coverage.
In `@tests/integration/test_chain_build.rs`:
- Around line 299-301: Strengthen the error assertions in the integration test
cases around the existing err.to_string() checks so they verify the Clip/Filter
tail-kind guard specifically, rather than merely matching “record-stream”.
Assert the stage-specific error text or the expected tail kind for both affected
cases, while preserving the existing failure context.
---
Outside diff comments:
In `@src/lib/commands/group.rs`:
- Around line 1310-1328: Add a regression test covering the error branch around
assign_umi_groups_impl: arrange inputs or a test double that makes it return
Err, then assert the group’s accepted_templates contribution is subtracted from
total_filter_metrics while discard counters remain unchanged. Keep the existing
successful assignment behavior and failure return path intact.
🪄 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: da8c0dfe-dfe4-4eec-918d-835108ad2749
📒 Files selected for processing (6)
src/lib/commands/extract.rssrc/lib/commands/group.rssrc/lib/commands/zipper.rssrc/lib/pipeline/chains/builder.rssrc/lib/template.rstests/integration/test_chain_build.rs
1e2abc2 to
8364590
Compare
|
@coderabbitai Addressed the outside-diff-range comment on |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
fd group.rs --full-path src/lib/commands
rg -n "assignment_failure_rolls_back_accepted_templates_but_not_discards" -A 40 src/lib/commands/group.rs | head -80Length of output: 2187 Confirmed — the test is in |
|
@coderabbitai review |
✅ Action performedReview finished.
|
8364590 to
1ad6381
Compare
… path too The read-structure guard (total template reads must be 1 or 2, i.e. a valid SAM template) lived only in the standalone Extract command's validate; the chain/runall path (ChainBuilder::add_extract) skipped it and would silently emit malformed BAM templates (0 template reads -> no sequence; 3+ -> more segments than R1/R2 can hold). Extract the check into a shared extract::validate_template_count and call it from both the standalone command and add_extract. Adds an rstest case table (1/2 accepted, 0/3 rejected).
…uped input On a name mismatch the zipper compared the mapped and unmapped head names with natural_compare and errored if the mapped head sorted first. That assumes queryname-SORTED input, but the tool documents (and fgbio ZipperBams supports) query-GROUPED input too — records with the same name consecutive, but groups in any order. For grouped input a later group can natural-sort before the current one, so the guard spuriously rejected valid input when the mapped BAM was merely missing a read. Match fgbio: compare by name equality only; on mismatch emit the unmapped read as unmapped-only and advance. A genuinely orphaned mapped read is still caught by the existing end-of-stream 'mapped reads remaining' check. Adds a regression test with grouped, non-natural-order input and a missing mapped read.
… of dispatch-panicking Clip and Dedup group the record stream internally (GroupBam / GroupByPosition), so they consume a DecodedRecordBatch tail. An ordering-valid but type-incompatible chain (e.g. Group -> Clip, where Group emits BamTemplateBatch) passed the stage-ordering validators but built a mis-typed, type-erased pipeline that panicked at dispatch. Guard chain_tail_kind == DecodedRecordBatch at the top of add_clip and add_dedup and bail with a clear message otherwise. Latent today (no CLI/runall constructs these specs) but defends the chain builder. Adds a build-time Group -> Clip rejection test.
…o avoid under-counting RSS Template had two divergent size methods: heap_size() (used by the typed-step pipeline's byte-bounded queues via BamTemplateBatch) summed logical record len() + name.len(), while MemoryEstimate::estimate_heap_size() summed record capacity() + Vec<RawRecord> overhead + name capacity(). The len-based heap_size under-counts over-allocated buffers and ignores container overhead, so --max-memory under-estimates real RSS and the pipeline can buffer past its budget and risk OOM — and it was inconsistent with DecodedRecordBatch, which already sizes by capacity. Make heap_size compute the true allocated footprint and have estimate_heap_size delegate to it (single source of truth). Adds a regression test (spare-capacity name -> heap_size counts capacity and equals estimate_heap_size).
…unt + sort-order docs) Two divergences between group's single-threaded fast path and the chain path: - Metrics (V3): on UMI-assignment failure the chain path drops the whole group and zeroes its accepted_templates, but the single-threaded path merged this group's filter metrics (including accepted_templates) before assignment and only logged + returned on failure, over-counting accepted_records. Roll back this group's accepted_templates on failure to match the (empty) output and the chain path; the genuine discard counters are kept. - Help (V4): the long-about claimed 'Accepts reads in any order (including unsorted)', but both paths hard-reject input that is not template-coordinate sorted (or queryname sorted with --allow-unmapped). Unlike fgbio, fgumi does not re-sort internally. Correct the help to state the actual requirement.
|
@coderabbitai review |
✅ Action performedReview finished.
|
This branch was previously deployed
1 participant
Part 3 of a 3-PR audit-fix stack. Base is
nh/audit-2-sort-data(PR 2); merge PRs 1 and 2 first, then this.Fixes in this PR (command parity + validator guard + memory accounting)
natural_comparemismatch guard rejected valid query-grouped (non-sorted) input. fgbio'sZipperBamsmatches by name equality only; a genuinely orphaned mapped read is still caught by the existing end-of-stream check.Group -> Clip, where Clip consumes a record stream but Group emits templates) built a mis-typed pipeline that panicked at dispatch. Guard the tail kind inadd_clip/add_dedup. Latent today.Template::heap_size(the pipeline byte budget) summed logicallen(), whileMemoryEstimatesummedcapacity()+ container overhead — so--max-memoryunder-counted RSS and could OOM. Unify on the capacity-based footprint.accepted_recordson UMI-assignment failure (the chain path zeroes it), and the help claimed "any order (including unsorted)" while the code requires template-coordinate/queryname sorted input. Fix both.Each fix has a regression test (V4 is a help/doc correction).
Summary by CodeRabbit