refactor(renderer): let the worker declare K3's per-image token - #155
Merged
Merged
Conversation
Kimi-K3 renders one placeholder per image, and the two serving engines want different tokens there: vLLM rebuilds the model's native media sequence from `<|kimi_image_placeholder|>`, while SGLang only repeats `<|media_pad|>` up to the feature count. That difference was expressed as two trait accessors plus a boolean mode flag on the consumer side, with the consumer rewriting rendered segments after the fact. Rendering ran before the engine's preference was known, so the rewrite existed only to fix up that ordering. Take the token as a constructor argument instead and emit it directly. `KimiK3Formatter::new` gains an `image_placeholder_token` parameter, threaded to the single `control(...)` call that emits an image, and `kimi_k3_formatter_for` forwards it. `None` — and an empty string, which `push_segment` would drop and so silently emit no token at all — falls back to the checkpoint default, keeping every non-declaring caller byte-identical. `image_pad_token()` and `image_placeholder_template()` both leave the public trait. Neither had a caller inside this crate: the Jinja content-flattening path reads the private struct field directly, and the trait method existed only so the consumer's rewrite could read it. BREAKING CHANGE: `OAIPromptFormatter::image_pad_token` and `OAIPromptFormatter::image_placeholder_template` are removed, and `kimi_k3_formatter_for` takes a fourth argument.
|
📊 Conformance matrix rendered — view in CI summary |
The consumer-side check this pointed at has been dropped: it could not see the case that is actually known-bad (the default marker shattering on the vLLM path, where nothing is declared) and it fired on inert declarations from Jinja-rendered models. State the property directly instead — `allow_special` selects tiktoken's special-aware method, it does not guarantee a single token id — and stop promising a check elsewhere.
furionw
marked this pull request as ready for review
July 28, 2026 21:29
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![dynamo-review-agent[bot]](https://avatars.githubusercontent.com/u/201626793?s=60&v=4){kind=small}
rmccorm4
changed the title
Contributor
Removed the breaking change "!" major version bump from PR title as this didn't exist on main yet |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
rmccorm4
reviewed
Jul 29, 2026
rmccorm4
left a comment
rmccorm4
left a comment
Contributor
There was a problem hiding this comment.
Left a comment on the dynamo repo side of this PR, suggesting to keep it simple and always send the same thing on the frontend side here, and handle adapting on the backend side: https://github.com/ai-dynamo/dynamo/pull/12281/changes#r3671318711
Supersedes the worker-declared-token approach from this branch's first commit, per review on ai-dynamo/dynamo#12281. The frontend now emits one canonical token per image for every engine, rather than the token each engine happens to want. `<|media_pad|>` is the right canonical form because it is a registered special in the K3 tokenizer (`config.json`'s `media_placeholder_token_id`), so it encodes to exactly one id and stays one id regardless of surrounding text. The checkpoint's other spelling, `<|kimi_image_placeholder|>`, is not in the vocabulary: it BPE-shatters into several ids whose boundaries depend on neighbouring text, which makes it a poor thing for a worker to find and substitute. Engines that want that form convert from the pad worker-side, where a single known id is a reliable anchor. This also happens to be the checkpoint's own intended shape -- `encoding_k3.build_chat_segments(image_prompts=[...])` is the model author's hook for choosing per-image content, and `<|kimi_image_placeholder|>` is only its `None` fallback. So `KimiK3Formatter::new` loses the token argument and the token is a constant again, which reverts the parameter threading through `build_chat_segments`/`render_role_message`/`render_assistant_segments`/ `render_content_segments`. The pad is still emitted as a control segment: that is load-bearing now rather than incidental, since only the special-aware encode path yields its single id. `image_pad_token()` and `image_placeholder_template()` still leave the public trait -- with no rewrite step and no declaration, neither has a consumer. Neither had a caller inside this crate either: the Jinja content-flattening path reads the private struct field directly. BREAKING CHANGE: `OAIPromptFormatter::image_pad_token` and `OAIPromptFormatter::image_placeholder_template` are removed.
furionw
added a commit
to ai-dynamo/dynamo
that referenced
this pull request
Jul 29, 2026
Adopts the review proposal on #12281 and drops this branch's earlier `image_placeholder_token` runtime-config field. The frontend now emits one canonical `<|media_pad|>` per image for every engine (ai-dynamo/frontend-crates#155). SGLang consumes that directly. vLLM's K3 processor instead matches the checkpoint's `<|kimi_image_placeholder|>` and expands it into the media sequence, so the conversion happens in the vLLM adapter's `build_tokens_prompt()`, gated on raw media being present. Converting in this direction is what makes it dependable: `<|media_pad|>` is a single vocabulary id (`config.json`'s `media_placeholder_token_id`), so locating it is exact. `<|kimi_image_placeholder|>` is a plain string that is NOT in the vocabulary -- it BPE-shatters into several ids whose boundaries move with surrounding text -- so a frontend emitting it would leave the worker nothing reliable to anchor on. vLLM's own processor concedes this, falling back to decode/string-replace/re-encode when the token match fails. Doing it here rather than through discovery keeps it off the public Python bindings surface, where it would be sticky to remove, and leaves room for video/audio to need the same treatment without a field each. Both values come from checkpoint metadata, so no registration field and no SGLang processor introspection are needed -- the probe added earlier on this branch is gone entirely. Mismatched pad counts are rejected rather than guessed: silently misaligning images against embedding slots is worse than a hard error. An already-native prompt has no pads and passes through untouched, which is the rollout path -- deploy this adapter first, then switch the frontend to emit pads. BREAKING CHANGE: `ModelRuntimeConfig.expands_image_pad_token` is removed with no replacement field. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: furionw <qiwa@nvidia.com>
rmccorm4
reviewed
Jul 29, 2026
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
3 participants
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.
Based on #145.
Why
K3 needs a stable frontend image contract across serving engines. The renderer now emits one registered
<|media_pad|>special token per image; backends needing another representation adapt it worker-side. This avoids engine-specific formatter configuration and matching a placeholder that can split into context-dependent BPE tokens.What Change
OAIPromptFormatter.Test Plan
cargo test -p dynamo-renderercargo clippy --workspace --all-targets --all-features --locked -- -D warnings