Repository navigation
fix(router): emit usage on the disaggregated /v1/chat/completions responses - #707
Merged
Merged
Conversation
…ponses PR #392 (issue #387) made the router carry the worker's authoritative generated-token count over the wire for the chat path, but only used it to fix finish_reason; the count was resolved and then discarded, so router chat clients got no token accounting at all, unlike the single-node chat route and the router's own /v1/completions path. Non-streaming route_chat now threads the already-resolved prompt_tokens (previously discarded as _prompt_tokens) and completion_tokens (from the existing resolve_completion_tokens call) into chat_completion_json, which adds a usage object with prompt_tokens, completion_tokens, and total_tokens matching the single-node ChatCompletionResponse shape. Streaming route_chat now reads stream_options.include_usage the same way the completions path does, resolves completion_tokens once for both finish_reason and the usage chunk, and emits a new chat_chunk_usage SSE chunk (empty choices, populated usage) after the finish chunk on success only; a handoff failure still emits no usage chunk, mirroring the completions path's existing behavior. Added unit tests in router_front_tests.rs for chat_completion_json's usage field and chat_chunk_usage's shape, and a new E2E test in tests/disaggregated_router_e2e.rs (disaggregated_router_chat_usage_matches_single_node_byte_fallback) that compares router vs single-node chat usage for both non-streaming and streaming on a byte-fallback tokenizer (Gemma), gated behind the same checkpoint-presence guard as the other real-model tests in that file. Refs #398
Member
Author
Implementation Review SummaryIntent
Findings Addressed
Remaining Items
Acceptance criteria (issue #398)
Parity notes
Verification
|
4 tasks done
…niCPM-2B The gemma-3-1b-it-4bit fixture can never pass this test: gemma3 is a model-owned paged family, and a disaggregated handoff request for it crashes the prefill node's serving loop (PagedBlockPool::read_block_contents: layer 0 has no pool tensors to read from) before any usage can be emitted - discovered on the first-ever real run of the Gemma byte-fallback E2Es and filed as issue #708. MiniCPM-2B (llama-format export, mlx-community/MiniCPM-2B-sft-4bit-llama-format-mlx) keeps the byte-fallback tokenizer requirement (Llama-style SentencePiece with byte_fallback = true) while running as a plain dense Llama model, which is pool-backed Fp16 and inside the handoff scope, so the test now actually runs: non-stream and stream usage parity (prompt 20 / completion 16 / total 36) and finish_reason both match single-node on the three-node harness.
The chat streaming usage chunk added in 13b9058 is gated on `stream_options.include_usage`, but nothing exercised that gate directly: the two builder-function unit tests only check chunk shape, and the E2E test only exercises the opted-in (true) path, so a regression that always (or never) sent the usage chunk regardless of the request would go undetected. Extract the gate's boolean resolution into `wants_stream_usage` (pure function, behavior-preserving) and add three unit tests covering the omitted-`stream_options`, explicit-`false`, and explicit-`true` cases. Also update docs/CONTINUOUS_BATCHING.md's usage/finish_reason limitation bullet, which was stale: it still described the pre-#387 emitted-text-piece counting as the primary path, when #387 already made the worker's authoritative wire-carried count (issue #387) the primary source and text-piece counting only the mixed-version-cluster fallback. Added a bullet noting that /v1/chat/completions now reports the same usage shape as /v1/completions in both modes (issue #398). Validation: - cargo test --lib server::router_front (15 passed, including the 3 new tests) - cargo fmt --all -- --check - cargo clippy --lib --tests --features metal,accelerate -- -D warnings - python3 scripts/ci/check_cross_repo_refs.py (advisory pass; all flagged refs verified as real lablup/mlxcel issues) Refs #398
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The disaggregated router's
/v1/chat/completionspath never emitted ausageobject, diverging from the single-node chat route (which reportsusagein both streaming and non-streaming responses) and from the router's own/v1/completionspath. PR #392 (issue #387) had already made the router resolve the worker's authoritative generated-token count for the chat path, but only used it forfinish_reason; this PR wires that already-resolved count, plus the previously-discarded prompt token count, into the chatusageblock.What changed
src/server/router_front.rs: non-streamingroute_chatnow passesprompt_tokens(previously discarded as_prompt_tokens) and the resolvedcompletion_tokensintochat_completion_json, which now serializes ausageobject (prompt_tokens,completion_tokens,total_tokens) matching the single-nodeChatCompletionResponseshape.src/server/router_front.rs: streamingroute_chatnow readsstream_options.include_usagethe same way the router's/v1/completionsstreaming path does, resolvescompletion_tokensonce for bothfinish_reasonand the usage chunk, and emits a newchat_chunk_usageSSE chunk (emptychoices, populatedusage) after the finish chunk, only on a successful handoff — a handoff failure still emits no usage chunk, matching the existing completions-path behavior.src/server/router_front_tests.rs: unit tests forchat_completion_json's newusagefield andchat_chunk_usage's chunk shape.tests/disaggregated_router_e2e.rs: a new E2E test,disaggregated_router_chat_usage_matches_single_node_byte_fallback, that spins up a three-process disaggregated stack plus a single-node reference and assertsusage.prompt_tokens/usage.completion_tokens/usage.total_tokens(andfinish_reason) match between router and single-node, for both the non-streaming response and the streaming final usage chunk. Gated behind the same checkpoint-presence guard as the other real-model tests in that file.E2E fixture note (issue #708)
The test was first written against the Gemma byte-fallback checkpoint (mirroring the issue #387 completions test) and its first-ever real run uncovered a latent main defect: gemma3 is a model-owned paged family, and a disaggregated handoff request for it crashes the prefill node's serving loop (
PagedBlockPool::read_block_contents: layer 0 has no pool tensors to read from), after which the router 500s every request. That defect is filed as issue #708 (with root cause: the vacuous.all()guard inCachePool::extract_paged_blocksplus the fatal serving-loop exit); the pre-existing #392 completions E2E is unrunnable for the same reason. The chat-usage E2E in this PR is therefore targeted at MiniCPM-2B (mlx-community/MiniCPM-2B-sft-4bit-llama-format-mlx), which keeps the byte-fallback tokenizer requirement (Llama-style SentencePiece,byte_fallback = true) while running as a plain dense Llama model — pool-backed Fp16, inside the handoff scope.Test plan
cargo check --lib --testscargo test --lib server::router_front(12 passed, including the 3 new unit tests)cargo clippy --lib --tests -- -D warningscargo fmt --all -- --checkcargo test --test disaggregated_router_e2e --release --features metal,accelerate disaggregated_router_chat_usage_matches_single_node_byte_fallback -- --ignored— passed on the real three-node stack withminicpm-2b-4bit: non-stream and stream usage parity (prompt_tokens20 /completion_tokens16 /total_tokens36) andfinish_reasonboth match single-node; the usage chunk carrieschoices: []and is emitted only whenstream_options.include_usageis set.cargo test --test disaggregated_router_e2e --release --features metal,accelerate disaggregated_router_streams_correct_output -- --ignored— pre-existing qwen3 router E2E still green on this branch.Closes #398