Repository navigation
Conversation
sungsooha
force-pushed
the
inkling/structural-tag
branch
from
August 27, 2026 23:35
08b6e9d to
533e800
Compare
sungsooha
added a commit
to sungsooha/dynamo
that referenced
this pull request
Aug 28, 2026
… marker Refreshes the vendored patch from vllm-project/vllm#54120, which now begins the tool/text tags at `<|content_invoke_tool_json|>` / `<|content_text|>` instead of at `<|message_model|>`. Inkling's chat template prefills the role marker into the generation prompt (it ends `...<|end_message|><|message_model|>`), so generation resumes inside the model message and never re-emits it. The tags are `tags_with_separator` with `at_least_one=true`, so they anchor rather than trigger -- the old grammar therefore required a SECOND role marker. Compiling the tag with xgrammar shows the inversion, for forced/required/auto alike: suffix-only with duplicate marker before rejected accepted after accepted rejected It appeared to work only because the parser absorbed the duplicate, while manufacturing exactly the unconsumed-control-marker residue ai-dynamo#12510 strips. Validated on Inkling-NVFP4 / GB300, vLLM 0.28.0, MTP 8, 2-replica AGG, applied over the shipped image via ConfigMap: forced tool choice honoured 5/5 against a prompt matching the other tool; required = 1 call; parallel_tool_calls true/false = 2/1; no control-marker residue in content -- all in both streaming and non-streaming. Full p0 blocking count unchanged at 2 (the unrelated reasoning_budget_zero vLLM limitation). Note the probe suite could NOT have caught this: control_marker_residue passed before the fix too, because the parser was absorbing the duplicate. The grammar-level acceptance test is what exposed it. Reported by dynamo-review-agent on ai-dynamo#13954. Signed-off-by: Sungsoo Ha <sungsooh@nvidia.com>
sungsooha
marked this pull request as ready for review
August 28, 2026 05:27
sungsooha
requested review from
aarnphm,
bbrowning,
chaunceyjiang and
sfeng33
as code owners
August 28, 2026 05:27
sungsooha
added a commit
to sungsooha/dynamo
that referenced
this pull request
Aug 28, 2026
…ing patch Syncs both of our unmerged fixes to their reviewed state and re-validates the container on real hardware. frontend (ai-dynamo#13947, P1 review): reasoning tokens are now counted by the pinned vLLM parser's own `count_reasoning_tokens` over a per-choice ledger of the ORIGINAL generated ids, appended before any branch in process_output. The per-chunk classifier and the terminal `tokenizer.encode(saved_reasoning)` retokenisation are both removed -- the latter drifted because detokenisation is not injective (generated id 77150 decodes to '10' and re-encodes to two tokens). vLLM patch (vllm-project/vllm#54120): regenerated against the v0.28.0 tree, now also accepting `<|content_model_end_sampling|>`. The Inkling parser treats it as a co-equal terminator -- `is_reasoning_end` returns True on it and `count_reasoning_tokens` closes a block on either -- so permitting only `<|end_message|>` masked a valid natural stop. Applies clean with the Dockerfile's own `patch --batch --forward -p1`. Validated on Inkling-NVFP4 / GB300, vLLM 0.28.0, MTP 8, on BOTH topologies with the full patch set overlaid on the shipped image. p0: 13/15 with 2 blocking on each, and the two blockers are the same unpatchable vLLM limitation (reasoning_budget_zero, NVBug 6678449a): 2-replica AGG structural_tag_grammar pass · reasoning ratio 1.00 · kv_routing pass 1P1D disagg structural_tag_grammar pass · reasoning ratio 1.00 p0 was extended to cover these changes, because it structurally could not before: the tag can be wrong while every wire-level probe passes (the parser absorbs the malformed output -- control_marker_residue passed before BOTH tag defects), and reasoning_tokens can be non-zero but the wrong order (the pre-fix code reported a constant 7 against 324 real tokens). Signed-off-by: Sungsoo Ha <sungsooh@nvidia.com>
sungsooha
added a commit
to sungsooha/dynamo
that referenced
this pull request
Aug 28, 2026
Bumps the vLLM runtime base to v0.28.0-ubuntu2404 and layers the Inkling structural-tag patch onto it, following the Nemotron-3 Ultra pattern (ai-dynamo#10234). Why 0.28.0: Inkling's tool parser and the structural-tag registry the patch extends only exist from that release. The tag is a multi-arch manifest list (linux/arm64 + linux/amd64), so GB300 aarch64 is covered. xpu and cpu stay at 0.27.1 -- untested at 0.28.0, and Inkling does not use them. Why the patch: vLLM ships an Inkling tool parser but registers no structural tag for it, so Dynamo finds no grammar to install for a tool request and silently degrades to tool_choice="auto" -- HTTP 200, no warning, and a forced tool choice is simply not honoured. Upstream as vllm-project/vllm#54120; the .patch here is cut against the exact v0.28.0 tree and applies with `patch --forward -p1` clean. validate_inkling_runtime.py asserts the POSTCONDITIONS rather than trusting patch's exit code: 'inkling' registered in both the builtin set and the builder registry, structural_tag_model == "inkling", and supports_required_and_named equal to the value AbstractToolParser.__init_subclass__ derives from VLLM_ENFORCE_STRICT_TOOL_CALLING (setting it by hand is redundant and silently overwritten). Verified against the real patched runtime, and verified to FAIL when the tag is unregistered or the vLLM version differs. The build-time version test is load-bearing: `patch --forward` would otherwise skip or fuzz a hunk on a different base and leave a half-patched image that still builds successfully. NOTE: the image alone is not sufficient. The worker must also run with --dyn-enable-structural-tag AND --structured-outputs-config '{"enable_in_reasoning": true}'; without the second, vLLM never fills the grammar bitmask for a reasoning model and the grammar is inert. That belongs in the recipe, not here. Signed-off-by: Sungsoo Ha <sungsooh@nvidia.com>
sungsooha
added a commit
to sungsooha/dynamo
that referenced
this pull request
Aug 28, 2026
… marker Refreshes the vendored patch from vllm-project/vllm#54120, which now begins the tool/text tags at `<|content_invoke_tool_json|>` / `<|content_text|>` instead of at `<|message_model|>`. Inkling's chat template prefills the role marker into the generation prompt (it ends `...<|end_message|><|message_model|>`), so generation resumes inside the model message and never re-emits it. The tags are `tags_with_separator` with `at_least_one=true`, so they anchor rather than trigger -- the old grammar therefore required a SECOND role marker. Compiling the tag with xgrammar shows the inversion, for forced/required/auto alike: suffix-only with duplicate marker before rejected accepted after accepted rejected It appeared to work only because the parser absorbed the duplicate, while manufacturing exactly the unconsumed-control-marker residue ai-dynamo#12510 strips. Validated on Inkling-NVFP4 / GB300, vLLM 0.28.0, MTP 8, 2-replica AGG, applied over the shipped image via ConfigMap: forced tool choice honoured 5/5 against a prompt matching the other tool; required = 1 call; parallel_tool_calls true/false = 2/1; no control-marker residue in content -- all in both streaming and non-streaming. Full p0 blocking count unchanged at 2 (the unrelated reasoning_budget_zero vLLM limitation). Note the probe suite could NOT have caught this: control_marker_residue passed before the fix too, because the parser was absorbing the duplicate. The grammar-level acceptance test is what exposed it. Reported by dynamo-review-agent on ai-dynamo#13954. Signed-off-by: Sungsoo Ha <sungsooh@nvidia.com>
sungsooha
added a commit
to sungsooha/dynamo
that referenced
this pull request
Aug 28, 2026
…ing patch Syncs both of our unmerged fixes to their reviewed state and re-validates the container on real hardware. frontend (ai-dynamo#13947, P1 review): reasoning tokens are now counted by the pinned vLLM parser's own `count_reasoning_tokens` over a per-choice ledger of the ORIGINAL generated ids, appended before any branch in process_output. The per-chunk classifier and the terminal `tokenizer.encode(saved_reasoning)` retokenisation are both removed -- the latter drifted because detokenisation is not injective (generated id 77150 decodes to '10' and re-encodes to two tokens). vLLM patch (vllm-project/vllm#54120): regenerated against the v0.28.0 tree, now also accepting `<|content_model_end_sampling|>`. The Inkling parser treats it as a co-equal terminator -- `is_reasoning_end` returns True on it and `count_reasoning_tokens` closes a block on either -- so permitting only `<|end_message|>` masked a valid natural stop. Applies clean with the Dockerfile's own `patch --batch --forward -p1`. Validated on Inkling-NVFP4 / GB300, vLLM 0.28.0, MTP 8, on BOTH topologies with the full patch set overlaid on the shipped image. p0: 13/15 with 2 blocking on each, and the two blockers are the same unpatchable vLLM limitation (reasoning_budget_zero, NVBug 6678449a): 2-replica AGG structural_tag_grammar pass · reasoning ratio 1.00 · kv_routing pass 1P1D disagg structural_tag_grammar pass · reasoning ratio 1.00 p0 was extended to cover these changes, because it structurally could not before: the tag can be wrong while every wire-level probe passes (the parser absorbs the malformed output -- control_marker_residue passed before BOTH tag defects), and reasoning_tokens can be non-zero but the wrong order (the pre-fix code reported a constant 7 against 324 real tokens). Signed-off-by: Sungsoo Ha <sungsooh@nvidia.com>
…enforced
Inkling's tool parser sets `structural_tag_model = None` and
`supports_required_and_named = False`, so `tool_choice="required"` and a named
tool choice install no constraint and silently degrade to `auto`. Measured on
Inkling-NVFP4: forcing `get_weather` on a prompt matching a second tool returned
`calculate` 5/5.
A bare JSON-schema constraint is the wrong fix -- Inkling wraps its tool JSON in
content markers (`<|message_model|><|content_invoke_tool_json|>{...}<|end_message|>`),
so constraining raw JSON conflicts with the wire format. Register an `inkling`
structural tag constraining markers and payload together, modelled on the
existing vLLM-owned `hermes` / `kimi_k3` builders:
auto -- tool calls OR a text block (the model may answer instead)
required -- at least one call from the offered set, no text alternative
forced -- exactly the named tool, wrapped in the same container format as
required; a bare TagFormat at the root is a *triggered* constraint
and never anchors
With the tag registered, forced `get_weather` is honoured 5/5 and
`tool_choice=required` returns a single call instead of 1-7 mixed calls, with
speculative decoding (MTP8) enabled and no FSM failures.
NOTE for deployers: a reasoning model additionally needs
`--structured-outputs-config '{"enable_in_reasoning": true}'`. Otherwise
`StructuredOutputManager.should_fill_bitmask` never fills the grammar bitmask
(gated on `reasoning_ended`; `enable_in_reasoning` defaults to False), so the tag
compiles and is inert -- HTTP 200, no error, unconstrained output. Inkling's
`is_reasoning_end` scans backwards and returns False on `<|message_model|>`, so a
tool block never flips it.
Signed-off-by: Sungsoo Ha <sungsooh@nvidia.com>
…ural_tag_model `AbstractToolParser.__init_subclass__` forces `supports_required_and_named = False` whenever `structural_tag_model` is set and `VLLM_ENFORCE_STRICT_TOOL_CALLING` is on (abstract_tool_parser.py:65-71). That is the intended design: the structural tag supersedes the generic required/named JSON path. Setting the flag to True in the subclass was therefore both redundant and silently overwritten. Drop it and document why, so the next reader does not re-add it. No behaviour change -- the measured results (forced get_weather honoured 5/5 with MTP8) were obtained with the effective value already False. Signed-off-by: Sungsoo Ha <sungsooh@nvidia.com>
…arker
The tag constrains the GENERATED SUFFIX, but it began at `<|message_model|>`.
Inkling's chat template prefills that role marker into the generation prompt --
the prompt ends `...<|end_message|><|message_model|>` -- so generation resumes
inside the model message and never re-emits it.
Because the tool/text tags are `tags_with_separator` with `at_least_one=true`,
they anchor rather than merely trigger, so the grammar required a SECOND role
marker. Compiling the tag with xgrammar and feeding it token sequences shows the
inversion directly, for forced/required/auto alike:
suffix-only with duplicate marker
before rejected accepted
after accepted rejected
It appeared to work only because the parser absorbed the duplicate -- while
manufacturing exactly the unconsumed-control-marker residue that dynamo#12510
exists to strip.
Begin the tags at `<|content_invoke_tool_json|>` / `<|content_text|>` instead;
`_INKLING_MESSAGE_MODEL` is then unused and removed.
Validated on Inkling-NVFP4 / GB300 (vLLM 0.28.0, MTP 8), 2-replica AGG: forced
tool choice honoured 5/5 against a prompt matching the other tool, required = 1
call, parallel_tool_calls true/false = 2/1, and no control-marker residue in
content -- in both streaming and non-streaming. p0 blocking count unchanged at 2
(the unrelated reasoning_budget_zero limitation).
Reported by dynamo-review-agent on ai-dynamo/dynamo#13954.
Signed-off-by: Sungsoo Ha <sungsooh@nvidia.com>
… tag
The tag permitted only `<|end_message|>`, but the Inkling parser treats
`<|content_model_end_sampling|>` as a co-equal terminator:
* the parser docstring states "sampling may also end a block with the
standalone <|content_model_end_sampling|> token";
* `is_reasoning_end()` returns True on it (`token_id in (text_id,
end_sampling_id)`);
* `count_reasoning_tokens()` closes a reasoning block on either
(`end_ids = {END_MESSAGE, CONTENT_MODEL_END_SAMPLING}`).
Constraining to one terminator therefore masks a valid natural stop and forces
the model to keep generating to reach the permitted token -- and it removes one
of the two paths that flip `is_reasoning_end`, which the reasoning plumbing
keys on.
`TagFormat.end` accepts `list[str]`, so both are permitted directly rather than
via an OrFormat; `AnyTextFormat.excludes` now lists both so the text block
cannot swallow either terminator.
Verified by compiling the tag with xgrammar, forced/required/auto:
<|end_message|> <|content_model_end_sampling|>
before accepted REJECTED
after accepted accepted
The duplicate-role-marker fix from the previous commit is unaffected (a leading
`<|message_model|>` stays rejected).
Reported by dynamo-review-agent on ai-dynamo/dynamo#13954.
Signed-off-by: Sungsoo Ha <sungsooh@nvidia.com>
sungsooha
force-pushed
the
inkling/structural-tag
branch
from
August 28, 2026 16:03
cebd319 to
b8ae1fc
Compare
Contributor
|
This pull request has merge conflicts that must be resolved before it can be |
…tag-v2 # Conflicts: # vllm/tool_parsers/structural_tag_registry.py Signed-off-by: Sungsoo Ha <sungsooh@nvidia.com>
Contributor
|
This pull request has merge conflicts that must be resolved before it can be |
Contributor
|
This pull request has merge conflicts that must be resolved before it can be |
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Inkling's tool parser sets
structural_tag_model = Noneandsupports_required_and_named = False, sotool_choice="required"and a named tool choice install no constraint and silently degrade toauto.Measured on Inkling-NVFP4 (GB300, TP4): forcing
get_weatheron a prompt that matches a second tool returnedcalculate5/5.This is the Inkling instance of the family tracked in #50477 (which names
inkling_tool_parser.pyexplicitly), alongside #49712 and #53363.Why not a JSON-schema constraint
Inkling wraps its tool JSON in content markers:
Constraining raw JSON conflicts with that wire format. A structural tag constrains markers and payload together, which is exactly what
hermesandkimi_k3already do.Change
Register an
inklingbuilder modelled on the existing vLLM-owned ones:autorequiredforcedrequiredforceddeliberately does not use a bareTagFormatat the root: that is a triggered constraint, so it never anchors and the model stays free to call a different tool.kimi_k3wraps forced the same way for the same reason.Validation
With the tag registered, on Inkling-NVFP4 / GB300 TP4, speculative decoding (MTP8) enabled:
get_weatherhonoured 5/5 (was 0/5)tool_choice=requiredreturns a single call (was 1–7 mixed calls)Failed to advance FSMerrors⚠ Deployer note — reasoning models need
enable_in_reasoningA reasoning model additionally needs
--structured-outputs-config '{"enable_in_reasoning": true}'. OtherwiseStructuredOutputManager.should_fill_bitmasknever fills the grammar bitmask (it is gated onreasoning_ended, andenable_in_reasoningdefaults toFalse), so the tag compiles and is completely inert — HTTP 200, no error, unconstrained output.Inkling's
is_reasoning_endscans token ids backwards and returnsFalseon<|message_model|>, so a tool block never flips it. The same mechanism was described in #37388 (self-closed with "I realized that I can useenable_in_reasoning: True") and fixed model-side for Muse Glimmer in #52390.A follow-up making Inkling's
is_reasoning_endrecognise a tool-block opening — as #50528 and #49876 did for adjacent Inkling cases — would remove the need for the flag. Happy to take that on in a separate PR if maintainers prefer it here.Notes
Draft — opened for early feedback. ⚠ #53099 refactors
structural_tag_registry.py; happy to rebase onto it.