Repository navigation
refactor(pipeline): relocate reusable types out of unified_pipeline (R6a) - #929
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: Essentials Run ID: 📒 Files selected for processing (2)
Included review availability: 0 reviews are currently available. Your included PR review attempts over the past 7 days set your current allowance at 1 review per hour. WalkthroughShared pipeline contracts move to public neutral modules. Backpressure logic moves to ChangesPipeline shared contracts
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Refactor Merge Risk: ⚪ Minimal · up to This change relocates shared pipeline contracts while preserving legacy compatibility paths and reports no behavior or CLI changes. No current merge-blocking risk is identified. Suggested labels: 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
Comment |
|
@coderabbitai pause |
✅ Action performedReviews paused. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #929 +/- ##
==========================================
- Coverage 94.38% 94.37% -0.02%
==========================================
Files 302 303 +1
Lines 150364 150442 +78
==========================================
+ Hits 141918 141973 +55
- Misses 8446 8469 +23 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
@coderabbitai review |
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
…R6a) Repoint the surviving importers (fastq_parse, grouper, mi_group, template, commands/common) at fgumi_bam_io directly for DecodedRecord, GroupKey, GroupKeyConfig, Grouper, and MemoryEstimate, which already live there and were only reached through a unified_pipeline re-export. Relocate the three items still actually defined in unified_pipeline that surviving code needs: the BatchWeight trait moves to a new root-crate module (src/lib/batch_weight.rs) rather than fgumi-bam-io, since its surviving impls target foreign types and would violate the orphan rule there; SchedulerStrategy moves to commands/common.rs, next to its only surviving consumer (SchedulerOptions); the backpressure constants and stage_high_water_mark move to a new src/lib/pipeline/backpressure.rs. unified_pipeline keeps pub use shims for all three so the still-present legacy command paths keep compiling unchanged across every feature configuration. Repoint the two grouper unit tests that called the 4-arg unified_pipeline::compute_group_key_from_raw to the canonical 3-arg fgumi_bam_io version, dropping the discarded UMI offset return value the tests never used; the resulting GroupKey is unchanged for valid records. BamPipelineConfig stays legacy-only and is left in place.
7b3bb5b to
ad76bc5
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
This branch was successfully deployed
1 participant
What
Relocate the reusable types that live in
src/lib/unified_pipeline/but are consumed by code outside it, so a later PR can delete the wholeunified_pipelinemodule (campaign phase R6a → R6b). Behavior-preserving: no output or CLI change.The relocation
fgumi-bam-io(DecodedRecord,GroupKey,GroupKeyConfig,Grouper,MemoryEstimate) —unified_pipelineonly re-exported them. Surviving importers (fastq_parse.rs,grouper.rs,mi_group.rs,template.rs,commands/common.rs) now import fromfgumi_bam_io::directly. No definitions moved.unified_pipelineand needed by surviving code are moved intra-crate (deliberately not intofgumi-bam-io):BatchWeight(trait) → newsrc/lib/batch_weight.rs. Its impls target foreign types (RecordBuf,RawRecord,Vec<u8>), so moving the trait tofgumi-bam-iowould break the orphan rule — it stays root-local.SchedulerStrategy(enum) → newsrc/lib/scheduler_strategy.rs. It is consumed by both the scheduler dispatch (create_scheduler) and the CLISchedulerOptions, so it lives in a neutral module both depend on downward (not in the commands layer).BACKPRESSURE_THRESHOLD_BYTES/Q5_BACKPRESSURE_THRESHOLD_BYTES/stage_high_water_mark→ newsrc/lib/pipeline/backpressure.rs(with their unit tests).pub usere-export shims are left inunified_pipeline(base.rs,scheduler/mod.rs) so the still-present legacy command paths keep compiling unchanged. Those shims disappear withunified_pipelinein the R6b deletion.BamPipelineConfig(legacy-only — the chain path buildsPipelineConfig, not it) and the 4-argcompute_group_key_from_raw(the 3-argfgumi-bam-ioversion is canonical) both stay inunified_pipelineto die with R6b.Notes
grouper.rstests that called the 4-argcompute_group_key_from_raw(passingNone, discarding the umi offset) are repointed to the canonical 3-argfgumi_bam_io::grouping::compute_group_key_from_raw— byte-identical grouping key for the inputs exercised (confirmed by reading both bodies and by the passing tests).fgumi-bam-iodoes not depend on the root crate, and all three moves are intra-crate. The 8 in-flight per-command legacy-path retirement PRs are not touched — they resolve theirunified_pipeline::{BatchWeight,SchedulerStrategy,...}imports through the shims, which the--no-default-featurescheck confirms.Full gate green:
cargo checkin all three feature configs (default,--all-features,--no-default-features),ci-fmt/ci-lint/ci-doc, andci-test(9987 passed / 31 skipped). This is the R6a step; R6b (deleteunified_pipeline) follows once the per-command legacy retirements land.Risk: command output changes—none; grouping remains pinned to
fgumi_bam_io.unsafechanges—none;CLAUDE.mdneeds no update. Memory bounds, queue capacity, and thread/backpressure policy changes—none.unified_pipeline.BatchWeightandSchedulerStrategy.pipeline::backpressurewith compatibility re-exports.fgumi_bam_iotypes.BamPipelineConfig, and CLI behavior.