Repository navigation
Conversation
78a9e9d to
bae3926
Compare
1f4a48c to
b1d282a
Compare
b1d282a to
cc2ce15
Compare
ee44623 to
f4e5319
Compare
…ync bound Companion to the earlier Frontend/SegmentSource/Ingress restore: the `Sink<Req>` impls for `SegmentSink` and `ServiceBackend` also had their `Req: PipelineIO + Sync` bound relaxed during the bidirectional pass. That relaxation was only needed while `ManyIn<T>` was a `!Sync` boxed stream; after the `ManyIn` redesign (`Context<RequestStream<T>>`, where the `Mutex` makes the wrapper unconditionally `Sync`) it is no longer required. Restore both bounds to match the rest of the pipeline-graph machinery and the base. Build, clippy `-D warnings`, all 166 pipeline tests, and the bidirectional E2E test pass with the tighter bounds, confirming they are not needed for the bidirectional path. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…nseType The bidirectional pass added `PartialEq` to the `RequestType` and `ResponseType` enums, but no code compares them with `==` / `assert_eq` — every use is either construction or a `matches!` pattern (push_handler.rs, the network.rs control-message tests), neither of which needs `PartialEq`. Remove the derive to match the base. `StreamType`'s `Hash` derive is left in place: it is required for the `(StreamType, String)` tuples that live in the per-instance subject `HashSet`. Build, clippy `-D warnings`, all 166 pipeline tests, and the bidirectional E2E test pass without the derive. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ispatch Builds on the IngressDispatch strategy now in main (#10100). Adds the bidirectional shape as a second strategy + delegate, leaving main's unary impl untouched: - `IngressDispatch for Ingress<ManyIn<T>, ManyOut<U>>` — parses the header-only bidirectional envelope, validates the ManyIn request type, dials in the request stream, spawns the frame-deserializing forwarder, and builds `Context<RequestStream<T>>`. - `PushWorkHandler for Ingress<ManyIn<T>, ManyOut<U>>` — a thin delegate to the shared `handle_payload_shared`, identical in shape to the unary delegate. Imports `ManyIn`, `RequestStream`, `AsyncEngineContext`, and `futures::StreamExt` for the new impl. Updates the `IngressDispatch` trait doc to describe both shapes now that the single-impl framing (from the extraction PR) no longer holds. This is the bidirectional half that was deliberately kept out of the extracted refactor PR; it lands here on top of the rebased main. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…-layer RequestStream The `AsyncEngineStream` doc described an "input side" backed by `RequestStream<T>` / `ManyIn<T>`. That was accurate under the old model where `ManyIn` was an alias of `EngineStream`, but after the `ManyIn` redesign (`ManyIn = Context<RequestStream<T>>`, a struct that does not implement `AsyncEngineStream`) the relationship no longer exists — and `RequestStream` is a pipeline-layer type this engine-layer trait should not name. Trim the doc to the output side this trait actually backs: the boxed [`EngineStream<T>`] (= `ManyOut<T>`) with [`ResponseStream`] as the canonical implementor. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
24e625a to
2805e9b
Compare
WalkthroughThis PR implements bidirectional streaming for the dynamo runtime. It introduces a single-consumption ChangesBidirectional Streaming Architecture
🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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 `@lib/runtime/src/pipeline/network/egress/addressed_router.rs`:
- Around line 154-162: The control message currently sets
RequestControlMessage.frontend_send_ts_ns to None, which disables
WORK_HANDLER_NETWORK_TRANSIT_SECONDS; update the constructor for
RequestControlMessage (in addressed_router.rs) to read the frontend send
timestamp emitted by dispatch_buffer (the request-plane header), parse/convert
it to the same type used by frontend_send_ts_ns, and set frontend_send_ts_ns =
Some(parsed_value) so the timestamp is serialized into the control envelope;
ensure the code path that constructs the control message (function creating
control_message) gracefully handles a missing header (leave None) and matches
the deserialization logic in Ingress::handle_payload_shared.
In `@lib/runtime/src/pipeline/network/egress/push_router.rs`:
- Around line 1084-1088: The current bidi path calls select_next_worker() which
returns None for RouterMode::PowerOfTwoChoices, RouterMode::LeastLoaded, and
RouterMode::DeviceAwareWeighted, losing occupancy tracking and incorrectly
erroring; modify the bidi handling in push_router.rs so that before calling
bidirectional_dispatch() you branch on the router mode and either (a) perform
the mode-specific selection/permit flow used by the corresponding unidirectional
paths (replicating the occupancy/permit acquisition logic used by those
selectors) and pass the chosen instance_id to
bidirectional_dispatch(instance_id, input).await, or (b) if bidi is not
supported for a given mode, explicitly return a clear error rejecting those
RouterMode variants (PowerOfTwoChoices, LeastLoaded, DeviceAwareWeighted)
instead of calling select_next_worker(); locate logic around
select_next_worker() and bidirectional_dispatch() to implement the mode-based
routing/permit acquisition or explicit rejection.
🪄 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: CHILL
Plan: Pro
Run ID: ec4b0be4-4000-4ee1-9b5d-8fb661132b86
📒 Files selected for processing (9)
lib/llm/src/engines.rslib/llm/src/http/service/realtime.rslib/runtime/src/engine.rslib/runtime/src/pipeline.rslib/runtime/src/pipeline/network.rslib/runtime/src/pipeline/network/egress/addressed_router.rslib/runtime/src/pipeline/network/egress/push_router.rslib/runtime/src/pipeline/network/ingress/push_handler.rslib/runtime/tests/bidirectional_e2e.rs
…patch paths
Address review feedback (avoid panicking the worker in production):
- `RequestStream::take()` in `EchoBidirectionalEngine` (engines.rs) and
`AddressedPushRouter::generate_bidirectional` now return `Err` instead
of `expect()` when the stream was already taken — a caller bug should
surface as an error, not a panic.
- `dispatch_and_finalize`'s `recv_registered.expect(...)` becomes an
`ok_or_else(...)?` — the response-stream registration is an invariant
(we always pass `enable_response_stream=true`) but a missing half
should error rather than panic.
- `register_streams` now returns `Result`: the `StreamOptions` builder
uses `?` instead of `.unwrap()`, and the request/response presence
post-conditions become `debug_assert_eq!` — they assert a
transport-layer invariant, so they're checked in debug builds without
risking a production panic.
No behavior change on the happy path.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…al dispatch The bidirectional `generate` only wires RoundRobin/Random worker selection. `PowerOfTwoChoices`, `LeastLoaded`, and `DeviceAwareWeighted` make `select_next_worker()` return `None` (they rely on occupancy/ load-aware selection the bidirectional path doesn't implement yet), which previously surfaced as a misleading "no instances available for bidirectional routing" error even with healthy workers. Reject those three modes explicitly with a clear message before reaching `select_next_worker()`. Full bidirectional support for the load-aware (and KV/Direct) modes is tracked in #10320. Adds `bidirectional_generate_rejects_unsupported_load_aware_modes` asserting each rejected mode errors with the "not yet supported" message rather than "no instances". Addresses review comments from @krishung5 and CodeRabbit. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…erating The recv-side forwarder that pumps request-stream frames into the engine's `ManyIn<T>` only checked `is_killed()`. If the engine called `stop_generating()` (soft cancel), the forwarder kept deserializing and pushing frames into a channel the engine had abandoned. Check `is_stopped()` as well, matching the send-side `spawn_request_stream_forwarder` which selects on both `killed()` and `stopped()`. Addresses review comment from @furionw. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Cosmetic/clarity fixes from review (no behavior change):
- `build_request_envelope`: match on `data` by value and move the
payload into `from_parts` instead of `d.clone()` (needless clone).
- `generate_bidirectional` doc: fix typos "becasue"/"trivail".
- `dispatch_and_finalize`: destructure `into_parts()` for the request
provider instead of the opaque `.1`.
- ingress: use `{e}` (Display) rather than `{:?}` (Debug) for the
user-facing "Failed to create … stream" errors.
- ingress: drop the redundant per-method `use crate::pipeline::network::Ingress`
(the type is already in scope via the impl target); one of them was
unused entirely.
- bidirectional_e2e: single-year SPDX (new file), and trim the
module docstring to the essentials (drop the verbose call-chain and
the internal followup-note reference).
Addresses review comments from @furionw and @krishung5.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… and bidi
The unary and bidirectional `IngressDispatch` impls each duplicated the
envelope decode + control-message deserialization (and the
DESERIALIZATION / INVALID_MESSAGE error-counter bookkeeping). Extract a
shared `Ingress::decode_control_message` that returns
`(RequestControlMessage, Option<Bytes>)`:
- `HeaderAndData` → `(control, Some(data))` (unary wire shape)
- `HeaderOnly` → `(control, None)` (bidirectional wire shape)
The helper owns the parse + error counters; each caller decides whether
its path expects the data payload:
- unary: `ok_or_else(...)` the data half — a header-only envelope is an
error ("expected a request payload").
- bidi: reject if `data.is_some()` — a data payload means the unary
wire shape was used.
Returning the payload as `Option` (rather than the helper deciding)
keeps the wire-shape expectation with the caller that knows it.
Addresses @furionw's request to reuse the control-message decode.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
RE: review feedback Thanks @furionw, @krishung5. Addressed in 5 commits (per-thread replies point at each):
One dismissed: the |
…-dynamo#9674) Signed-off-by: Guan Luo <41310872+GuanLuo@users.noreply.github.com> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: shenls <shenlinshan@kanzhun.com>
…-dynamo#9674) Signed-off-by: Guan Luo <41310872+GuanLuo@users.noreply.github.com> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: shenls <shenlinshan@kanzhun.com>
PR description (Rust-only 3/N)
Title (under 70 chars):
feat: bidirectional request-plane dispatch + runtime-side ingressBase branch:
gluo/dis-1859-multimodal-streaming-input-2n-add-bidirectionalstreaming(PR #9308). Retarget tomainonce #9308 squash-merges.Summary
PushRouter::generate(ManyIn<T>)now reaches a real worker throughAddressedPushRouter::generate_bidirectional+ a newIngress<ManyIn<T>, ManyOut<U>>::PushWorkHandlerimpl, completing the runtime side of the streaming-input feature.bidirectional_dispatchresolves the sticky-selected worker, dispatches via the request plane, awaits both call-home stream halves, and pumps frames in both directions.RequestType::ManyIn; all request frames (including the first) flow on the request-stream socket. Wire-format choice deliberately diverges from the unary[control_msg, request]shape.Where to start reading
lib/runtime/src/pipeline/network.rs—RequestControlMessage::request_stream_connection_infofield + the consolidatedpump_response_streamhelper onIngress.lib/runtime/src/pipeline/network/tcp/{client,server}.rs— symmetriccreate_request_stream/process_request_streampair filling the previously-stubbed request-streaming half of the call-home transport.lib/runtime/src/pipeline/network/egress/addressed_router.rs—generate_bidirectional. Reusesfinalize_response_streamwith the unary path.lib/runtime/src/pipeline/network/egress/push_router.rs— de-bailed bidirectionalAsyncEngineimpl;bidirectional_dispatchsharesresolve_transport/check_workers_available/wrap_with_fault_detectionwith the unary path.lib/runtime/src/pipeline/network/ingress/push_handler.rs—PushWorkHandler for Ingress<ManyIn<T>, ManyOut<U>>; sharespump_response_streamwith the unary impl.lib/runtime/tests/bidirectional_e2e.rs— full-stack echo round-trip in its own integration-test binary.Wire-format choice
RequestType::SingleIn: envelope isHeaderAndData(control + request body) — unchanged.RequestType::ManyIn: envelope isHeaderOnly(control only); all request frames flow on the request-stream socket.RequestControlMessage.request_stream_connection_info: Option<ConnectionInfo>isNonefor unary,Some(_)for bidirectional. Wire-compatible: older readers ignore the field via#[serde(default, skip_serializing_if = "Option::is_none")]. Each ingress impl strictly accepts only its own envelope shape and rejects the other.Sync-bound relaxation
This PR also relaxes
Req: PipelineIO + SynctoReq: PipelineIOin four places —Frontend::generate, theSegmentSource/ServiceFrontendmacro,ServiceBackend::Sink,SegmentSink::Sink— and dropsSyncfromIngress::new/add_metrics/link/for_pipeline/for_engine. The bounds were vestigial:AsyncEngineonly requiresReq: Send + 'staticand the bodies don't actually needSync. Needed soIngress<ManyIn<T>, ManyOut<U>>(whoseManyIn<T> = Pin<Box<dyn AsyncEngineStream<T>>>isSendbut notSync) can be constructed via the existingIngress::for_enginepath. Pure relaxation — no caller becomes stricter.Worker selection: reserve-before-observe
The bidirectional
PushRouter::generate(ManyIn<T>)reserves a sticky worker BEFORE polling the first frame. Router-mode strategies (RoundRobin,Random,PowerOfTwoChoices,LeastLoaded,DeviceAwareWeighted) don't consume frame content, so waiting for the first frame added no information. Reserve-up-front lets connection setup (registerboth halves, build envelope, send) run concurrently with the client producing its first frame. A client that connects but never sends releases the slot via the normal response-stream-drop /cancel_bothcleanup path.Test plan
cargo test --lib -p dynamo-runtime— 150 passed.cargo test --test bidirectional_e2e— 1 passed (full-stack echo round-trip).cargo clippy --no-deps --all-targets -- -D warnings— clean.cargo fmt --check— clean.cargo test --test bidirectional_e2e -- --nocaptureto confirm the wire path on their machine.Follow-ups
PyAsyncRequestStream+PythonBidirectionalEngine+serve_realtime_endpoint+ Python E2E test) — split into its own slice. Tracked as issue feat: Python bridge for bidirectional engine (multimodal streaming input 3.5/N) #9673 (3.5/N).GLOBAL_TCP_SERVER's accept loop dies with whichever#[tokio::test]runtime first lazy-initializes it. Worked around by running the E2E test in its owntests/binary (its own process). Followup written up at.feature-development/followup-global-tcp-server-runtime-lifetime.md; file at PR-open time.TODO(#171)— three newpanic!paths inhandle_request_readerfollow the existinghandle_readerprecedent; ci: Trigger TRTLLM build when the associated files are updated #171 grows in urgency.Related Issues
PushRoutershell with thebail!this PR fills. Currently the base of this branch; will retarget tomainonce feat(llm/realtime): wire /v1/realtime through ModelManager + bidirectional PushRouter #9308 squash-merges./v1/realtimeroute this dispatch serves.ModelManageraccessor surface the watcher arm uses to register discovered realtime workers.RouterMode::KV/Directbidirectional support (out of scope here).Internal Linear tickets DIS-1861 and DGH-860 mirror the GitHub issues above (cross-linked per
.ai/linear-ticket-refs.md).Summary by CodeRabbit
New Features
Refactor
Tests