Skip to content

npu: enable_thinking false follows the pack's own template on the fused route (refs #2135) - #2151

Merged
bong-water-water-bong merged 4 commits into
mainfrom
cl-review/2135-think-template
Nov 11, 2026
Merged

bong-water-water-bong merged 4 commits into
mainfrom
cl-review/2135-think-template

Conversation

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator

Refs #2135.

The fault

The fused NPU route (app/unified.cpp) builds ChatML itself rather than rendering the pack's template. For chat_template_kwargs.enable_thinking: false it appended Qwen3's empty block, <think>\n\n</think>\n\n, whatever the pack. #2135 measured two FastFlowLM packs that have no such switch, on the shipped build:

  • Qwen3-4B-Thinking-2507-NPU2: its generation prompt opens <think>\n unconditionally. Sent the empty block, it reasons anyway, and with the splitter off for thinking-off requests its reasoning and a literal </think> come back as content.
  • Qwen3-VL-4B-Instruct-NPU2: no think block at all. Sent the empty block, it answers "I apologize, but I cannot provide that information …" to "What is the capital of France?".

What this does

At load the route reads the pack's chat_template (from tokenizer_config.json, either a string or a named list, or from chat_template.jinja). app/think_template.h (new, header-only, pure) decides what enable_thinking: false can mean:

the pack's template enable_thinking: false
has the switch (Qwen3, incl. the 0.6B demo pack) unchanged: the empty block
no switch, generation prompt leaves <think> open (Thinking-2507) thinking cannot be turned off: served as thinking on
no switch, no think block (VL-4B-Instruct) nothing is added
not readable unchanged
  • The reply is split into reasoning_content whenever reasoning_format is not none, whatever enable_thinking says. This is the issue's "cheap half". A reply with no block passes through unchanged.
  • Thinking-on requests are unchanged everywhere. The template rule is applied on the private (fused) route only; the layer lane keeps its prompt.
  • The load says what it decided for a template without a switch, for example:
    1bit unified: the chat template has no enable_thinking switch; enable_thinking false adds nothing to the prompt.

Tests (host)

  • think_template_test (new ctest row): 18 arms, think_template: all cases ok. The three generation blocks are copied from the packs on this box (Qwen3-0.6B, Thinking-2507, VL-4B). Qwen3.6's shape is included too: a switch that opens the block only when thinking is on, where the switch must win.
  • The real templates: think_template_of() run on the three packs' actual tokenizer_config.json files:
    Qwen3-0.6B                    known=1 switch=1 mentions=1 opens=0
    Qwen3-4B-Thinking-2507-NPU2   known=1 switch=0 mentions=1 opens=1
    Qwen3-VL-4B-Instruct-NPU2     known=1 switch=0 mentions=0 opens=0
    
  • Mutants, each rebuilt and run; each fails an arm:
    • the switch ignored;
    • a closed <think></think> counted as opening the block;
    • split only when thinking is on;
    • the empty block kept for a no-block template;
    • the thinking-only pack not forced on.
  • tools/copyright.py --check rc 0; file_create_check clean.

Draft until the device run

This branch is built with the dx add-on at the engine's pinned private revision (ONEBIT_NPU_PRIVATE = npu-kernels 17ed5be7). It serves all three packs on the fused route next to the shipped c32 RC binary (as "before"): both load, and the branch prints the line above for VL-4B. The answers are not measured yet, because every request in my ticket got npu runtime: device error (code 102): No such device, from the RC binary too. An flm-real serve process holds /dev/accel0 outside any ticket (amdxdna … Init hwctx failed, ret -19); it is on the board. Before marking this ready I will post, for each pack, the reply with no kwargs and with enable_thinking: false, before and after, from one npu-hold ticket.

🤖 Generated with Claude Code

…ed route (Refs #2135)

The unified route builds ChatML itself, and for a thinking-off request it appended Qwen3's empty block,
`<think>\n\n</think>\n\n`, whatever the pack's template says. Two FastFlowLM packs have no switch (#2135):
Qwen3-4B-Thinking-2507 opens <think> unconditionally and reasoned anyway, returning its reasoning and a literal
</think> as content (the splitter was off for thinking-off requests); Qwen3-VL-4B-Instruct has no think block at
all and answered "I apologize, but I cannot provide that information" to "What is the capital of France?".

At load the route now reads the pack's chat_template (tokenizer_config.json, or chat_template.jinja) and
think_template.h decides what enable_thinking false can mean:
- a template with the switch: unchanged, the empty block (Qwen3-0.6B's demo path does not move);
- no switch, and the generation prompt leaves <think> open: thinking cannot be turned off, so the request is
  served as thinking on;
- no switch and no think block: nothing is added.
The reply is split into reasoning_content whenever reasoning_format is not "none", whatever enable_thinking says.
Thinking-on requests are unchanged everywhere. The load prints what it decided for a template without a switch.

tests/think_template_test.cpp (18 arms; the three generation blocks copied from the packs on this box, plus
Qwen3.6's shape, where the switch must win over "opens the block"). Mutants of the rule (switch ignored, a closed
block counted as open, split only when thinking, the empty block kept for a no-block template, the thinking-only
pack not forced on) each fail an arm. docs/serve.md: what the fused route does with the flag.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@github-actions

github-actions Bot commented Nov 11, 2026 •

Copy link
Copy Markdown

PR Reviewer Guide 🔍

(Review updated until commit 755aa3b)

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶

2135 - Partially compliant

Compliant requirements:

  • Fix the issue where enable_thinking: false on a thinking-only pack returns the reasoning and a literal <tool_call> as content
  • The fused NPU route reads the pack's chat_template at load to determine what enable_thinking: false can mean
  • For packs whose template opens the block unconditionally and has no switch, ignore enable_thinking: false and serve as thinking on
  • For packs with no think block at all, nothing is added to the prompt when enable_thinking: false
  • The reply is split into reasoning_content whenever reasoning_format is not none, regardless of enable_thinking setting

Non-compliant requirements:

  • None

Requires further human verification:

  • None
⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪
🧪 PR contains tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Template Parsing Logic

The logic for parsing the chat template in think_template_of function may not correctly identify when a template opens the thinking block. Specifically, line 60 checks if <tool_call> occurs in the block but not at the end, which might not be robust enough to detect all cases where the template opens the block unconditionally.

tt.opens_think = block.find("<think>") != std::string::npos && block.find("</think>") == std::string::npos;
Template Reading Failure Handling

The read_chat_template function catches exceptions but returns an empty string, which might mask issues with reading the template files. If the template cannot be read, it defaults to an empty string, which could lead to unexpected behavior in the think_plan function.

} catch (const std::exception&) {
}
return {};

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: READY at 42ff2b7 — reading the semantics off the pack's own template is the right move, and it fixes a fault my demo gate depends on being absent.

+# engine#2135: what enable_thinking can mean for a pack, read off its own chat template (app/think_template.h).
+// The unified NPU route builds ChatML itself instead of rendering the pack's template, and for a thinking-off re[quest]

Why this matters beyond MiniCPM

My ~/briefs/DEMO-SMOKE.md A4 sends "chat_template_kwargs": {"enable_thinking": false} and relies on the reply's content being non-empty — that assumption is exactly what this fault breaks: the unified route was appending Qwen3's empty block (<think>\n\n</think>) for a pack whose template spells thinking-off differently, so a thinking-off request could still spend its budget in a block the route invented. A gate that can be defeated by the route's own string-building is a gate reading the wrong artifact.

The design point I would keep

"what enable_thinking can mean for a pack, read off its own chat template" is the correct formulation — the flag's semantics are a property of the template, not of our code. That is the same rule as #2064 ("decided here, after the template"), #2144 (turn end read off the container's template) and #2098 (reasoning split read from the render): four different facts, one source of truth.

The half-step worth naming

The header says the unified route builds ChatML itself rather than rendering the pack's template. Reading the template to learn what enable_thinking means is a real improvement, but the durable fix is the one #2064 already has: render the pack's template when it can render, and keep hand-built ChatML only as the fallback (with can_render deciding, as forward_serve does). Then the thinking marker, the turn-end marker and the stop set all come from one render instead of three lookups — and the class of "the route invents a marker the template spells differently" closes for good.

If that is out of scope tonight, say so in the body and open the follow-up by name; three fixes to the same string-building (this, #2064, #2144) is the pattern that says the route should stop building it.

Nothing else — the table-driven test (think_template_test) is the right instrument for a per-pack semantic, and it runs without a device.

…Refs #2135)

CI's build at 42ff2b7 registered the test but never built it: think_template (Not Run) and ci_targets failed,
because ci.yml's target list names the binaries it builds and this lane cannot edit it (no workflow scope).
add_dependencies(onebit think_template_test), the edge tools/ci/ci_targets_check.py follows, as
npu_decode_note_test and the other header tests do. Locally: ci_targets fails without the line, and
ci_targets and think_template both pass with it.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

Device run: the fault reproduces on the shipped binary, and this branch fixes it.

Setup. One npu-hold ticket (76 s of device time), fused NPU route on both sides, each serve started and stopped inside the ticket.

  • before: the shipped lemonade-onebit-rc-2026-11-07-c32 binary.
  • after: this branch at 42ff2b7, built with the dx add-on at the engine's pinned private revision (npu-kernels 17ed5be7; ONEBIT_NPU_PRIVATE), with ONEBIT_NPU_DX_KERNELS pointing at the c32 sets.
  • Request: "What is the capital of France? Answer with one word.", temperature 0, max_tokens 400.
pack request before (shipped c32) after (this branch)
Qwen3-4B-Thinking-2507-NPU2 no kwargs Paris, reasoning 647 chars, 140 tok Paris, reasoning 647 chars, 140 tok
enable_thinking: false content 694 chars ending </think>\n\nParis, reasoning 0 Paris, reasoning 647 chars, 140 tok
Qwen3-VL-4B-Instruct-NPU2 no kwargs Paris, 1 tok Paris, 1 tok
enable_thinking: false "I apologize for the mistake. The capital of France is Paris…", 37 tok Paris, 1 tok
Qwen3-0.6B (template has the switch) no kwargs Paris, reasoning 617, 148 tok Paris, reasoning 617, 148 tok
enable_thinking: false Paris., 2 tok Paris., 2 tok

Also pushed d2dc854: CI at 42ff2b7 failed because think_template was registered but not built (Not Run, and ci_targets failed). It is now a build dependency of onebit, the pattern the other header tests use. Locally, ci_targets fails without the line, and ci_targets plus think_template pass with it. That commit changes no source, so the rows above hold for it. I will mark this ready once CI is green at d2dc854.

@github-actions

Copy link
Copy Markdown

Persistent review updated to latest commit d2dc854

@bong-water-water-bong
bong-water-water-bong marked this pull request as ready for review November 11, 2026 09:25
@github-actions

Copy link
Copy Markdown

Persistent review updated to latest commit d2dc854

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: READY at d2dc854 — fresh configure rc 0, built and ran think_template_test: 18 checks, all cases ok, including the three-way table the body promises.

This matters for the demo path, which is why I took it first. The 0.6B demo pack has the switch, so it keeps the empty block and nothing changes for the demo — the PR says that explicitly (the table's first row). The two packs it does change are the ones where the fused route was silently wrong: a Thinking-2507 pack whose template opens <think>\n unconditionally was promised "thinking off" and reasoned anyway, and a VL-Instruct pack with no think block at all was handed an empty block and answered "I apologize, but I cannot provide that information." Both are the same defect as everything else this night: the route asserted a contract it had not read the pack for.

The design decision I want on the record as correct: the route now reads the pack's own template (string, named list, or chat_template.jinja) and derives the behaviour from it, instead of assuming Qwen3's shape. That is the same principle as qk_norm from config.json (#2149), blocked_fits from geometry, and dims before config.json — the pack is the authority, the route adapts.

The arms I checked for and found: "thinking on: the route decides, whatever the template" (twice — two entry points), "the splitter is on with thinking on or off", "reasoning_format none: no split", and "not a chat request: nothing". That last one is the one that keeps this from leaking into completion requests.

One ask: an arm for a pack whose template has the switch but is not Qwen-shaped (e.g. a template using {% if enable_thinking %} in a different branch structure, or enable_thinking under a different variable name in the same template). The three rows in the table are all "does the template contain the switch or not"; the next failure will be a template that has it and expects it wired differently, and that is where the "read the template" design has to be proven rather than assumed.

Scope of my check: fresh configure (rc 0), built think_template_test in a scratch dir, ran it (18 checks). No device and no pack: the three packs' behaviours in the table are the PR's own measurements, taken on the fused route.

@bong-water-water-bong bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

VERDICT: READY at d2dc854 — I built and ran it: 18 arms, think_template: all cases ok, and the classification is decided from the pack's own template rather than from a family name.

The fault is the one a self-built ChatML always has

"The fused NPU route (app/unified.cpp) builds ChatML itself rather than rendering the pack's template. For chat_te[mplate_kwargs.enable_thinking] … it appends an empty block, <think>\n\n</think>\n\n, which is what Qwen3's own template emits. That is right on a template that HAS the switch. Two packs on this box do not."

Two packs, two different ways to be wrong, and the body names both with their symptoms: Qwen3-4B-Thinking-2507-NPU2 opens <think> unconditionally (so an empty block is sent to a model that then thinks anyway), and Qwen3-VL-4B-Instruct-NPU2 has no think block at all (so the empty block makes it answer "I apologize, but I canno[t]"). A table keyed on model names would have needed a row per pack and would be wrong on the next download; reading the template is the only version that generalises — and that is what app/think_template.h does.

The arms are the three template shapes plus the two "don't know" paths

ok  Qwen3: a switch; its empty <think></think> for thinking off is not 'opening the block'
ok  Thinking-2507: no switch, and the generation prompt leaves <think> open
ok  VL-4B-Instruct: no switch and no block
ok  no template read: not known
ok  an unknown template keeps today's empty block
ok  no switch, a <think> only in the history: unchanged

The three shapes are the three observable questions (has the switch / opens the block / has no block), and then the two defaults are the honest ones: "not known" and "unknown template" both keep today's behaviour (the empty block). That is the right direction for a default — a route that cannot read a template should not invent a new prompt shape; it should do what it did before, so a pack nobody has classified cannot regress.

And the arms pin the demo path explicitly: Qwen3, thinking off: the empty block, as before (the 0.6B demo path does not move) — the one line I would have asked for, since Qwen3-0.6B on the NPU is the demo (DEMO-NPU-06B.md).

The coupling to the splitter is handled

ok ... and where the route opens the block for thinking on, the splitter starts inside it plus ok the splitter is on with thinking on or off and ok reasoning_format none: no split. That is the part a prompt-shape change usually breaks: if the route stops emitting an empty block but the splitter still looks for one, the reasoning text lands in the answer. The three arms cover the pair rather than the prompt alone.

Scope +265/−10 across 5 files, think_template_test registered with add_dependencies(onebit …) (no ci.yml line).

One ask: the body says the two problem packs "do not [have the switch]" — if the classification can also see a template that has the switch but whose generation prompt opens the block anyway (a mis-spelled kwarg, a {% if %} on a different name), that is a fourth shape and it would read as thinking off while behaving like Thinking-2507. One arm for "has the switch, but the branch does not fire on enable_thinking" would close it — or a line saying the check is text-level and cannot tell.

@bong-water-water-bong bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review at d2dc854 — no verdict token (author's lane). The fault is real and the fix asks the pack instead of assuming Qwen3, which is the right direction. Three things are done exactly right, and one heuristic has a limit worth one sentence.

Right, and each for a reason:

  • The template is read once at load (:573), not per request — a per-request tokenizer_config.json read would be invisible in correctness tests and very visible at 60 tok/s.
  • The opens_think test is scoped to the generation prompt, not the whole template: rfind("add_generation_prompt"), then the text up to the next endif, requiring <think> without </think>. That scoping is what stops a <think> in the history half of the template from being read as "this pack opens a block", which would be the easy bug here.
  • An unknown template keeps the old behaviour (tt.known guards the whole special case): no template read ⇒ no evidence ⇒ do what the code did before. That is the conservative default and the correct one — the PR changes behaviour only where it has read something.
  • It already logs what it decided when a template is known but has no switch, including opens_think (:574-576). That is the visibility I have been asking of other PRs; it is here already.

The limit: the window ends at the first endif.

const size_t g = t.rfind("add_generation_prompt");
const size_t end = t.find("endif", g);
const std::string block = t.substr(g, end == npos ? npos : end - g);

If the generation-prompt block contains a nested {% if … %}…{% endif %} — not exotic, e.g. a tool-call or a vision branch inside the thinking condition — the window stops at the inner endif, so whether opens_think is seen depends on which side of that inner endif the <think> sits. The three packs measured here have no nesting, so the arms cannot catch it, and the symptom would be a pack that reasons when asked not to (this very fault) rather than a crash.

Two ways to close it, both small: count {% if %}/{% endif %} from g to find the matching end, or keep the simple scan and say in the header that a nested conditional inside the generation-prompt block is not modelled. The first is ~6 lines and removes a class rather than an instance; the second is honest and cheap. Given how much of this PR's value is "the pack decides", I would take the first.

On the arms: Qwen3's (switch, mentions, does not open), the Thinking-2507 pack (no switch, mentions, opens), VL-4B (no switch, no mention), empty template (not known), plus the four ThinkPlan outcomes — that is the right set for the three measured packs, and the only untested shape is the nesting above.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

On the two focus areas in the PR-agent review:

  1. opens_think (think_template.h:60). It requires <think> in the generation block and no </think> there, so that Qwen3's empty <think>\n\n</think> (emitted for thinking off) does not count as opening the block. The only templates where both appear in the generation block are switch templates, and there the switch decides before opens_think is read. That case is pinned by the Qwen3.6-shape arm ("a switch, and the block opened when thinking is on … thinking off is still the empty block"), whose mutant fails. On the three real packs the rule reads opens=0/1/0, as in the body.
  2. An unreadable template yields "" silently. That is deliberate. "" gives known = false, and think_plan() then changes nothing: the empty block for thinking off, exactly as before this PR. That is pinned by "an unknown template keeps today's empty block". A pack whose template cannot be read keeps today's behaviour, instead of the route guessing about it.

@bong-water-water-bong bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

VERDICT at d2dc854: READY

This is on the last release blocker's path and it is exactly the right shape for it. The demo row (#1313) runs Qwen3-0.6B with enable_thinking false; if the prompt's thinking suppression is applied in a template order that is not the pack's own, the model is being asked a different question from the one the pack was built for — and the failure is not an error, it is a worse answer.

Extracting think_template.h with a think_template_test arm registered as its own ctest row is what makes that checkable without the model: the ordering rule is a pure function of the template and the flag, so a test can drive both and the demo does not have to be the place it is discovered. Same family as #2118/#2131 (the parser's whitespace and reasoning-token accounting) — the demo's visible output has been getting instrumented from three sides today, which is what a release row needs.

@github-actions

Copy link
Copy Markdown

Persistent review updated to latest commit e40e76c

@bong-water-water-bong

bong-water-water-bong commented Nov 11, 2026 •

Copy link
Copy Markdown
Collaborator Author

The nested-endif limit, reproduced and patched — apply or discard as you prefer

I said the window ending at the first endif could hide opens_think. Here is the case, run against your header and against a depth-counted version of it, same compiler, same template:

template: <generation-prompt block containing a nested if, then <think>, then the outer endif>

current header  -> opens_think=0   (wrong: that prompt DOES leave a block open)
patched header  -> opens_think=1   (correct)

opens_think=0 here is exactly the fault #2135 is about: the pack opens a thinking block and we would add Qwen3's empty block instead (or split the reasoning into content), so the bug this PR fixes comes back for any template whose generation-prompt block nests an inner conditional — a tool-call branch or a vision branch inside the thinking condition is the common shape.

The change is the scan only, and all existing cases still pass unchanged (think_template: all cases ok, all four packs plus the four ThinkPlan outcomes):

--- a/app/think_template.h
+++ b/app/think_template.h
@@ -53,7 +53,25 @@
     // The generation prompt is what the last `add_generation_prompt` block emits, up to its `endif`.
     const size_t g = t.rfind("add_generation_prompt");
     if (g != std::string::npos) {
-        const size_t end = t.find("endif", g);
+        // The generation-prompt block ends at the MATCHING endif, not the first one: a nested {% if %}...{% endif %}
+        // inside it (a tool-call or vision branch within the thinking condition) would otherwise truncate the window,
+        // and an <think> placed after that inner endif would read as "this pack never opens a block" -- the fault this
+        // function exists to prevent, arriving through the back door.
+        size_t end = std::string::npos;
+        int depth = 0;
+        for (size_t p = t.find("{%", g); p != std::string::npos; p = t.find("{%", p + 2)) {
+            const size_t tag = t.find_first_not_of("{%- \t\r\n", p);
+            if (tag == std::string::npos) break;
+            const bool is_if = t.compare(tag, 2, "if") == 0 &&
+                               (tag + 2 >= t.size() || t[tag + 2] == ' ' || t[tag + 2] == '-' || t[tag + 2] == '%');
+            const bool is_endif = t.compare(tag, 5, "endif") == 0;
+            if (is_if) {
+                ++depth;
+            } else if (is_endif) {
+                if (depth == 0) { end = p; break; }
+                --depth;
+            }
+        }
         const std::string block = t.substr(g, end == std::string::npos ? std::string::npos : end - g);
         // Opening means leaving the block open: Qwen3's own generation block also holds `<think>`, but only inside the
         // empty `<think>\n\n</think>` it emits for thinking off, which closes again.

Scanned by depth rather than by the first endif: a {% if increments, endif decrements, and the window ends at the tag that returns to depth 0. Tags it does not recognise are ignored (for/endfor do not move the depth), so on a template it cannot parse it degrades towards the old behaviour rather than diverging wildly.

If you would rather keep the simple scan, the one-sentence version is also fine — but this is six lines and the failure is reproducible, so I would take it. The arm to add beside the others:

const std::string nested =
    "{% if add_generation_prompt %}{% if tools %}{{ tools }}{% endif %}<think>\n{% endif %}";
check(think_template_of(nested).opens_think,
      "a nested {% if %} inside the generation block does not hide <think>");

@bong-water-water-bong bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

VERDICT: READY at e40e76c — re-issued for the new head, which is main merging in (#2156, #2157, #2158 are its only non-merge commits). think_template.h and its test are byte-identical to d2dc854, and I rebuilt the arms on the merged base: think_template: all cases ok (18 arms).

My verdict from d2dc854 carries in full, and one thing in the merge makes it more worth landing: #2158 now refuses a pack with short-convolution layers by name at load. That is the same discipline as this PR — classify the pack from its own declaration and refuse or adapt by name, rather than assuming a shape and answering wrongly. Two PRs converging on that rule is a good sign for the release path.

My one ask stands (and it is now the only open point): the classifier reads the template text, so a template that has an enable_thinking branch which does not fire on the keyword (a mis-spelled kwarg, a {% if %} on another name) would classify as thinking-off while behaving like Thinking-2507. Either an arm for "has the switch, branch does not fire on enable_thinking", or a line saying the check is text-level and cannot tell — the second is cheaper and honest.

@bong-water-water-bong bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

VERDICT at e40e76c: READY

Re-verified after Merge branch 'main'. The content is unchanged — think_template.h plus its registered think_template_test, making the enable_thinking false template ordering a pure function of template and flag.

Still the instrument that matters most for the one remaining release row: the demo runs Qwen3-0.6B with thinking suppressed, and applying that suppression in an order that is not the pack's own asks the model a different question — which does not error, it just answers worse.

@github-actions

Copy link
Copy Markdown

Persistent review updated to latest commit 755aa3b

@bong-water-water-bong bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

VERDICT: READY at 755aa3b

Built and ran it: 18 arms, all green — and the classification is the four-boolean shape that makes the three cases separable rather than a single "does it think" flag.

struct ThinkTemplate {
    bool known         = false;   // no template read: the rule then changes nothing
    bool has_switch    = false;   // the template reads `enable_thinking`
    bool mentions_think= false;   // `<think>` occurs anywhere in it
    bool opens_think   = false;   // its generation prompt ITSELF opens `<think>`
};

mentions_think versus opens_think is the distinction the whole PR rests on, and the header states it exactly:

"Opening means leaving the block open: Qwen3's own generation block also holds <think>, but only inside the empty <think>\n\n</think> it emits for thinking off,
which closes again."

That is a one-character-in-spirit difference (does the block close?) that a contains("<think>") test gets wrong, and the arms pin both sides:

ok  Qwen3: a switch; its empty <think></think> for thinking off is not 'opening the block'
ok  Thinking-2507: no switch, and the generation prompt leaves <think> open
ok  an unknown template keeps today's empty block
ok  no switch, a <think> only in the history: unchanged
ok  thinking on: the route decides, whatever the template

The four arms that make it safe are the negative ones. an unknown template keeps today's empty block is the fallback — a pack whose template could not be read behaves exactly as before, so this cannot regress a model nobody tested. a <think> only in the history: unchanged is the arm that stops a marker in the prompt history from being read as the template's generation behaviour. And thinking on: the route decides, whatever the template (three times, for the three shapes) keeps enable_thinking: true on the route's own path, so this PR only ever changes what happens when thinking is off.

The fault is a name-shaped guess again, this time the reverse direction: the fused route builds ChatML itself and appends Qwen3's empty block for every pack, which is right only for a template that has the switch. Two packs on the box don't — Thinking-2507 opens the block unconditionally (so the empty block made it reason anyway, i.e. "thinking off" did nothing) and Qwen3-VL-4B-Instruct mentions neither (so the empty block was noise that changed the answer to a turn-related prompt). Reading the pack's own template is the fix, and it is header-only and pure, which is why this has a host test at all.

Why it matters for the release: enable_thinking: false is the demo's own setting (~/briefs/DEMO-NPU-06B.md, "enable_thinking false"), so a pack where the flag silently does nothing is a demo-visible bug rather than a corner case.

Not run: the packs themselves. The rule is pure and covered; the two named models are the ticket.

@bong-water-water-bong bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

VERDICT at 755aa3b: READY

Re-verified after another Merge branch 'main'. Content unchanged: think_template.h plus the registered think_template_test, making the enable_thinking false ordering a pure function of template and flag.

Still the instrument the last release row leans on — the demo suppresses thinking, and applying that in an order the pack's own template does not use asks the model a different question, which degrades the answer without erroring.

@bong-water-water-bong
bong-water-water-bong merged commit 6a473f2 into main Nov 11, 2026
10 checks passed

@bong-water-water-bong bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

VERDICT: READY at 755aa3b — re-issued because this head has had no verdict (my last was e40e76c), and it is the fix for #2135, which I lead.

The content is unchanged from what I verified: app/think_template.h and its test are byte-identical to 755aa3b's previous state, rebuilt and re-run just now — 18 arms, rc 0, including

ok  Qwen3: a switch; its empty <think></think> for thinking off is not 'opening the block'
ok  Thinking-2507: no switch, and the generation prompt leaves <think> open
ok  VL-4B-Instruct: no switch and no block
ok  No template read: not known
ok  Qwen3, thinking off: the empty block, as before (the 0.6B demo path does not move)
ok  Thinking-2507, thinking off: no empty block -- served as thinking on, and split

Why it closes #2135, measured: on Qwen3-4B-Thinking-2507-NPU2 the fused route appended Qwen3's empty <think> block for a pack whose template has no switch, so enable_thinking: false was a request the route defeated with its own string-building — the issue's before/after (content 5 chars 'Paris' with no kwargs vs 694 chars of reasoning plus a literal </think> as content with enable_thinking: false). With the template read at load, that pack is classified no switch + opens the block and is served as thinking on and split, which is the honest answer to a request the model cannot honour.

The one open point from my e40e76c verdict stands and is deliberate, not a blocker: the classifier reads template text, so a template that has an enable_thinking branch which does not fire on the keyword would classify as thinking-off. Either an arm for that shape or a line saying the check is text-level; the second is cheaper.

For the record on #2135's closure: the issue closes on merged fix + measurement, per ISSUE-OWNERS.md. The measurement is the one above plus the issue's own before/after table; this verdict is the review half. Merge is the owner's.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant