Repository navigation
[Bugfix][Structured Output] Feed the token that implicitly ends reasoning to the grammar - #59627
Conversation
yzong-rh
left a comment
There was a problem hiding this comment.
Great catch! Thanks for the PR.
I attempted a fix that makes _get_constraint_start return reasoning_end_offset instead reasoning_end_offset + 1 for inclusive_reasoning_end_token_ids. However, it changed the output distribution under spec decode and was incorrect.
The correct fix, as implemented here, is to distinguish between grammar_start and constraint_start.
validate_tokensvalidate fromgrammar_startgrammar_bitmaskwill consume tokens fromgrammar_startbut only create bitmasks for tokens fromconstraint_startaccept_tokensadvance grammar fromgrammar_start
Note that failures due to implicit reasoning end can still occur during rejection sampling when the target rejects a draft and samples an grammar-invalid reasoning end marker (e.g. <tool_call> is sampled when grammar disallows tool calls). However, this is a pre-existing issue and not introduced by this PR. I'll create a bug report for more discussion on this.
NIT:
Current implementation adds more coupling between StructuredOuptutManager and ReasoningParser. The former now reaches into reasoner.inclusive_reasoning_end_token_ids.
A cleaner impl could be to
- rename and rewrite
ParserEngine.find_reasoning_end_offsetto also indicate if a reasoning end is implicit or explicit. - rename and rewrite
StructuredOutputManager._get_constraint_startto return both grammar start and constraint start (or constraint start and a grammar start offset) - trim abstract
ReasoningParserchanges and limit the fix to engine-based parsers
5805e42 to
bbb2421
Compare
|
@yzong-rh Thanks for the review and the cleaner design. I amended the commit as you suggested:
The flow tests pass with no change to their expected rows, and the CPU bonus-row check gives the same output as before. I added a test that a reasoner that is not engine-based keeps the old grammar start. Thanks for the rejection-sampling note. I added it to Limitations and will link your issue once it is filed. Disclaimer: this reply and the changes were drafted with the help of an AI assistant (Claude) and reviewed by me. |
…ning to the grammar Some reasoning parsers end reasoning on the tool-call start token when no </think> is emitted (e.g. GLM's <tool_call>). The structured-output manager started the grammar one token after that token, so the grammar never saw it and forced a second <tool_call>; required/named tool calls were lost. ParserEngine.find_reasoning_end now also says whether the end token is content (an implicit end), and StructuredOutputManager._get_constraint_bounds returns both the grammar start and the constraint start. The grammar consumes an implicit end token that it accepts, while masking still starts after it. The fix is limited to engine-based parsers; ReasoningParser is unchanged. Fixes vllm-project#59608 Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Signed-off-by: Simon Veitner <sveitner@redhat.com>
bbb2421 to
4d1ab63
Compare
|
/ci run |
|
✅ Triggered Buildkite CI #93616 for commit |
|
I think it might be cleaner to let This lets Diff implementing this: https://gist.github.com/yzong-rh/4b8a967a6d19671ede197483191d9eb7 I also consolidated the implicit-end coverage into Tested my diff with your repro, tool call remains correct. |
Let constraint_start only control where grammar masking begins in grammar_bitmask. validate_tokens and accept_tokens now work from grammar_start, so _get_constraint_bounds always sets grammar_start to an implicit reasoning-end token and no longer probes the grammar with validate_tokens. The grammar rejects invalid implicit markers through validate_tokens, and accepted markers are fed to it normally. Fold the implicit-end coverage into FlowCase (implicit_end_accepted, implicit_end_rejected) and drop the separate inclusive-end tests. Suggested in vllm-project#59627 (comment) Co-authored-by: Yifan Zong <263738792+yzong-rh@users.noreply.github.com> Signed-off-by: Simon Veitner <sveitner@redhat.com>
Head branch was pushed to by a user without write access
applied after local validation |
|
/ci run |
|
✅ Triggered Buildkite CI #93667 for commit |
An engine-based reasoning parser ends reasoning implicitly on a token that is also content, such as `<tool_call>`, and vllm-project#59627 feeds that token to the grammar. Bitmask rows stay unconstrained while the request reasons, so the model can sample such a token when the grammar cannot start with it (a JSON schema). accept_tokens then fails and the scheduler finishes the request with an error. While a request reasons, mask the implicit reasoning-end tokens its grammar rejects in its initial state. The explicit end and implicit ends the grammar accepts (required and named tool calls) stay allowed. The grammar does not advance before reasoning ends, so the set is computed once per request. Co-authored-by: Claude <noreply@anthropic.com> Signed-off-by: Maroon Ayoub <mayoub@redhat.com>
Overview
Structured output skipped a reasoning-end token that is also content, such as GLM's
<tool_call>with no</think>before it. The grammar then forced a second<tool_call>, andrequired/named tool calls were lost. This PR feeds that token to the grammar, which matches whatextract_content_idsalready does. Fixes #59608.Claims
ParserEngine-based parsers, the grammar now consumes a reasoning-end token thatextract_content_idskeeps as content, if the grammar accepts it. Close tags such as</think>are still skipped.ReasoningParserand legacy parsers do not change.<tool_call>signature went from 7/40 (required) and 8/40 (named) to 0/40. With MTP speculative decoding it went from 9/40 and 11/40 to 0/40.<tool_call>cannot force the end of reasoning, and when it is the last draft the bonus row is masked from the grammar state after it.qwen3+hermes), and the fix removes it there too. Correct-call rates on the GLM tool-result prompt did not go up, because of a separate argument bug (see Limitations).Validation
Unit tests
Result with 1 GPU visible: 6402 passed, 1 xfailed.
pre-commit run --files <changed files>and the manual mypy-3.12 hook pass.New tests are
test_inclusive_reasoning_end_tokenandtest_end_token_committed_aloneintests/v1/structured_output/test_structured_output_manager.py(xgrammar and guidance), andtest_implicit_reasoning_end_of_parsers(realglm47_moeconfig) intests/parser/engine/test_parser_engine.py. With grammar start forced to the constraint start, 8 of the 13 new cases fail:inclusive,inclusive_first,inclusive_lastandno_drafts, on both backends.CI runs these tests through
misc.yaml.tests/parser/cohereis left out because it needs the optionalcohere_melodypackage.A CPU script that drives the scheduler flow (validate drafts, bitmask, accept, next bitmask) with
<tool_call>as the last draft, the only draft, a mid-window draft and the recovered token gives a second<tool_call>in all 8 stock cases and none with the fix.Live runs
The live runs used the first version of this PR (5805e42cfa). The current commit only restructures that code, and the unit tests and the CPU check give the same results on both versions.
GLM-5.3-Flash (
RedHatAI/GLM-5.3-Flash-NVFP4), TP4 on H100, xgrammar, the tool-result repro from the issue, 40 seeds at temperature 0.7:<tool_call>signatures, 40 seedsrequiredrequired, MTPMTP draft acceptance was 59.1% before and 58.5% after.
Qwen3 (
Qwen/Qwen3-0.6B,--tool-call-parser hermes --reasoning-parser qwen3),logit_bias-100 on</think>and +5 on<tool_call>, temperature 0, 3 repeats:requiredauto(control)No standard tool-calling eval (such as BFCL) was run.
Details
Root cause.
_get_constraint_startreturned the index after the reasoning-end token. That is right for</think>, which is not content. A tool-call start that ends reasoning is content, so the grammar started one token after the content did. This came in with #55223, and #48200 kept it.Change.
ParserEngine.find_reasoning_end_offsetbecomesfind_reasoning_end, which returnsReasoningEnd(offset, implicit).implicitis True when the end token is not the close tag, the same ruleextract_content_idsuses.StructuredOutputManager._get_constraint_startbecomes_get_constraint_bounds, which returnsConstraintBounds(grammar_start, constraint_start). For an implicit end that the grammar accepts, the grammar starts at the end token and masking starts one token later. Otherwise both equal the old value.validate_tokens, the spec-decode loop ofgrammar_bitmaskandaccept_tokensfeed the grammar fromgrammar_start. Masked rows and early returns still useconstraint_start, so a drafted<tool_call>cannot force the target to end reasoning.json_schemaresponse format with tools present.Affected parsers. Every in-tree
ParserEngineconfig whose tool-call start ends reasoning now feeds that token to the grammar: glm47_moe (and ling3), qwen3 (and mimo), nemotron_v3, granite_thinking, seed_oss, gemma4, kimi_k2, minimax_m2, deepseek_v4, deepseek_v41, plamo3, and mistral withreasoning_encoding="special_token". A token counts only if the tokenizer has a single id for it. On the real DeepSeek-V4 tokenizer the DSML opener is several tokens, so deepseek_v4 does not change in practice. For plamo3 the token is the end-of-turn tag, which is normally a stop token; I expect that to be harmless but did not test it.Tradeoffs.
_get_constraint_boundsreturns before any reasoner call. Only a step with an implicit end token runs one extra single-tokenvalidate_tokenson the CPU. There is no CPU/GPU sync.Limitations.
<tool_call>token inside<arg_value>. When the model quotes tool-result text that contains it, arguments break (Write({}), missing arguments). Many responses this PR recovers now hit that bug. It will be reported separately.ParserEngine) parsers such ask2_horizonkeep the old behavior until they move toParserEngine.continue_final_message) still starts the grammar after it, and multi-token text markers cannot be expressed as a token-id set.Related. No open PR changes where the grammar starts after reasoning; #36138, #56403, #57718 and #57754 touch nearby paths.
This PR was investigated and drafted with an AI assistant (Claude). The tests and live runs listed here were run on real hardware, and a human reviewed the PR before submission.
Pull Request Checklist
I used vLLM's
/pr-checklistskill. (Mandatory for agents, optional for humans).AI assistance was used during the creation of this PR.
Design Fit: Minimizes impact on core components, reuses existing functionality, and justifies added complexity.
Testing and Validation: Validates the change and ensures any added tests are meaningful and reliable, with CI coverage or documented CI resource constraints and validation performed outside CI.
Code Quality and Style: Keeps code and comments clear and concise, and updates relevant documentation and examples.
Pull Request Contents: Includes a brief summary and relevant links, supports claims with evidence, explains root causes and implementation trade-offs, and follows the contributing guide.
🤖 Generated with Claude Code