refactor(preprocessor)!: let workers declare their per-image token - #12281
Closed
furionw wants to merge 7 commits into
Closed
refactor(preprocessor)!: let workers declare their per-image token#12281furionw wants to merge 7 commits into
furionw wants to merge 7 commits into
Conversation
Kimi-K3's prompt carries one placeholder per image, and the serving engines disagree on which token belongs there: vLLM builds the native media sequence from `<|kimi_image_placeholder|>`, while SGLang only repeats `<|media_pad|>` up to the feature count. `expands_image_pad_token` encoded that as a mode flag naming how SGLang's processor works internally, which the preprocessor then used to pick between two renderer-supplied strings and rewrite the rendered segments after the fact. The flag was only ever a proxy for the string, and the rewrite only existed because rendering ran before the engine's preference was consulted. Carry the string instead. `image_placeholder_token: Option<String>` is read from the processor the worker actually loaded, so it states that engine's real contract rather than an assumption about it, and the formatter renders it directly. `substitute_image_pad_token` and `swap_marker_segments` are gone; so is the notion of pad expansion. Containment is now structural rather than incidental. The token is threaded only into `kimi_k3_formatter_for`; no Jinja-templated family has a parameter to receive one, so SGLang declaring a token for every multimodal model it serves cannot reach Qwen-VL or anything else. The old rewrite ran for every model on every request and was a no-op only because Jinja prompts happen to carry no segments. Workers that declare nothing are unchanged: vLLM keeps rendering the checkpoint default, and `skip_serializing_if` keeps them byte-identical on the wire. Also adds `image_placeholder_token` to the `_core.pyi` stub, which the removed flag never had despite mypy covering the SGLang component. BREAKING CHANGE: `ModelRuntimeConfig.expands_image_pad_token` is replaced by `ModelRuntimeConfig.image_placeholder_token`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: furionw <qiwa@nvidia.com>
Review of the parent commit turned up that `allow_special: true` on a rendered segment only selects tiktoken's special-aware encode method — it is not a membership check. A token the checkpoint never registered falls through to ordinary BPE and silently shatters, so one image becomes several ids and the worker's one-token-per-image expectation breaks with no error on any path. Check the declaration once at preprocessor construction and warn. This lives in `new_with_parts` rather than `new` because the frontend's discovery path builds the formatter and preprocessor separately and never calls `new`. It is diagnostic only: a worker reading its own processor is a better authority than this heuristic, and silently overriding it would be harder to debug than a loud log. Also from review: - Blank declarations now fall back on `trim()`, not just `is_empty()`. Whitespace survived `push_segment`, so it kept cardinality while encoding to a meaningless id. - Split the SGLang accessor's three None branches by log level. The EPD encode worker registers with `engine=None` and can never declare, which is expected; a renamed SGLang attribute is not. Returning None for both made version drift indistinguishable from a text-only worker. The case that breaks multimodal serving is now an error, not a warning. - `_get_image_placeholder_token` and `_get_runtime_config` take `Optional[sgl.Engine]`; both are called with None. - Dropped the claim that `use_raw_prompt` bypasses this. Chat's `raw_prompt()` returns None, so it falls through to the formatter and the token is emitted; it holds only for completions, which carry no images. - Trimmed the field doc, and recorded that only the first-registered worker's declaration takes effect, since `mdcsum` does not cover `runtime_config`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: furionw <qiwa@nvidia.com>
The check read `runtime_config.image_placeholder_token` with no gate on whether the formatter consumed it, so it fired on every Qwen3-VL-on-SGLang startup: SGLang declares `<|vision_start|><|image_pad|><|vision_end|>`, that is three token ids, and the warning claimed the per-image count would not match — for a declaration the Jinja path never receives. Inert by design; that is the containment property this series relies on. It was also blind to the case that is actually known-bad: the default `<|kimi_image_placeholder|>` shattering on the vLLM path. vLLM declares nothing, and the default lives inside the formatter, so a check over the declared field cannot see it. Silent where it mattered, noisy where it did not. Checking atomicity needs to key off the token the formatter actually rendered, which needs the read-back accessor deferred with the mm-routing work. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: furionw <qiwa@nvidia.com>
The probe runs during registration for every SGLang model, so anything it says has to be true for all of them. It wasn't: five audio-only processors (qwen_audio, voxtral, glmasr, midashenglm, qwen3_asr) populate audio_token and leave image_token unset, and the previous version logged an error for each of them at startup. Having no image token is not a fault; it means the worker does not serve images. The mistake was letting the worker judge whether the absence mattered. It cannot: it has no idea whether the frontend will pick a native formatter for this model. Only the frontend knows, and it already logs which token it resolved and whether a worker declared one. So the probe now just reports what it finds and returns None otherwise. On the blast radius, since this is unconditional: the whole path is two to four reads of plain instance attributes -- `mm_processor` is assigned directly in `init_tokenizer_and_processor`, not a property -- wrapped in `except Exception`. There is no I/O, no side effect, and nothing that can fail registration for a model that doesn't need it. Gating the call on the model being K3 would buy nothing and would put the model knowledge this series removed back into the worker. Collapses the six single-shape tests into one parametrised case covering every "no image token" shape, asserting nothing at WARNING or above. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: furionw <qiwa@nvidia.com>
The probe ran during registration for every SGLang model, to serve one. Kimi-K3 is the only family Dynamo renders with a native Rust formatter that has to choose a per-image token; everything else carries its per-image token in its own chat template, so the frontend never reads a declaration for it. Gate on `model_type` (`config.json` says `kimi_k3`, the same string `is_kimi_k3` matches on in the renderer, so worker scope and formatter selection cannot drift). The scope is also what makes reading `mm_tokens.image_token` sound in the first place. That field is "the string this processor scans for", and its shape differs per model: one token for Kimi (`<|media_pad|>`), a three-token wrapper for Qwen-VL (`<|vision_start|><|image_pad|><|vision_end|>`), a text pattern for MiniCPM, indirected attributes for InternVL/Phi4MM/Mllama4. Only for Kimi does it coincide with "the one token to emit per image". Reading it for everyone meant interpreting a field we understand for exactly one family and publishing the result into every multimodal worker's card, where nothing consumes it. The spurious error on audio-only processors was the first symptom of that, not an unlucky edge case. Scoping also drops the coupling this had to SGLang's `mm_tokens` layout for models that never needed it, which `components/src/dynamo/sglang/CLAUDE.md` warns against: SGLang is pre-1.0 and moves internal APIs between releases. With the probe scoped, a miss becomes actionable again -- the engine is known to be one that will not consume the formatter's default -- so an unreadable K3 processor warns rather than staying quiet. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: furionw <qiwa@nvidia.com>
furionw
marked this pull request as ready for review
July 28, 2026 21:29
rmccorm4
reviewed
Jul 29, 2026
furionw
added a commit
to ai-dynamo/frontend-crates
that referenced
this pull request
Jul 29, 2026
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.
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>
Prompts on this path reach 100k+ tokens while pads number in the single digits, so the per-token Python loop was the wrong shape: it paid O(n) interpreter overhead to relocate a handful of ids. Locate the pads with `list.index` and splice with slices instead. Both are C-level, and `index` short-circuits at the first hit, so an already-native prompt (the rollout path, and every non-K3 request that gets this far) costs one scan and zero copies rather than a full traversal plus a `count()` pre-pass. Measured on a 131072-token prompt with one image: 2.0ms -> 0.9ms. The no-pad case is unchanged at ~0.44ms, since both spellings do a single C-level scan. Differentially checked against the previous implementation over 309 cases including empty, leading, trailing and adjacent pads. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: furionw <qiwa@nvidia.com>
|
🎯 Code Coverage (details) 🔗 Commit SHA: d369e47 | Docs | Datadog PR Page | Give us feedback! |
Contributor
Author
|
Superseded by the smaller draft stack:\n\n- #12396 — native Kimi-K3 frontend (base)\n- #12394 — vLLM media-pad expansion (stacks on #12396)\n- #12393 — SGLang ServerArgs override compatibility (stacks on #12396)\n- #12395 — SGLang Kimi-K3 multimodal alignment (stacks on #12393)\n\nThis preserves the agreed lineage while allowing the vLLM and SGLang work to review and land independently. |
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.
Summary
Kimi-K3's prompt carries one placeholder per image, and the serving engines disagree on which token belongs there: vLLM builds the native media sequence from
<|kimi_image_placeholder|>, while SGLang only repeats<|media_pad|>up to the feature count.expands_image_pad_tokenencoded that as a mode flag naming how SGLang's processor works internally. The preprocessor used it to pick between two renderer-supplied strings and rewrite the rendered segments after the fact. The flag was only ever a proxy for the string, and the rewrite only existed because rendering ran before the engine's preference was consulted.This carries the string instead.
image_placeholder_token: Option<String>is read from the processor the worker actually loaded, so it states that engine's real contract rather than an assumption about it, and the formatter renders it directly.substitute_image_pad_tokenandswap_marker_segmentsare gone, and so is the notion of pad expansion.Why this is safe for non-K3 models
SGLang now declares a token for every multimodal model it serves, not just K3. Containment is structural: the token is threaded only into
kimi_k3_formatter_for, and no Jinja-templated family has a parameter to receive one. For Qwen3-VL,is_kimi_k3()returns false, execution falls through toPromptFormatter::from_parts, and the field is never read by any code path.That is stronger than what it replaces. The old rewrite ran for every model on every request and was inert only because both trait accessors had to be
Someand Jinja prompts carry no segments.Workers that declare nothing are unchanged: vLLM keeps rendering the checkpoint default, and
skip_serializing_ifkeeps them byte-identical on the wire.Follow-up commit
Review found that
allow_special: trueon a rendered segment only selects tiktoken's special-aware encode method — it is not a membership check. An unregistered token falls through to ordinary BPE and silently shatters, so one image becomes several ids. The preprocessor now checks the declaration once at construction and warns. It sits innew_with_partsbecause the discovery path builds the formatter and preprocessor separately and never callsnew.Breaking changes
ModelRuntimeConfig.expands_image_pad_tokenis replaced byModelRuntimeConfig.image_placeholder_token.ModelRuntimeConfighas nodeny_unknown_fields, so an old worker publishingexpands_image_pad_tokenat a new frontend deserializes toNoneand reverts to the default with no error.Known limitations (pre-existing, not introduced here)
mdcsumdoes not coverruntime_config, so a mixed vLLM+SGLang pool for one model is first-card-wins. Same shape as the flag it replaces; now recorded in the field doc.routing_image_token_idis still resolved fromconfig.jsonand is deliberately not wired to the declared token. Verified behaviour-preserving: token ids are byte-identical before and after for both engines, and no family in the lightseek registry was ever affected. Wiring it needs akimi_k3registry entry and a fix to themm_hashesgate first.Test plan
cargo test -p dynamo-llm --no-default-features --features mm-routing— preprocessor suite 15/15cargo clippy -p dynamo-llm --no-default-features --features mm-routing --all-targets -- -D warnings— cleancargo checkonlib/bindings/python— cleanpre-commit runon all changed files — cleandeclared_token_does_not_change_a_jinja_rendered_prompt, the regression guard for the containment claim abovemm_tokenslogged distinctly, engine without a tokenizer manager--no-default-features;block-manageris Linux-only. The 9 ZMQ/event-plane lib-test failures in that environment were confirmed pre-existing by reproducing the identical set on the unmodified base. Linux CI is the real gate.Merge order
Requires ai-dynamo/frontend-crates#155 to merge into
feat/Kimi-K3first. CI on this PR will be red until then — the branch callskimi_k3_formatter_forwith four arguments and the pinned renderer still takes three.After that lands,
cargo update -p dynamo-rendereris needed for both manifests — the rootCargo.tomlandlib/bindings/python/Cargo.toml, which carries its own[patch.crates-io]block because the bindings crate is outside the workspace.🤖 Generated with Claude Code