Repository navigation
feat(bam-io): add the shared grouping and library-lookup domain types (R0) - #734
Conversation
|
Note Reviews pausedUse the following commands to manage reviews:
Use the checkboxes below for quick actions:
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 (2)
WalkthroughAdds shared BAM library indexing and hashing utilities. Adds normalized grouping keys, decoded-record UMI caching, raw BAM key computation, and the ChangesBAM grouping infrastructure
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant BAMRecord
participant GroupKeyConfig
participant LibraryIndex
participant GroupKey
participant Grouper
BAMRecord->>GroupKeyConfig: provide raw BAM record
GroupKeyConfig->>LibraryIndex: hash RG and cell metadata
LibraryIndex-->>GroupKeyConfig: return metadata values
GroupKeyConfig->>GroupKey: build normalized grouping key
GroupKey-->>Grouper: add decoded record
Grouper-->>Grouper: emit completed groups or retain pending state
Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
Comment |
|
@coderabbitai pause |
✅ Action performedReviews paused. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main-runall #734 +/- ##
==============================================
Coverage ? 93.95%
==============================================
Files ? 232
Lines ? 126650
Branches ? 0
==============================================
Hits ? 119000
Misses ? 7650
Partials ? 0 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
@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-bam-io/src/grouping.rs`:
- Around line 574-779: Add a focused test for the primary branch of
compute_group_key_from_raw, constructing a mapped paired record with an MC tag,
an RG tag resolved by a non-default LibraryIndex, and a CB value passed through
cell_tag: Some(CB). Assert the complete GroupKey, including populated mate
ref/position/strand, non-zero library_idx, and cell_hash, then build the mate
and verify both records normalize to the same key. Keep the test’s flag and
input coverage aligned with the corresponding typed/raw sibling implementations.
- Around line 192-203: Make the data field of DecodedRecord private, keeping
raw_bytes_mut as the sole mutable accessor so cache invalidation remains
enforced. Update any necessary access within DecodedRecord’s own implementation
without exposing data to crate-internal callers.
In `@crates/fgumi-bam-io/src/library.rs`:
- Around line 96-193: Add a #[cfg(test)] mod tests alongside LibraryIndex
covering from_header, get, library_name, and build_library_lookup. Construct
headers with missing, literal "unknown", and shared LB values to verify index 0
fallback, deduplication, and stable library names; also assert absent RG hashes
and out-of-range indices return unknown. Verify build_library_lookup keys
entries by RG identifier rather than LB value, using the existing test utilities
and public symbols.
🪄 Autofix
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: b5e82451-7127-4221-a859-b074ee8d7da9
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock,!**/*.lock
📒 Files selected for processing (4)
crates/fgumi-bam-io/Cargo.tomlcrates/fgumi-bam-io/src/grouping.rscrates/fgumi-bam-io/src/lib.rscrates/fgumi-bam-io/src/library.rs
f3698f5 to
804bb73
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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-bam-io/src/grouping.rs`:
- Around line 433-441: Extract the duplicated empty-name handling into a shared
helper near name_hash_key, using LibraryIndex::hash_name(None) for empty names
and Some(name) otherwise. Update both name_hash_key and
compute_group_key_from_raw to call this helper, preserving identical name-hash
behavior between the two paths.
- Around line 646-674: In the grouping test around the paired-key assertions,
replace the non-unknown check for key.pos2 with an assertion against the
expected MC-derived mate coordinate. Remove the tautological expected
GroupKey::paired construction and equality assertion, since it only
re-normalizes key’s own fields; retain the direct assertions for the other key
components.
- Around line 443-476: Update compute_group_key_from_raw so secondary and
supplementary records preserve the TC template coordinate by reusing the
existing read_tc_template_coordinate logic before returning the grouped key,
rather than returning only name_hash. Add coverage for mapped records with an
empty CIGAR and ensure the implementation handles unclipped_5prime_from_raw_bam
returning i32::MAX consistently.
In `@crates/fgumi-bam-io/src/library.rs`:
- Around line 20-35: Update the public documentation comments around
`LibraryLookup` and the referenced sections to wrap Rust identifiers and type
names—including `LB`, `ReadInfo::from`, `GroupKey`, `AHash`, and `u16`—in
backticks, while preserving the existing prose and links.
🪄 Autofix
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: 456a9e1f-fe71-44dc-9804-e23fdd949386
📒 Files selected for processing (2)
crates/fgumi-bam-io/src/grouping.rscrates/fgumi-bam-io/src/library.rs
The typed-step pipeline tree needs `DecodedRecord`, `GroupKey`,
`GroupKeyConfig`, `Grouper`, `compute_group_key_from_raw` and
`name_hash_key`. Today those live in `src/lib/unified_pipeline/{base,bam}.rs`
(and `name_hash_key` does not exist at all), so the new tree would have to
depend on the tree it is meant to replace.
Move them next to the raw-record helpers they operate on, in `fgumi-bam-io`,
along with the `LibraryLookup`/`LibraryIndex` read-group-to-library machinery
the group key depends on. Both files are self-contained — their only imports are
`noodles::sam` and each other.
`unified_pipeline` keeps its own copies for now; the duplicate is retired when
that tree is deleted.
804bb73 to
fbc534f
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
…one definition `DecodedRecord`, `GroupKey`, `GroupKeyConfig`, `LibraryIndex`, `LibraryLookup`, `build_library_lookup`, and the `Grouper` trait each existed twice: once in `fgumi-bam-io` (ported in #734) and once in `unified_pipeline` / `read_info`. The copies were textually identical apart from field visibility and doc formatting, and the duplication was deliberate — #734 left the umbrella copies in place so nothing had to be rewired. That worked while the ported step library only ever *produced* these values. It stops working at the first ported step that hands one to an umbrella grouper: `GroupBam` calls `TemplateGrouper::add_records`, and the two identical definitions are two incompatible types, so the call cannot be written at all. Keep the `fgumi-bam-io` definition and re-export it from both former homes, so every existing `crate::unified_pipeline::DecodedRecord` and `crate::read_info::LibraryIndex` path resolves unchanged. Two supporting changes fall out: - `GroupKey::has_mate_position` only existed on the umbrella copy; ported to `fgumi-bam-io` so the surviving definition is a superset of both. - `grouper.rs` reached into `DecodedRecord`'s fields directly, which are private on the `fgumi-bam-io` copy (made so during #734's review). Those five sites now go through `into_raw_bytes` / `record` / `raw_bytes`, which is what the accessors are for. Net -483 lines with no behavior change; the full suite is unchanged at 8322.
…one definition `DecodedRecord`, `GroupKey`, `GroupKeyConfig`, `LibraryIndex`, `LibraryLookup`, `build_library_lookup`, and the `Grouper` trait each existed twice: once in `fgumi-bam-io` (ported in #734) and once in `unified_pipeline` / `read_info`. The copies were textually identical apart from field visibility and doc formatting, and the duplication was deliberate — #734 left the umbrella copies in place so nothing had to be rewired. That worked while the ported step library only ever *produced* these values. It stops working at the first ported step that hands one to an umbrella grouper: `GroupBam` calls `TemplateGrouper::add_records`, and the two identical definitions are two incompatible types, so the call cannot be written at all. Keep the `fgumi-bam-io` definition and re-export it from both former homes, so every existing `crate::unified_pipeline::DecodedRecord` and `crate::read_info::LibraryIndex` path resolves unchanged. Two supporting changes fall out: - `GroupKey::has_mate_position` only existed on the umbrella copy; ported to `fgumi-bam-io` so the surviving definition is a superset of both. - `grouper.rs` reached into `DecodedRecord`'s fields directly, which are private on the `fgumi-bam-io` copy (made so during #734's review). Those five sites now go through `into_raw_bytes` / `record` / `raw_bytes`, which is what the accessors are for. Net -483 lines with no behavior change; the full suite is unchanged at 8322.
…one definition `DecodedRecord`, `GroupKey`, `GroupKeyConfig`, `LibraryIndex`, `LibraryLookup`, `build_library_lookup`, and the `Grouper` trait each existed twice: once in `fgumi-bam-io` (ported in #734) and once in `unified_pipeline` / `read_info`. The copies were textually identical apart from field visibility and doc formatting, and the duplication was deliberate — #734 left the umbrella copies in place so nothing had to be rewired. That worked while the ported step library only ever *produced* these values. It stops working at the first ported step that hands one to an umbrella grouper: `GroupBam` calls `TemplateGrouper::add_records`, and the two identical definitions are two incompatible types, so the call cannot be written at all. Keep the `fgumi-bam-io` definition and re-export it from both former homes, so every existing `crate::unified_pipeline::DecodedRecord` and `crate::read_info::LibraryIndex` path resolves unchanged. Two supporting changes fall out: - `GroupKey::has_mate_position` only existed on the umbrella copy; ported to `fgumi-bam-io` so the surviving definition is a superset of both. - `grouper.rs` reached into `DecodedRecord`'s fields directly, which are private on the `fgumi-bam-io` copy (made so during #734's review). Those five sites now go through `into_raw_bytes` / `record` / `raw_bytes`, which is what the accessors are for. Net -483 lines with no behavior change; the full suite is unchanged at 8322.
…one definition `DecodedRecord`, `GroupKey`, `GroupKeyConfig`, `LibraryIndex`, `LibraryLookup`, `build_library_lookup`, and the `Grouper` trait each existed twice: once in `fgumi-bam-io` (ported in #734) and once in `unified_pipeline` / `read_info`. The copies were textually identical apart from field visibility and doc formatting, and the duplication was deliberate — #734 left the umbrella copies in place so nothing had to be rewired. That worked while the ported step library only ever *produced* these values. It stops working at the first ported step that hands one to an umbrella grouper: `GroupBam` calls `TemplateGrouper::add_records`, and the two identical definitions are two incompatible types, so the call cannot be written at all. Keep the `fgumi-bam-io` definition and re-export it from both former homes, so every existing `crate::unified_pipeline::DecodedRecord` and `crate::read_info::LibraryIndex` path resolves unchanged. Two supporting changes fall out: - `GroupKey::has_mate_position` only existed on the umbrella copy; ported to `fgumi-bam-io` so the surviving definition is a superset of both. - `grouper.rs` reached into `DecodedRecord`'s fields directly, which are private on the `fgumi-bam-io` copy (made so during #734's review). Those five sites now go through `into_raw_bytes` / `record` / `raw_bytes`, which is what the accessors are for. Net -483 lines with no behavior change; the full suite is unchanged at 8322.
…one definition `DecodedRecord`, `GroupKey`, `GroupKeyConfig`, `LibraryIndex`, `LibraryLookup`, `build_library_lookup`, and the `Grouper` trait each existed twice: once in `fgumi-bam-io` (ported in #734) and once in `unified_pipeline` / `read_info`. The copies were textually identical apart from field visibility and doc formatting, and the duplication was deliberate — #734 left the umbrella copies in place so nothing had to be rewired. That worked while the ported step library only ever *produced* these values. It stops working at the first ported step that hands one to an umbrella grouper: `GroupBam` calls `TemplateGrouper::add_records`, and the two identical definitions are two incompatible types, so the call cannot be written at all. Keep the `fgumi-bam-io` definition and re-export it from both former homes, so every existing `crate::unified_pipeline::DecodedRecord` and `crate::read_info::LibraryIndex` path resolves unchanged. Two supporting changes fall out: - `GroupKey::has_mate_position` only existed on the umbrella copy; ported to `fgumi-bam-io` so the surviving definition is a superset of both. - `grouper.rs` reached into `DecodedRecord`'s fields directly, which are private on the `fgumi-bam-io` copy (made so during #734's review). Those five sites now go through `into_raw_bytes` / `record` / `raw_bytes`, which is what the accessors are for. Net -483 lines with no behavior change; the full suite is unchanged at 8322.
…one definition `DecodedRecord`, `GroupKey`, `GroupKeyConfig`, `LibraryIndex`, `LibraryLookup`, `build_library_lookup`, and the `Grouper` trait each existed twice: once in `fgumi-bam-io` (ported in #734) and once in `unified_pipeline` / `read_info`. The copies were textually identical apart from field visibility and doc formatting, and the duplication was deliberate — #734 left the umbrella copies in place so nothing had to be rewired. That worked while the ported step library only ever *produced* these values. It stops working at the first ported step that hands one to an umbrella grouper: `GroupBam` calls `TemplateGrouper::add_records`, and the two identical definitions are two incompatible types, so the call cannot be written at all. Keep the `fgumi-bam-io` definition and re-export it from both former homes, so every existing `crate::unified_pipeline::DecodedRecord` and `crate::read_info::LibraryIndex` path resolves unchanged. Two supporting changes fall out: - `GroupKey::has_mate_position` only existed on the umbrella copy; ported to `fgumi-bam-io` so the surviving definition is a superset of both. - `grouper.rs` reached into `DecodedRecord`'s fields directly, which are private on the `fgumi-bam-io` copy (made so during #734's review). Those five sites now go through `into_raw_bytes` / `record` / `raw_bytes`, which is what the accessors are for. Net -483 lines with no behavior change; the full suite is unchanged at 8322.
…one definition `DecodedRecord`, `GroupKey`, `GroupKeyConfig`, `LibraryIndex`, `LibraryLookup`, `build_library_lookup`, and the `Grouper` trait each existed twice: once in `fgumi-bam-io` (ported in #734) and once in `unified_pipeline` / `read_info`. The copies were textually identical apart from field visibility and doc formatting, and the duplication was deliberate — #734 left the umbrella copies in place so nothing had to be rewired. That worked while the ported step library only ever *produced* these values. It stops working at the first ported step that hands one to an umbrella grouper: `GroupBam` calls `TemplateGrouper::add_records`, and the two identical definitions are two incompatible types, so the call cannot be written at all. Keep the `fgumi-bam-io` definition and re-export it from both former homes, so every existing `crate::unified_pipeline::DecodedRecord` and `crate::read_info::LibraryIndex` path resolves unchanged. Two supporting changes fall out: - `GroupKey::has_mate_position` only existed on the umbrella copy; ported to `fgumi-bam-io` so the surviving definition is a superset of both. - `grouper.rs` reached into `DecodedRecord`'s fields directly, which are private on the `fgumi-bam-io` copy (made so during #734's review). Those five sites now go through `into_raw_bytes` / `record` / `raw_bytes`, which is what the accessors are for. Net -483 lines with no behavior change; the full suite is unchanged at 8322.
…one definition `DecodedRecord`, `GroupKey`, `GroupKeyConfig`, `LibraryIndex`, `LibraryLookup`, `build_library_lookup`, and the `Grouper` trait each existed twice: once in `fgumi-bam-io` (ported in #734) and once in `unified_pipeline` / `read_info`. The copies were textually identical apart from field visibility and doc formatting, and the duplication was deliberate — #734 left the umbrella copies in place so nothing had to be rewired. That worked while the ported step library only ever *produced* these values. It stops working at the first ported step that hands one to an umbrella grouper: `GroupBam` calls `TemplateGrouper::add_records`, and the two identical definitions are two incompatible types, so the call cannot be written at all. Keep the `fgumi-bam-io` definition and re-export it from both former homes, so every existing `crate::unified_pipeline::DecodedRecord` and `crate::read_info::LibraryIndex` path resolves unchanged. Two supporting changes fall out: - `GroupKey::has_mate_position` only existed on the umbrella copy; ported to `fgumi-bam-io` so the surviving definition is a superset of both. - `grouper.rs` reached into `DecodedRecord`'s fields directly, which are private on the `fgumi-bam-io` copy (made so during #734's review). Those five sites now go through `into_raw_bytes` / `record` / `raw_bytes`, which is what the accessors are for. Net -483 lines with no behavior change; the full suite is unchanged at 8322.
…one definition `DecodedRecord`, `GroupKey`, `GroupKeyConfig`, `LibraryIndex`, `LibraryLookup`, `build_library_lookup`, and the `Grouper` trait each existed twice: once in `fgumi-bam-io` (ported in #734) and once in `unified_pipeline` / `read_info`. The copies were textually identical apart from field visibility and doc formatting, and the duplication was deliberate — #734 left the umbrella copies in place so nothing had to be rewired. That worked while the ported step library only ever *produced* these values. It stops working at the first ported step that hands one to an umbrella grouper: `GroupBam` calls `TemplateGrouper::add_records`, and the two identical definitions are two incompatible types, so the call cannot be written at all. Keep the `fgumi-bam-io` definition and re-export it from both former homes, so every existing `crate::unified_pipeline::DecodedRecord` and `crate::read_info::LibraryIndex` path resolves unchanged. Two supporting changes fall out: - `GroupKey::has_mate_position` only existed on the umbrella copy; ported to `fgumi-bam-io` so the surviving definition is a superset of both. - `grouper.rs` reached into `DecodedRecord`'s fields directly, which are private on the `fgumi-bam-io` copy (made so during #734's review). Those five sites now go through `into_raw_bytes` / `record` / `raw_bytes`, which is what the accessors are for. Net -483 lines with no behavior change; the full suite is unchanged at 8322.
…one definition `DecodedRecord`, `GroupKey`, `GroupKeyConfig`, `LibraryIndex`, `LibraryLookup`, `build_library_lookup`, and the `Grouper` trait each existed twice: once in `fgumi-bam-io` (ported in #734) and once in `unified_pipeline` / `read_info`. The copies were textually identical apart from field visibility and doc formatting, and the duplication was deliberate — #734 left the umbrella copies in place so nothing had to be rewired. That worked while the ported step library only ever *produced* these values. It stops working at the first ported step that hands one to an umbrella grouper: `GroupBam` calls `TemplateGrouper::add_records`, and the two identical definitions are two incompatible types, so the call cannot be written at all. Keep the `fgumi-bam-io` definition and re-export it from both former homes, so every existing `crate::unified_pipeline::DecodedRecord` and `crate::read_info::LibraryIndex` path resolves unchanged. Two supporting changes fall out: - `GroupKey::has_mate_position` only existed on the umbrella copy; ported to `fgumi-bam-io` so the surviving definition is a superset of both. - `grouper.rs` reached into `DecodedRecord`'s fields directly, which are private on the `fgumi-bam-io` copy (made so during #734's review). Those five sites now go through `into_raw_bytes` / `record` / `raw_bytes`, which is what the accessors are for. Net -483 lines with no behavior change; the full suite is unchanged at 8322.
…one definition `DecodedRecord`, `GroupKey`, `GroupKeyConfig`, `LibraryIndex`, `LibraryLookup`, `build_library_lookup`, and the `Grouper` trait each existed twice: once in `fgumi-bam-io` (ported in #734) and once in `unified_pipeline` / `read_info`. The copies were textually identical apart from field visibility and doc formatting, and the duplication was deliberate — #734 left the umbrella copies in place so nothing had to be rewired. That worked while the ported step library only ever *produced* these values. It stops working at the first ported step that hands one to an umbrella grouper: `GroupBam` calls `TemplateGrouper::add_records`, and the two identical definitions are two incompatible types, so the call cannot be written at all. Keep the `fgumi-bam-io` definition and re-export it from both former homes, so every existing `crate::unified_pipeline::DecodedRecord` and `crate::read_info::LibraryIndex` path resolves unchanged. Two supporting changes fall out: - `GroupKey::has_mate_position` only existed on the umbrella copy; ported to `fgumi-bam-io` so the surviving definition is a superset of both. - `grouper.rs` reached into `DecodedRecord`'s fields directly, which are private on the `fgumi-bam-io` copy (made so during #734's review). Those five sites now go through `into_raw_bytes` / `record` / `raw_bytes`, which is what the accessors are for. Net -483 lines with no behavior change; the full suite is unchanged at 8322.
…one definition (#741) `DecodedRecord`, `GroupKey`, `GroupKeyConfig`, `LibraryIndex`, `LibraryLookup`, `build_library_lookup`, and the `Grouper` trait each existed twice: once in `fgumi-bam-io` (ported in #734) and once in `unified_pipeline` / `read_info`. The copies were textually identical apart from field visibility and doc formatting, and the duplication was deliberate — #734 left the umbrella copies in place so nothing had to be rewired. That worked while the ported step library only ever *produced* these values. It stops working at the first ported step that hands one to an umbrella grouper: `GroupBam` calls `TemplateGrouper::add_records`, and the two identical definitions are two incompatible types, so the call cannot be written at all. Keep the `fgumi-bam-io` definition and re-export it from both former homes, so every existing `crate::unified_pipeline::DecodedRecord` and `crate::read_info::LibraryIndex` path resolves unchanged. Two supporting changes fall out: - `GroupKey::has_mate_position` only existed on the umbrella copy; ported to `fgumi-bam-io` so the surviving definition is a superset of both. - `grouper.rs` reached into `DecodedRecord`'s fields directly, which are private on the `fgumi-bam-io` copy (made so during #734's review). Those five sites now go through `into_raw_bytes` / `record` / `raw_bytes`, which is what the accessors are for. Net -483 lines with no behavior change; the full suite is unchanged at 8322.
…#734) The typed-step pipeline tree needs `DecodedRecord`, `GroupKey`, `GroupKeyConfig`, `Grouper`, `compute_group_key_from_raw` and `name_hash_key`. Today those live in `src/lib/unified_pipeline/{base,bam}.rs` (and `name_hash_key` does not exist at all), so the new tree would have to depend on the tree it is meant to replace. Move them next to the raw-record helpers they operate on, in `fgumi-bam-io`, along with the `LibraryLookup`/`LibraryIndex` read-group-to-library machinery the group key depends on. Both files are self-contained — their only imports are `noodles::sam` and each other. `unified_pipeline` keeps its own copies for now; the duplicate is retired when that tree is deleted.
…one definition (#741) `DecodedRecord`, `GroupKey`, `GroupKeyConfig`, `LibraryIndex`, `LibraryLookup`, `build_library_lookup`, and the `Grouper` trait each existed twice: once in `fgumi-bam-io` (ported in #734) and once in `unified_pipeline` / `read_info`. The copies were textually identical apart from field visibility and doc formatting, and the duplication was deliberate — #734 left the umbrella copies in place so nothing had to be rewired. That worked while the ported step library only ever *produced* these values. It stops working at the first ported step that hands one to an umbrella grouper: `GroupBam` calls `TemplateGrouper::add_records`, and the two identical definitions are two incompatible types, so the call cannot be written at all. Keep the `fgumi-bam-io` definition and re-export it from both former homes, so every existing `crate::unified_pipeline::DecodedRecord` and `crate::read_info::LibraryIndex` path resolves unchanged. Two supporting changes fall out: - `GroupKey::has_mate_position` only existed on the umbrella copy; ported to `fgumi-bam-io` so the surviving definition is a superset of both. - `grouper.rs` reached into `DecodedRecord`'s fields directly, which are private on the `fgumi-bam-io` copy (made so during #734's review). Those five sites now go through `into_raw_bytes` / `record` / `raw_bytes`, which is what the accessors are for. Net -483 lines with no behavior change; the full suite is unchanged at 8322.
…#734) The typed-step pipeline tree needs `DecodedRecord`, `GroupKey`, `GroupKeyConfig`, `Grouper`, `compute_group_key_from_raw` and `name_hash_key`. Today those live in `src/lib/unified_pipeline/{base,bam}.rs` (and `name_hash_key` does not exist at all), so the new tree would have to depend on the tree it is meant to replace. Move them next to the raw-record helpers they operate on, in `fgumi-bam-io`, along with the `LibraryLookup`/`LibraryIndex` read-group-to-library machinery the group key depends on. Both files are self-contained — their only imports are `noodles::sam` and each other. `unified_pipeline` keeps its own copies for now; the duplicate is retired when that tree is deleted.
…one definition (#741) `DecodedRecord`, `GroupKey`, `GroupKeyConfig`, `LibraryIndex`, `LibraryLookup`, `build_library_lookup`, and the `Grouper` trait each existed twice: once in `fgumi-bam-io` (ported in #734) and once in `unified_pipeline` / `read_info`. The copies were textually identical apart from field visibility and doc formatting, and the duplication was deliberate — #734 left the umbrella copies in place so nothing had to be rewired. That worked while the ported step library only ever *produced* these values. It stops working at the first ported step that hands one to an umbrella grouper: `GroupBam` calls `TemplateGrouper::add_records`, and the two identical definitions are two incompatible types, so the call cannot be written at all. Keep the `fgumi-bam-io` definition and re-export it from both former homes, so every existing `crate::unified_pipeline::DecodedRecord` and `crate::read_info::LibraryIndex` path resolves unchanged. Two supporting changes fall out: - `GroupKey::has_mate_position` only existed on the umbrella copy; ported to `fgumi-bam-io` so the surviving definition is a superset of both. - `grouper.rs` reached into `DecodedRecord`'s fields directly, which are private on the `fgumi-bam-io` copy (made so during #734's review). Those five sites now go through `into_raw_bytes` / `record` / `raw_bytes`, which is what the accessors are for. Net -483 lines with no behavior change; the full suite is unchanged at 8322.
…#734) The typed-step pipeline tree needs `DecodedRecord`, `GroupKey`, `GroupKeyConfig`, `Grouper`, `compute_group_key_from_raw` and `name_hash_key`. Today those live in `src/lib/unified_pipeline/{base,bam}.rs` (and `name_hash_key` does not exist at all), so the new tree would have to depend on the tree it is meant to replace. Move them next to the raw-record helpers they operate on, in `fgumi-bam-io`, along with the `LibraryLookup`/`LibraryIndex` read-group-to-library machinery the group key depends on. Both files are self-contained — their only imports are `noodles::sam` and each other. `unified_pipeline` keeps its own copies for now; the duplicate is retired when that tree is deleted.
…one definition (#741) `DecodedRecord`, `GroupKey`, `GroupKeyConfig`, `LibraryIndex`, `LibraryLookup`, `build_library_lookup`, and the `Grouper` trait each existed twice: once in `fgumi-bam-io` (ported in #734) and once in `unified_pipeline` / `read_info`. The copies were textually identical apart from field visibility and doc formatting, and the duplication was deliberate — #734 left the umbrella copies in place so nothing had to be rewired. That worked while the ported step library only ever *produced* these values. It stops working at the first ported step that hands one to an umbrella grouper: `GroupBam` calls `TemplateGrouper::add_records`, and the two identical definitions are two incompatible types, so the call cannot be written at all. Keep the `fgumi-bam-io` definition and re-export it from both former homes, so every existing `crate::unified_pipeline::DecodedRecord` and `crate::read_info::LibraryIndex` path resolves unchanged. Two supporting changes fall out: - `GroupKey::has_mate_position` only existed on the umbrella copy; ported to `fgumi-bam-io` so the surviving definition is a superset of both. - `grouper.rs` reached into `DecodedRecord`'s fields directly, which are private on the `fgumi-bam-io` copy (made so during #734's review). Those five sites now go through `into_raw_bytes` / `record` / `raw_bytes`, which is what the accessors are for. Net -483 lines with no behavior change; the full suite is unchanged at 8322.
Prerequisite for porting
src/lib/pipeline/steps/. Additive: nothing calls the new modules yet.Why
The typed-step tree imports
fgumi_bam_io::{DecodedRecord, GroupKey, GroupKeyConfig, Grouper, compute_group_key_from_raw, name_hash_key}. On this branch those types live insrc/lib/unified_pipeline/{base,bam}.rs, andname_hash_keydoes not exist at all — so the new pipeline tree would have to import from the tree it is meant to replace, which inverts the dependency and breaks whenunified_pipelineis eventually deleted.This moves them next to the raw-record helpers they operate on, matching where
feat-runallput them.feat-runall's ownpipeline/mod.rsstates the intent: "Shared grouping/decoded-record domain types … live in thefgumi-bam-iocrate, next to the raw-record helpers they operate on."What
crates/fgumi-bam-io/src/grouping.rs(779 lines) —DecodedRecord,GroupKey,GroupKeyConfig,Grouper,compute_group_key_from_raw,name_hash_keycrates/fgumi-bam-io/src/library.rs(193 lines) —LibraryLookup,LibraryIndex,build_library_lookup, which the group key depends onlib.rsahashadded to the crate's dependencies, viaworkspace = true(this branch's convention;feat-runallpins"0.8"directly). It was already a workspace dependency, used byfgumi-consensus,fgumi-pipeline-core,fgumi-sort,fgumi-umi.Both files are self-contained: their only imports are
noodles::samand each other.Duplication
unified_pipelinekeeps its own copies of the equivalent types. That duplicate is intentional and temporary — it retires whenunified_pipelineis deleted, after the commands migrate. Nothing is changed inunified_pipelinehere, so no existing behaviour moves.Verification
Full local gate on this branch:
cargo ci-fmt,cargo ci-lint,cargo ci-tag-literals— cleanRUSTDOCFLAGS="-D warnings" cargo ci-doc— clean./scripts/publish-crates.sh --check— cleancargo ci-test— 8192 passed, 30 skipped (up 6 from 8186; the ported files bring their own tests)Risk: command output changes — none; unsafe changes — none, and the CLAUDE.md allowlist is unchanged; memory bounds, queue capacity, and thread/backpressure policy changes — none.
fgumi-bam-io.GroupKey,DecodedRecord,GroupKeyConfig,Grouper, and raw-record grouping helpers.LibraryLookup,LibraryIndex, and SAM-header library mapping.ahashdependency.unified_pipelinetypes remain temporarily. No command uses the new modules yet.