feat(sort): extract framework-light sort CLI into standalone crates (#446) - #447
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThree new workspace crates ( ChangesSort CLI and pipeline I/O crate extraction
Sequence Diagram(s)sequenceDiagram
rect rgba(100, 149, 237, 0.5)
Note over Sort: fgumi-sort-cli
participant Sort
participant build_sort_step
participant SortBamFile
end
rect rgba(60, 179, 113, 0.5)
Note over ReadBgzfBlocks,WriteBgzfFile: fgumi-pipeline-io (streaming path)
participant ReadBgzfBlocks
participant SortAndSpill
participant SortSpillDecompress
participant SortMerge
participant WriteBgzfFile
end
Sort->>build_sort_step: SortStepCaptures (effective_memory, order, threads)
build_sort_step->>SortBamFile: RawExternalSorter + stats_slot
Sort->>ReadBgzfBlocks: read_bam_auto (byte-bounded output queue)
ReadBgzfBlocks->>SortAndSpill: BgzfBlock (output_byte_limit)
SortAndSpill->>SortSpillDecompress: SortPhase1Event (SpillReady/MemoryChunk/AllAnnounced)
SortSpillDecompress->>SortSpillDecompress: greedy per-slot decompression with backpressure
SortSpillDecompress->>SortMerge: SortPhase2Event + decompressed slot blocks
SortMerge->>WriteBgzfFile: RecordBatch (ByItemOrdinal, cooperative drain)
WriteBgzfFile->>WriteBgzfFile: deferred HeaderHandle resolution then BGZF_EOF
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 @@
## feat-runall #447 +/- ##
==============================================
Coverage ? 93.99%
==============================================
Files ? 111
Lines ? 48654
Branches ? 0
==============================================
Hits ? 45730
Misses ? 2924
Partials ? 0 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Member
Author
|
@coderabbitai resume |
Member
Author
|
@coderabbitai review |
✅ Action performedReviews resumed. |
✅ Action performedReview finished.
|
Member
Author
|
@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: 2
🤖 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-cli-common/src/lib.rs`:
- Around line 105-112: The docstring for the detect_cpu_count() function claims
it respects cgroup CPU quotas, but the underlying num_cpus crate has
inconsistent cgroup v2 handling that may cause it to return physical core count
instead of respecting quotas. Either update the docstring to soften the language
(e.g., change "Respects" to "May respect") to accurately reflect the limitation,
or consider replacing num_cpus::get() with std::thread::available_parallelism()
which has better cgroup v2 support and has been stable since Rust 1.59.
In `@crates/fgumi-pipeline-io/src/source/read_bam.rs`:
- Around line 126-127: The uncompressed_size field assignment uses
unwrap_or(u32::MAX) which silently masks invalid BGZF metadata by saturating to
the maximum value. Instead of this silent fallback, replace the
unwrap_or(u32::MAX) with proper error handling that returns an InvalidData error
when the conversion fails, ensuring that invalid uncompressed sizes are properly
rejected rather than masked.
🪄 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: a5b4a8ff-84fd-4a4d-9d2f-252bee261225
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock,!**/*.lock
📒 Files selected for processing (37)
Cargo.tomlcrates/fgumi-cli-common/Cargo.tomlcrates/fgumi-cli-common/src/lib.rscrates/fgumi-pipeline-io/Cargo.tomlcrates/fgumi-pipeline-io/src/lib.rscrates/fgumi-pipeline-io/src/sink/mod.rscrates/fgumi-pipeline-io/src/sink/write_bgzf.rscrates/fgumi-pipeline-io/src/sort/and_spill.rscrates/fgumi-pipeline-io/src/sort/and_spill/tests.rscrates/fgumi-pipeline-io/src/sort/merge.rscrates/fgumi-pipeline-io/src/sort/merge/tests.rscrates/fgumi-pipeline-io/src/sort/mod.rscrates/fgumi-pipeline-io/src/sort/protocol.rscrates/fgumi-pipeline-io/src/sort/spill_decompress.rscrates/fgumi-pipeline-io/src/sort/spill_decompress/tests.rscrates/fgumi-pipeline-io/src/sort/tests.rscrates/fgumi-pipeline-io/src/source/mod.rscrates/fgumi-pipeline-io/src/source/read_bam.rscrates/fgumi-pipeline-io/src/types.rscrates/fgumi-sort-cli/Cargo.tomlcrates/fgumi-sort-cli/src/chains.rscrates/fgumi-sort-cli/src/lib.rscrates/fgumi-sort-cli/src/sort.rscrates/fgumi-sort-cli/src/version.rssrc/lib/commands/command.rssrc/lib/commands/common.rssrc/lib/commands/sort.rssrc/lib/pipeline/chains/commands/sort.rssrc/lib/pipeline/steps/sink/write_bgzf.rssrc/lib/pipeline/steps/sort/and_spill/tests.rssrc/lib/pipeline/steps/sort/merge/tests.rssrc/lib/pipeline/steps/sort/mod.rssrc/lib/pipeline/steps/sort/spill_decompress.rssrc/lib/pipeline/steps/sort/spill_decompress/tests.rssrc/lib/pipeline/steps/source/read_bam.rssrc/lib/pipeline/steps/types.rssrc/main.rs
💤 Files with no reviewable changes (4)
- src/lib/pipeline/steps/sort/spill_decompress/tests.rs
- src/lib/pipeline/steps/sort/merge/tests.rs
- src/lib/pipeline/steps/sort/and_spill/tests.rs
- src/lib/pipeline/steps/sort/spill_decompress.rs
nh13
force-pushed
the
446/nh/feat-sort-cli
branch
from
June 20, 2026 17:51
d4a2918 to
2ae025e
Compare
Member
Author
|
@coderabbitai resume |
Member
Author
|
@coderabbitai review |
✅ Action performedReviews resumed. |
✅ Action performedReview finished.
|
Member
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/fgumi-pipeline-io/src/sort/tests.rs (1)
423-433: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winAssert
SortBamFilepublishes stats.This test would still pass if
SortBamFile::try_runstopped fillingstats_out; assert the shared slot afterPipeline::runbecauseSortFinalizeHookconsumes that contract.Proposed test assertion
+ let stats_slot = Arc::new(parking_lot::Mutex::new(None)); let step = SortBamFile::new( sorter, input.clone(), pipeline_out.clone(), - Arc::new(parking_lot::Mutex::new(None)), + Arc::clone(&stats_slot), ); @@ let pipeline = builder.build().expect("Pipeline::build"); pipeline.run(PipelineConfig { threads: 1, ..Default::default() }).expect("Pipeline::run"); + let stats = stats_slot.lock().take().expect("SortBamFile should publish SortStats"); + assert_eq!(stats.total_records, records.len() as u64); + assert_eq!(stats.output_records, records.len() as u64); let legacy_records = read_all_records(&legacy_out);Based on supplied context,
SortFinalizeHookreads this shared stats slot afterPipeline::run.🤖 Prompt for 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. In `@crates/fgumi-pipeline-io/src/sort/tests.rs` around lines 423 - 433, After the pipeline.run() call in the test, add an assertion to verify that the stats_out shared slot contains stats data. The stats_out variable (the Arc<parking_lot::Mutex<Option<T>>> passed to SortBamFile::new) should be locked and asserted to contain Some(...) rather than None to ensure that SortBamFile::try_run actually published the expected stats before the pipeline completed, since SortFinalizeHook depends on consuming this contract.
🤖 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-pipeline-io/src/sort/merge.rs`:
- Around line 27-28: The MAX_EVENTS_PER_LOCK constant caps the input drain at 16
events which causes upstream queue cycling and contention. Remove or
significantly increase this constant and modify the drain loop logic (located
around the areas that check against this constant in the event processing) to
drain events unboundedly from the input queue, while keeping the existing bounds
on the producer and output queues. The issue appears in multiple locations where
the drain is capped by this constant (around lines 244-274 and 439-440), so
ensure all input drain loops are made unbounded.
In `@crates/fgumi-pipeline-io/src/sort/spill_decompress.rs`:
- Around line 153-160: In the profile method of the SortSpillDecompress struct,
the output_queues field currently uses QueueSpec::CountBounded which only limits
the number of events, but since SortPhase2Event::MemoryChunk retains sorted
record chunks that can consume significant memory, this needs to be byte-bounded
instead. Replace the QueueSpec::CountBounded variant with the appropriate
byte-bounded QueueSpec variant (using HeapSize accounting) to ensure memory
retention is capped by bytes based on configuration rather than event count.
Update the capacity parameter accordingly to use byte bounds instead of count
bounds.
- Around line 78-130: The current implementation only enforces a per-slot cap
(PHASE2_DECOMP_CAP) in the try_fill_some_slot method, allowing total
decompressed data to grow unbounded across all slots. Introduce a global shared
budget or capacity counter for total decompressed data across all slots, and
gate the read_blocks call to only proceed if there is remaining global budget.
Track the bytes/blocks added to each slot.decompressed queue against this global
budget when data is pushed, and ensure that SortMerge (the component consuming
this data) decrements the global budget as it drains decompressed batches to
maintain memory as a function of configuration rather than input size.
In `@crates/fgumi-pipeline-io/src/types.rs`:
- Around line 28-31: The HeapSize implementations are reporting logical vector
length instead of allocated capacity, which underestimates actual heap memory
when vectors are pre-allocated with capacity exceeding current length. This
causes memory backpressure to undercount and bypass configured limits. In the
heap_size method of the HeapSize impl for BgzfBlock (line 30), change
self.bytes.len() to self.bytes.capacity(). Apply the same fix to the other
HeapSize impls at lines 55 and 196 where Vec::len() is used. Additionally, check
for any ranges.len() calls and change them to ranges.capacity() to properly
account for allocated heap space rather than just populated elements.
In `@crates/fgumi-sort-cli/src/chains.rs`:
- Around line 137-142: The `resolve_memory_budget` function is being called
redundantly in `build_sort_step` (chains.rs) when it has already been computed
in `execute_sort` (sort.rs). To fix this, add an `effective_memory: usize` field
to the `SortStepCaptures` struct, pass the already-computed `effective_memory`
value from `execute_sort` to `SortStepCaptures`, and then use this field value
in `build_sort_step` instead of calling `resolve_memory_budget` again. This
eliminates the redundant syscall to `detect_total_memory()` and ensures
consistency between the logged and configured values.
---
Outside diff comments:
In `@crates/fgumi-pipeline-io/src/sort/tests.rs`:
- Around line 423-433: After the pipeline.run() call in the test, add an
assertion to verify that the stats_out shared slot contains stats data. The
stats_out variable (the Arc<parking_lot::Mutex<Option<T>>> passed to
SortBamFile::new) should be locked and asserted to contain Some(...) rather than
None to ensure that SortBamFile::try_run actually published the expected stats
before the pipeline completed, since SortFinalizeHook depends on consuming this
contract.
🪄 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: bfca4119-72b1-4f57-84d3-fc5660c2eece
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock,!**/*.lock
📒 Files selected for processing (37)
Cargo.tomlcrates/fgumi-cli-common/Cargo.tomlcrates/fgumi-cli-common/src/lib.rscrates/fgumi-pipeline-io/Cargo.tomlcrates/fgumi-pipeline-io/src/lib.rscrates/fgumi-pipeline-io/src/sink/mod.rscrates/fgumi-pipeline-io/src/sink/write_bgzf.rscrates/fgumi-pipeline-io/src/sort/and_spill.rscrates/fgumi-pipeline-io/src/sort/and_spill/tests.rscrates/fgumi-pipeline-io/src/sort/merge.rscrates/fgumi-pipeline-io/src/sort/merge/tests.rscrates/fgumi-pipeline-io/src/sort/mod.rscrates/fgumi-pipeline-io/src/sort/protocol.rscrates/fgumi-pipeline-io/src/sort/spill_decompress.rscrates/fgumi-pipeline-io/src/sort/spill_decompress/tests.rscrates/fgumi-pipeline-io/src/sort/tests.rscrates/fgumi-pipeline-io/src/source/mod.rscrates/fgumi-pipeline-io/src/source/read_bam.rscrates/fgumi-pipeline-io/src/types.rscrates/fgumi-sort-cli/Cargo.tomlcrates/fgumi-sort-cli/src/chains.rscrates/fgumi-sort-cli/src/lib.rscrates/fgumi-sort-cli/src/sort.rscrates/fgumi-sort-cli/src/version.rssrc/lib/commands/command.rssrc/lib/commands/common.rssrc/lib/commands/sort.rssrc/lib/pipeline/chains/commands/sort.rssrc/lib/pipeline/steps/sink/write_bgzf.rssrc/lib/pipeline/steps/sort/and_spill/tests.rssrc/lib/pipeline/steps/sort/merge/tests.rssrc/lib/pipeline/steps/sort/mod.rssrc/lib/pipeline/steps/sort/spill_decompress.rssrc/lib/pipeline/steps/sort/spill_decompress/tests.rssrc/lib/pipeline/steps/source/read_bam.rssrc/lib/pipeline/steps/types.rssrc/main.rs
💤 Files with no reviewable changes (4)
- src/lib/pipeline/steps/sort/merge/tests.rs
- src/lib/pipeline/steps/sort/spill_decompress.rs
- src/lib/pipeline/steps/sort/spill_decompress/tests.rs
- src/lib/pipeline/steps/sort/and_spill/tests.rs
nh13
force-pushed
the
446/nh/feat-sort-cli
branch
from
June 20, 2026 22:45
2ae025e to
e382c96
Compare
Member
Author
|
@coderabbitai resume |
Member
Author
|
@coderabbitai review |
✅ Action performedReviews resumed. |
✅ Action performedReview finished.
|
Member
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
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-pipeline-io/src/sink/write_bgzf.rs`:
- Around line 159-168: The Drop implementation for WriteBgzfFile is writing
BGZF_EOF even when the sink is dropped abnormally (before the normal finish
path), which causes incomplete output to appear as valid BGZF streams. Remove
the lines that write BGZF_EOF (the write_all(&BGZF_EOF) and flush() calls) from
the Drop implementation. Instead, ensure BGZF_EOF is only written through an
explicit finish or drain method, not during cleanup. This prevents silent
corruption where partial output becomes seemingly complete.
🪄 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: 1ad39900-5a7f-41a7-a561-86f059e53355
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock,!**/*.lock
📒 Files selected for processing (39)
Cargo.tomlcrates/fgumi-cli-common/Cargo.tomlcrates/fgumi-cli-common/src/lib.rscrates/fgumi-pipeline-core/src/handles.rscrates/fgumi-pipeline-io/Cargo.tomlcrates/fgumi-pipeline-io/src/lib.rscrates/fgumi-pipeline-io/src/sink/mod.rscrates/fgumi-pipeline-io/src/sink/write_bgzf.rscrates/fgumi-pipeline-io/src/sort/and_spill.rscrates/fgumi-pipeline-io/src/sort/and_spill/tests.rscrates/fgumi-pipeline-io/src/sort/merge.rscrates/fgumi-pipeline-io/src/sort/merge/tests.rscrates/fgumi-pipeline-io/src/sort/mod.rscrates/fgumi-pipeline-io/src/sort/protocol.rscrates/fgumi-pipeline-io/src/sort/spill_decompress.rscrates/fgumi-pipeline-io/src/sort/spill_decompress/tests.rscrates/fgumi-pipeline-io/src/sort/tests.rscrates/fgumi-pipeline-io/src/source/mod.rscrates/fgumi-pipeline-io/src/source/read_bam.rscrates/fgumi-pipeline-io/src/types.rscrates/fgumi-sort-cli/Cargo.tomlcrates/fgumi-sort-cli/src/chains.rscrates/fgumi-sort-cli/src/lib.rscrates/fgumi-sort-cli/src/sort.rscrates/fgumi-sort-cli/src/version.rssrc/lib/commands/command.rssrc/lib/commands/common.rssrc/lib/commands/sort.rssrc/lib/pipeline/chains/builder.rssrc/lib/pipeline/chains/commands/sort.rssrc/lib/pipeline/steps/sink/write_bgzf.rssrc/lib/pipeline/steps/sort/and_spill/tests.rssrc/lib/pipeline/steps/sort/merge/tests.rssrc/lib/pipeline/steps/sort/mod.rssrc/lib/pipeline/steps/sort/spill_decompress.rssrc/lib/pipeline/steps/sort/spill_decompress/tests.rssrc/lib/pipeline/steps/source/read_bam.rssrc/lib/pipeline/steps/types.rssrc/main.rs
💤 Files with no reviewable changes (4)
- src/lib/pipeline/steps/sort/merge/tests.rs
- src/lib/pipeline/steps/sort/and_spill/tests.rs
- src/lib/pipeline/steps/sort/spill_decompress/tests.rs
- src/lib/pipeline/steps/sort/spill_decompress.rs
Member
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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-cli-common/src/lib.rs`:
- Around line 548-549: The compression_level field in the struct is missing a
range validator to enforce the documented 1-12 bounds for valid compression
levels. Add a value_parser with range validation to the existing #[arg]
attribute on the compression_level field by specifying
value_parser!(u32).range(1..=12) to reject out-of-range inputs immediately at
parse time instead of allowing any u32 value to be accepted.
In `@crates/fgumi-pipeline-io/src/types.rs`:
- Around line 254-261: The assertion in the test function
`record_batch_total_bytes_sums_record_lengths` at line 260 uses a hardcoded
expected heap_size value that assumes Vec::with_capacity allocates exactly the
requested capacity, but allocators may allocate larger blocks. Instead of using
the hardcoded formula, capture the actual allocated capacities of the internal
backing and ranges vectors after constructing the RecordBatch object, then
calculate the expected heap_size based on those actual capacities. Follow the
same pattern demonstrated in the earlier test function
`heap_size_counts_allocated_capacity_not_logical_len` in this module, which
already shows the correct approach for handling variable allocator behavior.
In `@crates/fgumi-sort-cli/src/sort.rs`:
- Around line 474-477: The IndexBamFinalizeHook finalization is currently
running unconditionally when self.write_index is true, even if the pipeline.run
operation failed. The fix is to gate the finalization on successful sort
completion by ensuring IndexBamFinalizeHook { output_path }.finalize() is only
called when run_result is Ok. Modify the conditional logic to check
run_result.is_ok() in addition to self.write_index before executing the finalize
call, or use and_then to chain the operations so finalization only happens after
a successful pipeline run result.
🪄 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: 47f40acc-ba6c-4eea-b094-74ac6a54b872
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock,!**/*.lock
📒 Files selected for processing (40)
.coderabbit.yamlCargo.tomlcrates/fgumi-cli-common/Cargo.tomlcrates/fgumi-cli-common/src/lib.rscrates/fgumi-pipeline-core/src/handles.rscrates/fgumi-pipeline-io/Cargo.tomlcrates/fgumi-pipeline-io/src/lib.rscrates/fgumi-pipeline-io/src/sink/mod.rscrates/fgumi-pipeline-io/src/sink/write_bgzf.rscrates/fgumi-pipeline-io/src/sort/and_spill.rscrates/fgumi-pipeline-io/src/sort/and_spill/tests.rscrates/fgumi-pipeline-io/src/sort/merge.rscrates/fgumi-pipeline-io/src/sort/merge/tests.rscrates/fgumi-pipeline-io/src/sort/mod.rscrates/fgumi-pipeline-io/src/sort/protocol.rscrates/fgumi-pipeline-io/src/sort/spill_decompress.rscrates/fgumi-pipeline-io/src/sort/spill_decompress/tests.rscrates/fgumi-pipeline-io/src/sort/tests.rscrates/fgumi-pipeline-io/src/source/mod.rscrates/fgumi-pipeline-io/src/source/read_bam.rscrates/fgumi-pipeline-io/src/types.rscrates/fgumi-sort-cli/Cargo.tomlcrates/fgumi-sort-cli/src/chains.rscrates/fgumi-sort-cli/src/lib.rscrates/fgumi-sort-cli/src/sort.rscrates/fgumi-sort-cli/src/version.rssrc/lib/commands/command.rssrc/lib/commands/common.rssrc/lib/commands/sort.rssrc/lib/pipeline/chains/builder.rssrc/lib/pipeline/chains/commands/sort.rssrc/lib/pipeline/steps/sink/write_bgzf.rssrc/lib/pipeline/steps/sort/and_spill/tests.rssrc/lib/pipeline/steps/sort/merge/tests.rssrc/lib/pipeline/steps/sort/mod.rssrc/lib/pipeline/steps/sort/spill_decompress.rssrc/lib/pipeline/steps/sort/spill_decompress/tests.rssrc/lib/pipeline/steps/source/read_bam.rssrc/lib/pipeline/steps/types.rssrc/main.rs
💤 Files with no reviewable changes (4)
- src/lib/pipeline/steps/sort/spill_decompress/tests.rs
- src/lib/pipeline/steps/sort/merge/tests.rs
- src/lib/pipeline/steps/sort/and_spill/tests.rs
- src/lib/pipeline/steps/sort/spill_decompress.rs
nh13
force-pushed
the
446/nh/feat-sort-cli
branch
from
June 21, 2026 10:10
97f3e47 to
72aa4bd
Compare
Member
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/fgumi-pipeline-io/src/sort/protocol.rs (1)
80-87:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCharge a non-zero heap cost for control events.
SpillReadycan carry an empty path andAllAnnouncedreports0, so the byte-bounded event queues can still accept an unbounded count of control events. Add a fixed per-event cost while preserving theMemoryChunkpayload accounting.As per coding guidelines, no transport queue may grow without a byte/size bound; memory must be a function of configuration.
Proposed fix
impl HeapSize for SortPhase1Event { fn heap_size(&self) -> usize { + let base = std::mem::size_of::<Self>(); match self { - Self::SpillReady { path, .. } => path.as_os_str().len(), - Self::MemoryChunk { chunk, .. } => chunk.approx_heap_bytes(), - Self::AllAnnounced { .. } => 0, + Self::SpillReady { path, .. } => base + path.as_os_str().len(), + Self::MemoryChunk { chunk, .. } => base + chunk.approx_heap_bytes(), + Self::AllAnnounced { .. } => base, } } } @@ impl HeapSize for SortPhase2Event { fn heap_size(&self) -> usize { + let base = std::mem::size_of::<Self>(); match self { - Self::SpillReady { path, .. } => path.as_os_str().len(), - Self::MemoryChunk { chunk, .. } => chunk.approx_heap_bytes(), - Self::AllAnnounced { .. } => 0, + Self::SpillReady { path, .. } => base + path.as_os_str().len(), + Self::MemoryChunk { chunk, .. } => base + chunk.approx_heap_bytes(), + Self::AllAnnounced { .. } => base, } } }Also applies to: 100-107
🤖 Prompt for 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. In `@crates/fgumi-pipeline-io/src/sort/protocol.rs` around lines 80 - 87, The HeapSize implementation for SortPhase1Event currently reports zero heap cost for control events (SpillReady and AllAnnounced), allowing unbounded counts of these events in byte-bounded queues. Modify the heap_size method to add a fixed per-event cost for both SpillReady and AllAnnounced variants (for example, a small constant overhead per control event) while preserving the existing MemoryChunk payload accounting. This ensures control events contribute to the byte budget and cannot accumulate unboundedly. Apply the same fix to any other similar HeapSize implementations for related event types.Source: Coding guidelines
🤖 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-cli-common/src/lib.rs`:
- Around line 648-674: Add `rstest = "0"` to the `[dev-dependencies]` section in
`crates/fgumi-cli-common/Cargo.toml`. Then refactor the
`test_compression_level_rejects_out_of_range` function by splitting it into two
separate test functions: create a new parameterized test using the `#[rstest]`
macro with the parameter values [0, 1, 6, 12] that tests the in-range
compression levels, and keep the default value assertion and out-of-range
rejection assertions in a separate standard test function. Use the `#[rstest]`
procedural macro pattern consistent with other tests in the workspace to
parameterize the compression level values rather than using a manual for loop.
---
Outside diff comments:
In `@crates/fgumi-pipeline-io/src/sort/protocol.rs`:
- Around line 80-87: The HeapSize implementation for SortPhase1Event currently
reports zero heap cost for control events (SpillReady and AllAnnounced),
allowing unbounded counts of these events in byte-bounded queues. Modify the
heap_size method to add a fixed per-event cost for both SpillReady and
AllAnnounced variants (for example, a small constant overhead per control event)
while preserving the existing MemoryChunk payload accounting. This ensures
control events contribute to the byte budget and cannot accumulate unboundedly.
Apply the same fix to any other similar HeapSize implementations for related
event types.
🪄 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: 1a39adec-3d38-49a3-b43b-ec801ce9fce7
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock,!**/*.lock
📒 Files selected for processing (39)
Cargo.tomlcrates/fgumi-cli-common/Cargo.tomlcrates/fgumi-cli-common/src/lib.rscrates/fgumi-pipeline-core/src/handles.rscrates/fgumi-pipeline-io/Cargo.tomlcrates/fgumi-pipeline-io/src/lib.rscrates/fgumi-pipeline-io/src/sink/mod.rscrates/fgumi-pipeline-io/src/sink/write_bgzf.rscrates/fgumi-pipeline-io/src/sort/and_spill.rscrates/fgumi-pipeline-io/src/sort/and_spill/tests.rscrates/fgumi-pipeline-io/src/sort/merge.rscrates/fgumi-pipeline-io/src/sort/merge/tests.rscrates/fgumi-pipeline-io/src/sort/mod.rscrates/fgumi-pipeline-io/src/sort/protocol.rscrates/fgumi-pipeline-io/src/sort/spill_decompress.rscrates/fgumi-pipeline-io/src/sort/spill_decompress/tests.rscrates/fgumi-pipeline-io/src/sort/tests.rscrates/fgumi-pipeline-io/src/source/mod.rscrates/fgumi-pipeline-io/src/source/read_bam.rscrates/fgumi-pipeline-io/src/types.rscrates/fgumi-sort-cli/Cargo.tomlcrates/fgumi-sort-cli/src/chains.rscrates/fgumi-sort-cli/src/lib.rscrates/fgumi-sort-cli/src/sort.rscrates/fgumi-sort-cli/src/version.rssrc/lib/commands/command.rssrc/lib/commands/common.rssrc/lib/commands/sort.rssrc/lib/pipeline/chains/builder.rssrc/lib/pipeline/chains/commands/sort.rssrc/lib/pipeline/steps/sink/write_bgzf.rssrc/lib/pipeline/steps/sort/and_spill/tests.rssrc/lib/pipeline/steps/sort/merge/tests.rssrc/lib/pipeline/steps/sort/mod.rssrc/lib/pipeline/steps/sort/spill_decompress.rssrc/lib/pipeline/steps/sort/spill_decompress/tests.rssrc/lib/pipeline/steps/source/read_bam.rssrc/lib/pipeline/steps/types.rssrc/main.rs
💤 Files with no reviewable changes (4)
- src/lib/pipeline/steps/sort/merge/tests.rs
- src/lib/pipeline/steps/sort/and_spill/tests.rs
- src/lib/pipeline/steps/sort/spill_decompress/tests.rs
- src/lib/pipeline/steps/sort/spill_decompress.rs
nh13
force-pushed
the
446/nh/feat-sort-cli
branch
2 times, most recently
from
June 21, 2026 19:27
ebc5567 to
6af43c3
Compare
Member
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
Member
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
Member
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/fgumi-pipeline-io/src/sort/merge.rs (1)
442-450:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFail closed when setup is incomplete at input drain.
If
ctx.inputis drained whileis_ready_to_merge()is still false, this path still transitions toMerging, which can silently merge an incomplete setup. Return an error for any non-empty/partially-announced setup; only allow the explicit empty-input case.Proposed fix
@@ if matches!(&self.state, SortMergeState::WaitingForSetup { .. }) { @@ let absorbed = self.absorb_events_into_setup(ctx); if !self.is_ready_to_merge() { if absorbed > 0 { return Ok(StepOutcome::Progress); } if !ctx.input.is_drained() { return Ok(StepOutcome::NoProgress); } + + let SortMergeState::WaitingForSetup { + slots, + memory_chunks, + expected_slot_count, + expected_memory_chunk_count, + .. + } = &self.state + else { + unreachable!("state checked above"); + }; + + let saw_payload = !slots.is_empty() || memory_chunks.total_len() > 0; + let saw_expectations = + expected_slot_count.is_some() || expected_memory_chunk_count.is_some(); + if saw_payload || saw_expectations { + return Err(io::Error::other(format!( + "SortMerge: setup incomplete at drain (slots={}, chunks={}, expected_slots={expected_slot_count:?}, expected_chunks={expected_memory_chunk_count:?})", + slots.len(), + memory_chunks.total_len() + ))); + } } self.transition_to_merging(); }🤖 Prompt for 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. In `@crates/fgumi-pipeline-io/src/sort/merge.rs` around lines 442 - 450, When ctx.input is drained but is_ready_to_merge() returns false, the code currently transitions to merging unconditionally, which allows incomplete setups to proceed silently. Instead of calling transition_to_merging() at the end of this block, first check that is_ready_to_merge() is true before transitioning; if the setup is incomplete when input is drained, return an error to fail closed rather than allowing a partial merge to proceed.
🤖 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-cli-common/src/lib.rs`:
- Around line 542-550: The CompressionOptions struct derives Default, which
initializes compression_level to 0 instead of the intended default of 1, causing
a mismatch with the CLI default_value_t. Remove Default from the derive macro on
CompressionOptions, then manually implement the Default trait for
CompressionOptions to explicitly set compression_level to 1. Additionally, add a
test that verifies CompressionOptions::default().compression_level equals 1 to
prevent future drift between the programmatic and CLI defaults.
---
Outside diff comments:
In `@crates/fgumi-pipeline-io/src/sort/merge.rs`:
- Around line 442-450: When ctx.input is drained but is_ready_to_merge() returns
false, the code currently transitions to merging unconditionally, which allows
incomplete setups to proceed silently. Instead of calling
transition_to_merging() at the end of this block, first check that
is_ready_to_merge() is true before transitioning; if the setup is incomplete
when input is drained, return an error to fail closed rather than allowing a
partial merge to proceed.
🪄 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: cdf56c48-9191-41b2-8062-ecca0b39132e
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock,!**/*.lock
📒 Files selected for processing (39)
Cargo.tomlcrates/fgumi-cli-common/Cargo.tomlcrates/fgumi-cli-common/src/lib.rscrates/fgumi-pipeline-core/src/handles.rscrates/fgumi-pipeline-io/Cargo.tomlcrates/fgumi-pipeline-io/src/lib.rscrates/fgumi-pipeline-io/src/sink/mod.rscrates/fgumi-pipeline-io/src/sink/write_bgzf.rscrates/fgumi-pipeline-io/src/sort/and_spill.rscrates/fgumi-pipeline-io/src/sort/and_spill/tests.rscrates/fgumi-pipeline-io/src/sort/merge.rscrates/fgumi-pipeline-io/src/sort/merge/tests.rscrates/fgumi-pipeline-io/src/sort/mod.rscrates/fgumi-pipeline-io/src/sort/protocol.rscrates/fgumi-pipeline-io/src/sort/spill_decompress.rscrates/fgumi-pipeline-io/src/sort/spill_decompress/tests.rscrates/fgumi-pipeline-io/src/sort/tests.rscrates/fgumi-pipeline-io/src/source/mod.rscrates/fgumi-pipeline-io/src/source/read_bam.rscrates/fgumi-pipeline-io/src/types.rscrates/fgumi-sort-cli/Cargo.tomlcrates/fgumi-sort-cli/src/chains.rscrates/fgumi-sort-cli/src/lib.rscrates/fgumi-sort-cli/src/sort.rscrates/fgumi-sort-cli/src/version.rssrc/lib/commands/command.rssrc/lib/commands/common.rssrc/lib/commands/sort.rssrc/lib/pipeline/chains/builder.rssrc/lib/pipeline/chains/commands/sort.rssrc/lib/pipeline/steps/sink/write_bgzf.rssrc/lib/pipeline/steps/sort/and_spill/tests.rssrc/lib/pipeline/steps/sort/merge/tests.rssrc/lib/pipeline/steps/sort/mod.rssrc/lib/pipeline/steps/sort/spill_decompress.rssrc/lib/pipeline/steps/sort/spill_decompress/tests.rssrc/lib/pipeline/steps/source/read_bam.rssrc/lib/pipeline/steps/types.rssrc/main.rs
💤 Files with no reviewable changes (4)
- src/lib/pipeline/steps/sort/merge/tests.rs
- src/lib/pipeline/steps/sort/spill_decompress/tests.rs
- src/lib/pipeline/steps/sort/and_spill/tests.rs
- src/lib/pipeline/steps/sort/spill_decompress.rs
…446) Extract the `fgumi sort` command and its pipeline steps into three new crates so sort-only downstreams (e.g. mako) can depend on sort without compiling the UMI/consensus stack: - fgumi-cli-common: shared CLI infrastructure (Command trait, OperationTimer, memory/compression options, validation, memory detection). - fgumi-pipeline-io: generic BGZF source/sink steps (ReadBgzfBlocks, WriteBgzfFile) plus the typed-step sort steps (SortBamFile, SortAndSpill, SortMerge, SortSpillDecompress). - fgumi-sort-cli: the `Sort` command, options, finalize hooks, and step factory. `Sort::execute` builds the typed-step pipeline directly via `fgumi_pipeline_core::Pipeline::builder` rather than the umbrella's monolithic `ChainBuilder`, keeping the dependency graph free of fgumi-consensus, fgumi-umi, and fgumi-simd-fastq. The umbrella `fgumi` crate re-exports the moved items at their original paths so runall, chains, merge, and main.rs compile unchanged; the chains layer imports the sort step and factory from the new crates one-way. Sorted output is byte-identical across coordinate, queryname, queryname::natural, and template-coordinate orders (and stdin), so this is a behavior-preserving extraction.
nh13
force-pushed
the
446/nh/feat-sort-cli
branch
from
June 21, 2026 21:09
6af43c3 to
b2a13ae
Compare
Member
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
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-cli-common/src/lib.rs`:
- Around line 615-620: The test_parse_memory_size_errors function is missing
test coverage for scientific notation rejection, which is handled in the
parse_memory_size function around lines 298-304. Add an additional assert
statement within test_parse_memory_size_errors that calls parse_memory_size with
a scientific notation input (such as "1e9") and verifies that it returns an
error using is_err(), consistent with the existing test assertions for empty
strings, negative numbers, and zero.
🪄 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: 53e7d820-e997-4d29-813c-d81f0a93190c
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock,!**/*.lock
📒 Files selected for processing (39)
Cargo.tomlcrates/fgumi-cli-common/Cargo.tomlcrates/fgumi-cli-common/src/lib.rscrates/fgumi-pipeline-core/src/handles.rscrates/fgumi-pipeline-io/Cargo.tomlcrates/fgumi-pipeline-io/src/lib.rscrates/fgumi-pipeline-io/src/sink/mod.rscrates/fgumi-pipeline-io/src/sink/write_bgzf.rscrates/fgumi-pipeline-io/src/sort/and_spill.rscrates/fgumi-pipeline-io/src/sort/and_spill/tests.rscrates/fgumi-pipeline-io/src/sort/merge.rscrates/fgumi-pipeline-io/src/sort/merge/tests.rscrates/fgumi-pipeline-io/src/sort/mod.rscrates/fgumi-pipeline-io/src/sort/protocol.rscrates/fgumi-pipeline-io/src/sort/spill_decompress.rscrates/fgumi-pipeline-io/src/sort/spill_decompress/tests.rscrates/fgumi-pipeline-io/src/sort/tests.rscrates/fgumi-pipeline-io/src/source/mod.rscrates/fgumi-pipeline-io/src/source/read_bam.rscrates/fgumi-pipeline-io/src/types.rscrates/fgumi-sort-cli/Cargo.tomlcrates/fgumi-sort-cli/src/chains.rscrates/fgumi-sort-cli/src/lib.rscrates/fgumi-sort-cli/src/sort.rscrates/fgumi-sort-cli/src/version.rssrc/lib/commands/command.rssrc/lib/commands/common.rssrc/lib/commands/sort.rssrc/lib/pipeline/chains/builder.rssrc/lib/pipeline/chains/commands/sort.rssrc/lib/pipeline/steps/sink/write_bgzf.rssrc/lib/pipeline/steps/sort/and_spill/tests.rssrc/lib/pipeline/steps/sort/merge/tests.rssrc/lib/pipeline/steps/sort/mod.rssrc/lib/pipeline/steps/sort/spill_decompress.rssrc/lib/pipeline/steps/sort/spill_decompress/tests.rssrc/lib/pipeline/steps/source/read_bam.rssrc/lib/pipeline/steps/types.rssrc/main.rs
💤 Files with no reviewable changes (4)
- src/lib/pipeline/steps/sort/merge/tests.rs
- src/lib/pipeline/steps/sort/spill_decompress/tests.rs
- src/lib/pipeline/steps/sort/and_spill/tests.rs
- src/lib/pipeline/steps/sort/spill_decompress.rs
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
1 participant
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.
Summary
Closes #446. Extracts the
fgumi sortcommand and its pipeline steps into three new crates so a sort-only downstream (e.g. mako) can depend on the full sort command without compiling the UMI/consensus stack. This is a behavior-preserving extraction — no functional changes.Previously the only way to reuse the
sortcommand (clap struct +--order/--verify/memory wiring +@PG/header plumbing) was to depend on the umbrellafgumicrate, which pulls the entire dependency tree (~227 crates) regardless of features, becauseSort::executeroutes through the monolithicpipeline::chains::build_forthat compiles every stage (group →fgumi-umi, simplex/duplex →fgumi-consensus, extract →fgumi-simd-fastq).Approach — framework-light
fgumi sortstill runs on the typed-step pipeline framework (fgumi-pipeline-core); it just builds a single-stagesource → sort → sinkpipeline directly viaPipeline::builder()instead of going through the all-stagesChainBuilder. That keeps the sort path on the unified execution model while dropping the heavy stage dependencies.Three new crates:
fgumi-cli-common— shared CLI infrastructure: theCommandtrait,OperationTimer+ logging helpers, theMemoryLimit/MemoryReserve/CompressionOptions+ parse cluster, file validation, and memory detection.fgumi-pipeline-io— generic BGZF source/sink steps (ReadBgzfBlocks,WriteBgzfFile) plus the typed-step sort steps (SortBamFile,SortAndSpill,SortMerge,SortSpillDecompress).fgumi-sort-cli— theSortcommand,SortOptions/MultiSortOptions,SortOrderArg, the finalize hooks, and the step factory. Its dependency graph isfgumi-pipeline-core+fgumi-pipeline-io+fgumi-sort+fgumi-bam-io+fgumi-sam+fgumi-cli-common+fgumi-cli-macros— nofgumi-consensus,fgumi-umi, orfgumi-simd-fastq.The umbrella
fgumicrate re-exports the moved items at their original module paths, sorunall,chains,merge, andmain.rscompile unchanged. The chains layer /runallimport the sort step and step factory from the new crates one-way (umbrella → new crates); intermediate/fused sort within a chain is untouched.Verification
cargo build --release,cargo ci-test(all pass),cargo ci-lint(clippy pedantic),cargo ci-fmt, andcargo check --workspace --no-default-features --all-targetsall green.fgumi-sort-cliresolves with zero offgumi-consensus/fgumi-umi/fgumi-simd-fastq/nalgebra/matrixmultiply/statrs.--verifyruns through the new crate. (The git-augmented@PG VN/CLstamp is preserved via a process-global version override installed by the umbrella'smain.)Reading order
crates/fgumi-cli-common/src/lib.rs— the shared leaf helpers.crates/fgumi-pipeline-io/— the moved steps (mostly verbatim relocations; git shows them as renames).crates/fgumi-sort-cli/src/{sort.rs,chains.rs}— theSortcommand and its direct pipeline builder.src/lib/commands/{command,common,sort}.rs,src/lib/pipeline/...,src/main.rs).Follow-up
A stacked PR on top of this branch addresses the CodeRabbit findings (all of which are pre-existing — in
fgumi-pipeline-coreand in the verbatim-moved sort steps — and are intentionally kept out of this behavior-preserving extraction).Summary by CodeRabbit
fgumi sort@PGmetadata version reporting and added shared CLI option parsing (memory budgeting and compression level validation).