fix(sgl-router): mirror the engine's tool_choice default in the K3 render - #5
Open
yan-lgtm wants to merge 2 commits into
Open
Conversation
…nder
An absent `tool_choice` is not `auto`. The engine's protocol layer fills the
field in before the chat template ever runs (`ChatCompletionRequest`'s
`set_tool_choice_default`): `none` when the request declares no tools, `auto`
otherwise. The K3 router encoder read the field literally, so an omitted
`tool_choice` skipped the `tool_choice=none` preamble and rendered 38 tokens
short of the engine on nearly every plain chat request. Because the router
forwards its ids as `input_ids`, that shortened prompt is what the engine
actually generated from, losing "You MUST NOT call any tools in the next
message."
Measured on a live two-replica K3 endpoint, identical messages, engine-direct
vs through the router:
omits tool_choice engine=142 router=104 diverges
tool_choice="none" engine=142 router=142 match
tool_choice="auto" engine=104 router=104 match
It also split the prefix cache by modality: text-only turns are rendered by the
router (no preamble) while image-carrying turns fall through to the engine
(preamble present), so the same conversation hashed to two different prefixes
across an image boundary.
The fixture-driven parity tests cannot catch this. They build `RenderOpts` from
`FixtureOpts`, which mirrors the reference encoder, and the reference never sees
the protocol layer's defaults — so this is pinned with a request-level test
instead.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… only
The engine's `set_tool_choice_default` reads only the TOP-LEVEL `tools` key
(`values.get("tools") is None`); it never inspects message-level tools. The
previous mirror added a `system`/`developer` message scan, so a request with
no top-level `tools` but truthy message-level tools was resolved to `auto`
(no preamble) while the engine emitted the `tool_choice=none` preamble —
reproducing the 38-token divergence this PR fixes, on tool-carrying agent
traffic. Mirror the real predicate: top-level `tools` only.
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
An absent
tool_choiceis notauto. The engine's protocol layer fills the field in before the chat template ever runs —ChatCompletionRequest.set_tool_choice_defaultinpython/sglang/srt/entrypoints/openai/protocol.py:kimi_k3::resolve_render_optsread the field literally, so an omittedtool_choicemapped toToolChoice::Unsetand skipped the preamble thatencoding_k3emits fortool_choice=none:That is 38 tokens, dropped on essentially every plain chat request — clients rarely send
tool_choiceexplicitly. And because the router forwards its ids asinput_ids, the shortened prompt is what the engine actually generated from, so the model never saw "You MUST NOT call any tools in the next message."Evidence
Measured on a live two-replica Kimi-K3 endpoint, identical messages, engine-direct (
localhost:30000) vs through the router:tool_choicetool_choice: "none"tool_choice: "auto"Decoding
return_prompt_token_idsfrom both paths showed the tool-choice block as the entire difference. The router's 107 raw ids were byte-identical to the referenceapply_chat_templateoutput — the encoder is faithful toencoding_k3; what it did not mirror was the protocol layer's default.A second effect worth noting: this split the prefix cache by modality. Text-only turns are rendered by the router (no preamble) while image-carrying turns deliberately fall through to the engine (preamble present), so the same conversation hashed to two different prefixes across an image boundary — which undercuts cache-aware routing precisely where a long multimodal conversation should benefit most.
Fix
resolve_render_optsnow distinguishes an absent/nulltool_choicefrom a present one and resolves the absent case through a newdeclares_toolshelper mirroring the engine predicate. Two details of the Python are load-bearing and pinned by tests:toolsis tested byis None, not truthiness — an explicittools: []is notNonein Python, so it still yieldsauto. Testing emptiness would emit a block the engine never emits.toolscount only onsystem/developerroles, and there by truthiness (bool(msg.get("tools"))), sotools: []on a system message does not count.An explicit
tool_choicestill wins in both directions;required,none,auto, and a named-function object are unchanged.Why the existing tests missed it
The fixture-driven parity tests build
RenderOptsfromFixtureOpts::to_render_opts, which mirrors the reference encoder, and the reference never sees the protocol layer's defaults. No fixture can express "the field was absent and the server filled it in." Pinned with a request-level test instead:absent_tool_choice_mirrors_the_engine_protocol_default.Testing
cargo test --lib— 854 passed, 0 failed, 1 ignored. Includes all three reference-encoder parity tests (segment_parity_with_reference_encoder,render_parity_with_reference_encoder,rendered_text_parity_with_reference_encoder) and the new test.cargo clippy --lib --all-features -- -D warnings— clean.cargo fmt --check— clean.Not done: the router image was not rebuilt and re-measured against the live endpoint. The correctness chain is the live measurement above plus the unit test — an explicit
tool_choice: "none"already rendered router=142=engine, and the new code maps absent-plus-no-tools onto that sameToolChoice::None.🤖 Generated with Claude Code
CI States
Latest PR Test (Base):⚠️ Run #30671627874⚠️ Run #30671627725
Latest PR Test (Extra):