Repository navigation
fix: keep record order when reads are parallelised - #105
Merged
Merged
Conversation
Reading is serial, so the per-record work is parallelised with par_iter, which preserves order; par_bridge().collect() returned records in arbitrary order. That became visible once the default worker count changed to 'all cores': the FASTQ write test compares records element-wise and flaked with SIGILL-free but spurious 'record2 != record1' failures on MSRV. - bam reader read_all/filter_by_mapping_quality/extract_read_pairs/extract_features/ count_chimeric, bam2fq, chimeric count/events, and fa select_record_from_fa_by_stream now read in file order and map in parallel - bam2fq propagates malformed records instead of unwrapping - predicts: pt files collected in sorted walk order so max_predicts is deterministic - tests: FASTQ write asserts the sequential file order and compares the parallel read as a multiset; new bam read_order test pins order across worker counts (fails if par_bridge is reintroduced)
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Unresolved ordering and large-input memory regressions remain in several BAM processing paths.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 6
Open (7)
What changed in this PR
This PR restores deterministic record ordering while retaining parallel processing and makes prediction-file selection deterministic.
Changes:
- Replaces unordered parallel reads with ordered processing.
- Sorts prediction files before applying
max_predicts. - Adds ordering regression tests and updates FASTQ tests.
| File | Summary |
|---|---|
crates/deepbiop-fq/src/predicts.rs |
Deterministic prediction-file selection. |
crates/deepbiop-fq/src/io.rs |
Updated parallel FASTQ ordering test. |
crates/deepbiop-fa/src/io.rs |
Ordered FASTA selection. |
crates/deepbiop-bam/tests/read_order.rs |
BAM ordering regression tests. |
crates/deepbiop-bam/src/reader/mod.rs |
Ordered BAM reads and transformations. |
crates/deepbiop-bam/src/io.rs |
Ordered BAM-to-FASTQ conversion. |
crates/deepbiop-bam/src/chimeric/event.rs |
Chimeric event processing. |
crates/deepbiop-bam/src/chimeric/count.rs |
Chimeric-read filtering. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+65
to
+68
| Ok(records | ||
| .into_par_iter() | ||
| .filter(|record| is_retain_record(record) && is_chimeric_record(record)) | ||
| .collect::<Vec<bam::Record>>()) |
Comment on lines
+202
to
+206
| let events = records | ||
| .into_par_iter() | ||
| .filter(|record| { | ||
| is_retain_record(record) | ||
| && is_chimeric_record(record) |
Comment on lines
+32
to
+33
| let name = | ||
| String::from_utf8_lossy(record.name().unwrap_or_default().as_ref()).to_string(); |
Comment on lines
+103
to
+105
| Ok(records | ||
| .into_par_iter() | ||
| .filter(|record| { |
Comment on lines
+241
to
+245
| let records = self | ||
| .reader | ||
| .records() | ||
| .par_bridge() | ||
| .try_fold( | ||
| || 0usize, | ||
| |acc, result| { | ||
| let record = result.context("Failed to read BAM record")?; | ||
| Ok::<usize, anyhow::Error>(if record.flags().is_supplementary() { | ||
| acc + 1 | ||
| } else { | ||
| acc | ||
| }) | ||
| }, | ||
| ) | ||
| .try_reduce( | ||
| || 0usize, | ||
| |a: usize, b: usize| Ok::<usize, anyhow::Error>(a + b), | ||
| ) | ||
| .collect::<std::io::Result<Vec<_>>>() | ||
| .context("Failed to read BAM records")?; |
Comment on lines
+277
to
+280
| let records = reader | ||
| .records() | ||
| .par_bridge() | ||
| .filter_map(|record| { | ||
| let record = record.unwrap(); | ||
| .collect::<std::io::Result<Vec<_>>>() | ||
| .context("Failed to read FASTA records")?; |
Comment on lines
+75
to
+79
| let mut feature_reader = BamReader::open(Path::new(TEST_BAM), None).unwrap(); | ||
| let features = feature_reader.extract_features().unwrap(); | ||
| assert_eq!( | ||
| features.len(), | ||
| all.len(), |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
main's MSRV job (Rust 1.90) failed ondeepbiop-fq'sio::tests::test_write_fq_parallel:The test reads records with
par_bridge()and compares them element-wise with the source order, butpar_bridge().collect()does not preserve order. It only passed before becausecalculate_worker_count(None)returned 1; now thatNonemeans "all cores", the ordering assumptions are exercised — and several library functions silently returned records in arbitrary order (flagged in review as "par_bridge() scrambles record order").Fix
Record decoding is inherently serial (a single
&mutreader behindpar_bridge's mutex), so the reads now happen in file order andinto_par_iter()parallelises the per-record work, which preserves order:deepbiop-bam:read_all,filter_by_mapping_quality,extract_read_pairs,extract_features,count_chimeric,bam2fq,chimeric::count,chimeric::eventdeepbiop-fa:select_record_from_fa_by_streamdeepbiop-fq::predicts: pt files collected in sorted walk order somax_predictskeeps the same files every runbam2fqnow propagates malformed records instead ofunwrap()ingTests
crates/deepbiop-bam/tests/read_order.rs: asserts identical record order acrossworker_count1/2/default and forfilter_by_mapping_quality/extract_features; verified to fail whenpar_bridgeis reintroducedtest_write_fq_parallelnow asserts the sequential file order and compares the parallel read as a multiset