From 04c10880883dfc067fa6e1b8f94e584438481e57 Mon Sep 17 00:00:00 2001 From: kami Date: Fri, 31 Jul 2026 13:30:22 +0400 Subject: [PATCH] =?UTF-8?q?Measure=20thinking=20off=20on=20routing=20prope?= =?UTF-8?q?rly=20=E2=80=94=20it=20does=20not=20win=20(#376)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The 67.1% "thinking off" column in ROUTING-EVAL-31-07-2026.md was an artefact. It came from a hand-rolled HTTP client in the eval test that did not send repeat_penalty, so it differed from the reference run on two axes and the penalty was the one that mattered. Re-scored back to back on an idle box with everything else held equal: thinking off is identical to thinking on, case for case, same confusion matrix, same three unparseable replies. A direct probe of the running llama-server shows enable_thinking, thinking and reasoning_budget are all ignored for this model on this build, so there was nothing to turn off. No defaults changed. The misleading third configuration is removed from internal/router/eval so its table cannot be quoted again. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01CGeSZxh1DCtRxmFVSYVGvJ --- ROUTING-EVAL-31-07-2026.md | 69 +++++++++++++--- internal/router/eval/llmrouter_test.go | 110 ++++--------------------- 2 files changed, 73 insertions(+), 106 deletions(-) diff --git a/ROUTING-EVAL-31-07-2026.md b/ROUTING-EVAL-31-07-2026.md index 87d2bb5..7bf81fa 100644 --- a/ROUTING-EVAL-31-07-2026.md +++ b/ROUTING-EVAL-31-07-2026.md @@ -66,15 +66,64 @@ Three things this run settles: `запиши что…` phrasings toward fact, and that suspicion stands — all five `ru-note-*` cases now land on fact. Tracked as Vikunja #375. -**Thinking off is the best configuration measured so far**, on both accuracy and latency -(Vikunja #376). That is worth understanding before flipping: routing is a short -classification into a fixed enum with grammar-constrained output, so there is little to -reason about, and the thinking trace mostly gives a small model room to talk itself out of -the right answer. Phrasing is a different job and needs measuring separately. +The `thinking off` column above read as the best configuration measured so far (Vikunja #376). +**It was wrong** — see the controlled re-run below. Ignore that column. Still `6 / 6` missed clarify — the router has no way to say "I don't know" (Vikunja #359). That is unchanged by anything here. +## Thinking off — 31-07-2026, controlled re-run (Vikunja #376) + +The "thinking off wins by 6 points" observation above **does not hold**. It was a measurement +artefact, and the earlier table's `thinking off` column should be ignored. + +The thinking-off variant was scored by a hand-rolled HTTP client living in the test file +instead of `llm.Client`. That copy did not send `repeat_penalty`, which the real router does +send (`routeRepeatPenalty = 1.15`). So the two columns differed on two axes at once, and the +one that mattered was the penalty, not the thinking mode. + +Re-measured with everything else held equal — same fixture, same prompt, same grammar, same +sampling, same idle box, the three configurations run back to back and never concurrently: + +| | llm-only, thinking on | llm-only, thinking off | cascade+llm | +|---|---|---|---| +| intent-only accuracy | 59.2% (45/76) | 59.2% (45/76) | 61.8% (47/76) | +| full accuracy (intent+slots+gate) | 38.2% (29/76) | 38.2% (29/76) | 57.9% (44/76) | +| route errors | 3 | 3 | 0 | +| grammar violations | 3 (all 3 route errors) | 3 (same 3 cases) | 0 | +| missed clarify | 5 / 6 | 5 / 6 | 5 / 6 | +| p50 latency | 836ms | 920ms | 810ms | +| p95 latency | 1.41s | 2.00s | 1.31s | + +Thinking off is not just a tie on the headline numbers — it is identical case for case, with +the same confusion matrix and the same three unparseable replies. The latency difference is +run-to-run noise on one box, and it points the wrong way here. + +The reason is simpler than any accuracy argument: **this llama-server build ignores the +request-level thinking switch for this model.** Probed directly against the running server +with `chat_template_kwargs.enable_thinking = false`, `chat_template_kwargs.thinking = false` +and top-level `reasoning_budget = 0` — all three return a byte-identical answer with the +thinking trace still in `reasoning_content`, and the server reports the prompt prefix as +cached, meaning the rendered template did not change. There was never anything being turned +off, which is also why the numbers match exactly. + +Nothing was defaulted. `internal/llm` still has no `chat_template_kwargs` field, `VoiceConfig` +has no thinking flag, and `deploy/mavend.json` is unchanged. The misleading third +configuration is removed from `internal/router/eval` so the table it produced cannot be quoted +again. + +Two caveats worth saying out loud: + +- **The fixture is 76 cases.** A 6-point difference on 76 cases is roughly 4-5 cases and would + not have been worth trusting even if it had reproduced. This one was exactly 0 cases, which + is a much easier call. +- **This is one server build and one checkpoint** (`b9351`, Qwen3.5-0.8B Q4_K_M). If the + #122 checkpoint or a newer llama.cpp does honour the switch, the question reopens — but it + reopens as an unmeasured question, not as a 6-point win. + +Phrasing was **not** measured. Whether thinking helps there is still open, and now also blocked +on the same "can we even turn it off" question. + ## Findings ### 1. The resident model does route better — 50.0% vs 36.8% @@ -145,11 +194,11 @@ Note the grammar's `string ::= "\"" ([^"\\] | "\\" .)* "\""` is unbounded, so no ### 7. Two hypotheses tested and closed -- **Thinking mode is a non-issue.** Qwen3.5's template defaults `thinking = 1`, so - grammar-constrained JSON lands in `reasoning_content` with `content` empty — - `llm.Client`'s fallback handles it. A `thinking off` run scored *identically* (18/76, - 48.7%, same p50). `internal/llm` deliberately does **not** grow a `chat_template_kwargs` - field. +- **Thinking mode is a non-issue.** Confirmed twice now, the second time properly — see the + controlled re-run section. Grammar-constrained JSON lands in `reasoning_content` with + `content` empty and `llm.Client`'s fallback handles it; the request-level switch does + nothing on this build. `internal/llm` deliberately does **not** grow a + `chat_template_kwargs` field. - **Runaway array repetition does not reproduce.** An isolated smoke test with a stripped grammar emitted `{"intent":"reminder"}` until `MaxTokens`; under the real `routeSystem` prompt the few-shot examples anchor it to one object. 2 errors in 76, not 76. diff --git a/internal/router/eval/llmrouter_test.go b/internal/router/eval/llmrouter_test.go index 0c3210a..940eea9 100644 --- a/internal/router/eval/llmrouter_test.go +++ b/internal/router/eval/llmrouter_test.go @@ -1,11 +1,8 @@ package eval import ( - "bytes" "context" - "encoding/json" "fmt" - "net/http" "os" "strings" "testing" @@ -29,13 +26,20 @@ import ( // a bake-off across checkpoints (#278, #250) produces tables you can tell // apart. Point the variable at one server at a time. // -// Three configurations, because "the LLM router" is ambiguous and the three -// numbers answer different questions: +// Two configurations, because "the LLM router" is ambiguous and the two numbers +// answer different questions: // -// llm-only — the model alone. Measures the prompt + grammar contract. -// cascade+llm — what #320 would actually ship: stage-0 grammar, then the -// model, then the classifier as the failure floor. -// llm-no-thinking — diagnostic only, not a shippable path (see below). +// llm-only — the model alone. Measures the prompt + grammar contract. +// cascade+llm — what #320 would actually ship: stage-0 grammar, then the +// model, then the classifier as the failure floor. +// +// There used to be a third, "thinking off", which looked 6 points better. It is +// gone: it was measured with a hand-rolled HTTP client that quietly dropped +// repeat_penalty, so the gap was the missing penalty and not the thinking mode. +// Re-measured with everything else held equal, thinking off scores exactly the +// same, case for case — and a direct probe shows this llama-server build ignores +// enable_thinking / reasoning_budget for this model anyway, so there was nothing +// to turn off. Full write-up in ROUTING-EVAL-31-07-2026.md (Vikunja #376). func TestLLMRouterBaseline(t *testing.T) { base := os.Getenv("MAVEN_LLM_URL") if base == "" { @@ -96,104 +100,18 @@ func TestLLMRouterBaseline(t *testing.T) { } t.Log("\n" + repCascade.String() + repCascade.Failures()) - // llm-no-thinking: same prompt and grammar with the chat template's - // thinking mode off. Qwen3.5's template defaults thinking=1, so under a - // grammar the constrained JSON lands in reasoning_content with content - // empty — llm.Client's ReasoningContent fallback is what makes the router - // work at all today, by accident rather than design. - // - // MEASURED 2026-07-31: this variant scores identically to as-deployed - // (18/76, 48.7% intent-only, 2 errors, same p50). Thinking mode is a - // non-issue under a grammar — llama.cpp constrains the same token stream - // either way. Kept so the question stays answered instead of being - // re-asked, and so internal/llm does NOT grow a chat_template_kwargs field - // for a problem that does not exist. - repNoThink, err := Score(ctx, "llm-only ("+model+", thinking off) [diagnostic]", - RouterFunc(func(ctx context.Context, u string, now time.Time) (router.Decision, error) { - d, ok, err := router.NewLLMRouter(&noThinkCompleter{base: base, http: &http.Client{Timeout: 60 * time.Second}}).Route(ctx, u, now) - if err != nil { - return d, err - } - if !ok { - return d, fmt.Errorf("llm router declined without an error") - } - return d, nil - }), f) - if err != nil { - t.Fatalf("Score no-thinking: %v", err) - } - t.Log("\n" + repNoThink.String() + repNoThink.Failures()) - // Reports rather than asserts — the numbers are inputs to the #320 // decision, and an assertion here would be this test inventing the bar. // The one thing worth failing on is a harness fault: if every single case // errors, the run measured infrastructure, not routing, and the report // must not be mistaken for a score. - for _, rep := range []Report{repLLM, repCascade, repNoThink} { + for _, rep := range []Report{repLLM, repCascade} { if rep.Errors == rep.Total { t.Errorf("%s: all %d cases errored — harness fault, not a measurement", rep.Name, rep.Total) } } } -// noThinkCompleter — llm.Client with chat_template_kwargs.enable_thinking -// false. A test-local copy rather than a change to internal/llm: whether the -// daemon should send it is the open question, and answering it here by adding -// the field would prejudge #320. -type noThinkCompleter struct { - base string - http *http.Client -} - -func (c *noThinkCompleter) Complete(ctx context.Context, r llm.Req) (string, error) { - payload := map[string]any{ - "messages": []map[string]string{ - {"role": "system", "content": r.System}, - {"role": "user", "content": r.User}, - }, - "max_tokens": r.MaxTokens, - "temperature": 0, - "grammar": r.Grammar, - "chat_template_kwargs": map[string]any{"enable_thinking": false}, - } - b, err := json.Marshal(payload) - if err != nil { - return "", err - } - req, err := http.NewRequestWithContext(ctx, "POST", c.base+"/v1/chat/completions", bytes.NewReader(b)) - if err != nil { - return "", err - } - req.Header.Set("Content-Type", "application/json") - resp, err := c.http.Do(req) - if err != nil { - return "", err - } - defer resp.Body.Close() - if resp.StatusCode != 200 { - return "", fmt.Errorf("status %d", resp.StatusCode) - } - var out struct { - Choices []struct { - Message struct { - Content string `json:"content"` - ReasoningContent string `json:"reasoning_content"` - } `json:"message"` - } `json:"choices"` - } - if err := json.NewDecoder(resp.Body).Decode(&out); err != nil { - return "", err - } - if len(out.Choices) == 0 { - return "", fmt.Errorf("no choices") - } - m := out.Choices[0].Message - if m.Content != "" { - return m.Content, nil - } - return m.ReasoningContent, nil -} - func ping(ctx context.Context, c *llm.Client) error { ctx, cancel := context.WithTimeout(ctx, 90*time.Second) defer cancel() -- 2.52.0