feat(bam-io): unify the single-threaded raw-BAM reader onto fgumi-bgzf (#800) - #842
Conversation
…f, honoring verify_crc (#800) Add FgumiBgzfReader, a safe streaming Read/BufRead adapter over fgumi-bgzf's read_raw_blocks + decompress_block_into_opts, and route the single-threaded raw-BAM reader (create_raw_bam_reader[_with_opts]) through it instead of noodles-bgzf. This makes PipelineReaderOpts.verify_crc live: clip, codec, duplex, simplex, and downsample now honor --check-crc/--no-check-crc in their single-threaded fast paths (previously they always verified via noodles, which has no skip knob). correct and group keep the noodles path (their single- threaded mode reads through create_bam_reader_for_pipeline_with_opts), so the help text lists them as --threads-only; threads>1 still uses noodles' multithreaded decoder. Decode is at parity with noodles single-threaded (~2% either way, within CI); the deliverable is bringing the CRC-skip policy to these paths. Retires half of the second-decoder inconsistency (fgumi-sort/extract remain follow-ups).
|
Note Reviews pausedUse the following commands to manage reviews:
Use the checkboxes below for quick actions:
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
|
@coderabbitai pause |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #842 +/- ##
========================================
Coverage 94.46% 94.46%
========================================
Files 187 187
Lines 115301 115597 +296
========================================
+ Hits 108921 109201 +280
- Misses 6380 6396 +16 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
✅ Action performedReviews paused. |
Closes #800. Supersedes #801 (auto-closed when its base branch
786/nhomer/perf-bgzf-crc-skipwas deleted by the #798 squash-merge; GitHub cannot reopen a PR whose base branch was deleted). Same head branch, rebased ontomain— the one now-redundant doc commit dropped as already upstream, leaving the single substantive change.Problem
fgumi-bam-iohad two BGZF decoder stacks. The single-threaded raw-BAM reader (create_raw_bam_reader[_with_opts]) decoded via noodles-bgzf, which always verifies CRC32 and has no skip knob — so the--check-crc/--no-check-crcpolicy from #798 (backed by our ownfgumi-bgzf) never reached the single-threaded fast paths, andPipelineReaderOpts.verify_crcwas a dead carrier field.Change
Add
FgumiBgzfReader, a safe streamingRead/BufReadadapter overfgumi-bgzf'sread_raw_blocks+decompress_block_into_opts(nounsafe; one block decoded per refill, so a CRC/size error is tied to the bytes the caller actually reads), and route the single-threaded raw reader through it via a newBgzfReaderEnum::Fgumiarm.PipelineReaderOpts.verify_crcis now live.Coverage after this change (
--check-crc/--no-check-crc):clip/codec/duplex/simplex/downsample— honored single- or multi-threaded (their single-threaded reader now decodes through fgumi-bgzf).correct/group— honored only with--threads N; their single-threaded mode reads throughcreate_bam_reader_for_pipeline_with_opts+ a manual noodles decode, so they keep the noodles path (help text says so).threads > 1still uses noodles' multithreaded decoder (a threaded fgumi-bgzf raw reader is out of scope).fgumi-sortandextracthave their own reader stacks and remain follow-ups.Tests
TDD: the reader-level
verify_crctest (corrupted-CRC file accepted withverify_crc=false, rejected withtrue) was written first and confirmed red against the noodles path before implementing. Plus a multi-block roundtrip test (exercises block-boundary buffering) and three end-to-enddownsampleCRC tests (--no-check-crcaccepts a corrupted file; default and--check-crcreject it). The reader also gains a sticky-poison test: after a mid-stream decode failure bothreadandfill_bufstay in error. All 228fgumi-bam-io+fgumi-bgzftests green on the rebased tree.