From 8f322aa627d191b55cbbc2a57bed539b538b4b94 Mon Sep 17 00:00:00 2001 From: Unsloth Date: Sun, 26 Jul 2026 18:32:50 -0700 Subject: [PATCH] studio: address third review round on per-model settings - Fall back to a case-insensitive override lookup. The browser normalizes ids to lowercase before storing, so the backfill wrote "org/model:q4_k_m" while the resolver asked for "Org/Model:Q4_K_M" and never matched, which made the whole migration a no-op. Exact match still wins and an ambiguous fallback matches nothing, so two POSIX paths differing only in case stay distinct. - Record a detail revision only when a fetch actually started. requestDetail's in-flight guard can refuse, and recording anyway meant a revision that landed during an earlier fetch was skipped for good once updated_at stopped moving, leaving a terminal request showing a stale running payload. - Discard superseded settings opens. The GGUF variant lookup is async, so opening a second row while the first was pending let whichever finished last win, and the page could then save or load settings for the wrong model. - Raise the model_id cap to PATH_MAX plus a quant suffix. A local model's id is its filesystem path and LoadRequest.model_path is unbounded, so the old 512 limit 422d the server sync while the local save succeeded, leaving the UI showing settings the API would never apply. --- studio/backend/routes/settings.py | 18 ++++++++----- .../backend/tests/test_openai_auto_switch.py | 27 +++++++++++++++++++ .../utils/openai_auto_switch_settings.py | 25 ++++++++++++++--- .../features/api-monitor/api-monitor-page.tsx | 7 +++-- .../features/api-monitor/use-api-monitor.ts | 10 ++++--- studio/frontend/src/features/hub/hub-page.tsx | 9 +++++++ 6 files changed, 82 insertions(+), 14 deletions(-) diff --git a/studio/backend/routes/settings.py b/studio/backend/routes/settings.py index c6f64f8f16..672d9941cc 100644 --- a/studio/backend/routes/settings.py +++ b/studio/backend/routes/settings.py @@ -126,6 +126,17 @@ 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. +_MAX_VARIANT_SUFFIX_LEN = 64 + +# A local model's id is its filesystem path, optionally with a quant suffix, and +# LoadRequest.model_path is unbounded. A limit under PATH_MAX would 422 the server +# sync while the local save succeeded, leaving the UI showing settings the API +# never applies. +MAX_MODEL_OVERRIDE_KEY_LEN = 4096 + 1 + _MAX_VARIANT_SUFFIX_LEN + + class ModelOverridePayload(BaseModel): """One model's saved launch config, applied when the API loads that model. @@ -136,7 +147,7 @@ class ModelOverridePayload(BaseModel): mode) are left to it, since their valid sets follow the llama.cpp build. """ - model_id: str = Field(..., min_length = 1, max_length = 512) + model_id: str = Field(..., min_length = 1, max_length = MAX_MODEL_OVERRIDE_KEY_LEN) # None means "leave the stored value alone": the settings UI has no control # for launch flags, so a save from it must not wipe flags set through this # API. An explicit [] clears them (that is how "forget this model" arrives). @@ -329,11 +340,6 @@ def get_openai_auto_switch_overrides( return ModelOverridesResponse(overrides = get_model_overrides()) -# 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. -_MAX_VARIANT_SUFFIX_LEN = 64 - - def _bare_model_id(model_id: str) -> Optional[str]: """``repo`` for a ``repo:QUANT`` key, or None when there is no quant suffix.""" head, sep, tail = model_id.rpartition(":") diff --git a/studio/backend/tests/test_openai_auto_switch.py b/studio/backend/tests/test_openai_auto_switch.py index fc02f3f538..aa20752aee 100644 --- a/studio/backend/tests/test_openai_auto_switch.py +++ b/studio/backend/tests/test_openai_auto_switch.py @@ -4220,3 +4220,30 @@ def test_remove_false_with_real_fields_saves_normally(monkeypatch): "tester", ) assert resp.overrides["unsloth/B-GGUF"]["max_seq_length"] == 8192 + + +def test_override_lookup_falls_back_to_case_insensitive(monkeypatch): + # The browser lowercases ids before storing them, so the backfill writes + # "unsloth/qwen3-8b-gguf:q4_k_m" while the resolver asks for the repo's real + # casing. Without this fallback every migrated entry is invisible. + _mock_override_store(monkeypatch) + settings.set_model_override("unsloth/qwen3-8b-gguf:q4_k_m", max_seq_length = 8192) + got = settings.get_model_override("unsloth/Qwen3-8B-GGUF:Q4_K_M") + assert got["max_seq_length"] == 8192 + + +def test_exact_override_match_beats_a_case_variant(monkeypatch): + _mock_override_store(monkeypatch) + settings.set_model_override("/models/foo.gguf", max_seq_length = 1024) + settings.set_model_override("/models/Foo.gguf", max_seq_length = 8192) + assert settings.get_model_override("/models/Foo.gguf")["max_seq_length"] == 8192 + assert settings.get_model_override("/models/foo.gguf")["max_seq_length"] == 1024 + + +def test_ambiguous_case_fallback_matches_nothing(monkeypatch): + # Two POSIX paths differing only in case are two different files. Guessing + # between them would apply one model's settings to another. + _mock_override_store(monkeypatch) + settings.set_model_override("/models/foo.gguf", max_seq_length = 1024) + settings.set_model_override("/models/FOO.gguf", max_seq_length = 8192) + assert settings.get_model_override("/models/Foo.gguf") == {} diff --git a/studio/backend/utils/openai_auto_switch_settings.py b/studio/backend/utils/openai_auto_switch_settings.py index ba63c47212..ca2a7c5d10 100644 --- a/studio/backend/utils/openai_auto_switch_settings.py +++ b/studio/backend/utils/openai_auto_switch_settings.py @@ -433,9 +433,28 @@ def get_model_overrides() -> dict[str, dict]: def get_model_override(model_id: str) -> dict: - """The launch override applied when auto-switch loads ``model_id`` (or empty).""" - override = get_model_overrides().get(model_id) - return override if isinstance(override, dict) else {} + """The launch override applied when auto-switch loads ``model_id`` (or empty). + + Falls back to a case-insensitive match when nothing matches exactly. Repo ids + and quants are case-insensitive in practice ("Q4_K_M" and "q4_k_m" name one + file), and the browser normalizes them to lowercase before storing, so an + exact-only lookup misses entries written from that side. Exact still wins, and + an ambiguous fallback matches nothing, so two POSIX paths differing only in + case stay distinct. + """ + overrides = get_model_overrides() + override = overrides.get(model_id) + if isinstance(override, dict): + return override + if not isinstance(model_id, str): + return {} + folded = model_id.casefold() + matches = [ + value + for key, value in overrides.items() + if isinstance(key, str) and key.casefold() == folded and isinstance(value, dict) + ] + return matches[0] if len(matches) == 1 else {} def set_model_override( 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 50b27672c5..ad84c77601 100644 --- a/studio/frontend/src/features/api-monitor/api-monitor-page.tsx +++ b/studio/frontend/src/features/api-monitor/api-monitor-page.tsx @@ -496,8 +496,11 @@ export function ApiMonitorPage(): ReactElement { if (!selectedIsMissing && lastFetchedRef.current === revision) { return; } - lastFetchedRef.current = revision; - requestDetail(selectedId_); + // Only remember the revision when a fetch really started; the in-flight + // guard can refuse, and recording it anyway skips that revision for good. + if (requestDetail(selectedId_)) { + lastFetchedRef.current = revision; + } }, [selectedId_, selectedUpdatedAt, selectedIsMissing, requestDetail]); // The desktop webview's origin is tauri://, not the API server, and the diff --git a/studio/frontend/src/features/api-monitor/use-api-monitor.ts b/studio/frontend/src/features/api-monitor/use-api-monitor.ts index 84be9e3084..3a40990120 100644 --- a/studio/frontend/src/features/api-monitor/use-api-monitor.ts +++ b/studio/frontend/src/features/api-monitor/use-api-monitor.ts @@ -168,7 +168,7 @@ interface UseApiMonitorResult { /** Full prompt/reply for entries the user expanded, keyed by entry id. */ details: Record; loadingDetails: ReadonlySet; - requestDetail: (id: string) => void; + requestDetail: (id: string) => boolean; } /** @@ -247,9 +247,12 @@ export function useApiMonitor({ }; }, [paused, intervalMs]); - const requestDetail = useCallback((id: string): void => { + // Returns whether a fetch actually started. Callers that remember "I have + // fetched revision N" must not record it when the in-flight guard turned them + // away, or that revision is skipped for good once updated_at stops moving. + const requestDetail = useCallback((id: string): boolean => { if (inFlightDetails.current.has(id)) { - return; + return false; } inFlightDetails.current.add(id); setLoadingDetails((prev) => new Set(prev).add(id)); @@ -275,6 +278,7 @@ export function useApiMonitor({ return next; }); }); + return true; }, []); const clear = useCallback(async (): Promise => { diff --git a/studio/frontend/src/features/hub/hub-page.tsx b/studio/frontend/src/features/hub/hub-page.tsx index 8175774689..50e35a690b 100644 --- a/studio/frontend/src/features/hub/hub-page.tsx +++ b/studio/frontend/src/features/hub/hub-page.tsx @@ -1226,8 +1226,12 @@ export function ModelsPage() { const [settingsTarget, setSettingsTarget] = useState( null, ); + // Bumped per open so a slow variant lookup for a row the user has moved on + // from cannot land on top of the row they actually chose. + const settingsOpenSeq = useRef(0); const openModelSettings = useCallback( async (row: CachedInventoryRow | LocalInventoryRow) => { + const openSeq = ++settingsOpenSeq.current; // loadId is what the loader accepts; repoId is only a display/API alias. const id = row.loadId; // Whether the loaded model is this row, under any of the names it goes by. @@ -1278,6 +1282,11 @@ export function ModelsPage() { } } } + // The variant lookup above is async, so a second row opened while it was + // pending would otherwise be overwritten by whichever call finished last. + if (settingsOpenSeq.current !== openSeq) { + return; + } const leaf = id.split(/[\\/]/).filter(Boolean).pop() ?? id; setSettingsTarget({ id,