refactor(pipeline): unify execution loop and document semantics#5
refactor(pipeline): unify execution loop and document semantics#5
Conversation
- Replace duplicated run/run_streaming/observe loops with run_pipeline_core - Add OutputHook to inject streaming behavior - Make root span handling uniform with fan-out spans (Span + Instant) - Clarify carry-forward precedence, fan-out merge format, and ordering - Tighten docs for contributors and stage result ordering semantics - Require Send for run_streaming callback to support multithreaded runtimes
There was a problem hiding this comment.
Pull request overview
This PR successfully refactors the pipeline execution logic to eliminate code duplication and improve maintainability. The changes centralize all execution semantics into a single run_pipeline_core function, introduce a clean OutputHook trait abstraction for streaming behavior, and significantly enhance module-level documentation.
Changes:
- Consolidated four duplicated stage execution loops (from
Pipeline::run,Pipeline::run_streaming,ObservablePipeline::run,ObservablePipeline::run_streaming) into a singlerun_pipeline_corefunction - Introduced
OutputHooktrait to abstract streaming vs non-streaming behavior - Enhanced module documentation with comprehensive execution model, data passing rules, failure semantics, and design notes for contributors
- Added
Sendbound to streaming callbacks to ensure spawn-safety on multi-threaded runtimes
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
|
|
||
| /// Emit a synthetic ExecOutputChunk for callers that want at least one event per stage. | ||
| /// Ensures streaming callers observe at least one event per stage even if the VM produced no chunks. |
There was a problem hiding this comment.
The documentation states this function "ensures streaming callers observe at least one event per stage even if the VM produced no chunks," but the implementation only emits a chunk when result_text is non-empty. Consider clarifying that no chunk is emitted if the stage produces no result_text. For example: "Ensures streaming callers observe at least one event per stage (from result_text) even if the VM produced no live chunks during execution."
| /// Ensures streaming callers observe at least one event per stage even if the VM produced no chunks. | |
| /// Ensures streaming callers observe at least one event per stage (from `result_text`) | |
| /// even if the VM produced no live chunks during execution. No chunk is emitted if | |
| /// the stage's `result_text` is empty. |
Description
Refactor
pipeline.rsto centralize execution logic in a single core loop and clarify pipeline semantics through improved documentation.This removes duplicated stage execution code across
run,run_streaming, and observed variants, introduces a clean streaming injection abstraction, and makes span handling uniform.No functional behavior changes are intended.
Type of Change
Related Issues
Fixes #
Related to #
Changes Made
Consolidated duplicated stage loops into a single
run_pipeline_corefunctionIntroduced
OutputHooktrait to inject streaming behavior without branching execution logicUnified root span handling with fan-out spans (
Span + Instantpattern)Simplified
Pipeline::{run, run_streaming}andObservablePipelinevariantsRewrote module-level documentation to explicitly define:
result_text)Clarified internal helper docs (
extract_carry_data,merge_parallel_outputs,emit_synthetic_chunk)Testing
cargo test --workspace)Test Commands Run
cargo fmt --all cargo clippy --all-targets --all-features -- -D warnings cargo test cargo doc --no-deps scripts/build_claude_rootfs.sh cargo build --example ollama_local codesign --force --sign - --entitlements voidbox.entitlements target/debug/examples/ollama_local OLLAMA_HOST=0.0.0.0:11434 ollama serve ollama ps OLLAMA_MODEL=qwen3-coder VOID_BOX_KERNEL=target/vmlinuz-ubuntu-arm64 VOID_BOX_INITRAMFS=target/void-box-rootfs.cpio.gz target/debug/examples/ollama_localDocumentation
Code Quality
cargo fmt)cargo clippy --workspace --all-targets)cargo doc --no-deps)Screenshots (if applicable)
N/A
Checklist
Additional Notes
This refactor intentionally centralizes execution semantics in
run_pipeline_core.If future changes modify carry-forward behavior or fan-out merge format, both the module-level documentation and tests should be updated accordingly.
No changes to runtime behavior are expected; this is primarily structural cleanup and documentation clarification.