From 9c722b9059aa9cd1edc6a7907c85e777b77d9998 Mon Sep 17 00:00:00 2001 From: Unsloth Date: Sun, 26 Jul 2026 23:15:51 -0700 Subject: [PATCH] studio: address fourth review round on per-model settings - Never case-fold a filesystem path when looking up an override. Two POSIX paths differing only in case are two different files, so a near miss must load defaults rather than replay another model's context and GPU pin. Repo ids still fold, which is what the migration needs. - Match a quant suffix against the loader's own quant pattern instead of a length heuristic. "/models/foo:bar.gguf" is one valid POSIX filename, and splitting it grafted /models/foo's launch flags onto an unrelated model. - Retry a load once without the saved gpu_ids when the loader rejects the pin. The pre-flight check cannot mirror every rule the loader applies (a Vulkan diffusion GGUF refuses GPU selection outright, and the rules move), so this stops chasing them one at a time: a stale placement preference must never be the reason a request cannot be served. - Make an explicit remove win over config fields sent in the same payload. - Seed only finished requests on the monitor's first snapshot. A request still running when Studio loads is traffic the user has not seen, not history. - Nudge the detail effect when the in-flight guard refuses a fetch. Nothing else changes its deps when the older fetch settles, so a terminal reply could stay truncated forever. - Invalidate pending row lookups when opening settings from a detail card, not just from a row. - Mark the covered detail pane inert so it leaves the focus order. - Refuse to open settings for a variant-required GGUF whose quant could not be resolved: the picker matches variants exactly and would never find the saved config, while the API falls back to the bare key and would apply it. - Mirror to the server only for GGUFs. The auto-switch resolver indexes GGUFs, so a safetensors config was being advertised as applied on API load when no API request could ever apply it. --- studio/backend/routes/inference.py | 39 ++++- studio/backend/routes/settings.py | 48 +++--- .../backend/tests/test_openai_auto_switch.py | 140 ++++++++++++++++++ .../utils/openai_auto_switch_settings.py | 13 ++ .../api-monitor/api-monitor-overlay.tsx | 13 +- .../features/api-monitor/api-monitor-page.tsx | 16 +- studio/frontend/src/features/hub/hub-page.tsx | 27 +++- .../api/migrate-model-overrides.ts | 5 +- .../components/model-config-page.tsx | 5 +- 9 files changed, 274 insertions(+), 32 deletions(-) diff --git a/studio/backend/routes/inference.py b/studio/backend/routes/inference.py index fd1d3cc1cb..db593c4bf0 100644 --- a/studio/backend/routes/inference.py +++ b/studio/backend/routes/inference.py @@ -3779,12 +3779,39 @@ async def _maybe_auto_switch_model( # Reuse the load impl so its dedup, tensor fallback, and threading # apply. Call the impl directly: we already hold the lifecycle gate # the /load route would otherwise take, so the route would deadlock. - await _load_model_impl( - LoadRequest(**load_kwargs), - fastapi_request, - current_subject, - current_request_counted = True, - ) + try: + await _load_model_impl( + LoadRequest(**load_kwargs), + fastapi_request, + current_subject, + current_request_counted = True, + ) + except HTTPException as exc: + # The pre-flight check above cannot mirror every rule the + # loader applies to gpu_ids (a Vulkan diffusion GGUF refuses + # GPU selection outright, and the rules move). Rather than + # duplicating them, retry once without the saved pin: a + # stale placement preference must never be the reason an + # API request cannot be served. + if not ( + exc.status_code == 400 + and load_kwargs.get("gpu_ids") + and "gpu" in str(exc.detail).lower() + ): + raise + logger.warning( + "Retrying %s without saved gpu_ids %s: %s", + override_id, + load_kwargs.get("gpu_ids"), + exc.detail, + ) + load_kwargs.pop("gpu_ids", None) + await _load_model_impl( + LoadRequest(**load_kwargs), + fastapi_request, + current_subject, + current_request_counted = True, + ) # Advertise the repo id (not the concrete load path) as the loaded # model's public id and override key for /v1/models and idle stash. get_llama_cpp_backend()._openai_advertised_id = override_id diff --git a/studio/backend/routes/settings.py b/studio/backend/routes/settings.py index 672d9941cc..f3b40bdcdb 100644 --- a/studio/backend/routes/settings.py +++ b/studio/backend/routes/settings.py @@ -126,8 +126,10 @@ class OpenAIAutoSwitchResponse(BaseModel): auto_unload_keep_kv: bool = DEFAULT_AUTO_UNLOAD_KEEP_KV -# A quant suffix, as modelOverrideKey builds it: no path separator and short. -# Guards against splitting "C:\\models\\x.gguf", where the colon is a drive letter. +# A quant suffix, as modelOverrideKey builds it. Matched against the loader's own +# quant pattern rather than a length heuristic: a POSIX path may legitimately +# contain a colon ("/models/foo:bar.gguf"), and treating "bar.gguf" as a quant +# would graft an unrelated model's launch flags onto this one. _MAX_VARIANT_SUFFIX_LEN = 64 # A local model's id is its filesystem path, optionally with a quant suffix, and @@ -342,11 +344,16 @@ def get_openai_auto_switch_overrides( def _bare_model_id(model_id: str) -> Optional[str]: """``repo`` for a ``repo:QUANT`` key, or None when there is no quant suffix.""" + from core.inference.llama_cpp import _GGUF_KNOWN_QUANT_RE + head, sep, tail = model_id.rpartition(":") if not sep or not head or not tail: return None if len(tail) > _MAX_VARIANT_SUFFIX_LEN or "/" in tail or "\\" in tail: return None + # Must actually look like a quant, not just like a short path segment. + if _GGUF_KNOWN_QUANT_RE.fullmatch(tail) is None: + return None return head @@ -382,21 +389,28 @@ def update_openai_auto_switch_override( if bare_id: requested_extra_args = get_model_override(bare_id).get("llama_extra_args") extra_args = validate_extra_args(requested_extra_args) - set_model_override( - payload.model_id, - llama_extra_args = extra_args, - max_seq_length = payload.max_seq_length, - custom_context_length = payload.custom_context_length, - kv_cache_dtype = payload.kv_cache_dtype, - speculative_type = payload.speculative_type, - spec_draft_n_max = payload.spec_draft_n_max, - tensor_parallel = payload.tensor_parallel, - chat_template_override = payload.chat_template_override, - gpu_memory_mode = payload.gpu_memory_mode, - gpu_layers = payload.gpu_layers, - n_cpu_moe = payload.n_cpu_moe, - gpu_ids = payload.gpu_ids, - ) + if payload.remove is True: + # An explicit remove wins over anything else in the payload: a stale + # form field must not turn "forget this model" into an update that + # keeps it. Only the explicit flag short-circuits; the legacy + # inferred path still just gates launch-flag carry-over. + set_model_override(payload.model_id, llama_extra_args = [], max_seq_length = None) + else: + set_model_override( + payload.model_id, + llama_extra_args = extra_args, + max_seq_length = payload.max_seq_length, + custom_context_length = payload.custom_context_length, + kv_cache_dtype = payload.kv_cache_dtype, + speculative_type = payload.speculative_type, + spec_draft_n_max = payload.spec_draft_n_max, + tensor_parallel = payload.tensor_parallel, + chat_template_override = payload.chat_template_override, + gpu_memory_mode = payload.gpu_memory_mode, + gpu_layers = payload.gpu_layers, + n_cpu_moe = payload.n_cpu_moe, + gpu_ids = payload.gpu_ids, + ) except ValueError as exc: raise log_and_http_error( exc, diff --git a/studio/backend/tests/test_openai_auto_switch.py b/studio/backend/tests/test_openai_auto_switch.py index b66781375e..bfd1754301 100644 --- a/studio/backend/tests/test_openai_auto_switch.py +++ b/studio/backend/tests/test_openai_auto_switch.py @@ -4264,3 +4264,143 @@ def test_request_used_api_key_distinguishes_key_from_session(): # A malformed request object must read as "not an API key", never raise, since # this runs on the hot path of every tracked request. assert inference_route._request_used_api_key(object()) is False + + +def test_case_fallback_never_applies_to_a_posix_path(monkeypatch): + # Two files that differ only in case are two different models on Linux, so a + # near miss must load defaults rather than another model's context and GPU pin. + _mock_override_store(monkeypatch) + settings.set_model_override("/models/foo.gguf", max_seq_length = 8192, gpu_ids = [1]) + assert settings.get_model_override("/models/Foo.gguf") == {} + assert settings.get_model_override("/models/foo.gguf")["max_seq_length"] == 8192 + + +def test_case_fallback_never_applies_to_a_windows_path(monkeypatch): + _mock_override_store(monkeypatch) + settings.set_model_override(r"C:\models\foo.gguf", max_seq_length = 8192) + assert settings.get_model_override(r"C:\models\FOO.gguf") == {} + + +def test_case_fallback_still_covers_repo_ids(monkeypatch): + # The migration case this fallback exists for. + _mock_override_store(monkeypatch) + settings.set_model_override("unsloth/qwen3-8b-gguf:q4_k_m", max_seq_length = 8192) + assert settings.get_model_override("unsloth/Qwen3-8B-GGUF:Q4_K_M")["max_seq_length"] == 8192 + + +def test_explicit_remove_wins_over_config_fields_in_the_same_payload(monkeypatch): + # remove is the operation discriminator, so a stale form field alongside it + # must not quietly turn "forget this model" into an update. + 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", remove = True, max_seq_length = 8192, tensor_parallel = True + ), + "tester", + ) + assert "unsloth/B-GGUF" not in resp.overrides + + +def test_posix_colon_in_a_path_is_not_treated_as_a_quant(monkeypatch): + # "/models/foo:bar.gguf" is one valid POSIX filename, not repo + quant. + # Splitting it would graft /models/foo's launch flags onto a different model. + import routes.settings as settings_route + + _mock_override_store(monkeypatch) + settings.set_model_override("/models/foo", llama_extra_args = ["--flash-attn"]) + resp = settings_route.update_openai_auto_switch_override( + settings_route.ModelOverridePayload(model_id = "/models/foo:bar.gguf", max_seq_length = 4096), + "tester", + ) + assert "llama_extra_args" not in resp.overrides["/models/foo:bar.gguf"] + + +def test_real_quant_suffix_on_a_path_still_carries_flags_over(monkeypatch): + import routes.settings as settings_route + + _mock_override_store(monkeypatch) + settings.set_model_override("/models/x.gguf", llama_extra_args = ["--flash-attn"]) + resp = settings_route.update_openai_auto_switch_override( + settings_route.ModelOverridePayload(model_id = "/models/x.gguf:Q4_K_M", max_seq_length = 4096), + "tester", + ) + assert resp.overrides["/models/x.gguf:Q4_K_M"]["llama_extra_args"] == ["--flash-attn"] + + +def test_load_retries_without_gpu_ids_when_the_loader_rejects_the_pin(monkeypatch): + # The pre-flight check cannot mirror every rule the loader applies (a Vulkan + # diffusion GGUF refuses GPU selection outright). A stale placement preference + # must never be the reason a request cannot be served. + from fastapi import HTTPException + + backend = _FakeBackend(None) + rec = _LoadRecorder(backend) + _wire( + monkeypatch, + enabled = True, + resolves_to = ("unsloth/B-GGUF", "Q4_K_M", "unsloth/B-GGUF"), + backend = backend, + recorder = rec, + ) + monkeypatch.setattr( + settings, "get_model_override", lambda mid: {"gpu_ids": [0], "max_seq_length": 4096} + ) + + async def _usable(ids): + return True + + monkeypatch.setattr(inference_route, "_override_gpu_ids_still_resolve", _usable) + + calls = {"n": 0} + + async def _load(request, *args, **kwargs): + calls["n"] += 1 + if calls["n"] == 1: + raise HTTPException( + status_code = 400, + detail = "GPU selection (gpu_ids) is not supported for a DiffusionGemma GGUF", + ) + return await rec(request, *args, **kwargs) + + monkeypatch.setattr(inference_route, "_load_model_impl", _load) + + _run_hook("unsloth/B-GGUF") + assert calls["n"] == 2 + served = rec.calls[-1] + assert not served.gpu_ids + assert served.max_seq_length == 4096 + + +def test_a_non_gpu_load_failure_is_not_retried(monkeypatch): + from fastapi import HTTPException + + backend = _FakeBackend(None) + rec = _LoadRecorder(backend) + _wire( + monkeypatch, + enabled = True, + resolves_to = ("unsloth/B-GGUF", "Q4_K_M", "unsloth/B-GGUF"), + backend = backend, + recorder = rec, + ) + monkeypatch.setattr(settings, "get_model_override", lambda mid: {"gpu_ids": [0]}) + + async def _usable(ids): + return True + + monkeypatch.setattr(inference_route, "_override_gpu_ids_still_resolve", _usable) + + calls = {"n": 0} + + async def _load(request, *args, **kwargs): + calls["n"] += 1 + raise HTTPException(status_code = 400, detail = "Corrupt GGUF header") + + monkeypatch.setattr(inference_route, "_load_model_impl", _load) + + with pytest.raises(HTTPException): + _run_hook("unsloth/B-GGUF") + assert calls["n"] == 1 diff --git a/studio/backend/utils/openai_auto_switch_settings.py b/studio/backend/utils/openai_auto_switch_settings.py index ca2a7c5d10..830d3cd3f7 100644 --- a/studio/backend/utils/openai_auto_switch_settings.py +++ b/studio/backend/utils/openai_auto_switch_settings.py @@ -426,6 +426,14 @@ def model_override_load_kwargs(override: dict[str, Any], *, is_gguf: bool) -> di return kwargs +def _looks_like_filesystem_path(model_id: str) -> bool: + """True for an absolute path id, as the ./models and LM Studio scanners emit.""" + if model_id.startswith(("/", "\\")): + return True + # Windows drive letter, e.g. "C:\models\x.gguf". + return len(model_id) >= 3 and model_id[1] == ":" and model_id[2] in ("\\", "/") + + def get_model_overrides() -> dict[str, dict]: """Per-model launch configs keyed by model id (see normalize_model_override).""" raw = _cached_setting(MODEL_OVERRIDES_SETTING_KEY, None) @@ -448,6 +456,11 @@ def get_model_override(model_id: str) -> dict: return override if not isinstance(model_id, str): return {} + # Only repo-style ids fold. A POSIX path is case-sensitive and names a + # different file, so matching "/models/Foo.gguf" against an entry saved for + # "/models/foo.gguf" would replay another model's context and GPU pin. + if _looks_like_filesystem_path(model_id): + return {} folded = model_id.casefold() matches = [ value 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 377694d7f5..22128411c0 100644 --- a/studio/frontend/src/features/api-monitor/api-monitor-overlay.tsx +++ b/studio/frontend/src/features/api-monitor/api-monitor-overlay.tsx @@ -168,8 +168,17 @@ export function ApiMonitorOverlay(): ReactElement | null { const ids = data.entries.map((entry) => entry.id); if (!seededRef.current) { seededRef.current = true; - seenIdsRef.current = new Set(ids); - return; + // Seed finished requests only. A request that is still running when the + // first snapshot lands started while Studio was loading, so it is live + // traffic the user has not seen, not history to adopt silently. + seenIdsRef.current = new Set( + data.entries + .filter((entry) => entry.status !== "running") + .map((entry) => entry.id), + ); + if (!data.entries.some((e) => e.via_api_key && e.status === "running")) { + return; + } } const seen = seenIdsRef.current; // Only API-key traffic counts. Studio's own chat goes through these same diff --git a/studio/frontend/src/features/api-monitor/api-monitor-page.tsx b/studio/frontend/src/features/api-monitor/api-monitor-page.tsx index ad84c77601..30d9aad545 100644 --- a/studio/frontend/src/features/api-monitor/api-monitor-page.tsx +++ b/studio/frontend/src/features/api-monitor/api-monitor-page.tsx @@ -486,6 +486,7 @@ export function ApiMonitorPage(): ReactElement { const selectedUpdatedAt = selected?.updated_at ?? null; const selectedIsMissing = selectedId_ != null && details[selectedId_] == null; const lastFetchedRef = useRef(null); + const [retryTick, setRetryTick] = useState(0); useEffect(() => { if (selectedId_ == null) { return; @@ -500,8 +501,21 @@ export function ApiMonitorPage(): ReactElement { // guard can refuse, and recording it anyway skips that revision for good. if (requestDetail(selectedId_)) { lastFetchedRef.current = revision; + setRetryTick(0); + } else { + // Refused because an older fetch is still running. Nothing in this effect's + // deps will change when that one settles, so without a nudge a revision + // rejected here is never fetched, and a terminal reply stays truncated. + const timer = window.setTimeout(() => setRetryTick((n) => n + 1), 250); + return () => window.clearTimeout(timer); } - }, [selectedId_, selectedUpdatedAt, selectedIsMissing, requestDetail]); + }, [ + selectedId_, + selectedUpdatedAt, + selectedIsMissing, + requestDetail, + retryTick, + ]); // The desktop webview's origin is tauri://, not the API server, and the // packaged app picks its port dynamically. Same source as the Agents tab. diff --git a/studio/frontend/src/features/hub/hub-page.tsx b/studio/frontend/src/features/hub/hub-page.tsx index 50e35a690b..1fb98d3d58 100644 --- a/studio/frontend/src/features/hub/hub-page.tsx +++ b/studio/frontend/src/features/hub/hub-page.tsx @@ -23,6 +23,7 @@ import { } from "@/features/model-picker"; import { useDebouncedValue } from "@/hooks/use-debounced-value"; import { useGpuInfo } from "@/hooks/use-gpu-info"; +import { toast } from "@/lib/toast"; import { cn } from "@/lib/utils"; import { useNavigate, useSearch } from "@tanstack/react-router"; import { @@ -1276,11 +1277,20 @@ export function ModelsPage() { downloaded[0]?.quant ?? null; } catch { - // Offline or an unreadable cache: fall through with no variant. The - // settings page still works, it just cannot pin a specific quant. ggufVariant = null; } } + if (!ggufVariant) { + // A model that needs a quant cannot be configured without one: the + // picker matches variants exactly and would never find the saved + // config, while the API falls back to the bare key and would apply it. + // Opening the editor here would quietly create that mismatch. + toast.error("Couldn't determine which quant to configure.", { + description: + "Settings for this model are per quant. Check the connection or the model's cache, then try again.", + }); + return; + } } // The variant lookup above is async, so a second row opened while it was // pending would otherwise be overwritten by whichever call finished last. @@ -1349,6 +1359,9 @@ export function ModelsPage() { const openSelectedModelSettings = useCallback( (ggufVariant: string | null) => { if (!selectedModel) return; + // Share the sequence with openModelSettings: a row's variant lookup may + // still be pending, and it must not land on top of this one. + settingsOpenSeq.current += 1; const id = selectedModel.resource.runId; const leaf = id.split(/[\\/]/).filter(Boolean).pop() ?? id; setSettingsTarget({ @@ -1751,7 +1764,10 @@ export function ModelsPage() { {splitMode ? ( detailOpen ? ( -
+
+
{ return; } const local = listPerModelConfigs().filter( - (entry) => !isDefaultConfig(entry.config), + // A quant means it is a GGUF, which is the only thing API auto-switch + // resolves. Backfilling a safetensors config would claim an API behaviour + // that does not exist. + (entry) => entry.ggufVariant != null && !isDefaultConfig(entry.config), ); if (local.length === 0) { markRan(); diff --git a/studio/frontend/src/features/model-picker/components/model-config-page.tsx b/studio/frontend/src/features/model-picker/components/model-config-page.tsx index edf0cb96ea..91e71d8272 100644 --- a/studio/frontend/src/features/model-picker/components/model-config-page.tsx +++ b/studio/frontend/src/features/model-picker/components/model-config-page.tsx @@ -886,7 +886,10 @@ export function ModelConfigPage({ // Skipped when the local write failed (quota, a future-schema entry): the // browser and the server would otherwise permanently disagree about this // model, with no way for the user to tell which one the next load used. - if (!saveFailed) { + // GGUF only: the API auto-switch resolver indexes GGUFs, so mirroring a + // safetensors config to the server would advertise settings on the monitor's + // "applied on API load" list that no API request can ever apply. + if (!saveFailed && target.isGguf) { syncModelOverride( target.id, target.ggufVariant,