[Render] Add reasoning/tool parsing to /derender + fix byte-fallback FFFD - #45919
Conversation
Head branch was pushed to by a user without write access
|
I've checked that 70a8a12 fixes the regression, thanks for a quick fix! |
Will add corresponding CI to prevent this from happening next time. |
|
Did you use the /derender endpoint? |
Oh I didn't, my issue was about chat completions endpoint to be clear. |
|
Wait, how does pr 43606 affect chat completions. |
|
I think 43606 is only about add two independent endpoint? |
|
I haven't read the PR through and report, but a git bisect on my end points to that PR. Is there other places where the |
|
weird, you sure? I mean, this is only called by /derender endpoint related stuff. It matters a lot to me as I want to make sure that pr introduced no regression. Could you provide more context? |
|
It seems I should have posted a proper issue body. The context is: We are testing glm-5.1 online server on near-weekly basis, the previous commit that have no issue was 7852e50 (dated 2026.06.11) and the next commit that we got trouble was b997071. So I have bisected the culprit commit with the following test script and arrived at 43f0e02. # full server launch script
vllm serve zai-org/GLM-5.1-FP8 \
--tensor-parallel-size 8 \
--enable-expert-parallel \
--max-model-len 202752 \
--speculative-config.method mtp \
--speculative-config.num_speculative_tokens 1 \
--reasoning-parser glm45 \
--enable-auto-tool-choice \
--tool-call-parser glm47 \
--hf-overrides.index_topk_freq 4
# client-side test script - try to make the model spit some hangul(non-ascii)
curl http://localhost:8080/v1/chat/completions -H 'Content-Type: application/json' \
-d '{"model": "zai-org/GLM-5.1-FP8", "messages": [{"role": "user", "content": "삼성전자 주주총회는 어디에서 열려?"}], "stream": true}'
# then the model response contains a U+FFFD in the middle of the response content for about 1-in-3 chance
# an example case is like: ...영�구에 있는... |
|
May I reach out to you via slack, let's keep in touch and I will try to reproduce it tomorrow. aoshen524@gmail.com/ |
Thanks for your time and effort, I am on slack by the profile Juhyun Kim. Come to think of it, my bisect logic relies on probabilistic sampling (I ran a pre-existing set of ~12 requests and searched for the existence of U+FFFD per each commit) so it may point to a wrong commit, I'll try to find a more stable reproducer and do the bisect again as well. |
23eaadb to
1bd20e7
Compare
0168464 to
e4dd08d
Compare
4245656 to
4a56fca
Compare
Add parser support to derender_chat_response so the RL training loop can receive structured (reasoning, content, tool_calls) instead of raw text. When chat_request is provided, instantiate the configured Parser (DelegatingParser for Qwen3/DeepSeek/Nemotron, HarmonyParser for GPT-OSS) and call parser.parse() inline. Build the ChatMessage directly — no build_chat_message extraction, no finish_reason logic, no parallel tool call filtering. Only the "auto" tool_choice path is needed for RL. Without chat_request, falls back to plain detokenization (existing behavior). E2E tests cover: plain roundtrip, token identity invariant, non-ASCII, reasoning parsing, tool call extraction, and combined reasoning+tool. Co-authored-by: Ao Shen <aoshen@inferact.ai> Signed-off-by: aoshen524 <aoshen524@gmail.com>
ed39301 to
710f694
Compare
sfeng33
left a comment
There was a problem hiding this comment.
Giving a yes because the parser wiring looks generally correct, assuming you have done the testing on the expected /derender behvaiour
37316b6 to
710f694
Compare
|
Hi @aoshen02, the pre-commit checks have failed. Please run: uv pip install pre-commit>=4.5.1
pre-commit install
pre-commit run --all-filesThen, commit the changes and push to your branch. For future commits, |
d918770 to
b87c065
Compare
Signed-off-by: aoshen524 <aoshen524@gmail.com>
034194f to
9054119
Compare
Co-authored-by: Claude <noreply@anthropic.com> Signed-off-by: aoshen524 <aoshen524@gmail.com>
- Use args.reasoning_parser instead of args.structured_outputs_config.reasoning_parser in init_render_app_state (the Namespace doesn't go through AsyncEngineArgs post-processing, so the structured_outputs_config field stays at its default empty string). - Pass empty list instead of None for ChatMessage.tool_calls (Pydantic rejects None for list[ToolCall] field). - Wrap tool_call test output in <think>...</think> so DeepSeek R1 reasoning parser doesn't consume the <tool_call> tags as reasoning content. Co-authored-by: Claude <noreply@anthropic.com> Signed-off-by: aoshen524 <aoshen524@gmail.com>
The openai-harmony Rust library downloads o200k_harmony.tiktoken from openaipublic.blob.core.windows.net, which returns 404 in environments without pre-cached vocab. Skip gracefully instead of crashing. Co-authored-by: Claude <noreply@anthropic.com> Signed-off-by: aoshen524 <aoshen524@gmail.com>
59262c8 to
e7deb15
Compare
Indeed, I and @aoshen02 had discussed on slack and reached a confirmation that the derender PR is not the root cause. I couldn't narrowed down the root cause (mostly due to reproducibility) but it seems @bbrowning's analysis sounds valid - in the sense that the commit ranges that I have bisected are in the middle of unified parser refactoring. |
Please do try this, [Bugfix][Parser] Fix U+FFFD leak at reasoning-to-content transition in engine parsers by bbrowning · Pull Request #46159 · vllm-project/vllm |
| exclude_tools_when_tool_choice_none=args.exclude_tools_when_tool_choice_none, | ||
| tool_parser=args.tool_call_parser, | ||
| reasoning_parser=args.structured_outputs_config.reasoning_parser, | ||
| reasoning_parser=args.reasoning_parser, |
There was a problem hiding this comment.
Are you sure this is equivalent?
There was a problem hiding this comment.
Yes, please check create_engine_config() (arg_utils.py:2200-2201), but init_render_app_state never calls create_engine_config() (no engine needed), so it stays empty. args.reasoning_parser always has a value, regardless of whether the engine is initialized.
| ) | ||
|
|
||
| chat_template_kwargs: dict[str, Any] = {} | ||
| if not self.use_harmony: |
There was a problem hiding this comment.
Harmony parser like GptOssReasoningParser does not need chat_template_kwargs to get any values while most parsers like qwen3 parser need chat_template_kwargs to
: self.thinking_enabled = chat_kwargs.get("enable_thinking").
We can also remove the check, what do you think.
…FFFD (vllm-project#45919) Signed-off-by: aoshen524 <aoshen524@gmail.com> Co-authored-by: Martin Hickey <martin.hickey@ie.ibm.com>
|
Thanks @aoshen02 for taking the parsing on board and driving this PR. Sorry but I was OOTO and missed the fun! :) |
…FFFD (vllm-project#45919) Signed-off-by: aoshen524 <aoshen524@gmail.com> Co-authored-by: Martin Hickey <martin.hickey@ie.ibm.com>
…FFFD (vllm-project#45919) Signed-off-by: aoshen524 <aoshen524@gmail.com> Co-authored-by: Martin Hickey <martin.hickey@ie.ibm.com> Signed-off-by: Qiang Li <qiang.li2@amd.com>
…FFFD (vllm-project#45919) Signed-off-by: aoshen524 <aoshen524@gmail.com> Co-authored-by: Martin Hickey <martin.hickey@ie.ibm.com>
…FFFD (vllm-project#45919) Signed-off-by: aoshen524 <aoshen524@gmail.com> Co-authored-by: Martin Hickey <martin.hickey@ie.ibm.com>
…FFFD (vllm-project#45919) Signed-off-by: aoshen524 <aoshen524@gmail.com> Co-authored-by: Martin Hickey <martin.hickey@ie.ibm.com>
Summary
Merges the reasoning/tool-call parsing from PR #45045 and fixes the byte-fallback U+FFFD regression in derender logprobs.
Reasoning & tool call parsing (from #45045)
build_chat_message()inserve/utils/chat_message_builder.py— single source of truth shared by coupled chat path and/derenderderender_chat_response()acceptschat_requestfor parser-aware parsing (reasoning + tool calls)chat_completion/serving.pydelegates ~268 lines of inline logic tobuild_chat_message()OpenAIServingRender.__init__initializesparser_cls,tool_call_id_typeByte-fallback FFFD fix
resolve_token_id_placeholderusestokenizer.decode()instead ofconvert_ids_to_tokens+convert_tokens_to_string_resolve_logprobstracks context token IDs and appliescorrect_decoded_tokenwhen U+FFFD is detected, mirroringv1/engine/logprobs.pyNot duplicating an existing PR
PR #45045 is open but stale (based on older codebase, conflicts with current main). This PR adapts its changes to the current codebase and adds the FFFD fix + solid CI.
Test plan
test_derender_byte_fallback.py— 17 unit tests: baseline DOES produce U+FFFD, fix does NOT (emoji, CJK, mixed, consecutive)test_render_derender_e2e.py— render→derender roundtrip tests: plain roundtrip, reasoning extraction, tool call extraction, token-identity, JSON boolean drift, non-ASCII roundtriptest_derender.py— updated from [Render] Add reasoning and tool call parsing to/derender#45045 with parity testsAI assistance was used (Claude). Co-authored with @hickeyma (PR #45045 original author).
🤖 Generated with Claude Code