Repository navigation
docs(pipeline): pin the deliberate Step2 reorder-stage asymmetry - #441
Conversation
|
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 (1)
WalkthroughAdds a comment block to ChangesStep2 reorder-stage comment and regression test
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes 🚥 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 #441 +/- ##
==============================================
Coverage ? 93.81%
==============================================
Files ? 119
Lines ? 50555
Branches ? 0
==============================================
Hits ? 47429
Misses ? 3126
Partials ? 0 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
9e62c27 to
eff004f
Compare
ee82d46 to
84c299e
Compare
A review of `TypedStep2::build_output_set` asked why it doesn't apply the Serial/Exclusive reorder-collapse that `TypedStep::build_output_set` has. Document that the asymmetry is intentional and add a test that pins it. The single-input collapse is sound because of a universal property: a Serial single-input step consumes one already-ordered stream, so any input ordinal it propagates onto an output is emitted in push order, making the reorder stage redundant. A two-input MERGE has no such guarantee — it interleaves two branches and can push a `ByItemOrdinal` output out of ordinal order even under serial execution, so its reorder stage is load-bearing. Collapsing it would silently deliver records out of order. Today's production Step2s happen not to need it (`PairRawFastq` uses `BranchOrdering::None`; `ZipperMergeStep` assigns fresh sequential ordinals), but the framework cannot assume that for an arbitrary merge. Adds `step2_serial_byitemordinal_output_is_reordered_not_collapsed`, which pushes a Serial Step2's ordered output out of ordinal order and asserts the consumer receives it in ordinal order — it fails if the single-input collapse is ever added to the Step2 path (verified). Stacked on the fgumi-pipeline-core extraction (#440); rebases onto feat-runall once that merges.
84c299e to
a5dcaa1
Compare
|
@coderabbitai resume |
✅ Action performedReviews resumed. |
|
@coderabbitai resume |
|
@coderabbitai review |
✅ Action performedReviews resumed. |
✅ Action performedReview finished.
|
Summary
Resolves a review question on
TypedStep2::build_output_set(raised against thefgumi-pipeline-coreextraction, #440): why doesn't it apply the Serial/Exclusive reorder-collapse that the single-inputTypedStep::build_output_sethas?Because applying it would be a correctness bug. This PR documents the deliberate asymmetry and adds a test that pins it.
Why the asymmetry is intentional
The single-input collapse rests on a universal property: a Serial single-input step consumes one already-ordered stream, so any input ordinal it propagates onto an output is emitted in push order — the reorder stage is redundant, and collapsing
ByItemOrdinal→Noneis a free optimization.A two-input merge has no such guarantee. It interleaves two branches, so a
ByItemOrdinaloutput that propagates an input's ordinal can be pushed out of ordinal order even under serial (mutex-serialized, single-dispatcher) execution — e.g. when branch B's next-needed ordinal hasn't arrived but branch A's later one has. For such a step the reorder stage is load-bearing; dropping it would silently deliver records out of order.Today's production
Step2s happen not to need it —PairRawFastqdeclaresBranchOrdering::None, andZipperMergeStepassigns fresh sequential output ordinals (push order already equals ordinal order) — but the framework cannot assume that for an arbitrary merge, so it conservatively keeps the reorder for every orderedStep2output.Changes
TypedStep2::build_output_setexplaining the deliberate difference from the single-input path.step2_serial_byitemordinal_output_is_reordered_not_collapsed: pushes a SerialStep2'sByItemOrdinaloutput out of ordinal order (2, 0, 1) and asserts the consumer receives it in ordinal order (0, 1, 2). It fails if the single-input collapse is ever added to theStep2path (verified by temporarily applying the collapse).Note
Stacked on #440 (the
fgumi-pipeline-coreextraction) so it lives in the new crate location; targets that branch and rebases ontofeat-runallonce #440 merges. No behavior change — documentation + a pinning test.Summary by CodeRabbit