Repository navigation
fix(group): fail the run when UMI assignment fails - #631
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 (1)
Walkthrough
ChangesUMI assignment failure handling
Estimated code review effort: 3 (Moderate) | ~20 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✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #631 +/- ##
==========================================
+ Coverage 93.53% 93.56% +0.02%
==========================================
Files 175 175
Lines 106543 106523 -20
==========================================
+ Hits 99655 99668 +13
+ Misses 6888 6855 -33 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
Both `assign_umi_groups_impl` call sites caught the error, logged a warning, and substituted an empty position group. `fgumi group` then exited 0 having written a header-only BAM with the entire input silently discarded, and downstream consensus calling saw only an empty file. The trigger is easy to hit: `--strategy paired` needs the RX tag to carry two `-`-delimited segments, so running it against single-segment UMIs fails for every position group. A missing RX tag or mixed UMI lengths do the same. In each case the reads cannot be grouped at all, which is a usage error the run should surface rather than a group worth skipping. A warning on stderr is not a sufficient signal for total data loss. Propagate the error from both paths: the streaming `process_fn` returns an `InvalidData` `io::Error`, and the single-threaded path adds context and returns. The accepted-template rollback both sites performed is gone with them — nothing continues past the failure to over-report. `test_umi_assignment_failure_does_not_overcount_accepted` asserted the warn-and-continue behaviour; it is rewritten as `test_umi_assignment_failure_fails_the_run`, keeping its two cases so both the threaded and single-threaded call sites stay covered.
4d1d6d9 to
91ad310
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
This branch was previously deployed
1 participant
The bug
Both
assign_umi_groups_implcall sites ingroup.rscaught the error, logged a warning, and substituted an empty position group:fgumi groupthen exited 0 having written a header-only BAM with the entire input silently discarded. Downstream consensus calling sees only an empty file, and nothing in the exit status distinguishes that from a clean run over a small input.The trigger is easy to hit.
--strategy pairedrequires theRXtag to carry two--delimited segments, so pointing it at single-segment UMIs fails for every position group — the whole BAM vanishes. A missingRXtag or mixed UMI lengths reach the same path. In each case the reads cannot be grouped at all, which is a usage error worth surfacing, not a group worth skipping.The fix
Propagate the error from both paths:
process_fnreturns anInvalidDataio::Error.The accepted-template rollback both sites performed goes away with them — nothing continues past the failure, so there is no tally left to over-report and no
accepted_before_assignmentto remember.Tests
test_umi_assignment_failure_does_not_overcount_acceptedasserted the warn-and-continue contract this PR replaces — it checked that the metrics agreed with an empty output BAM. It is rewritten astest_umi_assignment_failure_fails_the_run, keeping both#[case]s so the threaded (--allow-unmapped, viashould_use_parallel) and single-threaded call sites each stay covered, since they swallowed the error independently.Verified RED against the unfixed source — both cases fail with
group must fail when UMI assignment fails, not emit an empty BAM: ()— then green after the fix.Full suite green: 5694 passed, plus
ci-fmtandci-lint.Note on scope
This is a behaviour change: inputs that previously produced an empty BAM and exit 0 now fail loudly. That is the point, but it is worth a look if anyone is relying on the old behaviour to skip pathological groups. I could not find a case where a partial input failure is expected — the assigner errors are all "this input cannot be grouped with these settings" rather than "this particular group is odd".
Summary by CodeRabbit
fgumi groupnow stops with an error when UMI assignment fails instead of silently dropping affected reads.