Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d410bec728
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| let events = vec![ | ||
| Ok(DecodedTextEvent::Start { | ||
| prompt_token_ids: Arc::from([]), |
There was a problem hiding this comment.
Pass real prompt IDs into parser replay
When parsed derender is used, this synthetic Start initializes the chat parser with an empty prompt. Several parsers derive their starting mode from the rendered prompt tail (for example Kimi K3 starts generation inside a prefilled think/response channel and does not re-emit the opener), so those models will classify generated reasoning/response text as the wrong stream when derendering even though the normal chat pipeline would parse it correctly. Please render/tokenize the supplied chat_request and pass the actual prompt token IDs into the replayed stream before parsing.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 4cfeb02 — the parser replay now renders and tokenizes the embedded chat_request and feeds the real prompt_token_ids into the Start event, so prompt-dependent parsers start in the right mode. Added a regression test with deepseek_r1 where a prompt ending in now yields pure content instead of misclassified reasoning.
|
|
||
| for choice in &request.generate_chunk.choices { | ||
| let (new_text, updated_state) = | ||
| detok::detokenize_delta(&ctx.tokenizer, &choice.token_ids, &state, skip_special)?; |
There was a problem hiding this comment.
Bound streaming derender chunks before decoding
The streaming handlers only validate state and choice count before calling detokenize_delta, unlike the non-streaming path which rejects token_ids longer than max_model_len before decoding. A caller can therefore POST a single streaming derender chunk with an oversized token_ids vector and force token-by-token detokenization past the server's configured resource bound. Please apply the same max_model_len check to generate_chunk before this decode step.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 4cfeb02 — streaming chunks now get the same max_model_len/max_logprobs bounds check before decoding. This intentionally goes beyond Python, which only validates the non-streaming payloads.
| choices.push(ChatCompletionChoice { | ||
| index: choice.index, | ||
| message, | ||
| logprobs: resolved_logprobs, |
There was a problem hiding this comment.
Drop logprobs when hidden reasoning is suppressed
When parsed derender is used with chat_request.include_reasoning=false and the generate response includes choice.logprobs, the message hides the reasoning text but this still attaches logprobs for the entire generated token stream. The regular chat completion path omits output metadata in that case to avoid leaking hidden reasoning tokens through per-token data, so this endpoint should apply the same gate before returning logprobs.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 4cfeb02 — verified the normal chat path gates this way and matched it: choice logprobs are now dropped when reasoning was parsed but hidden via include_reasoning=false.
| tokenizer: &DynTokenizer, | ||
| pieces: &[String], | ||
| ) -> Result<String, ApiError> { | ||
| let ids: Vec<u32> = pieces.iter().filter_map(|piece| tokenizer.token_to_id(piece)).collect(); |
There was a problem hiding this comment.
Avoid lossy piece round-trips for streaming detok
This reconstructs text by converting id_to_token pieces back through token_to_id, but the tiktoken backend stores base-token byte pieces as lossy UTF-8 when the token bytes are not valid alone. For streamed output where a UTF-8 character is split across byte-fallback tokens, those pieces become replacement characters or fail to map back and are dropped, so derendered streaming text no longer matches one-shot decode. Please carry token IDs in the state or use a lossless byte-piece conversion for this path.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 4cfeb02 — confirmed tiktoken stores base-token pieces via from_utf8_lossy. DerenderStreamState now optionally carries prev_token_ids alongside prev_tokens, and the detok window reconstructs from IDs; Python ignores the extra field, and foreign states without IDs fall back to the old piece mapping, so cross-frontend compatibility holds either way.
| choices, | ||
| usage: Usage { | ||
| prompt_tokens, | ||
| total_tokens: prompt_tokens + completion_tokens, |
There was a problem hiding this comment.
Validate prompt token counts before adding usage
Because prompt_tokens is supplied by the request body, a caller can set it near usize::MAX; adding completion_tokens here then panics in checked builds and wraps in release builds, producing an invalid usage object. Please reject oversized prompt counts or use checked/saturating arithmetic anywhere derender aggregates caller-provided usage.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 4cfeb02 — all usage aggregation sites now use checked arithmetic and return 400 on overflow.
| ) -> Result<DerenderChatStreamResponse, ApiError> { | ||
| check_model(ctx, request.model.as_deref())?; | ||
|
|
||
| if ctx.has_parser() { |
There was a problem hiding this comment.
Allow plain streaming for tool-only parser models
This rejects streaming chat derender whenever the model has any parser registered, but normal chat output only enables the tool parser when the request actually has tools/tool_choice. For a tool-capable model with no tools in this request (or no chat_request), there is no tool markup to protect and the normal pipeline would be plain text, so this makes simple streaming derender return 400 unnecessarily. Please fail closed for reasoning parsers, but gate tool-parser rejection on the request actually enabling tool parsing.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 4cfeb02 — streaming now fails closed only for reasoning parsers. Tool-only parser models stream plainly unless the request actually engages tool parsing (tools present with tool_choice != none, or required/named). Deliberate deviation from Python, which rejects whenever any parser is configured; noted in the module docs.
|
|
||
| choices.push(CompletionChoice { | ||
| index, | ||
| text: decoded_text, |
There was a problem hiding this comment.
Honor completion echo in derender output
When the embedded completion_request has echo=true (especially the max_tokens=0 prompt-only case), returning only decoded_text drops the prompt that the regular completions path prefixes, and prompt-only requests will expose the internal generated token instead of just the prompt. This means render → generate → derender does not reproduce /v1/completions for supported echo requests; please use completion_request.echo/prompt-only handling when constructing the derendered choice text and logprob offsets.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 4cfeb02 — completion derender now honors echo, reusing completion_echo_text from the normal path, including the max_tokens=0 prompt-only case and prompt_logprobs with shifted text_offsets. Python's derender ignores echo entirely, so this closes a parity gap that exists there too.
sagearc
left a comment
There was a problem hiding this comment.
The python derender RFC split this work into phases so each contract could be validated independently. I think it would help to keep the same structure here, starting with shared detokenization and state, then adding tool and reasoning parsing and streaming.
…ase 1/3) Phase 1 of 3 splitting the Rust derender endpoints per review on vllm-project#53223, mirroring the Python phasing in vllm-project#43606 (detokenization first; parsing and streaming follow). Related issues: vllm-project#42729, vllm-project#47161. Adds /v1/chat/completions/derender and /v1/completions/derender to both the render-only and engine-backed servers: shared incremental detokenization foundation, DerenderStreamState, logprob placeholder resolution, and the plain non-streaming derender paths with bounds validation. Reasoning/ tool-call parsing (phase 2) and streaming (phase 3) are intentionally absent; a request body with stream=true fails deserialization with 400. This change was developed with AI assistance (Kimi Code CLI). Co-authored-by: Kimi Code CLI Signed-off-by: zireael <zireael@users.noreply.github.com>
4cfeb02 to
57d4a24
Compare
|
@sagearc Good call — split into phases mirroring the Python derender structure from #42729:
Each phase keeps its own route-test subset green ( |
|
This pull request has merge conflicts that must be resolved before it can be |
57d4a24 to
f25fe4f
Compare
…ase 1/3) Phase 1 of 3 splitting the Rust derender endpoints per review on vllm-project#53223, mirroring the Python phasing in vllm-project#43606 (detokenization first; parsing and streaming follow). Related issues: vllm-project#42729, vllm-project#47161. Adds /v1/chat/completions/derender and /v1/completions/derender to both the render-only and engine-backed servers: shared incremental detokenization foundation, DerenderStreamState, logprob placeholder resolution, and the plain non-streaming derender paths with bounds validation. Reasoning/ tool-call parsing (phase 2) and streaming (phase 3) are intentionally absent; a request body with stream=true fails deserialization with 400. This change was developed with AI assistance (Kimi Code CLI). Co-authored-by: Kimi Code CLI Signed-off-by: zireael <zireael@users.noreply.github.com>
…ase 1/3) Phase 1 of 3 splitting the Rust derender endpoints per review on vllm-project#53223, mirroring the Python phasing in vllm-project#43606 (detokenization first; parsing and streaming follow). Related issues: vllm-project#42729, vllm-project#47161. Adds /v1/chat/completions/derender and /v1/completions/derender to both the render-only and engine-backed servers: shared incremental detokenization foundation, DerenderStreamState, logprob placeholder resolution, and the plain non-streaming derender paths with bounds validation. Reasoning/ tool-call parsing (phase 2) and streaming (phase 3) are intentionally absent; a request body with stream=true fails deserialization with 400. This change was developed with AI assistance (Kimi Code CLI). Co-authored-by: Kimi Code CLI Signed-off-by: zireael <zireael@users.noreply.github.com> Signed-off-by: Tianer Zhou <ezhoureal@gmail.com>
f25fe4f to
f69f098
Compare
Phase 2 of the derender split per review on vllm-project#53223, mirroring the Python implementation from vllm-project#45919. Stacked on phase 1 (detokenization + state). Restores non-streaming chat reasoning/tool-call parsing: when a parser is configured and `chat_request` is supplied, generated tokens are replayed through the production chat output pipeline so the configured parser splits them into reasoning, content and tool calls; otherwise the endpoint falls back to plain detokenization. Hidden reasoning also suppresses per-token logprobs, matching the normal chat path. Streaming endpoints remain phase 3: `stream: true` bodies still fail deserialization with a 400. Related: vllm-project#42729 Co-authored-by: Kimi Code CLI Signed-off-by: zireael <zireael@users.noreply.github.com> Signed-off-by: Tianer Zhou <ezhoureal@gmail.com>
…(phase 3/3) Phase 3 of the 3-phase split per review on vllm-project#53223, stacked on phase 2 (rust-derender-2-parsing). Restores the remaining streaming functionality of the Rust /derender endpoints — streaming wire types and union variants, derender_chat_stream / derender_completion_stream handlers with validate_stream_bounds / stream_usage / tool_parsing_would_engage, stream dispatch in derender/mod.rs, and removal of the phase-1/2 #[allow(dead_code)] gates — and adds the two-process Python e2e test (RemoteRustRenderServer + test_derender_rust_e2e.py). Implements the client-carried DerenderStreamState protocol, mirroring the Python implementation from vllm-project#48617. Related: vllm-project#42729, vllm-project#47161. This change was made with AI assistance (Kimi Code CLI). Co-authored-by: Kimi Code CLI Signed-off-by: zireael <zireael@users.noreply.github.com> Signed-off-by: Tianer Zhou <ezhoureal@gmail.com>
Purpose
Phase 1/3 — split per @sagearc's review, mirroring the Python derender phasing in #42729 (#43606 detok → #45919 parsing → #48617 streaming). Follow-ups: #53418 (parsing), #53419 (streaming + GPU e2e).
Add
POST /v1/chat/completions/derenderandPOST /v1/completions/derenderto the Rust frontend, wire-compatible with the Python implementation invllm/entrypoints/scale_out/derender/+vllm/renderers/online_derenderer.py. The endpoints turn aGenerateResponsefrom/inference/v1/generateback into an OpenAIChatCompletionResponse/CompletionResponsewithout a GPU, completing the render/derender pair used by disaggregated serving. The Rust frontend already has the render side (/v1/*/render); this adds the inverse.Phase 1 ships the shared detokenization and state foundations plus the plain non-streaming endpoints — the equivalent of Python #43606:
derender/detok.rs: the incremental detokenization window (prev_tokens/prefix_offset/read_offset, capped at 1024) portingdetokenize_incrementally; shared with streaming in phase 3.derender/types.rs: non-streaming wire types plusDerenderStreamState(with validation) as the parsing-ready state;stream: truebodies are rejected with 400 until phase 3.derender/logprobs.rs:token_id:Nlogprob placeholders are resolved back to decoded tokens (with the U+FFFD byte-fallback repair), matching Python's_resolve_logprobs./derenderendpoints for disaggregated postprocessing #43606 → [Render] Add reasoning/tool parsing to /derender + fix byte-fallback FFFD #45919); completion derender flattensgenerate_responses× choices with re-indexing from 0, converts chat logprobs to the flat completion shape, aggregates usage, and passeskv_transfer_paramsthrough (dropped with a warning when responses disagree).max_model_len,max_logprobs, sequence counts) before any tokenizer work, mirroring_validate_derender_bounds.Related: #42729 (derender endpoints RFC) and #47161 (streaming derender RFC) — this stack ports both the non-streaming and streaming derender contract to the Rust frontend; the broader RFC scope stays open.
Duplicate-work check: searched open PRs via
gh pr list --searchforderender,derender path:rust,rust,frontend, and related keywords. No open PR implements derender inrust/. The open derender PRs (#50550 stream reasoning/tool calls, #47931 tool_calls finish reason) modify only the Python implementation; they do not overlap with this Rust port, and this stack deliberately matches current Python semantics so those Python changes can be ported as follow-ups.AI assistance: this PR was implemented with AI assistance (Kimi Code CLI). I have reviewed the full diff and run the tests below myself.
Test Plan
31 route tests in
rust/src/server/src/routes/tests.rsport the non-parsing, non-streaming behaviors asserted bytests/entrypoints/scale_out/derender/test_derender.py: text roundtrips, request_id echo, usage forwarding (supplied/omittedprompt_tokens),prompt_logprobs/kv_transfer_paramspassthrough, logprob placeholder resolution withbytes, all 400/404 error and bounds cases, and plain-detok fallback withoutchat_request.Test Result
cargo nextest run(4 touched crates): 825 passed, 1 skipped — includes all 31 phase-1 derender tests.cargo clippy --all-targets: clean.cargo fmt --check: clean.pre-commit runon the changed files: all applicable hooks passed (SPDX headers, Rust fmt, etc.).Model evals: N/A — this adds new endpoints to the Rust frontend without changing any existing serving behavior or model outputs; correctness is covered by the route-level tests above and wire compatibility with the existing Python derender contract. End-to-end GPU validation of the full stack (incl. Python↔Rust derender parity on real generations) lands with phase 3 (#53419).
Known deviations from Python (intentional)
chat_request/completion_requestare validated on lowering (e.g. unknownmodel→ 404); Python passes them to the parser unchecked.nulltoken_ids→ 400 at JSON parse; completion-side emptytoken_idsis a consistent 400 (Python leaks a 500 there).MAX_N_SEQUENCESis a constant (16384) with a TODO to plumbVLLM_MAX_N_SEQUENCESthrough server config.convert_tokens_to_stringis approximated via piece→id roundtrip + one-shot decode (exact for byte-level BPE tokenizers).