Repository navigation
feat(fastq): UMI-in-read-name output and BAM→FASTQ on the typed-step chain - #482
Conversation
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 38 seconds 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 ignored due to path filters (1)
📒 Files selected for processing (24)
WalkthroughAdds FASTQ UMI-in-header output, new terminal FASTQ sink/stage wiring, raw-byte and 3-way pipeline support, and matching validation, tests, and docs. ChangesFASTQ UMI output
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## feat-runall #482 +/- ##
==============================================
Coverage ? 94.18%
==============================================
Files ? 111
Lines ? 50744
Branches ? 0
==============================================
Hits ? 47794
Misses ? 2950
Partials ? 0 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![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 `@src/lib/commands/fastq.rs`:
- Around line 374-379: The UMI annotation block in fastq::write output uses
chained if let syntax that is not compatible with the pinned Rust 1.87.0 MSRV.
Update the logic in the umi_header/uh.lookup(record) path to use nested if let
statements in the fastq command code, or otherwise bump the workspace MSRV if
that syntax must stay.
🪄 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: 63f18774-067e-410d-ae50-250ee0ff8e98
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock,!**/*.lock
📒 Files selected for processing (3)
Cargo.tomlsrc/lib/commands/fastq.rstests/integration/test_fastq_command.rs
6ebeac1 to
7048cb9
Compare
nh13
changed the title
|
@coderabbitai review |
✅ Action performedReview finished.
|
7048cb9 to
c51f771
Compare
c51f771 to
02d4e32
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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/fastq.rs`:
- Around line 394-481: The paired FASTQ path in run_paired currently hardcodes
no_suffix to true, so --out1/--out2 mode can never emit /1 and /2 suffixes.
Update the suffix selection logic in run_paired to respect self.no_suffix (or an
explicit always-suffix option) when calling write_fastq_record, so users can
choose samtools -N-equivalent behavior instead of always suppressing suffixes.
Keep the change localized around the no_suffix variable and the
write_fastq_record call in run_paired.
- Around line 109-206: The FASTQ output path handling can oversubscribe CPU
because FastqSink::open_file creates a full compression pool per compressed
sink, and run_paired may open out1/out2/out0 at once. Update the thread
allocation so compressed outputs either share/divide the requested threads
across all active sinks or, at minimum, log the effective total worker count
when opening multiple BGZF sinks. Use the existing FastqSink::open_file,
run_paired, and log_config symbols to wire this in.
- Around line 751-831: Several of the test functions in this block are
table-driven and should be converted from single `#[test]` cases into `rstest`
parameterized tests. Update `parse_umi_tags_rejects_malformed`, the
`umi_annotation_*` tests, `classify_segment_by_flags`, and
`gz_extension_is_detected` to use `#[rstest]` with `#[case(...)]` for each
input/output pair, keeping the existing assertions but splitting repeated
scenarios into cases. Use the existing helpers `parse_umi_tags`,
`UmiHeader::from_args`, `write_annotation`, `classify_segment`, and
`path_is_gzip` as the targets for the cases.
- Around line 493-510: The output-path validation in the fastq command only
checks each of `self.output`, `self.out1`, `self.out2`, and `self.out0` against
`self.input`; extend this check to also reject any duplicate output destinations
among those four paths. Use the same lexical/canonicalized path comparison
already present in that loop, but compare outputs pairwise or via a set of
canonical paths before opening any sinks, and raise the same `anyhow::bail!`
error when two output flags resolve to the same file.
In `@tests/integration/test_fastq_command.rs`:
- Around line 609-632: Strengthen test_fastq_interleaved_gz_is_bgzf so it
verifies the actual FASTQ records, not just that two records were produced. In
the test_fastq_interleaved_gz_is_bgzf body, after read_fastq_maybe_gz(&out),
assert the expected record names and sequences (and any mate ordering if
relevant), using the same kind of identity checks as
test_fastq_paired_output_is_bgzf. Keep the BGZF header checks, but replace the
weak recs.len() only assertion with explicit per-record validation.
🪄 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: 6072f885-9169-4119-a1a7-30ba6a43aadf
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock,!**/*.lock
📒 Files selected for processing (7)
Cargo.tomlcrates/fgumi-bam-io/src/lib.rscrates/fgumi-bam-io/src/writer.rsdocs/src/guide/best-practices.mddocs/src/index.mdsrc/lib/commands/fastq.rstests/integration/test_fastq_command.rs
|
@coderabbitai On the On the per-output BGZF thread pools (fastq.rs:206): kept the undivided per-sink pools (benchmarked ~7% faster at low |
02d4e32 to
bc30a00
Compare
|
Tip For best results, initiate chat on the files or code changes.
No further action needed from me here. ✏️ Learnings added
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 `@docs/src/guide/best-practices.md`:
- Line 516: The section heading level is skipping too far under the existing
`##` parent, causing a heading-increment violation. Update the heading in this
part of `best-practices.md` to use the correct nested level (change the `####
Optional: Interleaved output and custom delimiters` heading to `###`), keeping
the section aligned with the surrounding markdown structure.
In `@src/lib/commands/fastq.rs`:
- Around line 520-548: The duplicate-output validation in fastq::Command::run
still misses equivalent paths when the target file does not exist yet because
canonical only resolves existing files; update that helper so it also normalizes
non-existent outputs by canonicalizing the parent directory and joining the
final component, then keep using it for both the input_key comparison and the
seen HashSet dedup in the output_paths loop. Preserve the /dev/null exception,
and use nested if let logic in canonical rather than a newer let-chain so the
fix stays compatible with the current MSRV.
- Around line 445-455: Reject aliased output paths in execute() before any
FastqSink::open_file calls by normalizing all requested outputs, including
non-existent ones, against the current working directory and comparing the
resolved paths for out1, out2, and out0. Update the path validation logic around
execute() so aliases like ./out.fq and out.fq are detected even when the files
do not yet exist, and fail early instead of allowing multiple sinks to open the
same target.
In `@tests/integration/test_fastq_command.rs`:
- Around line 518-524: The BGZF integration coverage is missing the `.bgz` path,
so a writer bug specific to that extension would not be caught. Add a
`.bgz`-suffixed variant of the existing `test_fastq_paired_output_is_bgzf` in
`test_fastq_command.rs` and use `read_fastq_maybe_gz` to validate it, so both
supported extensions are exercised. Keep the change focused on the FASTQ BGZF
test helpers and the paired-output test coverage.
- Around line 416-514: The UMI-in-header tests in test_fastq_command.rs only
verify fixed output strings, so add a parity check against the samtools fastq -U
formatting path for the annotating logic used by run_fastq_with_args and
--annotate-read-names. Keep the existing smoke tests, but introduce an
independent oracle for the formatting behavior so the duplex UMI annotation can
be validated without relying solely on hard-coded expected names, especially
around RX/OX selection and the UMI delimiter/separator handling.
🪄 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: cf1d55b0-bf2b-4436-a6eb-dbae03f22663
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock,!**/*.lock
📒 Files selected for processing (7)
Cargo.tomlcrates/fgumi-bam-io/src/lib.rscrates/fgumi-bam-io/src/writer.rsdocs/src/guide/best-practices.mddocs/src/index.mdsrc/lib/commands/fastq.rstests/integration/test_fastq_command.rs
bc30a00 to
cd12c10
Compare
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
#534) * fix(fastq): use DecodedRecordBatch accessors instead of private-field destructure The #482 fastq encode/paired steps destructured DecodedRecordBatch's now-private batch_serial/records fields directly, breaking the workspace build after those fields were made private to protect the cached total_bytes invariant. Capture the serial via batch_serial() and take ownership via into_records() instead. Behavior is unchanged; 117 fastq tests pass. * fix(sort): fail closed on poisoned spill-reader mutex instead of hanging Both spill-decompress fill paths (inline and block-parallel) acquired the per-slot reader mutex with try_lock() and treated any Err identically as 'contended, skip'. A poisoned mutex — a fill worker that panicked mid-read — was therefore swallowed as WouldBlock and the slot skipped forever: queue_eof never got set, so SortMerge spun on Contention and deadlocked instead of surfacing the panic. Distinguish the TryLockError variants: WouldBlock still skips; Poisoned now marks the slot failed (decomp_error + queue_eof) and returns Err so the failure propagates. Adds a regression test poisoning the reader mutex on both paths. * fix(align): latch consumer-gone on a reader-thread panic via RAII guard The align-and-merge reader thread called gate.mark_consumer_gone() only after reader_loop returned, so a panic in reader_loop skipped the latch: a writer parked in InFlightGate::acquire never woke, and Drop's writer_thread.join() hung the process with no surfaced error. Move the latch into a ConsumerGoneGuard whose Drop fires on every exit including unwind. Adds a unit test asserting a panic in the guarded scope still bails a blocked acquire(). * fix(align): drain aligner stderr byte-oriented so a non-UTF-8 line can't deadlock relay_stderr used BufRead::lines() and broke the drain loop on the first Err, which lines() yields for any non-UTF-8 line. Once draining stopped, the aligner blocked writing to its stderr pipe as soon as it filled (~64 KiB), hanging the whole align pipeline. Switch to read_until(b'\n') + from_utf8_lossy so invalid UTF-8 is decoded, not fatal, and the relay always drains to EOF. Adds a regression test feeding a non-UTF-8 line mid-stream. * fix(duplex): validate min-reads before the pipeline is built to avoid a worker panic The duplex per-worker init closure builds DuplexConsensusCaller with .expect(), so an invalid --min-reads ordering (e.g. '1 5', total < XY) panicked inside a worker thread on the threaded/runall path — likely wedging the pipeline — whereas the single-threaded path errored cleanly. Extract the min-reads validation out of DuplexConsensusCaller::new into a reusable DuplexConsensusCaller::validate_min_reads, and call it up front in ChainBuilder::add_duplex so every path surfaces a clean error before the pipeline is constructed. Codec/simplex already guard this; duplex was the outlier. Adds a unit test covering empty, >3 values, and both bad orderings.
This branch was previously deployed
1 participant
Reworks
fgumi fastqto (a) emit UMIs in the read name in Illumina DRAGEN format and (b) run all BAM→FASTQ conversion — interleaved and paired split — through the typed-step pipeline chain, sharing one work-stealing compression pool.Stacked on #476 (base
nh/sort-6-chains-wiring). Do not merge before #476. Review this PR's own diff via the 4 commits below.What's here (suggested reading order):
b8aaa812— UMI-in-read-name (--annotate-read-names/-U,--umi-tag, delimiters) matchingsamtools fastq -UDRAGEN output, plus paired-1/-2/-0BGZF split.899325db— route interleaved BAM→FASTQ through the chain (newWriteRawFileraw/BGZF sink).3f5911c8— new 3-output ordered pipeline primitive (OrderedBytesTuple3+Process3WithWorkerState), the one-branch-wider analog of the existingTuple2/Process2.cd12c109— route paired--out1/--out2/--out0through the chain via a 3-way fan-out; retire the standalonerun_pairedloop +FastqSink.Why the chain: the old paired loop opened a separate pool of
--threadsBGZF workers per output (three.gzoutputs at--threads N= 3N compression workers). On the chain the three per-file compressors share one pool, and RSS is bounded by the lean queue-memory default instead of growing per output.Correctness: the paired encode emits a block on all three ordered branches for every input batch (empty where a branch has no records) — a skipped batch serial would wedge that branch's reorder stage; a deterministic unit test pins this. Paired output is byte-parity with
samtools fastq -1/-2/-0(asserted in-test); interleaved withsamtools fastq -U.Validation: full suite green;
cargo ci-lint/ci-fmtclean;--no-default-featuresbuilds. tricorder on 4.2M records: 5.7× wall speedup at-@8, RSS bounded ~390 MB (flat-@4→-@8) vs ~6 GB under the old per-thread default.Closes #480.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation