Studio: add reasoning budget controls - #7701
Conversation
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
for more information, see https://pre-commit.ci
|
The normal flow works, the settings are wired through persistence, and they survive llama-server respawn. I found two issues that should be fixed before merging:
Please add regression coverage for multibyte overflow, embedded NUL, and whitespace in the last-wins passthrough value, including rejection before the active process is stopped. The existing focused checks passed: 570 backend tests and 161 frontend tests. |
for more information, see https://pre-commit.ci
…o feature/7524-reasoning-budget # Conflicts: # studio/backend/core/inference/llama_server_args.py
for more information, see https://pre-commit.ci
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 02fc4ba967
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
…o feature/7524-reasoning-budget
…g-budget # Conflicts: # studio/frontend/src/features/chat/api/chat-adapter.ts # studio/frontend/src/features/chat/shared-composer.tsx
for more information, see https://pre-commit.ci
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f712f0c727
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
|
Codex Review: Didn't find any major issues. Swish! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
SetupFresh Studio pair in
Both installed with their own Before the PR
On main, Studio has no first-class control for either. Confirmed empirically: After the PR
The issue is real, and the PR solves it. Not a fake/cosmetic fix. Merge riskIt does not add a hardware path — it's a launch-flag feature gated behind a capability probe, so the CPU/CUDA/Metal/Vulkan paths are untouched. But it is not safe to merge as-is: it breaks pre-existing tests. 7 tests that pass on 4 backend route tests — real code-path change. The new preflight at inference.py:6164 calls 3 source-contract tests — formatting churn. The PR reformats large stretches of untouched frontend code (80-col rewrapping in Four more things worth flagging, none of which break old pathways but all of which change behavior:
The pre-existing capability detection itself is safe: I ran the real probe against the same bundled binary on both trees and every pre-existing key is byte-identical ( Bottom line: real issue, correct approach, old pathways (non-GGUF, diffusion, plain GGUF loads, extras passthrough) are structurally preserved and gated — but 7 green-on-main tests go red, so it needs the preflight made defensive/late-bound and the formatting churn reverted before merging. ## SetupFresh Studio pair in
Both installed with their own Before the PR
On main, Studio has no first-class control for either. Confirmed empirically: After the PR
The issue is real, and the PR solves it. Not a fake/cosmetic fix. Merge riskIt does not add a hardware path — it's a launch-flag feature gated behind a capability probe, so the CPU/CUDA/Metal/Vulkan paths are untouched. But it is not safe to merge as-is: it breaks pre-existing tests. 7 tests that pass on 4 backend route tests — real code-path change. The new preflight at [inference.py:6164](unsloth_studio_000054/pr/repo/studio/backend/routes/inference.py:6164) calls 3 source-contract tests — formatting churn. The PR reformats large stretches of untouched frontend code (80-col rewrapping in Four more things worth flagging, none of which break old pathways but all of which change behavior:
The pre-existing capability detection itself is safe: I ran the real probe against the same bundled binary on both trees and every pre-existing key is byte-identical ( Bottom line: real issue, correct approach, old pathways (non-GGUF, diffusion, plain GGUF loads, extras passthrough) are structurally preserved and gated — but 7 green-on-main tests go red, so it needs the preflight made defensive/late-bound and the formatting churn reverted before merging. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4f28a4eac9
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| reasoningBudget: isGguf ? snapshot.reasoningBudget : -1, | ||
| reasoningBudgetMessage: isGguf ? snapshot.reasoningBudgetMessage : "", |
There was a problem hiding this comment.
Track reasoning fields in preset dirty state
With reasoning budget/message now captured in capturePresetLoadConfig(), the Preset section’s hasUnsavedPresetChanges and currentLoadSummary memos in chat-settings-sheet.tsx still only subscribe/depend on the older load settings through nParallel. If the preset sheet is already open and the user changes only Reasoning Budget or Reasoning Budget Message, the sheet does not re-render/recompute, so the active summary and Update button stay stale until some unrelated setting changes; add these runtime fields to the sheet selectors/dependency lists wherever capturePresetLoadConfig() is memoized.
Useful? React with 👍 / 👎.
| if reasoning_budget != -1 and caps.get("supports_reasoning_budget"): | ||
| flags.extend(["--reasoning-budget", str(reasoning_budget)]) | ||
| if reasoning_budget_message and caps.get("supports_reasoning_budget_message"): | ||
| flags.extend(["--reasoning-budget-message", reasoning_budget_message]) |
There was a problem hiding this comment.
Honor explicit reasoning resets over environment
When the Studio process has LLAMA_ARG_THINK_BUDGET or LLAMA_ARG_THINK_BUDGET_MESSAGE set, resetting the first-class controls to -1/empty still emits no CLI override here, so the child llama-server inherits the environment and the subsequent effective-state resolver reports the old budget/message again. This only affects users with those environment defaults configured, but it makes the new reset controls unable to clear them; distinguish omitted defaults from explicit resets and either emit supported reset flags or scrub the inherited env for that load.
Useful? React with 👍 / 👎.
The preflight resolved LLAMA_ARG_THINK_BUDGET* before asking whether anything was configured, so an env default alone counted as a request. With only that env var set and nothing set in Studio, /validate returned 400 for every undownloaded GGUF and every DiffusionGemma, and the capability gate rejected loads on any binary whose --help cannot be probed. All of these load on main, and the error names a setting the UI already shows as default, so there is no way out of it from the UI. This also disagreed with LlamaCppBackend's own load-time gate, which reads the explicit values only. Both now gate on the first-class fields plus passthrough extras. The env stays a reporting input, so effective state still reports the inherited value and llama-server keeps validating its own environment. Also subscribe the chat settings sheet to reasoningBudget and reasoningBudgetMessage. capturePresetLoadConfig() captures both through getState(), but the sheet listed neither in the hasUnsavedPresetChanges or currentLoadSummary dependency lists, so with the sheet open a reasoning budget change left the Update button and the summary stale.
|
Re-ran my review against the current head ( Already fixed since the earlier reviews
My earlier "7 red tests" note is staleI ran the full backend suite on this branch and on Two that were still live1. An inherited
All three load fine on Both now gate on explicit configuration only: the first-class fields plus passthrough extras. The environment stays a reporting input, so effective state still reports the inherited value exactly and llama-server keeps validating its own env. This is the same fail-open posture as the existing MTP probe. I updated 2. The preset dirty state does not react to a reasoning budget change.
VerificationEach fix has a regression test that I confirmed fails without the fix and passes with it.
One thing I left aloneResetting the control to Looks good to me now. |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b53774ae78
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (config.reasoningBudget !== -1) { | ||
| parts.push(`Reasoning ${config.reasoningBudget}`); | ||
| } |
There was a problem hiding this comment.
Include the budget message in preset summaries
When a preset sets only reasoningBudgetMessage while leaving the numeric budget at -1, hasPresetLoadConfig() correctly treats it as a non-default load configuration, but this formatter adds no summary part and returns null. Consequently, chat-settings-sheet.tsx suppresses both the “Active now” and “Saved in preset” summaries even though applying the preset changes llama-server behavior; include a summary marker for a non-empty message.
Useful? React with 👍 / 👎.
perModelConfigsEqual compares reasoningBudgetMessage, so hasPresetLoadConfig() counts a preset that sets only the message as a non-default load config. It is saved, applied, and does change llama-server behaviour, but the summary formatter had no part for it, so parts stayed empty and it returned null. The Preset section hides both "Active now" and "Saved in preset" on null, so the section showed no load settings at all for that preset. Push a marker rather than the text: the message is free prose up to 8 KiB. maxSeqLength and nCpuMoe have the same gap, but both predate this branch, so they are left alone here.
|
Went through the latest review round. Only one of the five inline comments is new against The new one: budget message missing from the preset summary
Fixed by pushing a I checked The other fourRe-verified each against the current head rather than the commit it was written on.
VerificationThe new test fails without the fix and passes with it.
Still good from my side. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 18de51401e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| store.activeGgufVariant != null || | ||
| store.ggufContextLength != null || | ||
| (store.params.checkpoint?.toLowerCase().endsWith(".gguf") ?? false); | ||
| !store.loadedIsDiffusion && |
There was a problem hiding this comment.
Keep diffusion models in GGUF context capture
When a DiffusionGemma model is using automatic context sizing (snapshot.customContextLength is null), this new condition classifies it as non-GGUF even though store.ggufContextLength contains its effective context. Saving a preset therefore records no customContextLength, whereas the previous code captured the resolved GGUF context, so applying that preset later can load with a different context and change memory use or experiment behavior. Keep the GGUF test independent of diffusion and suppress only the unsupported reasoning fields for diffusion models.
Useful? React with 👍 / 👎.
capturePresetLoadConfig folded !loadedIsDiffusion into isGguf, but isGguf also gates effectiveContextLength. A loaded DiffusionGemma reports both is_gguf and is_diffusion, so the flag flipped false and the resolved store.ggufContextLength stopped being captured. On automatic sizing that context is the only non-default knob, so coalescing left the config empty and the preset saved no loadConfig at all; on main it recorded the resolved context. isGguf goes back to the plain GGUF test and a separate capturesReasoning suppresses only the reasoning fields, which diffusion takes none of.
|
One new inline comment since Diffusion GGUFs were dropped from the preset context capture
Verified the premise rather than taking it at face value:
So with automatic sizing (
VerificationThe new test fails without the fix and passes with it.
For the record on the four older inline comments still anchored to the head: three are fixed and re-verified against the current source, and "Honor explicit reasoning resets over environment" stays open by choice, since |
Two call sites in the tests I added fit on one line under the repo's ruff-format hook. Committing the join so pre-commit.ci does not have to push an autofix.
|
No new review comments since Pushed:
|
Summary
reasoning_budgetandreasoning_budget_messageGGUF load settingsLLAMA_ARG_THINK_BUDGET*environment behavior with exact effective-state reporting/validatebefore the resident model is unloadedCloses #7524.
Test plan
cd studio/frontend && npm run typecheckcd studio/frontend && npm test(303 passed)git diff --check