fix(frontend): derive reasoning default from SGLang instead of a local table - #12350
fix(frontend): derive reasoning default from SGLang instead of a local table#12350GavinZhu-GMI wants to merge 3 commits into
Conversation
…l table
`resolve_request_force_reasoning` decided whether to enable the reasoning
parser from two hardcoded sets, `_THINKING_BY_DEFAULT` and `_THINKING_OPT_IN`.
SGLang already publishes this: every reasoning detector declares a
`reasoning_default`, and `serving_chat._get_reasoning_from_request` reads it.
Keeping a copy here means each new SGLang model is wrong until someone
remembers to update the sets, and the failure is silent -- an unlisted parser
falls through to `template_default`, which is False for any model shipping no
Jinja chat template.
Kimi-K3 is exactly that case. Its detector declares
`reasoning_default='thinking'` (on unless `chat_template_kwargs.thinking` is
False) with markers `<|open|>think<|sep|>` / `<|close|>think<|sep|>`, but it
appears in neither set and has no chat template. Result: the reasoning parser
never ran, `reasoning_content` came back null, and the raw
`<|close|>think<|sep|>` marker leaked into `content`.
Ask SGLang for the mode and apply the same dispatch it does (always / mistral /
thinking / enable_thinking / explicit_*). The static tables stay as a fallback
for parsers this SGLang build does not expose, so behaviour is unchanged for
everything already listed; `minimax-m3` and `mistral` keep their explicit
handling ahead of the lookup. Lookup is lru_cached -- detector construction is
not free and this runs per request.
Validation, against sglang 0.0.0.dev0 (v0.5.16) on this branch:
parser chat_template_kwargs before after
kimi_k3 {} False True
kimi_k3 {thinking: False} False False
kimi_k2 {} / {thinking: False} True / False unchanged
qwen3 {} / {enable_thinking:F} True / False unchanged
deepseek-v3 {} / {thinking: True} False / True unchanged
gemma4 {} / {enable_thinking:T} False / True unchanged
minimax-m3 {} / {thinking_mode:dis} True / False unchanged
unknown any template_default unchanged
Also verified end to end on a live Kimi-K3 deployment: `reasoning_content`
populated, no marker leak in `content`.
Adds components/src/dynamo/frontend/tests/test_sglang_reasoning_default.py
covering the regression, the unchanged families, and the unknown-parser
fallback.
Signed-off-by: Gavin.Zhu <gavin.z@gmicloud.ai>
GitHub Actions
Inactive
GitHub Actions
Inactive
|
👋 Hi GavinZhu-GMI! Thank you for contributing to ai-dynamo/dynamo. Just a reminder: The 🚀 |
github-actions
Bot
added
external-contribution
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| if mode is None: | ||
| return None | ||
| if mode == "always": | ||
| return True | ||
| if mode == "mistral": | ||
| reasoning_effort = request.get("reasoning_effort") | ||
| if reasoning_effort is None: | ||
| reasoning_effort = kwargs.get("reasoning_effort") | ||
| return reasoning_effort is not None and reasoning_effort != "none" | ||
| if mode in ("thinking", "enable_thinking"): | ||
| # on by default; the matching kwarg set to False opts out | ||
| return kwargs.get(mode) is not False | ||
| if mode in ("explicit_thinking", "explicit_enable_thinking"): | ||
| toggle = mode.replace("explicit_", "") | ||
| return kwargs.get(toggle) is True | ||
| return None |
There was a problem hiding this comment.
🔍 Correctness hinges on SGLang's reasoning_default string values matching this dispatch
_force_reasoning_from_sglang_default (sglang_prepost.py:150-173) assumes SGLang's reasoning_default takes exactly the values always/mistral/thinking/enable_thinking/explicit_thinking/explicit_enable_thinking. Any other value returns None and silently falls back to the static tables, and getattr(detector, 'reasoning_default', None) returns None if the attribute is renamed. Because the fallback is silent, if a future SGLang version changes these string constants or the attribute name, the derived-default behavior degrades to the stale static tables without warning — reintroducing exactly the silent-miss failure mode this PR sets out to fix (just for future models). The bundled tests skip when the parser is absent, so a mismatch would not necessarily fail CI. Worth a follow-up assertion or a warning log when a known parser yields an unrecognized mode.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Addressed in c23aead.
The dispatch's implemented modes now live in one place (_SGLANG_REASONING_MODES), and a parser SGLang knows but whose reasoning_default we do not implement logs a warning naming the parser and the unhandled mode before falling back. lru_cache holds that to once per parser rather than once per request.
On the point that a mismatch would not necessarily fail CI — agreed, and that is now covered directly: test_every_registered_detector_mode_is_implemented walks ReasoningParser.DetectorMap and asserts every declared reasoning_default is implemented here, so a new upstream mode is a test failure rather than a silent degradation. It is not skip-guarded.
Current state of this sglang build (0.0.0.dev0 / v0.5.16), which is what the dispatch is checked against — 25 detectors, 6 distinct modes, all implemented:
reasoning_default |
parsers |
|---|---|
always |
apertus2509, cohere_command4, deepseek-r1, gpt-oss, hunyuan, inkling, kimi, minimax-append-think, minimax-m3, step3, step3p5 |
enable_thinking |
glm45, interns1, minimax, nemotron_3, qwen3, qwen3-thinking |
explicit_enable_thinking |
gemma4, mimo, poolside_v1 |
explicit_thinking |
deepseek-v3, deepseek-v4 |
mistral |
mistral |
thinking |
kimi_k2, kimi_k3 |
On the attribute-rename half: reasoning_default is now read as a direct attribute rather than getattr(..., None), so a rename surfaces as AttributeError instead of a silent fallback.
WalkthroughChangesReasoning default resolution
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@components/src/dynamo/frontend/sglang_prepost.py`:
- Around line 140-147: In the detector lookup around ReasoningParser, catch only
ValueError for unsupported model types and return the static fallback in that
case; let all other SGLang failures propagate. Replace the getattr-based access
with direct detector.reasoning_default access so incompatible detector contracts
fail fast.
In `@components/src/dynamo/frontend/tests/test_sglang_reasoning_default.py`:
- Around line 70-109: The tests only cover resolve_request_force_reasoning and
omit direct coverage of _force_reasoning_from_sglang_default dispatch modes and
Mistral reasoning_effort precedence. Add a focused parametrized unit test for
_force_reasoning_from_sglang_default covering always and explicit_* branches,
including Mistral cases that verify top-level reasoning_effort takes precedence
over nested values.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 6c61a1b5-5a04-415a-8d19-d90bd35efd2b
📒 Files selected for processing (2)
components/src/dynamo/frontend/sglang_prepost.pycomponents/src/dynamo/frontend/tests/test_sglang_reasoning_default.py
![dynamo-review-agent[bot]](https://avatars.githubusercontent.com/u/201626793?s=60&v=4){kind=small}
Review follow-up. The first pass fell back to the static tables on any
deviation, which made the new code repeat the failure mode it was written to
remove: a silent miss.
Three changes:
- Catch only `ValueError` around `ReasoningParser(model_type=...)`. That is what
SGLang raises for a name outside its DetectorMap ("Unsupported model type"),
and it is the one case that legitimately means "unknown parser, use the
tables". `except Exception` also swallowed genuine SGLang failures.
- Read `detector.reasoning_default` directly instead of
`getattr(..., None)`. Every detector takes it from
`BaseReasoningFormatDetector`, so its absence means the detector contract
changed; an AttributeError is the correct outcome, not a quiet fallback.
- Warn when SGLang knows a parser but declares a `reasoning_default` this
dispatch does not implement, and keep the implemented set in one place
(`_SGLANG_REASONING_MODES`). Previously a new upstream mode would have routed
every affected model back to the stale tables with no signal. lru_cache holds
the warning to once per parser.
Adds a test asserting every reasoning_default across SGLang's registered
detectors is implemented here, so a new upstream mode fails CI instead of
degrading quietly; plus coverage for the warn-and-fall-back and
non-ValueError-propagates paths. 17 tests pass against sglang 0.0.0.dev0
(v0.5.16), which registers 25 detectors across 6 distinct modes.
Signed-off-by: Gavin.Zhu <gavin.z@gmicloud.ai>
GitHub Actions
Inactive
Review follow-up, and a real regression in the previous two commits.
The hunyuan detector declares `reasoning_default="always"`, but SGLang does not
honour that: `_get_reasoning_from_request` special-cases hunyuan ahead of its
own reasoning_default dispatch, because the Hy3-preview template emits no
<think> unless `reasoning_effort` asks for it --
if self.reasoning_parser == "hunyuan":
return request.reasoning_effort not in (None, "none", "no_think")
Deriving the default without that gate turned reasoning on unconditionally for
hunyuan, which routes the entire response into `reasoning_content`. Mirror the
gate alongside the existing minimax-m3 and mistral special cases, i.e. before
the SGLang-default lookup.
`reasoning_effort` resolution moves into `_request_reasoning_effort` so the
mistral and hunyuan gates read it identically; it keeps the pre-existing
top-level-then-chat_template_kwargs precedence, which is a frontend extension
of SGLang's top-level-only read.
Tests: hunyuan across unset / "none" / "no_think" / "low" / "high", plus an
assertion that its reasoning_default really is "always" so the branch is not
mistaken for dead code. Also adds direct parametric coverage of every
_force_reasoning_from_sglang_default branch, including mistral top-level vs
nested precedence, and a check that every mode in _SGLANG_REASONING_MODES has a
real branch. 43 tests pass against sglang 0.0.0.dev0 (v0.5.16).
Signed-off-by: Gavin.Zhu <gavin.z@gmicloud.ai>
GitHub Actions
Inactive
| reasoning_effort = _request_reasoning_effort(request, kwargs) | ||
| return reasoning_effort not in (None, "none", "no_think") | ||
|
|
||
| resolved = _force_reasoning_from_sglang_default( |
There was a problem hiding this comment.
With the repo-pinned sglang[diffusion]==0.5.15, ReasoningParser.DetectorMap does not contain kimi_k3, so this lookup returns None and the reported no-template Kimi-K3 case still falls through to template_default=False. Fix: keep kimi_k3 in the static fallback as a thinking-on-by-default parser until the SGLang dependency is bumped to a version that exposes it.
🤖 AI Fix
In components/src/dynamo/frontend/sglang_prepost.py, add "kimi_k3" to _THINKING_BY_DEFAULT and change the fallback flag_key selection in resolve_request_force_reasoning so both "kimi_k2" and "kimi_k3" use "thinking"; in components/src/dynamo/frontend/tests/test_sglang_reasoning_default.py, add a non-skipped absent-parser fallback test asserting _resolve("kimi_k3", {}, template_default=False) is True and _resolve("kimi_k3", {"thinking": False}, template_default=False) is False.
|
Hi @GavinZhu-GMI |
|
Hi @indrajit96 I will close this PR and however this is just a part of a series of patches when get dynamo to serve K3 using sglang, I will readapt everything on our patch, let's go! |
3 participants
Overview:
resolve_request_force_reasoningdecides whether to enable the reasoning parser from two hardcoded sets insglang_prepost.py,_THINKING_BY_DEFAULTand_THINKING_OPT_IN. SGLang already publishes this information — every reasoning detector declares areasoning_default, andserving_chat._get_reasoning_from_requestreads it.Maintaining a copy means each new SGLang model is wrong here until someone remembers to update the sets, and the failure is silent: a parser in neither set falls through to
template_default, which isFalsefor any model shipping no Jinja chat template. There is no warning and no error — reasoning simply never runs.Summary
Ask SGLang for the mode instead of duplicating its table, and apply the same dispatch it does (
always/mistral/thinking/enable_thinking/explicit_*).Kimi-K3 is exactly the failing case: its detector declares
reasoning_default='thinking'with markers<|open|>think<|sep|>/<|close|>think<|sep|>, but it is in neither set and ships no chat template. The parser never ran,reasoning_contentcame backnull, and the raw<|close|>think<|sep|>marker leaked intocontent.This is deliberately not a "add kimi_k3 to the list" fix — that would leave the next model equally broken.
Details:
_sglang_reasoning_default(parser_name)reads the detector'sreasoning_default.lru_cached, because detector construction is not free and this runs per request._force_reasoning_from_sglang_default(...)mirrors the mode dispatch inserving_chat._get_reasoning_from_request.minimax-m3andmistralspecial cases, so those keep their explicit handling, and before the static tables, which remain as the fallback for parsers a given SGLang build does not expose. Behaviour is therefore unchanged for everything already listed, and for unknown parsers (stilltemplate_default).Validation
Table-tested against sglang
0.0.0.dev0(v0.5.16), before = currentmain:chat_template_kwargskimi_k3{}kimi_k3{thinking: False}kimi_k2{}/{thinking: False}qwen3{}/{enable_thinking: False}deepseek-v3{}/{thinking: True}gemma4{}/{enable_thinking: True}minimax-m3{}/{thinking_mode: "disabled"}template_defaultAlso verified end to end on a live Kimi-K3 deployment (Dynamo + SGLang, disaggregated, GB300 NVL72):
reasoning_contentpopulated, no marker leak incontent, tool calling unaffected.Adds
components/src/dynamo/frontend/tests/test_sglang_reasoning_default.py— 14 cases covering the regression, the unchanged families, and the unknown-parser fallback. The SGLang-derived assertions skip when the build does not expose the parser, so the file stays valid across SGLang versions.isort/black/flake8clean on both changed files (flake8 W503 count is identical tomain, all pre-existing and outside this diff).Where should the reviewer start?
components/src/dynamo/frontend/sglang_prepost.py— the ordering insideresolve_request_force_reasoningis the part worth scrutinising: SGLang's answer must take precedence over the static tables, but not over theminimax-m3/mistralbranches above it.Related Issues
🔗 This PR is linked to an issue:
Summary by CodeRabbit
New Features
Bug Fixes
Tests