Repository navigation
chat : keep unparsed Qwen3-Coder output as content instead of dropping it - #235
danielhanchen wants to merge 3 commits into
Conversation
…g it The Qwen3-Coder parser has no end-of-input check. When the model writes a tool call that does not parse (a tool that is not offered, or a <tool_call> cut off by EOS), the final parse succeeds on the text before it and the rest is dropped. The client gets a reply that stops mid-sentence with finish_reason "stop" and no tool call. - qwen3-coder: with tool_choice auto, keep anything after the tool call section as content. The lazy grammar is built from the trigger rule only, so it is unchanged. tool_choice required is left alone. - common_chat_peg_parse: a final parse that still needs more input and recovered no tool call is retried without the lenient flag, so the fallback above can take the unfinished call.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1ba5b7b7bf
ℹ️ 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".
| common_chat_msg strict_msg; | ||
| strict_msg.role = "assistant"; | ||
| make_mapper(strict_msg)->from_ast(strict_ctx.ast, strict); | ||
| msg = std::move(strict_msg); |
There was a problem hiding this comment.
When a reasoning-capable Qwen response reaches EOS or max_tokens before </think>, the lenient parse has the prefix in reasoning_content and no tool call, so this retry runs; the strict parser rejects the incomplete reasoning branch, accepts the raw-content fallback, and this assignment replaces the parsed reasoning with <think>... in normal assistant content. Non-streaming responses consequently expose reasoning as the answer, while streaming responses emit it again as content after already emitting it as reasoning. Only use the fallback when it does not reclassify fields already recovered by the lenient parse, or scope it specifically to the unparsed tool-call-tail case.
Useful? React with 👍 / 👎.
|
|
||
| // a final parse that needs more input ended inside a construct and drops the rest | ||
| // retry strictly so a parser with a fallback for it (e.g. qwen3-coder) can keep the whole output | ||
| if (!is_partial && result.need_more_input() && msg.tool_calls.empty()) { |
There was a problem hiding this comment.
When parallel_tool_calls is enabled and a valid call is followed by a tail cut inside the next call, such as </tool_call>\n<tool_call>\n<function= at EOS, the lenient repetition returns need_more_input with the first call already mapped. This condition then skips the strict retry solely because msg.tool_calls is nonempty, so the new trailing rest() parser is never reached and the incomplete second tail is still silently discarded. The retry should also be considered when existing tool calls are present, provided the strict result preserves those calls.
Useful? React with 👍 / 👎.
A plain reply also ends the lenient parse on need-more-input, so the strict retry ran on every request and doubled the final parse (about 80 -> 190 us on an 8K char reply). Skip it when the output still ends with the parsed content or reasoning, which means nothing was dropped. The retry now runs only for output cut inside a construct, such as <tool_call>\n<function= at EOS.
This branch has not been deployed
1 participant
Problem
In Unsloth Studio, a Qwen3.8-27B reply ended in the middle of a code block (
const PIPE_SPEED: f32 =) and came back as a normalfinish_reason: "stop"with no tool call and no error, so there was nothing to continue from. The server had generated about 15 more tokens than the client received, which is what the bug below produces.The Qwen3-Coder parser (also used for Qwen3.5 / 3.6 / 3.8, Nemotron Nano 3 and StepFun) builds its top level as
with nothing requiring the end of input. When the text after
<tool_call>does not parse as a call, the final parse still succeeds on the prefix andcommon_chat_peg_parsemaps only that prefix. The rest is silently discarded:...f32 =...f32 =<tool_call>\n<function=then EOS...f32 =\n(tail dropped)<tool_call>then EOS...f32 =\n(tail dropped)tools...f32 =\n(whole call dropped)The same happens on upstream master (checked at ed7ac35).
Reproduced against a live server with the shipped b11160 mix build: a continuation whose prefill holds an unknown call generated 19 tokens and streamed 0 characters,
finish_reason: "stop".Change
common/parsers/qwen3-coder.cpp: withtool_choiceauto, appendp.content(p.rest())after the tool call section, so output that is not a valid call is returned as content instead of dropped. The lazy grammar is built from the trigger rule only, so it is unchanged.tool_choicerequired keeps the old parser, since its non-lazy grammar is built from the whole parser and would otherwise accept trailing text.common/chat.cpp: a final (non-partial) parse is still lenient, so an unfinished<tool_call>\n<function=at EOS returns need-more-input and never reaches that fallback. When a final parse needs more input and recovered no tool call, retry it without the lenient flag and use that result if it covers the whole output. Formats without a fallback fail the strict parse and keep the old result. A partial call that the lenient parse did recover (for example a named call with truncated arguments) is kept as before. Also folds the duplicated mapper selection into one lambda.Before / after
Parser output with the Qwen3.8 template and a
search_conversationtool, final parse:<tool_call>\n<function=at EOS<tool_call>at EOSThe generated grammars for
tool_choiceauto and required are byte-identical before and after.End to end, the shipped b11160 CUDA server with only
libllama-commonrebuilt from the same source plus this patch:lengthlengthstopstopPerformance
The grammar is unchanged, so sampling is unaffected. Parse cost, measured on an 8K char code reply streamed in 4 char chunks the way the server parses it (every prefix as partial, then the final parse), b11160 source with and without this PR, identical build flags, median of 6 interleaved rounds:
<tool_call>\n<function=at EOSThe per token differences are noise on a loaded machine (the arms swap order between rounds). The strict retry runs only when the lenient result lost the end of the output, which is the last row, once per request. The second commit adds that check: without it a plain reply also ended the lenient parse on need-more-input and every final parse ran twice (79 -> 183 us).
Tests
test_qwen3_coder_unparsed_tailintests/test-chat.cppforQwen3.5-4B.jinjaandQwen3-Coder.jinja: the unfinished and unknown-tool tails are kept, streamed deltas add up to the final message, a valid call still parses, and text after a valid call is kept. It parses directly rather than throughpeg_tester, because that harness also matches the input against the triggered grammar, which rejects exactly these calls.test-chatpasses.test-chat.pr-set.jsonin order, then this commit) it merges cleanly. On b11205 it also merges; the only conflicts there come from two existing pins (ggml-org#24423 and mtmd: test that every projector is registered and uniquely named #176).Not covered
If the model streams the start of a valid call and then breaks its syntax (for example
<parameter=query>PIPEwith no newline), the server has already streamed a tool call name that the final parse no longer finds, and the request fails withInvalid diff: now finding less tool calls. That happens before and after this change and is left for a separate fix.