Conversation
|
Documentation preview: https://vllm--50550.org.readthedocs.build/en/50550/ |
904701b to
8ab9649
Compare
chaunceyjiang
left a comment
There was a problem hiding this comment.
@hickeyma Thanks for your PR! I’ll test it locally.
1db318b to
ba69e39
Compare
|
This pull request has merge conflicts that must be resolved before it can be |
ba69e39 to
e5d1115
Compare
Review comment: - vllm-project#50550 (review) Signed-off-by: Martin Hickey <martin.hickey@ie.ibm.com>
Review comment: - vllm-project#50550 (review) Signed-off-by: Martin Hickey <martin.hickey@ie.ibm.com>
43c03cf to
856d7e7
Compare
Review comment: - github.com/vllm-project/pull/50550#pullrequestreview-4860834422 Signed-off-by: Martin Hickey <martin.hickey@ie.ibm.com>
Streaming chat derender used to raise NotImplementedError as soon as a reasoning or tool parser was configured, so a disaggregated render tier could only get parsed output from the non-streaming endpoint. This adds the parser path. Each chunk rebuilds the parser and replays every prior output token through parse_delt then runs the new tokens and emits the merged delta. Also fixes the batch path silently falling back to plain detokenization when chat_request was missing on a parser configured model. Both paths now reject that with a 400 instead of leaking raw markup into content. The streaming path additionally requires prompt_token_ids when a parser is configured. parse_delta needs it to tell whether the prompt left reasoning open (e.g. a chat template that pre-opens <think>) and a missing prompt_token_ids would otherwise silently misclassify reasoning as content instead of rejecting with a 400. Signed-off-by: Martin Hickey <martin.hickey@ie.ibm.com>
Signed-off-by: Martin Hickey <martin.hickey@ie.ibm.com>
Signed-off-by: Martin Hickey <martin.hickey@ie.ibm.com>
{exc!r} wraps arbitrary exception reprs in client facing errors which
can leak internal class/module details that sanitize_message() doesn't
strip. The fix is to use str(exc) instead.
Signed-off-by: Martin Hickey <martin.hickey@ie.ibm.com>
Co-authored-by: depthfirst-app[bot] <184448029+depthfirst-app[bot]@users.noreply.github.com> Signed-off-by: Martin Hickey <martin.hickey@ie.ibm.com>
Commit e4f5837fd47e623b213357266f9ff728f436820c Signed-off-by: Martin Hickey <martin.hickey@ie.ibm.com>
Review comment: - vllm-project#50550 (review) Signed-off-by: Martin Hickey <martin.hickey@ie.ibm.com>
Signed-off-by: Martin Hickey <martin.hickey@ie.ibm.com>
Review comment: - github.com/vllm-project/pull/50550#pullrequestreview-4860834422 Signed-off-by: Martin Hickey <martin.hickey@ie.ibm.com>
17c6a2b to
9242f54
Compare
prompt_token_ids wasn't checked against max_model_len even though the parser path rescans it in full on every chunk. Add it into the existing bound check alongside output_token_ids. Feddback from review: - vllm-project#50550 (comment) - vllm-project#50550 (comment) Signed-off-by: Martin Hickey <martin.hickey@ie.ibm.com>
|
Thanks @bongwoobak for the review. Updated and ready for review again. |
Signed-off-by: Martin Hickey <martin.hickey@ie.ibm.com>
| ) | ||
| return chunk, updated_state | ||
|
|
||
| def _derender_chat_stream_parsed( |
There was a problem hiding this comment.
Can we use something like a shared ChatStreamProcessor instead?
There was a problem hiding this comment.
This is the same idea RFC #47161 already calls out under "Future" as: an LRU of live Parser instances keyed by (request_id, choice_index) to skip replay on a cache hit. It is stated as deferral in the PR description above.
The reason that it's not implemented in this PR is that the render tier is stateless and horizontally scaled (see derenderer.md). Shared/cached parser is only safe as a best effort optimization with a correct fallback to full replay on a miss. Eviction, memory bounds and behavior when a load balancer doesn't route a stream's chunks to the same replica all need their own design and review, separate from this PR.
There was a problem hiding this comment.
Sorry @sagearc but I misread this one earlier and answered it as a caching question. Looking at it again, you are right that _derender_chat_stream_parsed duplicates most of the post parse handling in chat_completion_stream_generator.
Proposal: pull a per choice processor out of chat_completion_stream_generator that owns the parser, tools_streamed and the parse_delta -> finish_reason -> parallel tool call filter step. Serving uses it for live output. Derender uses it for both replay and the current chunk, keeping only the replay loop and tool call ID pinning on its side. I'd leave the Responses API loop out since it emits a different event shape.
@chaunceyjiang this touches the main chat streaming path. Would you prefer it as a small no behavior change PR that this one rebases on or folded into this PR? I'd lean towards the small PR so the serving change can be reviewed on its own.
There was a problem hiding this comment.
sounds good, also possible to start with the centralized decode logic pr
|
|
||
| # Replay history to reconstruct parser state. The result is thrown | ||
| # away and only the current chunk's emission goes to the client. | ||
| _feed(state.output_token_ids, finished=False) |
There was a problem hiding this comment.
Could we retain parser state per stream instead? Replaying the full history on every call makes the stream quadratic
There was a problem hiding this comment.
It can also help keeping tool-call id stable across retries
There was a problem hiding this comment.
Could we retain parser state per stream instead? Replaying the full history on every call makes the stream quadratic
I agree that the cost is real. It's the O(n³) already documented in derenderer.md's "Streaming cost" section and in the RFC. Same answer as the ChatStreamProcessor thread above. This is RFC #47161's "Future" item (live parser LRU cache), deliberately deferred to keep this PR's replay model minimal and correct first.
There was a problem hiding this comment.
It can also help keeping tool-call id stable across retries
Good point. Worth noting IDs are already pinned within a single normal stream via last_tool_call_ids (see test_tool_call_id_pinned_across_chunks). This is specifically about a retried call reusing an ID after the client never saw the first response. That's the same retain state server side mechanism as the cache above, so I'll add it into the design questions rather than solve it separately here.
There was a problem hiding this comment.
Related question from @sagearc on slack:
Is it possible to keep everything as is but not detokenize all token ids? only a fixed size window backwards - don't we do it on the non-streaming path?
The non-streaming path doesn't do that. It does one full tokenizer.decode() over all the tokens. The bounded window is on the plain streaming path (no parser) from #47301. It can't work for the parser path because the parsers need the full history to be correct. With a window those give wrong results, not approximate ones. Detok also isn't the expensive part, the replayed parse_delta calls are.
| tokenizer, [tok_id], detok_state, skip_special_tokens=False | ||
| ) | ||
| delta = parser.parse_delta( |
There was a problem hiding this comment.
Can we preserve the producer's delta granularity here? Splitting multi-token updates into one-token parser calls can diverge from standard serving.
There was a problem hiding this comment.
Following up here as I only fixed half of this. The live chunk now goes through parse_delta in one call but replay still feeds history one token at a time because DerenderStreamState only carries a flat output_token_ids. A multi-token step (e.g. spec decode) is still split on replay, so the rebuilt parser state can diverge from standard serving. For example history_tool_call_cnt is bumped once per parse_delta call that starts a tool call.
I'll carry the original chunk lengths in the state (output_chunk_lens: list[int]) so replay uses the same boundaries the chunks arrived with and add a test that replays a multi-token chunk.
Standard streaming calls parse_delta once per engine step with that step's full token_ids which can be more than one under speculative decoding. The derender's parser path split every chunk into one token calls regardless, diverging from that and from the plain (no parser) path which already batches its detokenize call. The live chunk now goes through parse_delta as a single call at whatever granularity it arrived with. Replay of prior history is unchanged and still runs one token at a time, since DerenderStreamState only stores a flat token list with no record of the original chunk boundaries. Dropped _merge_delta_messages which only existed for the old per token loop. Signed-off-by: Martin Hickey <martin.hickey@ie.ibm.com>
Purpose
/v1/chat/completions/derenderacceptsstream: truebut only ever detokenized plain text. Configure a reasoning or tool parser and it rejected with a 400 (NotImplementedErrorinternally, mapped throughcreate_error_response). This PR fills that in where the streaming path now emits realreasoning/content/tool_callsdeltas matching what a standard generatevllm servewould stream for the same tokens.What's not in this PR
Parserinstances keyed by(request_id, choice_index)to skip replay on a cache hit). Deliberately deferred to keep this PR minimal because adding a cache later is a pure optimization and not a behavior change.renderer_num_workers, default1), same as the rest of/render's CPU-bound work. A parser configured deployment expecting concurrent long streams should size that up. Documented inderenderer.md.Partial #47161
/cc @sagearc @bongwoobak @aoshen02
Test Plan
pytest -s -v tests/entrypoints/scale_out/derender/test_derender_stream.pypytest -s -v tests/entrypoints/scale_out/derender/test_derender.pyTest Result
All tests pass.