Repository navigation
Conversation
Signed-off-by: Guan Luo <41310872+GuanLuo@users.noreply.github.com>
Signed-off-by: Guan Luo <41310872+GuanLuo@users.noreply.github.com>
Signed-off-by: Guan Luo <41310872+GuanLuo@users.noreply.github.com>
…websocket-base-frontend
Signed-off-by: Guan Luo <41310872+GuanLuo@users.noreply.github.com>
Signed-off-by: Guan Luo <41310872+GuanLuo@users.noreply.github.com>
Signed-off-by: Guan Luo <41310872+GuanLuo@users.noreply.github.com>
…websocket-base-frontend
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
WalkthroughThis PR implements WebSocket ASR (Automatic Speech Recognition) support by relaxing trait bounds to enable bidirectional streaming engines, adding an echo-based bidirectional chat completion engine, exposing a ChangesWebSocket ASR Bidirectional Streaming
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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: 7
🧹 Nitpick comments (2)
lib/llm/src/engines.rs (1)
163-177: 💤 Low valueFallback text quietly conflates "no messages" and "non-text content".
messages.iter().next_back()returnsNonefor an emptymessagesarray, and the closure also returnsNonefor any non-User variant or non-Text content. All three of those cases collapse onto the same<chunk N: non-text content>placeholder, which is then character-echoed back and followed byFinishReason::Stop. For a mock that exists to demo the bidirectional plumbing this is fine, but the type-level contract on the public struct doc (lines 130-133) only mentions "missing/unsupported"; consider either widening the doc or splitting the placeholder ("empty request" vs "non-text content") so debuggers can tell the cases apart from a captured stream.🤖 Prompt for 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. In `@lib/llm/src/engines.rs` around lines 163 - 177, The fallback currently conflates an empty messages array and non-text content into the same placeholder when building summary (the let summary = ... using req.inner.messages.iter().next_back()), so change the logic to first detect an empty messages list (req.inner.messages.is_empty()) and return a distinct placeholder like "<chunk {chunk_index}: empty request>" for that case, otherwise inspect the last message (the existing next_back() / match on ChatCompletionRequestMessage::User and Text) and keep the current "<chunk {chunk_index}: non-text content>" only for non-User or non-Text variants; update the summary construction in engines.rs (the summary binding that uses req.inner.messages and chunk_index) accordingly so callers and logs can distinguish empty vs unsupported content.lib/llm/Cargo.toml (1)
201-201: 💤 Low valueUpdate
tokio-tungsteniteto a more recent version to avoid duplicate transitive dependencies.
tokio-tungstenite = "0.21"is significantly behind the latest stable release (v0.29.0). Since axum 0.8's ws feature internally uses tokio-tungstenite ^0.28.0, which depends on tungstenite ^0.29.0, pinning to 0.21 will pull an older tungstenite version intoCargo.lockalongside the one axum brings transitively, creating duplicate versions. As a dev-dependency this has low runtime impact, but updating to ≥0.28 keeps the lock file lean.🤖 Prompt for 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. In `@lib/llm/Cargo.toml` at line 201, Replace the old tokio-tungstenite pin in lib/llm/Cargo.toml (currently tokio-tungstenite = "0.21") with a modern compatible version (>= 0.28, e.g., tokio-tungstenite = "0.28" or the latest 0.29.x) so it aligns with axum's transitives; after updating the version entry run cargo update (or cargo update -p tokio-tungstenite) to refresh Cargo.lock and eliminate duplicate tungstenite transitive versions.
🤖 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/llm/src/http/service/asr.rs`:
- Around line 44-45: Replace the internal Linear ticket IDs found in comments in
lib/llm/src/http/service/asr.rs (e.g., the comment containing "DIS-1859" near
the ModelManager / `model_name` note and the other occurrence "DIS-1928") with
the corresponding GitHub issue references; locate the comment lines that mention
DIS-1859 and DIS-1928 and update them to the proper GitHub issue numbers/links
(e.g., "GH-<number>" or full "https://github.com/.../issues/<number>") while
preserving the existing comment text and context around ModelManager and the
placeholder note.
- Around line 97-100: Replace the unbounded_channel usage with a bounded channel
(capacity 64) to apply backpressure: create a bounded channel for
NvCreateChatCompletionRequest (keeping req_tx and req_rx names), swap
UnboundedReceiverStream::new(req_rx) for the appropriate
ReceiverStream::new(req_rx) wrapper (or equivalent stream wrapper for the
bounded receiver), and keep constructing RequestStream::new(...) with the boxed
pinned receiver stream and Context::new(()).context(); ensure imports/types are
adjusted to the bounded channel/receiver stream types.
In `@lib/llm/src/http/service/service_v2.rs`:
- Around line 552-558: Update the comment that mentions the internal Linear ID
DIS-1858 by replacing it with the appropriate GitHub issue reference (use prose
form like `#NNNN` or identifier form like GH-NNNN); edit the comment near the
asr_router usage (around symbols asr_router, asr_docs, asr_route,
inference_router) so it no longer contains "DIS-1858" and instead uses the
approved GitHub issue token (or remove the ticket reference entirely if none
applies).
In `@lib/llm/src/types.rs`:
- Around line 141-144: Replace the internal Linear ticket reference "DIS-1928"
in the comment block that starts with "[gluo TODO] DIS-1928: reuses
chat-completion request/response..." with a GitHub issue identifier and prose
form (use GH-NNNN for identifiers and `#NNNN` in prose); update the comment to
read e.g. "[gluo TODO] GH-NNNN: reuses chat-completion..." and change any inline
prose "DIS-1928" to "#NNNN" so the comment no longer contains internal Linear
IDs.
- Around line 152-153: The intra-doc link to RequestStream is broken because
RequestStream isn't exported from crate::types; update the doc comment that
currently points to `crate::types::RequestStream` to reference the actual
re-export location (e.g. `dynamo_runtime::pipeline::RequestStream` or the
original `dynamo_runtime::engine::RequestStream`) so rustdoc can resolve it;
modify the doc string for the `/v1/asr` WebSocket endpoint to use the correct
path (`dynamo_runtime::pipeline::RequestStream`) or the fully qualified original
(`dynamo_runtime::engine::RequestStream`) as appropriate.
In `@lib/llm/tests/http_websocket.rs`:
- Around line 27-28: In the comment containing "OnceLock" and "ModelManager"
replace the internal Linear ticket ID "DIS-1859" with the repository's GitHub
issue format (e.g., "#1859") so the line reads something like "will replace ...
with proper ModelManager-keyed lookups; this helper goes away then (`#1859`)";
ensure only the ticket identifier is changed and the rest of the comment stays
intact.
- Around line 29-35: The test helper ensure_echo_engine_installed mutates the
process env every call (unsafe std::env::set_var) which can race when tests run
in parallel; wrap the env mutation in a one-time initializer (e.g., use
std::sync::Once or OnceCell and call_once) so the unsafe set_var is executed
exactly once before calling asr::install_echo_engine(), and remove/limit the
unsafe block to that single-call site; also update the comment referencing
DIS-1859 to point to the correct GitHub issue number.
---
Nitpick comments:
In `@lib/llm/Cargo.toml`:
- Line 201: Replace the old tokio-tungstenite pin in lib/llm/Cargo.toml
(currently tokio-tungstenite = "0.21") with a modern compatible version (>=
0.28, e.g., tokio-tungstenite = "0.28" or the latest 0.29.x) so it aligns with
axum's transitives; after updating the version entry run cargo update (or cargo
update -p tokio-tungstenite) to refresh Cargo.lock and eliminate duplicate
tungstenite transitive versions.
In `@lib/llm/src/engines.rs`:
- Around line 163-177: The fallback currently conflates an empty messages array
and non-text content into the same placeholder when building summary (the let
summary = ... using req.inner.messages.iter().next_back()), so change the logic
to first detect an empty messages list (req.inner.messages.is_empty()) and
return a distinct placeholder like "<chunk {chunk_index}: empty request>" for
that case, otherwise inspect the last message (the existing next_back() / match
on ChatCompletionRequestMessage::User and Text) and keep the current "<chunk
{chunk_index}: non-text content>" only for non-User or non-Text variants; update
the summary construction in engines.rs (the summary binding that uses
req.inner.messages and chunk_index) accordingly so callers and logs can
distinguish empty vs unsupported content.
🪄 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: 4fb77fdf-d135-499e-be2f-0589aa0377a0
⛔ Files ignored due to path filters (4)
Cargo.lockis excluded by!**/*.locklib/bindings/kvbm/Cargo.lockis excluded by!**/*.locklib/bindings/python/Cargo.lockis excluded by!**/*.locklib/runtime/examples/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (10)
Cargo.tomllib/llm/Cargo.tomllib/llm/src/engines.rslib/llm/src/http/service.rslib/llm/src/http/service/asr.rslib/llm/src/http/service/service_v2.rslib/llm/src/types.rslib/llm/tests/http_websocket.rslib/runtime/src/engine.rslib/runtime/src/pipeline.rs
All inbound exit paths cancel the engine — move the call out of the per-arm break sites and into a single site before drop(req_tx). Also fixes a hole where ws_rx returning None (peer EOF without a Close frame) wasn't cancelling the engine, leaving it generating until outbound's send to the dead socket failed. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: Guan Luo <41310872+GuanLuo@users.noreply.github.com>
…websocket-base-frontend
Signed-off-by: Guan Luo <41310872+GuanLuo@users.noreply.github.com>
Signed-off-by: Guan Luo <41310872+GuanLuo@users.noreply.github.com>
|
had a peek with claude and the most concerns has been addressed. And the rest concern will be fixed in 2/N. LGTM. |
|
@GuanLuo Can you refresh the description / plan? There is no |
Axum 0.8.4 bundles tungstenite 0.26; pinning the integration-test dev-dep to 0.21 left two versions of `tokio-tungstenite` and `tungstenite` in every workspace Cargo.lock. Bump to 0.26 so the lockfiles carry one version each. Required adjustments to integration-test code: 0.26 changed `Message::Text(String)` → `Message::Text(Utf8Bytes)` and `Message::Binary(Vec<u8>)` → `Message::Binary(Bytes)`, so test sites now call `.into()` on the payloads. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: Guan Luo <41310872+GuanLuo@users.noreply.github.com>
parking_lot::Mutex is faster, fairer, and (for our use here) returns
the guard directly from `lock()` rather than a Result, so the three
`.expect("close_reason mutex poisoned")` calls become unnecessary.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Signed-off-by: Guan Luo <41310872+GuanLuo@users.noreply.github.com>
…websocket-base-frontend
Overview:
First slice (1/N) of the bidirectional streaming-input feature (parent design
"Streaming Request Support"): a WebSocket frontend at
/v1/realtimethataccepts
NvCreateChatCompletionRequestJSON frames over a single connectionand streams
NvCreateChatCompletionStreamResponsechunks back. The endpointis wired to a process-scoped mock
EchoBidirectionalEngineso the frontendslice can ship before the engine-side
ModelManagerrefactor that #9174 (2/N)delivers.
Reusing chat-completion request/response types is intentional for the PoC —
the request can carry audio data and the response can carry text data, which
is what realtime workloads need. A dedicated realtime-API protocol type will
replace the chat overload in a follow-up (#9175).
Details:
Frontend (new surface):
lib/llm/src/http/service/realtime.rs—/v1/realtimeWebSocket handler.Splits the socket, pumps
Message::Textinto a bounded mpsc (channel(64)for backpressure), wraps it as a
RequestStream, callsengine.generate(input), forwards theAnnotatedresponse stream as Textframes, rejects Binary frames. Inbound writes a non-NORMAL close
Messageinto a shared slot (
Arc<parking_lot::Mutex<Option<Message>>>) on protocolerrors (
INVALIDfor malformed JSON,UNSUPPORTEDfor binary) beforecancelling the engine via
stop_generating(); outbound takes from thatslot after the response stream ends. The outbound task drives the sink to
completion (
SinkExt::close) under a 5s timeout so a half-broken peercan't park the connection indefinitely.
lib/llm/tests/http_websocket.rs— three integration tests: (1)sequential request echo with
finish_reason=stopper request, (2)explicit server Close frame after client-initiated close, (3)
binary-frame rejection.
lib/llm/src/engines.rs—EchoBidirectionalEngine: per-request char-echofollowed by a terminal
FinishReason::Stop, processed sequentially.lib/llm/src/types.rs— newpub mod generic::realtimewithRealtimeBidirectionalEngine = BidirectionalStreamingEngine<NvCreateChatCompletionRequest, Annotated<NvCreateChatCompletionStreamResponse>>.Runtime-crate fix (originally out of scope):
dynamo_runtime::pipeline::ManyIn<T> = Context<DataStream<T>>wasuninstantiable:
DataStream<T>is!Sync(Pin<Box<dyn Stream + Send>>)while
Context<T: Data>requiresSend + Sync. The fix:AsyncEngine<Req>'sSyncbound toSend + 'static. Workspace-wide grep confirmed no implementation relied on
Req: Sync. The trait-object form's
Send + Syncproperties come from its supertrait bounds,not from
Req.ManyIn<T>onto the existingEngineStream<T>alias (the samealias
ManyOut<T>already used). Input and output sides resolve to thesame trait-object now.
RequestStream<R>as a type alias ofResponseStream<R>— onestruct backs both directions; the alias is purely documentary at use
sites.
ClientStreamingEngineandBidirectionalStreamingEnginealiases inpipeline.rsare now actuallyinstantiable. feat: ModelManager accessor for bidirectional engines (2/N) #9174 (2/N) can use them directly.
Wiring:
Cargo.toml—+ "ws"to axum's workspace feature list.lib/llm/Cargo.toml—tokio-tungstenite = "0.26"as dev-dep (matchesaxum's bundled tungstenite version, so only one version in Cargo.lock).
lib/llm/src/http/service.rs—pub mod realtime;.lib/llm/src/http/service/service_v2.rs—realtime_router()mergedinto the inference router (unconditional registration; the engine slot
is opt-in via
OnceLock).Where should the reviewer start?
Reading order, smallest mental model first:
lib/llm/tests/http_websocket.rs— what does this PR enable? The threetests show wire shape, client-close cleanup, and binary-frame rejection.
lib/runtime/src/engine.rs+lib/runtime/src/pipeline.rs— theruntime-crate fix. Read
RequestStream<R>andResponseStream<R>sideby side to see how minimally they differ.
lib/llm/src/types.rs—RealtimeBidirectionalEnginetypedef. Noteplacement under
generic::realtimerather thanopenai::chat_completions:the bidirectional path isn't a chat-completions concept; it just reuses
the schema for now.
lib/llm/src/engines.rs—EchoBidirectionalEngine. Pins the enginecontract the handler has to satisfy.
lib/llm/src/http/service/realtime.rs— the bridge. Now you know theI/O types and engine signature; the handler reads as plumbing.
To verify locally:
Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to)
ModelManageraccessor for bidirectionalengines; retires the
OnceLockregistry introduced here.chat-completion overload in
generic::realtime.Internal Linear tickets DIS-1858, DIS-1859, and DIS-1928 mirror the above
GitHub issues (cross-linked per
.ai/linear-ticket-refs.md).