fix(studio): honor run settings on initial model load (#7346)#7351
fix(studio): honor run settings on initial model load (#7346)#7351Souravrajvi0 wants to merge 19 commits into
Conversation
When loading a model from the gear-icon run-settings page, Context Length and KV Cache Dtype were ignored if the user clicked Load before blurring the context field, or before React flushed staged config into the store. - Add NumericValueInput.commit() to flush a focused draft on Load - Pass effectiveLoadConfig from model-config-page to onRun - Prefer selection.config in performLoad for all load knobs - Preserve meta.forceReload from the config-page reload path Fixes unslothai#7346
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
|
Manual check done locally (Playwright):
Also fixed a follow-up: clicking Load blurs the context field first, so |
Clicking Load blurs the context field before handleRun runs, so commit() returned the stale value prop. Keep draft in a ref and parse it even when the input is no longer focused.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0030839f5f
ℹ️ 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".
NumericValueInput.commit() now returns null unless the user actually changed the field, so GGUF Load/Save no longer pins the displayed native context into customContextLength when Auto was left untouched.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e3c776bfcf
ℹ️ 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".
After a normal blur commit, reset dirtyRef so a later Load cannot replay a stale draftRef when the user changed context via Reset or the slider.
|
Fixed in 78e3f3c (also covered in the consolidated reply on the P1 thread). P2 — Clear dirty state after blur commits: Valid. Verification: |
…i#7346 Lock Codex P1/P2: commit returns null unless dirty, blur clears dirtyRef, and handleRun only promotes a non-null committed context.
|
Ready for review — re-verified locally:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8bba0f06de
ℹ️ 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".
Blur can commit and clear dirtyRef before Load's onClick; stash that committed value for one imperative commit() so typed context is not lost.
|
Fixed in 94e49be. P2 — Preserve same-click draft after blur: Valid. Blur committed via Fix: Keep Tests: |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 03e68e11c9
ℹ️ 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".
23462f3 to
2378e47
Compare
e79d414 to
3f0453b
Compare
|
Thanks @shimmyshimmer for picking up the latest Codex round on Synced with your commits on |
|
@codex review |
|
Codex Review: Didn't find any major issues. Breezy! 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". |
|
Reviewed and verified. Run settings were ignored on the initial model load, so the first load did not honor the configured settings. This applies the run settings on the initial load so the first load matches subsequent ones. Confirmed the settings are applied on first load and simulated the load path. Codex review has no open items. The remaining red CI is pre-existing upstream breakage unrelated to this change. Good to merge. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0346dae006
ℹ️ 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".
| useEffect(() => { | ||
| lastBlurCommittedRef.current = null; | ||
| }, [value]); |
There was a problem hiding this comment.
Clear cached blur commits when Reset changes configuration
When a user explicitly enters the currently displayed native/loaded context (for example, 32768), blurs the field, then clicks Reset, value remains 32768 even though customContextLength changes back to null. This effect therefore does not clear lastBlurCommittedRef; a later Load/Save returns that stale blur value and recreates the explicit context pin that Reset was meant to remove. Clear this cache on configuration reset/external edits independently of a displayed-value change.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch. Typing the value already shown (for example the native 32768) commits no onChange because final === value, yet the blur handler still cached it in lastBlurCommittedRef. Since value never changes, the useEffect clear never fires, so a later Reset or external edit that leaves the displayed value unchanged cannot drop it, and the next commit() replays the stale value into an override. Fixed by only caching the blur result when it actually dispatched onChange (final !== value); when final === value the parent is already current and there is nothing to bridge. Added a Playwright regression that re-types the shown context and asserts no override is stored.
…own value NumericValueInput cached every blur commit in lastBlurCommittedRef, even when the draft equalled the current value and no onChange was dispatched. Because the displayed value never changed, the useEffect([value]) clear never fired, so a later Reset or external edit that leaves the shown value unchanged could not drop the cache and the next commit() replayed it into an override that Reset had removed. Only cache the blur result when it actually dispatched onChange (final !== value); when final === value the parent is already current and there is nothing to bridge. Add a Playwright regression that re-types the shown context and asserts no override is stored.
|
@codex review |
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 27a2886bbc
ℹ️ 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".
| const committedContext = target.isGguf | ||
| ? contextInputRef.current?.commit() | ||
| : undefined; |
There was a problem hiding this comment.
Commit every numeric draft before building load config
When the user edits another NumericValueInput-backed setting and immediately clicks Load/Reload—such as non-GGUF Max Seq Length or GGUF GPU Layers/MoE Layers—the draft is still only flushed by that input's blur handler, but this path synchronously commits only the GGUF context ref before building effectiveRuntimeConfig. In the same-click path the parent config can still contain the old numeric value, so the staged /load request ignores the value the user just typed; wire the imperative commit through the other numeric settings too, or otherwise wait for their blur commits before constructing the load config.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch. The imperative same-click commit only covered the GGUF Context Length input, so typing into Max Seq Length (non-GGUF) or GPU Layers / MoE Layers on CPU (GGUF) and clicking Load/Reload in the same gesture staged the load from a stale config. Wired a commit handle through those inputs too and fold every committed draft into the effective config, recomputing the non-GGUF load-time max sequence length from the committed draft. Fixed in 6b0842f.
… config The run-settings Load/Reload button flushed only the GGUF Context Length draft imperatively before building the load config. Max Seq Length (non-GGUF), GPU Layers and MoE Layers on CPU (GGUF) are the same NumericValueInput and stage their typed value only on blur, so editing one and clicking Load in the same gesture staged the load from a still-stale parent config and dropped the value the user just typed. Wire an imperative commit handle through those inputs too and fold every committed draft into the effective config, recomputing the non-GGUF load-time max sequence length from the committed draft.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6b0842fc9c
ℹ️ 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".
| // Only bridge the still-stale parent value when the blur actually | ||
| // dispatched onChange (final !== value). When final === value the | ||
| // parent is already current, so there is nothing to bridge; caching | ||
| // here would leave a stale pin that a later Reset or external edit | ||
| // (which doesn't change the displayed value) can never clear, so a | ||
| // following Load/Save would recreate the override Reset removed. | ||
| lastBlurCommittedRef.current = final !== value ? final : null; |
There was a problem hiding this comment.
Clear the blur cache when Reset restores an unchanged value
If a different setting makes Reset enabled while this numeric field is still showing its default (for example, a GGUF with a custom KV cache dtype), edit the field and click Reset without first blurring it. The blur stores the edited value here, but Reset restores the same numeric value it had before, so the [value] effect does not run to clear the cache. The subsequent Load/Save consumes lastBlurCommittedRef and silently reintroduces the numeric override that Reset was meant to remove.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed. The prior fix only skipped caching when final === value; a real edit (final !== value) that Reset then reverts to the same shown number nets value back unchanged, so the useEffect([value]) clear never re-ran and the stale pin survived into the next Load/Save. The bridge is only valid across the single synchronous same-click gesture that set it (blur commits on the button mousedown, the button onClick consumes it via commit() before React re-renders), so the clear now runs on every settled render instead of only on a value change. Any settled render means the gesture is over. Added a source-contract regression (test_blur_cache_cleared_on_every_settled_render).
| const effectiveRuntimeConfig = hasPending | ||
| ? { ...runtimeConfig, ...pendingPatch } | ||
| : runtimeConfig; |
There was a problem hiding this comment.
Recompute the fixed-layer context pin after committing GPU Layers
When an already auto-fit GGUF has a smaller activeLoadedContext, selecting Manual, typing a positive GPU Layers value, enabling Remember, and immediately clicking Reload builds runtimeConfig before the committed layer value is merged. It therefore saves gpuLayers with customContextLength: null instead of pinning the fitted context; a later fresh load sends the native context with fixed layers, which the surrounding policy explicitly identifies as an OOM risk.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch. pinFixedLayerContext was computed from the render-time config, before the same-click GPU Layers draft is committed, so typing a positive fixed-layer value and clicking Reload built runtimeConfig with customContextLength: null. handleRun now recomputes the pin from effectiveConfig (effectivePinFixedLayerContext) after folding in the committed drafts, so a committed fixed-layer value still pins activeLoadedContext and a later fresh load keeps the fitted placement instead of sending the native context. Added a source-contract regression (test_fixed_layer_pin_recomputed_after_committing_gpu_layers).
…che on every render Two run-settings edge cases on the model-config page: 1) pinFixedLayerContext was computed from the render-time config, before a same-click GPU Layers draft is committed in handleRun. Typing a positive fixed-layer value on an auto-fit GGUF and clicking Reload therefore built the runtime config with customContextLength: null, so a later fresh load sent the native context with fixed layers (the OOM the pin exists to avoid). Recompute the pin from the committed effectiveConfig. 2) NumericValueInput cleared its blur bridge only on a value change. A real edit (final !== value) that Reset then reverts to the same shown number nets value back unchanged, so the effect never re-ran and the stale pin survived into the next Load/Save, replaying the override Reset removed. The bridge is only valid across the single synchronous same-click gesture that set it, so clear it on every settled render instead. Add source-contract regressions for both.
|
@codex review |
for more information, see https://pre-commit.ci
|
@codex review |
1 similar comment
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 45e08d1508
ℹ️ 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".
| loadCustomContextLength = | ||
| pendingLoadConfig?.customContextLength ?? null; | ||
| loadSelectedGpuIds = | ||
| pendingLoadConfig?.selectedGpuIds !== undefined | ||
| ? reconcilePersistedGpuIds(pendingLoadConfig.selectedGpuIds) | ||
| : null; | ||
| loadGpuLayers = pendingLoadConfig?.gpuLayers ?? GPU_LAYERS_AUTO; |
There was a problem hiding this comment.
Validate staged GGUF context before unloading
When switching from an already-loaded model to a GGUF through Run settings with a staged customContextLength, this branch restores that staged context only after the validation request has been built. The validation path therefore uses validateCustomContextLength = null for a cross-model switch, while the subsequent /load sends the explicit context. In particular during an active training/coexistence check, validation can pass with auto sizing, then the frontend unloads the working model and /load rejects the larger staged context before rolling back. Keep the staged context in the validation inputs when pendingLoadConfig supplies it so both requests evaluate the same load.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
A staged Run settings config can't reach validation with a null context here. onRun stages the picked config into selection.config, which flows to pendingLoadConfig. Any non-null config makes applyModelLoadConfigToRuntime return true, which sets keepSpeculative: true, so resetsPerModelSettings is false. With resetsPerModelSettings false, validateCustomContextLength takes loadCustomContextLength (the staged pendingLoadConfig.customContextLength), not null, so validation and the follow-up load size against the same context. The reset block that only re-applies the staged context later runs solely when keepSpeculative is false, and in that branch selection.config (pendingLoadConfig) is undefined, so there is no staged context to lose. The two requests already evaluate the same load.
|
@codex review |
|
Codex Review: Didn't find any major issues. Bravo. 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". |
Fixes #7346
Problem
Gear icon, set Context Length or KV Cache Dtype, then click Load model. The first load ignored those values. Context defaulted to 4096 and KV cache defaulted to f16. Users had to reload and click Apply.
Two causes:
NumericValueInputonly committed on blur, so Load could run before the typed context reached the parent state.performLoadread the runtime store instead of the stagedselection.configfrom run settings.Fix
NumericValueInput.commit()to flush valid focused and same-click drafts synchronously.effectiveLoadConfigfrommodel-config-pagetoonRun.selection.configfor context, KV cache, GPU, and speculative load options.meta.forceReloadon config-page reloads.Verification
/api/inference/loadreceivedmax_seq_length=4096on the direct button click