Repository navigation
fix(reasoning): honor Poolside template thinking defaults - #32540
JustinTong0323 merged 6 commits into
Conversation
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
… detection - Re-key the Poolside template signature on the shared tool preamble instead of the enable_thinking default: with default(true) newly detected, the real Laguna-S-2.1 template no longer matched _is_poolside_v1 and --tool-call-parser auto silently resolved to qwen, which cannot parse the XML-KV tool format. - Centralize the default-filter regexes in _has_toggle_default_assignment: strip Jinja comments before matching (a commented-out default(false) shadowed a live default(true)), accept the documented d alias, keep two-argument boolean-mode forms excluded, and cover the thinking toggle symmetrically. - Add tests shaped like the real Laguna-S-2.1 template (no XS prose) asserting both the reasoning and tool-call parsers, plus the filter variants.
|
Pushed 68e2e5a to fix a side effect of the new detection:
Verification: 38 tests / 50 subtests pass, pre-commit clean; a sweep of 205 real HF chat templates shows exactly one resolution change (Laguna-S-2.1 → |
JustinTong0323
left a comment
JustinTong0323
left a comment
There was a problem hiding this comment.
The new default-filter matcher still misses valid toggle forms.
| literal = "true|True" if default else "false|False" | ||
| pattern = ( | ||
| rf"set\s+{param}\s*=\s*{param}\s*\|\s*" | ||
| rf"(?:default|d)\s*\(\s*(?:{literal})\s*\)" |
There was a problem hiding this comment.
Two-argument default() forms are excluded too broadly: default(true, false) is equivalent to the one-argument toggle, and default(false, true) still preserves False -> False and True -> True; rejecting them leaves reasoning_config=None and can reintroduce reasoning-marker leakage. Parse the Jinja Filter node and cover all four boolean/default combinations, excluding only forms that actually collapse the toggle such as default(true, true).
There was a problem hiding this comment.
Done in 9600889 — switched to parsing the Filter node (with a parse-only {% generation %} extension so transformers templates build an AST). All four combinations now classify by semantics: default(x, false) ≡ default(x), default(false, true) detects as a default-off toggle, and only collapsing forms (default(true, true) / boolean=true) stay excluded. Side benefits: comments/{% raw %}/string literals can no longer shadow a live assignment, and the filter is recognized outside set (the older gemma-4 revs use {%- if enable_thinking | default(false) -%} and now detect correctly — parser resolution unchanged, verified across the same 205-template sweep). Tests updated to cover each combination.
Address review: replace the regex with Jinja Filter-node parsing so all
boolean/default combinations are classified by semantics — default(x, false)
equals default(x), default(false, true) is a working default-off toggle, and
only collapsing forms like default(true, true) stay excluded. Comments,
{% raw %} blocks, and string literals can no longer shadow a live assignment,
and the filter is recognized outside set-assignments (gemma-4 style
'if enable_thinking | default(false)'). A parse-only {% generation %}
extension keeps transformers chat templates parseable.
detect_reasoning_pattern evaluates rules unguarded, and jinja parsing can raise beyond TemplateError (e.g. RecursionError on a pathologically nested template), which would take down template loading at startup. Degrade to no detection instead, mirroring detect_inline_system_support, and add a regression test.
…t#32540) Co-authored-by: Jiminator <69131491+Jiminator@users.noreply.github.com>
…t#32540) Co-authored-by: Jiminator <69131491+Jiminator@users.noreply.github.com>
3 participants
Motivation
Fixes #32536.
Laguna-S-2.1 and Laguna-XS-2.1 share the
poolside_v1reasoning parser but use opposite Jinja defaults forenable_thinking. Template detection did not recognize theenable_thinking | default(...)form, so Laguna-S-2.1 fell back to the XS-specific parser default and left reasoning disabled for ordinary requests.Modifications
enable_thinking | default(true/false)assignments when resolving a chat template's reasoning mode.default(value, boolean=true)forms excluded because they do not have the same toggle semantics.Accuracy Tests
This does not change model numerics. Verification performed:
poolside/Laguna-S-2.1tokenizer template resolves todefault_enabled=True.poolside/Laguna-XS-2.1tokenizer template resolves todefault_enabled=False.python3 -m py_compile python/sglang/srt/parser/template_detection.py test/registered/unit/parser/test_template_manager.pypassed.uvx pre-commit run --files python/sglang/srt/parser/template_detection.py test/registered/unit/parser/test_template_manager.pypassed.Speed Tests and Profiling
N/A. This adds two startup-time regex checks over the chat template and does not affect model execution.
Checklist
Review and Merge Process
/tag-and-rerun-ci,/tag-run-ci-label,/rerun-failed-ciCI States
Latest PR Test (Base): ❌ Run #30444413151
Latest PR Test (Extra): ❌ Run #30444412849