Repository navigation
fix(umi): fold case in the sequential edit assigner (audit P2) - #537
Conversation
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 28 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughThe sequential edit assigner now folds UMIs to ASCII uppercase before grouping and mapping results. Parallel edit tests verify matching mixed-case partitions, while documentation clarifies behavior for non-encodable UMIs other than ChangesUMI case handling
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## feat-runall #537 +/- ##
==============================================
Coverage ? 94.24%
==============================================
Files ? 111
Lines ? 51050
Branches ? 0
==============================================
Hits ? 48110
Misses ? 2940
Partials ? 0 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
113629f to
de53f80
Compare
de53f80 to
3633dea
Compare
3633dea to
d94da5b
Compare
d94da5b to
9a65127
Compare
9a65127 to
e984a5c
Compare
e984a5c to
debfcc7
Compare
debfcc7 to
00041ee
Compare
00041ee to
b883742
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: 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 `@crates/fgumi-umi/src/assigner.rs`:
- Around line 2202-2214: Add programmatically generated fgbio-baseline identity
assertions for mixed-case UMI assignments in
test_simple_error_assigner_is_case_insensitive, while keeping the default suite
Rust-only. Extend randomized parity coverage in src/lib/umi/parallel_assigner.rs
lines 1378-1437 to validate assignments against the fgbio-derived baseline
rather than only sequential/parallel agreement; update
crates/fgumi-umi/src/assigner.rs lines 2202-2214 to assert the mixed-case
partition against that same baseline.
🪄 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: 2b2ad2a3-8452-43f4-9a17-eebb404c0e8b
📒 Files selected for processing (2)
crates/fgumi-umi/src/assigner.rssrc/lib/umi/parallel_assigner.rs
The Edit strategy has a sequential (`SimpleErrorUmiAssigner`) and a parallel (`ParallelEditAssigner`) implementation documented to produce identical groupings. The parallel path uppercases before encoding, but the sequential path matched raw bytes case-sensitively, so mixed-case UMIs could group differently depending on `--threads` (the sequential path runs at `--threads 1`, the parallel path above the parallel threshold). Fold case in `SimpleErrorUmiAssigner::assign` so it agrees with the parallel edit assigner. This is a no-op for the uppercase UMIs the `group`/`dedup` CLI emits (`umi_for_read` already uppercases non-paired UMIs, and fgbio uppercases every UMI before assignment via `canonicalize(rawTag.toUpperCase)`), so it matches fgbio's user-facing output; it only changes behavior for direct library callers passing mixed case. Also correct two stale comments on `ParallelEditAssigner` that claimed it produces results "identical to"/"consistent with" the sequential assigner: the one genuine residual difference is un-`BitEnc`-encodable UMIs (length > 32 or a non-`ACGTN` base), which the parallel path singletons while the sequential path string-groups. `N`-containing UMIs never reach either assigner — those templates are discarded up front (`discarded_ns_in_umi`), matching fgbio — so this residual is unreachable via the CLI and left documented rather than converged. Add a sequential-vs-parallel parity test on mixed-case input and a direct case-insensitivity test for the sequential assigner.
b883742 to
d39b29e
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
This branch was previously deployed
1 participant
What
Converges the Edit UMI strategy's sequential (
SimpleErrorUmiAssigner) and parallel (ParallelEditAssigner) implementations on case. The parallel path uppercases before encoding; the sequential path matched raw bytes case-sensitively, so mixed-case UMIs could group differently depending on--threads. This folds case inSimpleErrorUmiAssigner::assignto match the parallel path and fgbio.Second-wave item P2 from the feat-runall audit (
findings/09-umi.md, finding 1).Target rationale (main vs feat-runall)
This is the same class of bug that
main's #455 ("make sequential and parallel assigners agree on UMI case") fixed — but #455 is not in feat-runall's history. feat-runall converged only the adjacency paths (via its own FU-004 raw-first-seen tie-break work) and left edit case-divergent. Sincemainalready carries the fix and feat-runall does not, this targets feat-runall (stacked onnh/audit-3-parity). It ports only the edit-path portion of #455's approach; feat-runall's adjacency convergence is intentionally left alone (it converged differently than #455 and will be reconciled at merge time).fgbio parity
fgbio's
GroupReadsByUmiuppercases every UMI before assignment (canonicalize(rawTag.toUpperCase)), so case-insensitive grouping is the fgbio-faithful behavior — this converges with fgbio's user-facing output rather than diverging from it. In production this is a no-op:umi_for_readalready uppercases non-paired UMIs before the assigner sees them, so only direct library callers passing mixed case change behavior.Residual divergence (documented, not converged)
After this change the only remaining edit seq/parallel difference is for UMIs that are not
BitEnc-encodable for a reason other thanN(length > 32 bases, or a non-ACGTNcharacter): the parallel path gives each its own molecule id, the sequential path groups them by string edit distance.N-containing UMIs never reach either assigner — those templates are discarded up front (discarded_ns_in_umi), matching fgbio — so this residual is unreachable via the CLI. The two previously-inaccurate "identical to"/"consistent with sequential" comments onParallelEditAssignerare corrected to state this precisely.The deeper Paired-strategy seq/parallel divergence (
findings/09finding 2) is a separate, out-of-scope item and is not touched here.Tests
test_edit_sequential_parallel_agree_on_mixed_case— sequential vs parallel produce the same grouping on mixed-case input (fails before, passes after).test_simple_error_assigner_is_case_insensitive— direct case-insensitivity check on the sequential assigner in thefgumi-umicrate.Full suite green:
cargo ci-test2333 passed / 8 skipped;cargo ci-lintclean.Summary by CodeRabbit