From 04e8beec62f8df1a6f9b6454271eda8e629970db Mon Sep 17 00:00:00 2001 From: Unsloth Date: Sun, 26 Jul 2026 17:43:30 -0700 Subject: [PATCH] studio: address second review round on per-model settings - Probe actual Vulkan devices in the GPU reconciliation helper. Vulkan ordinals are their own index space, so resolve_requested_gpu_ids only rejects malformed ones; the helper said "usable" and the load then 400d on the ggml probe it had skipped. - Send explicit save/remove intent. A save whose config is entirely default carries no fields, which is shape-identical to "forget this model", so it was wiping launch flags the UI cannot show or restore. A bare model_id without the flag still removes, keeping the original contract. - Backfill existing per-model settings into the server map once after upgrade. Without it, settings saved before this change never reached the server, so an API load used app defaults while the UI still showed the model as remembered. Never overwrites a server entry and retries until it fully succeeds. - Stop polling the monitor endpoint when auto-open is off and the panel is closed. It cannot open or display anything in that state, so every open Studio window was polling every five seconds for nothing. --- studio/backend/routes/inference.py | 23 ++++- studio/backend/routes/settings.py | 15 ++- .../backend/tests/test_openai_auto_switch.py | 99 ++++++++++++++++++- studio/frontend/src/app/routes/__root.tsx | 31 ++++-- .../api-monitor/api-monitor-overlay.tsx | 6 +- .../api/migrate-model-overrides.ts | 81 +++++++++++++++ .../model-picker/api/model-overrides.ts | 4 + .../model-config/per-model-config.ts | 34 ++++++- 8 files changed, 268 insertions(+), 25 deletions(-) create mode 100644 studio/frontend/src/features/model-picker/api/migrate-model-overrides.ts diff --git a/studio/backend/routes/inference.py b/studio/backend/routes/inference.py index beb9ebdb87..d804b3143d 100644 --- a/studio/backend/routes/inference.py +++ b/studio/backend/routes/inference.py @@ -3747,7 +3747,9 @@ async def _maybe_auto_switch_model( ) ) saved_gpu_ids = load_kwargs.get("gpu_ids") - if saved_gpu_ids and not _override_gpu_ids_still_resolve(saved_gpu_ids): + if saved_gpu_ids and not await _override_gpu_ids_still_resolve( + saved_gpu_ids + ): # A pin saved before a GPU was removed, before a # visibility-mask change, or on another host. Dropping the # one dead field beats 400ing the whole load. @@ -3977,11 +3979,13 @@ def _classify_diffusion_gguf(config: ModelConfig) -> Optional[bool]: return True if name_says_diffusion else None -def _override_gpu_ids_still_resolve(gpu_ids: List[int]) -> bool: +async def _override_gpu_ids_still_resolve(gpu_ids: List[int]) -> bool: """Whether a per-model GPU pin is usable on this machine right now. normalize_model_override cannot know the device list, so it stores whatever - was valid where the config was written. This is the load-time reconciliation. + was valid where the config was written. This is the load-time reconciliation, + and it has to make every check _resolve_gguf_gpu_ids_for_request would later + make, or the load 400s on the check this one skipped. """ try: from utils.hardware import DeviceType, get_device @@ -3991,7 +3995,18 @@ def _override_gpu_ids_still_resolve(gpu_ids: List[int]) -> bool: if get_device() == DeviceType.XPU and not is_vulkan: # gpu_ids is rejected outright on XPU. return False - resolve_requested_gpu_ids(gpu_ids, is_vulkan = is_vulkan) + resolved = resolve_requested_gpu_ids(gpu_ids, is_vulkan = is_vulkan) + if is_vulkan and resolved: + # Vulkan ordinals are their own index space, so resolve() only rejects + # malformed ones. Presence needs the same ggml probe the load does. + binary = LlamaCppBackend._find_llama_server_binary() + if binary: + probed = { + gpu[0] + for gpu in await asyncio.to_thread(LlamaCppBackend._get_gpu_memory, binary) + } + if not {int(gpu_id) for gpu_id in resolved}.issubset(probed): + return False return True except Exception: return False diff --git a/studio/backend/routes/settings.py b/studio/backend/routes/settings.py index 07a751cf61..c6f64f8f16 100644 --- a/studio/backend/routes/settings.py +++ b/studio/backend/routes/settings.py @@ -158,6 +158,10 @@ class ModelOverridePayload(BaseModel): gpu_layers: Optional[int] = Field(default = None, ge = -1, le = 1024) n_cpu_moe: Optional[int] = Field(default = None, ge = 0, le = 1024) gpu_ids: Optional[list[int]] = None + # Explicit intent. A save whose config is entirely default carries no fields + # at all, which is indistinguishable from "forget this model" by shape alone. + # None keeps the original contract: a bare model_id means remove. + remove: Optional[bool] = None @field_validator("chat_template_override") @classmethod @@ -354,11 +358,14 @@ def update_openai_auto_switch_override( # them and must not delete them). requested_extra_args = payload.llama_extra_args saved_fields = payload.model_dump( - exclude = {"model_id", "llama_extra_args"}, exclude_none = True + exclude = {"model_id", "llama_extra_args", "remove"}, exclude_none = True ) - is_removal = not payload.tensor_parallel and not { - key: value for key, value in saved_fields.items() if key != "tensor_parallel" - } + if payload.remove is not None: + is_removal = payload.remove + else: + is_removal = not payload.tensor_parallel and not { + key: value for key, value in saved_fields.items() if key != "tensor_parallel" + } if requested_extra_args is None and not is_removal: requested_extra_args = get_model_override(payload.model_id).get("llama_extra_args") if requested_extra_args is None: diff --git a/studio/backend/tests/test_openai_auto_switch.py b/studio/backend/tests/test_openai_auto_switch.py index 503604f03f..fc02f3f538 100644 --- a/studio/backend/tests/test_openai_auto_switch.py +++ b/studio/backend/tests/test_openai_auto_switch.py @@ -4091,7 +4091,11 @@ def test_stale_gpu_ids_are_dropped_not_fatal(monkeypatch): "get_model_override", lambda mid: {"gpu_ids": [0, 1], "max_seq_length": 4096}, ) - monkeypatch.setattr(inference_route, "_override_gpu_ids_still_resolve", lambda ids: False) + + async def _unusable(ids): + return False + + monkeypatch.setattr(inference_route, "_override_gpu_ids_still_resolve", _unusable) _run_hook("unsloth/B-GGUF") req = rec.calls[0] @@ -4111,7 +4115,11 @@ def test_usable_gpu_ids_are_kept(monkeypatch): recorder = rec, ) monkeypatch.setattr(settings, "get_model_override", lambda mid: {"gpu_ids": [0, 1]}) - monkeypatch.setattr(inference_route, "_override_gpu_ids_still_resolve", lambda ids: True) + + async def _usable(ids): + return True + + monkeypatch.setattr(inference_route, "_override_gpu_ids_still_resolve", _usable) _run_hook("unsloth/B-GGUF") assert rec.calls[0].gpu_ids == [0, 1] @@ -4126,4 +4134,89 @@ def test_override_gpu_ids_probe_never_raises(monkeypatch): raise RuntimeError("driver exploded") monkeypatch.setattr(hw, "resolve_requested_gpu_ids", boom) - assert inference_route._override_gpu_ids_still_resolve([0]) is False + assert asyncio.run(inference_route._override_gpu_ids_still_resolve([0])) is False + + +def test_vulkan_ordinal_absent_from_the_probe_is_unusable(monkeypatch): + # resolve_requested_gpu_ids only rejects malformed Vulkan ordinals, so + # presence needs the same ggml probe the load itself runs. Without it this + # helper says "fine" and the load 400s on the check it skipped. + from core.inference.llama_cpp import LlamaCppBackend + + monkeypatch.setattr(LlamaCppBackend, "_is_vulkan_backend", staticmethod(lambda: True)) + monkeypatch.setattr( + LlamaCppBackend, "_find_llama_server_binary", staticmethod(lambda: "/bin/llama-server") + ) + monkeypatch.setattr( + LlamaCppBackend, "_get_gpu_memory", staticmethod(lambda binary: [(0, 8192)]) + ) + assert asyncio.run(inference_route._override_gpu_ids_still_resolve([0])) is True + assert asyncio.run(inference_route._override_gpu_ids_still_resolve([7])) is False + assert asyncio.run(inference_route._override_gpu_ids_still_resolve([0, 1])) is False + + +def test_vulkan_probe_without_a_binary_does_not_block_the_load(monkeypatch): + # No binary means nothing to probe with. Refusing here would drop a valid + # pin on every load, so the later path stays the authority. + from core.inference.llama_cpp import LlamaCppBackend + + monkeypatch.setattr(LlamaCppBackend, "_is_vulkan_backend", staticmethod(lambda: True)) + monkeypatch.setattr(LlamaCppBackend, "_find_llama_server_binary", staticmethod(lambda: None)) + assert asyncio.run(inference_route._override_gpu_ids_still_resolve([0])) is True + + +def test_default_save_preserves_flags_instead_of_removing(monkeypatch): + # "Remember for this model" is on but every value is default, so the payload + # carries no fields. That is shape-identical to a removal, and guessing wrong + # wipes launch flags no UI can show or restore. + import routes.settings as settings_route + + _mock_override_store(monkeypatch) + settings.set_model_override("unsloth/B-GGUF", llama_extra_args = ["--flash-attn"]) + + resp = settings_route.update_openai_auto_switch_override( + settings_route.ModelOverridePayload(model_id = "unsloth/B-GGUF", remove = False), + "tester", + ) + assert resp.overrides["unsloth/B-GGUF"]["llama_extra_args"] == ["--flash-attn"] + + +def test_explicit_remove_still_clears_everything(monkeypatch): + import routes.settings as settings_route + + _mock_override_store(monkeypatch) + settings.set_model_override( + "unsloth/B-GGUF", llama_extra_args = ["--flash-attn"], max_seq_length = 4096 + ) + resp = settings_route.update_openai_auto_switch_override( + settings_route.ModelOverridePayload( + model_id = "unsloth/B-GGUF", remove = True, llama_extra_args = [] + ), + "tester", + ) + assert "unsloth/B-GGUF" not in resp.overrides + + +def test_bare_payload_without_remove_flag_still_removes(monkeypatch): + # The original contract, kept for any caller that predates the flag. + import routes.settings as settings_route + + _mock_override_store(monkeypatch) + settings.set_model_override("unsloth/B-GGUF", max_seq_length = 4096) + resp = settings_route.update_openai_auto_switch_override( + settings_route.ModelOverridePayload(model_id = "unsloth/B-GGUF"), "tester" + ) + assert "unsloth/B-GGUF" not in resp.overrides + + +def test_remove_false_with_real_fields_saves_normally(monkeypatch): + import routes.settings as settings_route + + _mock_override_store(monkeypatch) + resp = settings_route.update_openai_auto_switch_override( + settings_route.ModelOverridePayload( + model_id = "unsloth/B-GGUF", remove = False, max_seq_length = 8192 + ), + "tester", + ) + assert resp.overrides["unsloth/B-GGUF"]["max_seq_length"] == 8192 diff --git a/studio/frontend/src/app/routes/__root.tsx b/studio/frontend/src/app/routes/__root.tsx index 5dbeefa8f8..ff7de58a4f 100644 --- a/studio/frontend/src/app/routes/__root.tsx +++ b/studio/frontend/src/app/routes/__root.tsx @@ -3,25 +3,26 @@ import { AppSidebar } from "@/components/app-sidebar"; import { Navbar } from "@/components/navbar"; -import { fetchDeviceType, usePlatformStore } from "@/config/env"; import { SidebarInset, SidebarProvider } from "@/components/ui/sidebar"; -import { SettingsDialog, useSettingsDialogStore } from "@/features/settings"; +import { fetchDeviceType, usePlatformStore } from "@/config/env"; +import { ApiMonitorOverlay } from "@/features/api-monitor/api-monitor-overlay"; +import { hasAuthToken } from "@/features/auth"; import { ChatPage, + type ChatSearch, clearNewChatDraft, useChatRuntimeStore, - type ChatSearch, } from "@/features/chat"; -import { ApiMonitorOverlay } from "@/features/api-monitor/api-monitor-overlay"; -import { RemoteCodeConsentDialog } from "@/features/security"; -import { HfTokenWarningDialog } from "@/features/hf-auth"; -import { TransformersUpgradeDialog } from "@/features/transformers-upgrade"; -import { useTrainingUnloadGuard } from "@/features/training"; import { useExportRuntimeLifecycle } from "@/features/export"; -import { hasAuthToken } from "@/features/auth"; +import { HfTokenWarningDialog } from "@/features/hf-auth"; +import { backfillModelOverrides } from "@/features/model-picker/api/migrate-model-overrides"; import { usePersonalizationSync } from "@/features/profile"; +import { RemoteCodeConsentDialog } from "@/features/security"; +import { SettingsDialog, useSettingsDialogStore } from "@/features/settings"; +import { useTrainingUnloadGuard } from "@/features/training"; +import { TransformersUpgradeDialog } from "@/features/transformers-upgrade"; import { useSidebarPin } from "@/hooks/use-sidebar-pin"; -import { useT, type TranslationKey } from "@/i18n"; +import { type TranslationKey, useT } from "@/i18n"; import { Outlet, createRootRoute, @@ -174,6 +175,16 @@ function RootLayout() { : DEFAULT_DOCUMENT_TITLE; }, [documentTitle]); + // Settings saved before the server-side override map existed live only in this + // browser, so an API load would use app defaults while the UI still showed the + // model as remembered. Backfill once, after auth. + useEffect(() => { + if (isAuthFlowRoute) { + return; + } + void backfillModelOverrides(); + }, [isAuthFlowRoute]); + useEffect(() => { if (isAuthFlowRoute) { useSettingsDialogStore.getState().closeDialog(); diff --git a/studio/frontend/src/features/api-monitor/api-monitor-overlay.tsx b/studio/frontend/src/features/api-monitor/api-monitor-overlay.tsx index 4411e052ad..88a4915b93 100644 --- a/studio/frontend/src/features/api-monitor/api-monitor-overlay.tsx +++ b/studio/frontend/src/features/api-monitor/api-monitor-overlay.tsx @@ -111,7 +111,9 @@ export function ApiMonitorOverlay(): ReactElement | null { // One loop for both jobs: panel contents while open, traffic watch while // closed. Stands down on the full page, which polls for itself. useEffect(() => { - if (onFullPage) { + // Opted out and closed: the panel can neither open nor show anything, so + // polling would be pure background load on every open Studio window. + if (onFullPage || (!autoOpen && !isOpen)) { return; } let cancelled = false; @@ -146,7 +148,7 @@ export function ApiMonitorOverlay(): ReactElement | null { cancelled = true; if (timer !== undefined) window.clearTimeout(timer); }; - }, [isOpen, onFullPage]); + }, [isOpen, onFullPage, autoOpen]); const entries = useMemo(() => data?.entries ?? [], [data]); const stats = useMemo(() => computeStats(entries), [entries]); diff --git a/studio/frontend/src/features/model-picker/api/migrate-model-overrides.ts b/studio/frontend/src/features/model-picker/api/migrate-model-overrides.ts new file mode 100644 index 0000000000..945dc1fbdd --- /dev/null +++ b/studio/frontend/src/features/model-picker/api/migrate-model-overrides.ts @@ -0,0 +1,81 @@ +// SPDX-License-Identifier: AGPL-3.0-only +// Copyright 2026-present the Unsloth AI Inc. team. All rights reserved. See /studio/LICENSE.AGPL-3.0 + +// One-time backfill of per-model settings into the server override map. +// +// Settings used to live only in this browser, so on upgrade the server knows +// nothing about models the user already configured. Without this they keep +// showing as remembered in the UI while an API load quietly uses app defaults, +// which is the exact bug the server-side map exists to fix. + +import { + isDefaultConfig, + listPerModelConfigs, +} from "../model-config/per-model-config"; +import { + fetchModelOverrides, + modelOverrideKey, + putModelOverride, +} from "./model-overrides"; + +const DONE_FLAG = "unsloth_model_overrides_backfilled_v1"; + +function alreadyRan(): boolean { + try { + return window.localStorage.getItem(DONE_FLAG) === "1"; + } catch { + // Storage denied: treat as done rather than re-running on every mount. + return true; + } +} + +function markRan(): void { + try { + window.localStorage.setItem(DONE_FLAG, "1"); + } catch { + // Nothing to do; the backfill is idempotent anyway. + } +} + +/** + * Push local configs the server has never seen. Never deletes and never + * overwrites: an entry already on the server is the newer authority, and losing + * a setting here would be worse than leaving one unmigrated. + */ +export async function backfillModelOverrides(): Promise { + if (alreadyRan()) { + return; + } + const local = listPerModelConfigs().filter( + (entry) => !isDefaultConfig(entry.config), + ); + if (local.length === 0) { + markRan(); + return; + } + + let existing: Awaited>; + try { + existing = await fetchModelOverrides(); + } catch { + // Offline or not authenticated yet. Leave the flag unset so the next start + // tries again rather than silently skipping the migration forever. + return; + } + + let failed = false; + for (const entry of local) { + const key = modelOverrideKey(entry.modelId, entry.ggufVariant); + if (existing[key]) { + continue; + } + try { + await putModelOverride(entry.modelId, entry.ggufVariant, entry.config); + } catch { + failed = true; + } + } + if (!failed) { + markRan(); + } +} diff --git a/studio/frontend/src/features/model-picker/api/model-overrides.ts b/studio/frontend/src/features/model-picker/api/model-overrides.ts index e3a735884b..d4e89985b4 100644 --- a/studio/frontend/src/features/model-picker/api/model-overrides.ts +++ b/studio/frontend/src/features/model-picker/api/model-overrides.ts @@ -134,6 +134,10 @@ export async function putModelOverride( body: JSON.stringify({ // biome-ignore lint/style/useNamingConvention: API schema model_id: modelOverrideKey(modelId, ggufVariant), + // Say which operation this is. A save of an all-default config carries no + // fields, which is shape-identical to "forget this model", and guessing + // wrong wipes launch flags the UI cannot show or restore. + remove: config === null, // Launch flags have no UI control, so the backend preserves them when the // field is omitted. Forgetting a model means forgetting all of it, so that // path sends an explicit empty list to clear them. diff --git a/studio/frontend/src/features/model-picker/model-config/per-model-config.ts b/studio/frontend/src/features/model-picker/model-config/per-model-config.ts index ba6d4cec99..8511f3d58e 100644 --- a/studio/frontend/src/features/model-picker/model-config/per-model-config.ts +++ b/studio/frontend/src/features/model-picker/model-config/per-model-config.ts @@ -416,7 +416,10 @@ function writeMap(map: StoredMap): boolean { } } -function warnDroppedFields(raw: Record, version: number): void { +function warnDroppedFields( + raw: Record, + version: number, +): void { if (!import.meta.env?.DEV) { return; } @@ -436,7 +439,8 @@ function normalizeV1(partial: RawConfig): PerModelConfig { typeof partial.speculativeType === "string" ? canonicalizeSpeculativeType(partial.speculativeType) : null; - const speculativeType = rawSpecType ?? DEFAULT_PER_MODEL_CONFIG.speculativeType; + const speculativeType = + rawSpecType ?? DEFAULT_PER_MODEL_CONFIG.speculativeType; const specDraftNMax = speculativeType != null && MTP_SPECULATIVE_TYPES.has(speculativeType) && @@ -648,6 +652,32 @@ export function savePerModelConfig( return writeMap(map); } +/** Every saved per-model config, decoded back to the ids it was keyed by. */ +export function listPerModelConfigs(): { + modelId: string; + ggufVariant: string | null; + config: PerModelConfig; +}[] { + const out: { + modelId: string; + ggufVariant: string | null; + config: PerModelConfig; + }[] = []; + for (const [key, raw] of Object.entries(readMap())) { + const modelId = modelIdFromStorageKey(key); + if (!modelId) { + continue; + } + const variant = ggufVariantFromStorageKey(key); + out.push({ + modelId, + ggufVariant: variant ? variant : null, + config: normalize(raw), + }); + } + return out; +} + export function deletePerModelConfig( modelId: string, ggufVariant?: string | null,