feat(reasoning): per-model reasoning toggle — /think soft-switch (Category #1)#3031
feat(reasoning): per-model reasoning toggle — /think soft-switch (Category #1)#3031nsgds wants to merge 3 commits into
Conversation
|
Thanks for opening #3031. I think the prototype is useful, and the clean boundary is:
My read is that #2739 should own the first two parts: capability/control evidence, source, confidence, and endpoint-scoped model identity. #3031 should own the last two parts: stored runtime preference ( We added a small #2739 schema piece for this: canonical reasoning control mechanisms and values. The important distinction is:
So I think #3031 is the right follow-up direction, but it should stay draft until #2739 lands/rebases. After that, the main alignment changes I’d ask for are:
I do not think #2739 should absorb the toggle implementation. It should provide the evidence layer that #3031 consumes. |
…arity, tests Follow-up to review on odysseus-dev#3031 (RaresKeY): - resolve the per-model preference by endpoint identity (model membership) when several endpoint rows share a base_url, instead of first-URL-match; full stable-identity resolution remains the odysseus-dev#2739-aligned step - document scope explicitly as default-off /think enablement; name /no_think as the future off-direction increment - clarify auto/on/off is user preference (intent), not capability/adaptive metadata (a odysseus-dev#2739 concept) - TODO(odysseus-dev#2739): swap the name-substring dispatch for odysseus-dev#2739 control evidence once that evidence is wired at runtime - test: same-base-url disambiguation by model + no-leak for an unserved model Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Agreed on the split — #2739 owns the evidence (capability + control mechanism/value), #3031 owns the stored preference and the request mutation. Keeping it draft until #2739 lands/rebases, and I'll re-key the dispatch onto the control evidence then. What's in now (doesn't depend on #2739):
Deferred to post-#2739 (both isolated to one function, marked in-code):
Agreed too that #2739 shouldn't absorb the toggle — this stays the consumer of that evidence. |
…arity, tests Follow-up to review on odysseus-dev#3031 (RaresKeY): - resolve the per-model preference by endpoint identity (model membership) when several endpoint rows share a base_url, instead of first-URL-match; full stable-identity resolution remains the odysseus-dev#2739-aligned step - document scope explicitly as default-off /think enablement; name /no_think as the future off-direction increment - clarify auto/on/off is user preference (intent), not capability/adaptive metadata (a odysseus-dev#2739 concept) - TODO(odysseus-dev#2739): swap the name-substring dispatch for odysseus-dev#2739 control evidence once that evidence is wired at runtime - test: same-base-url disambiguation by model + no-leak for an unserved model Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
d9db927 to
d1720a0
Compare
…egory odysseus-dev#1) Odysseus had no per-model settings mechanism (supports_tools etc. are per-endpoint), so this adds a minimal one — ModelEndpoint.reasoning_modes, a JSON map {model_id: "on"|"off"} (absent = "auto" = leave the model's default), settable via PATCH /api/model-endpoints/{id} and exposed in the list response — and uses it to drive reasoning on/off. src/reasoning_control.py is the single boundary for the toggle logic. It catalogues all the ecosystem toggle mechanisms and implements Category odysseus-dev#1, the "/think" soft-switch (Nemotron-VL family): when a model's preference is "on", "/think" is injected into the latest user message. Categories odysseus-dev#2-odysseus-dev#5 (system-prompt directive, chat_template_kwargs, native bool, structured {type:enabled/disabled} object) are documented as future extension points; odysseus-dev#6/odysseus-dev#8 (graded reasoning_effort/budget) are noted as a separate setting. Wired at one point in stream_llm(); auto/unknown models and other providers are left unchanged (no leaked fields). - core/database.py: reasoning_modes column + migration (mirrors supports_tools) - routes/model_routes.py: GET exposes + PATCH accepts reasoning_modes - src/llm_core.py: resolve per-model preference -> inject /think - tests/test_reasoning_control.py: directive mapping, injection, degrade-to-auto Co-Authored-By: Claude <noreply@anthropic.com>
…arity, tests Follow-up to review on odysseus-dev#3031 (RaresKeY): - resolve the per-model preference by endpoint identity (model membership) when several endpoint rows share a base_url, instead of first-URL-match; full stable-identity resolution remains the odysseus-dev#2739-aligned step - document scope explicitly as default-off /think enablement; name /no_think as the future off-direction increment - clarify auto/on/off is user preference (intent), not capability/adaptive metadata (a odysseus-dev#2739 concept) - TODO(odysseus-dev#2739): swap the name-substring dispatch for odysseus-dev#2739 control evidence once that evidence is wired at runtime - test: same-base-url disambiguation by model + no-leak for an unserved model Co-Authored-By: Claude <noreply@anthropic.com>
CI runs bare pytest with DATABASE_URL=sqlite:///:memory: (per conftest); the in-memory model_endpoints table is not reliably present on the active connection by the time the same-base-url test seeds rows (a prior test in the full suite can recycle the singleton connection). Call Base.metadata.create_all(bind=engine) at the top of the test (no-op when the table already exists, e.g. in the Docker image where setup.py created the DB). Co-Authored-By: Claude <noreply@anthropic.com>
d1720a0 to
0e77384
Compare
RaresKeY
left a comment
There was a problem hiding this comment.
Now that #2739 has merged, please rebase this branch onto current dev, complete the two alignment items already discussed in this thread, and address the three additional issues below before moving it out of draft. I checked the latest head on a clean current-dev rebase; it applies mechanically, but it still relies on model-name/URL matching instead of canonical control evidence and stable endpoint/model identities.
Findings
issue (runtime): Apply the reasoning preference to non-streaming requests
-
Problem: Preference resolution and
/thinkinjection run only in_stream_llm_inner().llm_call()and the normalllm_call_async()path build payloads from unchanged messages, while publicPOST /api/chatcallsllm_call_async(). -
Impact: The same stored endpoint/model preference works in streaming chat but is silently ignored by non-streaming chat and the many sync/async consumers. An offline request capture with mode
onsenthelloinstead of/think hello. -
Ask: Move preference resolution and message mutation into a shared preparation boundary used by sync, async, and streaming builders, before sync/async cache-key calculation. Add payload-parity tests for all three paths and a cache-transition regression.
-
Location:
src/llm_core.py:1762,src/llm_core.py:1924,src/llm_core.py:2159,routes/chat_routes.py:504
issue (security): Redact endpoint credentials from reasoning-control error logs
-
Problem:
reasoning_mode_for()catches lookup or preference-decode failures and logsendpoint_urlverbatim, bypassing the repository'score.log_safety.redact_url()contract for configured URLs. -
Impact: With DEBUG logging enabled, a reasoning lookup failure can persist URL userinfo or query credentials in console/application logs. An offline probe confirmed both classes of fake marker were emitted.
-
Ask: Redact or omit the endpoint URL before logging it, and add an error-path regression proving URL userinfo, query, and fragment data never reach the log record.
-
Location:
src/reasoning_control.py:129
issue (correctness): Match an actual directive token when checking idempotency
-
Problem:
inject_directive()treats any substring match as an already-present directive. A prompt containing/thinkeris left unchanged even though it has no/thinkcontrol token; multimodal text uses the same check. -
Impact: An enabled preference can silently fail for otherwise ordinary prompts that happen to contain the character sequence.
-
Ask: Recognize an exact/leading directive token instead of an arbitrary substring, and cover false positives in both string and multimodal content.
-
Location:
src/reasoning_control.py:103andsrc/reasoning_control.py:106
Validation
- Focused combined-tree suites passed, including a 256-test model/route set; changed modules also compiled.
- Offline payload and logging probes reproduced both P2s.
- The full isolated suite reported 4,671 passed, 5 skipped, and the same 12 unrelated runner/baseline failures seen outside this change.
- Not run: live Nemotron/vLLM, full app/browser, existing-database upgrade, or GitHub CI on a post-#2739 public head.
PR Hygiene
The raw head and its checks predate #2739. After rebasing and addressing the alignment work and findings above, please refresh CI and request another review.
Summary
A first, deliberately narrow step toward per-model reasoning control.
Scope — read this first: this implements exactly one toggle mechanism, the
/thinksoft-switch (Category 1), which is only relevant to a small set of models — the Nemotron-VL family today (Qwen3 / Hunyuan use the same/thinkstyle). Every other model, and every other provider, is left byte-for-byte unchanged. It is intentionally a foundation, not a complete reasoning-control system.Why a per-model mechanism at all: Odysseus has no per-model settings today —
supports_tools,thinking_style, refresh config, etc. are all per-endpoint. But whether reasoning can be toggled (and how) is a property of the model, and one endpoint can serve several models that differ. So this adds a minimal per-model store —ModelEndpoint.reasoning_modes, a JSON map{model_id: "on"|"off"}(absent ="auto"= leave the model's default) — with the/thinkdirective as its first consumer. The logic sits behind a single boundary,src/reasoning_control.py: when a model's preference ison,/thinkis injected into the latest user turn;auto/unknown models emit nothing, so a directive is never sent to a model that wouldn't understand it.What this does NOT do (yet): the ecosystem has several other reasoning-toggle mechanisms. They're catalogued in
reasoning_control.pyas future extension points but are not implemented here:detailed thinking on/off) — Llama-Nemotron (incl. nim-nano)chat_template_kwargs.enable_thinking— Qwen3, DeepSeek-V3.1/3.2 self-host, GLM, Granitethinkbool — Ollama{type: enabled|disabled}object — Anthropic, GLM/Z.ai, DeepSeek/Kimi/Cohere hostedreasoning_effort— a separate setting, not a binary toggle — OpenAI o-series/GPT-5, Grok, MistralSo this is the smallest useful slice: the per-model store + one mechanism, structured so the rest can be added without reshaping.
Design was discussed and endorsed on #2737. It's opened as a shape-validation prototype (per that discussion) — not seeking to merge ahead of #2739: it currently uses a standalone
reasoning_modespreference, and the "which family / does it reason?" lookup is isolated behind one function so it can re-key onto #2739'sCAP_REASONING+ capability vocabulary once that lands.It complements the recently-merged #2449: that router handles reasoning output (response-side); this is the request-side on/off. They're in the same file (
src/llm_core.py) but non-overlapping regions — our 9-line call block sits before the provider branches; the harmony router is downstream in the stream loop.No UI in this PR:
reasoning_modesis set via the model-endpoints API (PATCH /api/model-endpoints/{id}), likesupports_toolstoday; a UI surface is a deliberate separate follow-up.Target branch
dev, notmain.Linked Issue
Closes #4146
Part of #315
Parent tracker: #2737
Related: #4146, #2737 / #2739 (design discussion + the capability schema this will build on), #2449 (the merged response-side router this complements), and #2905 — the reported "I disabled Reasoning but the model still thinks and stays slow" bug, which this addresses for
/think-style models (the display toggle only hides reasoning; this actually stops it).Type of Change
Checklist
dev/thinkon a live Nemotron-VL model and confirmed the reasoning block appears/disappears).How to Test
reasoning_modesmigration runs (nullable column onmodel_endpoints).PATCH /api/model-endpoints/{id}with body{"reasoning_modes": {"<model-id>": "on"}}./thinkis injected into your message → the reasoning/thinking block appears."off"(or omit it) → no reasoning. Confirm other models/providers are unaffected — defaultautosends nothing.python -m pytest tests/test_reasoning_control.py— covers the directive mapping (only/think-dialect models, only onon), the no-leak guard (other models/providers get nothing), message injection (string + multimodal), and degrade-to-auto.Visual / UI changes
No UI or CSS — backend + API only (
reasoning_modesis configured via the model-endpoints API, likesupports_tools). The visible effect is that a model set toonnow surfaces its reasoning where it previously didn't.