Skip to content

refactor(correct): eliminate RecordBuf duplication via raw-byte processing - #229

Merged
nh13 merged 1 commit into
mainfrom
nh/correct-extract-dedup
Apr 4, 2026
Merged

nh13 merged 1 commit into
mainfrom
nh/correct-extract-dedup

Conversation

@nh13

@nh13 nh13 commented Apr 4, 2026

Copy link
Copy Markdown
Member

Summary

  • Convert execute_single_thread_mode from RecordBuf + TemplateIterator to raw RawRecord reading with manual QNAME grouping, using extract_and_validate_template_umi_raw and apply_correction_to_raw
  • Remove the dead RecordBuf else-branch from the batch pipeline process_fn (raw-byte mode is already enabled at pipeline config)
  • Simplify CorrectProcessedBatch and CollectedCorrectMetrics by removing Vec<RecordBuf> fields
  • Delete extract_and_validate_template_umi (RecordBuf variant), apply_correction_to_record, and their 10 unit tests
  • Add characterization test verifying single-thread mode correctness (UMI correction + OX tag)

Net: -222 lines (276 added, 498 removed)

Test Plan

  • Characterization test test_single_thread_mode_produces_correct_output passes
  • All 2204 unit tests pass
  • All 140 integration tests pass
  • cargo ci-fmt and cargo ci-lint clean

…aw-byte processing

Convert both the single-threaded streaming path and the multi-threaded batch
pipeline path to use raw `Vec<u8>` records via `fgumi-raw-bam` exclusively,
removing the duplicated RecordBuf variants of `extract_and_validate_template_umi`
and `apply_correction_to_record`.

- Convert `execute_single_thread_mode` from `record_bufs()` + `TemplateIterator`
  to raw `RawRecord` reading with manual QNAME grouping
- Remove the RecordBuf else-branch from the batch pipeline `process_fn`
- Simplify `CorrectProcessedBatch` and `CollectedCorrectMetrics` structs
- Delete `extract_and_validate_template_umi` (RecordBuf) and
  `apply_correction_to_record` along with their 10 unit tests
- Add characterization test for single-thread mode output correctness
@nh13
nh13 temporarily deployed to github-actions April 4, 2026 07:08 — with GitHub Actions Inactive
@nh13
nh13 marked this pull request as ready for review April 4, 2026 07:09
@codecov

codecov Bot commented Apr 4, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 96.79487% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.27%. Comparing base (ea6e3fc) to head (801dd18).
⚠️ Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
src/commands/correct.rs 96.79% 5 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #229      +/-   ##
==========================================
+ Coverage   88.18%   88.27%   +0.09%     
==========================================
  Files         113      113              
  Lines       53388    53215     -173     
==========================================
- Hits        47080    46977     -103     
+ Misses       6308     6238      -70     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@coderabbitai

coderabbitai Bot commented Apr 4, 2026

Copy link
Copy Markdown
📝 Walkthrough

Walkthrough

The BAM correction pipeline in src/commands/correct.rs migrated from RecordBuf-based processing to raw-byte handling. Changes include removing kept_records/rejected_records vectors from CorrectProcessedBatch, eliminating RecordBuf UMI extraction and correction functions, and updating both multi-threaded and single-threaded execution paths to operate exclusively on raw-byte blocks. The single-threaded path was rewritten to use create_raw_bam_reader and stream records directly. Serialization now handles only raw-byte BAM blocks, and the pipeline signature was simplified to ignore the header argument. Tests were adjusted accordingly with new characterization coverage.

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed Title clearly and concisely describes the main refactoring: eliminating RecordBuf duplication by switching to raw-byte processing, which matches the changeset's core objective.
Description check ✅ Passed Description directly addresses the changeset, detailing the conversion from RecordBuf to raw-byte processing, removed functions, simplified structures, and test coverage.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch nh/correct-extract-dedup

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
src/commands/correct.rs (1)

838-844: unwrap_or_default() silently drops records if template isn't raw-byte mode.

The debug_assert! only fires in debug builds. In release, a misconfigured pipeline would silently lose records. Using .expect() provides fail-fast behavior.

Proposed fix
-                    let raw_records = template.into_raw_records().unwrap_or_default();
+                    let raw_records = template.into_raw_records().expect(
+                        "Template should be in raw-byte mode; RecordBuf mode is no longer supported"
+                    );
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/commands/correct.rs` around lines 838 - 844, The code currently calls
template.into_raw_records().unwrap_or_default() after a debug_assert!, which in
release builds will silently drop records; change this to a fail-fast call
(e.g., use expect or unwrap with a clear message) so misconfigured templates
cannot lose data silently: replace the unwrap_or_default() call on
template.into_raw_records() with expect("expected raw-byte mode template:
into_raw_records returned None") (or similar) and keep the debug_assert as-is;
this affects the block around template, into_raw_records, unwrap_or_default and
the subsequent call to Self::extract_and_validate_template_umi_raw so the
failure surfaces immediately with a descriptive message.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@src/commands/correct.rs`:
- Around line 838-844: The code currently calls
template.into_raw_records().unwrap_or_default() after a debug_assert!, which in
release builds will silently drop records; change this to a fail-fast call
(e.g., use expect or unwrap with a clear message) so misconfigured templates
cannot lose data silently: replace the unwrap_or_default() call on
template.into_raw_records() with expect("expected raw-byte mode template:
into_raw_records returned None") (or similar) and keep the debug_assert as-is;
this affects the block around template, into_raw_records, unwrap_or_default and
the subsequent call to Self::extract_and_validate_template_umi_raw so the
failure surfaces immediately with a descriptive message.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 5d35da37-2d1e-483b-868a-06a4a3c2c874

📥 Commits

Reviewing files that changed from the base of the PR and between 1ab1bd5 and 801dd18.

📒 Files selected for processing (1)
  • src/commands/correct.rs

@nh13
nh13 merged commit 0c6b0ae into main Apr 4, 2026
7 checks passed
@nh13
nh13 deleted the nh/correct-extract-dedup branch April 4, 2026 07:40
@nh13 nh13 mentioned this pull request Apr 4, 2026

This branch was previously deployed

1 inactive deployment
github-actions — 801dd18e Deployed Apr 4, 2026 by nh13 via coverage #859
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant