[Bugfix][Structured Output] Stop XGrammar token batches at termination - #52805
Conversation
Signed-off-by: sfeng33 <4florafeng@gmail.com>
russellb
left a comment
There was a problem hiding this comment.
Code review
Reviewed with the patch applied locally. Verification: both new tests fail on main (accept_tokens('mtp_req', [50256, 198]) returns False; xgrammar logs trying to accept new token with id 198) and pass with the fix. With the patch, tests/v1/spec_decode/test_mtp_structured_output.py (18 passed) and tests/v1/structured_output/ (59 passed) are green. I also checked the assumption the new validate_tokens depends on: in xgrammar 0.2.1, rollback(1) correctly undoes a stop-token acceptance (is_terminated() goes True -> rollback -> False -> re-accept works), so the break-then-rollback is safe.
The approach looks right. It brings xgrammar in line with backend_guidance.py, which already returns True from accept_tokens and [] from validate_tokens once stopped. Moving the _is_terminated sync inside the loop makes the desync structurally impossible rather than patching it after the fact, and the reset() change is a real latent bug fix (today a reset matcher stays flagged as terminated forever).
One issue outside the diff:
The overlapping open PR is not disclosed. #37506 has been open since March, touches the same two files, and addresses the same desync (syncing _is_terminated on the failed-accept path rather than preventing the overshoot). On that thread @chubby-cutie proposed essentially this approach. AGENTS.md asks that you explain the difference when an open PR already covers the fix. This looks like the better fix and should supersede #37506, but saying so lets a maintainer close the other one.
Minor scope note: backend_outlines.py and backend_lm_format_enforcer.py have no equivalent guard, and #52767's reporters hit this across many models. Keeping the fix xgrammar-only is reasonable, just worth stating.
Three inline comments below.
🤖 Generated with Claude Code
- If this code review was useful, please react with 👍. Otherwise, react with 👎.
russellb
left a comment
There was a problem hiding this comment.
agent comments were very minor. I'm happy with this when you're ready to merge it
Signed-off-by: sfeng33 <4florafeng@gmail.com>
|
/ci run |
|
✅ Triggered Buildkite CI #84456 for commit |
|
@sfeng33 Thank you! |
vllm-project#52805) Signed-off-by: sfeng33 <4florafeng@gmail.com> Signed-off-by: Wenhua Cheng <wenhua.cheng@intel.com>
Signed-off-by: Yifan Zong <yzong@redhat.com>
Stop accepting speculative token batches when the grammar matcher reaches its terminal state. Preserve terminal-state tracking across validation and acceptance calls so tokens after a complete structured value cannot be committed. This is the Infernal Invocation backport of vllm-project#52805 commits d8cde608cf1f3de406c75f081a76a0e6eb55a9cb, 1cf6f25351357354cf8c520c0b2976b029429668, and 1856abd22452c3da67364986ece7245fce52c950. Signed-off-by: Martin Vit <martin@voipmonitor.org>
Stop accepting speculative token batches when the grammar matcher reaches its terminal state. Preserve terminal-state tracking across validation and acceptance calls so tokens after a complete structured value cannot be committed. This is the Infernal Invocation backport of vllm-project#52805 commits d8cde608cf1f3de406c75f081a76a0e6eb55a9cb, 1cf6f25351357354cf8c520c0b2976b029429668, and 1856abd22452c3da67364986ece7245fce52c950. Signed-off-by: Martin Vit <martin@voipmonitor.org>
vllm-project#52805) Signed-off-by: sfeng33 <4florafeng@gmail.com>
vllm-project#52805) Signed-off-by: sfeng33 <4florafeng@gmail.com> Signed-off-by: Wyett <wyettzeng@gmail.com>
Signed-off-by: Yifan Zong <yzong@redhat.com>
vllm-project#52805) Signed-off-by: sfeng33 <4florafeng@gmail.com> Signed-off-by: Zhu, Zufang <zufang.zhu@intel.com>
…llm-project/vllm#52805) XgrammarGrammar.accept_tokens fed every token in a batch to the FSM matcher even after a terminating token (e.g. EOS landing in an MTP draft slot, not necessarily the batch's last token) had already terminated it -- producing "trying to accept new token" errors for the post-termination tokens and returning False (a hard failure) once _is_terminated was already set on a later call, instead of treating post-termination tokens as a benign MTP artifact. validate_tokens had the same gap (kept validating past a mid-batch termination instead of stopping) and never short-circuited when the grammar was already terminated. reset() also never cleared _is_terminated, so a request ID reused for a fresh sequence could stay wedged "terminated". Ported upstream's fix verbatim -- our backend_xgrammar.py's XgrammarGrammar (accept_tokens/validate_tokens/reset) matches upstream's pre-#52805 state closely enough for a direct line-level port, no adaptation needed: * accept_tokens: return True (not False) once already terminated; move the is_terminated() check inside the per-token loop and break as soon as it fires, instead of running the whole batch through an already-terminated matcher. * validate_tokens: early-return [] if already terminated; break out of the validation loop as soon as the matcher terminates mid-batch. * reset(): also reset _is_terminated to False. The PR author's own live-model test command matches this fork's deployment shape almost exactly: Qwen3.8-27B, --tensor-parallel-size 2, --enable-auto-tool-choice --tool-call-parser qwen3_coder --reasoning-parser qwen3, MTP with num_speculative_tokens=3. Upstream's own new tests live in tests/v1/spec_decode/test_mtp_structured_output.py, which doesn't exist in this tree (no prior xgrammar/structured-output test coverage here at all) and depends on fixtures/helpers not present. Added tests/v1/structured_output/test_xgrammar_termination.py instead: exercises the real XgrammarGrammar dataclass directly (no CUDA dependency in these methods) via a minimal duck-typed stand-in for xgr.GrammarMatcher, rather than a real compiled grammar/tokenizer -- avoids depending on the installed xgrammar version's compile API while still driving the actual production state-machine code. Verified both new tests fail against the pre-fix code (num_processed_tokens == 2 instead of 1; validate_tokens returns [EOS, TRAILING] instead of [EOS]) and pass after. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
vllm-project#52805) Signed-off-by: sfeng33 <4florafeng@gmail.com> Signed-off-by: khushali9 <khushali.desai9@gmail.com>
vllm-project#52805) Signed-off-by: sfeng33 <4florafeng@gmail.com>
Purpose
Prevent XGrammar from receiving additional tokens after accepting a terminating stop token within the same batch.
Fixes #52767.
This covers two related problems:
accept_tokens()could return early after rejection without synchronizing LLM's cached_is_terminatedstate.Test Plan
Unit test:
Live model test:
ignore_eos=trueto force another grammar advance after EOStool_choice=autoandrequired