From 8ffcba546cf23cfd08be0f513604382da0bf6bbd Mon Sep 17 00:00:00 2001 From: danielhanchen Date: Wed, 29 Jul 2026 07:38:23 +0000 Subject: [PATCH] Clear the derived gguf key on forget and key cached repos by repo id Three fixes. A standalone .gguf is keyed by its bare path, but a load also reads the `:LABEL` entry derived from the filename, which is how the picker keyed the same file before and what the backfill carries over from an upgraded browser. `resolve_model_override_key` cannot fold a bare path onto that spelling, so forgetting cleared a key that was never there and left the real one applying to every later API load, with the settings gone from the UI and nothing left to reach them. The remove branch now clears the derived key too, only for a bare `.gguf` path and only through the resolver, so an ambiguous fold removes nothing. `modelConfigIdentity` decided from the view kind what its own comment says depends on the row. Discover resolves a repo in an inactive HF cache into a discover-kind view whose resource is still the cache row with its snapshot-path run id, so Run from Discover read a key the Downloaded row never writes and loaded with default context, GPU and template, silently. Keyed off the resource instead. The detail view's settings button took the card's quant as given. That quant is a choice only when the user picked it in the selector; otherwise it comes off a store nothing re-reads while the window keeps focus. The card now says which it is, and a derived one defers to a fresh status read. --- studio/backend/routes/settings.py | 39 ++++++++++++ .../backend/tests/test_openai_auto_switch.py | 56 +++++++++++++++++ .../hub/catalog/local-on-device-card.tsx | 22 ++++++- studio/frontend/src/features/hub/hub-page.tsx | 60 ++++++++++++++----- tests/studio/test_model_picker_contracts.py | 32 +++++++++- 5 files changed, 190 insertions(+), 19 deletions(-) diff --git a/studio/backend/routes/settings.py b/studio/backend/routes/settings.py index d4641f7981..839f740c3f 100644 --- a/studio/backend/routes/settings.py +++ b/studio/backend/routes/settings.py @@ -399,6 +399,35 @@ def _bare_model_id(model_id: str) -> Optional[str]: return split[0] if split is not None else None +def _legacy_standalone_gguf_key(model_id: str) -> Optional[str]: + """The stored ``:LABEL`` entry for a bare standalone .gguf path, if any. + + A loose file has no quant to choose between, so it is keyed by the bare path, + but the label derived from its filename is never empty and that is how the + picker keyed the same file before, so an upgraded install carries entries + under it. The auto-switch loader reads that spelling after the bare path + misses; resolve_model_override_key does not, since folding a POSIX path only + touches an existing suffix. None for an id that already names a quant, for a + repo id, and when nothing is stored under the derived key. + """ + import os + + if not model_id.lower().endswith(".gguf"): + return None + # Already qualified, so the caller named the entry it meant. Mirrors the + # loader, which derives a label only when the resolver gave it no variant. + if _bare_model_id(model_id) is not None: + return None + from hub.utils.gguf import extract_quant_label + + label = extract_quant_label(os.path.basename(model_id)) + if not label: + return None + # Through the resolver rather than a raw lookup: the browser lowercases the + # variant, and an ambiguous fold resolves to nothing rather than guessing. + return resolve_model_override_key(f"{model_id}:{label}") + + @router.put("/openai-auto-switch/overrides", response_model = ModelOverridesResponse) def update_openai_auto_switch_override( payload: ModelOverridePayload, current_subject: str = Depends(get_current_subject) @@ -450,6 +479,16 @@ def update_openai_auto_switch_override( # casing before storing, so a stale entry would survive forgetting. target_id = resolve_model_override_key(payload.model_id) or payload.model_id set_model_override(target_id, llama_extra_args = [], max_seq_length = None) + # A standalone .gguf is keyed by its bare path now, but a load also + # reads the filename-derived :LABEL entry an upgraded install + # still holds. Clearing only what the resolver sees leaves that one + # applying to every later API load, with the settings gone from the + # UI and no way left to reach them. + legacy_id = _legacy_standalone_gguf_key(payload.model_id) + if legacy_id and legacy_id != target_id: + set_model_override( + legacy_id, llama_extra_args = [], max_seq_length = None, + ) else: # Save under the key a load resolves to, as the removal branch does. # Saving the literal id leaves two keys for one model, which makes every diff --git a/studio/backend/tests/test_openai_auto_switch.py b/studio/backend/tests/test_openai_auto_switch.py index 5fe77ef4e6..3ed4e9bb27 100644 --- a/studio/backend/tests/test_openai_auto_switch.py +++ b/studio/backend/tests/test_openai_auto_switch.py @@ -5016,6 +5016,62 @@ def test_removal_of_a_path_still_only_touches_the_exact_key(monkeypatch): assert settings.get_model_override("/models/foo.gguf")["max_seq_length"] == 8192 +def test_forget_clears_the_filename_derived_key_a_load_still_reads(monkeypatch): + # The picker keyed a standalone .gguf by the quant label from its filename + # before it settled on the bare path, and the backfill carries those entries + # over from an upgraded browser. Forget sends the bare path, which the + # resolver cannot fold onto the suffixed key, while the loader reads that + # suffixed key after the bare one misses: the settings leave the UI and go + # on being applied to every API load, unreachable. + import routes.settings as settings_route + + _mock_override_store(monkeypatch) + settings.set_model_override( + "/models/Qwen3-8B-Q4_K_M.gguf:q4_k_m", max_seq_length = 8192, + ) + settings_route.update_openai_auto_switch_override( + settings_route.ModelOverridePayload( + model_id = "/models/Qwen3-8B-Q4_K_M.gguf", remove = True, + ), + "tester", + ) + assert settings.get_model_override("/models/Qwen3-8B-Q4_K_M.gguf:Q4_K_M") == {} + assert settings.get_model_overrides() == {} + + +def test_forget_leaves_another_file_own_derived_key_alone(monkeypatch): + # The derived key is built from the forgotten file's own path and its own + # label, so a neighbour that happens to share a quant keeps its settings. + import routes.settings as settings_route + + _mock_override_store(monkeypatch) + settings.set_model_override("/models/Other-Q4_K_M.gguf:q4_k_m", max_seq_length = 4096) + settings_route.update_openai_auto_switch_override( + settings_route.ModelOverridePayload( + model_id = "/models/Qwen3-8B-Q4_K_M.gguf", remove = True, + ), + "tester", + ) + assert settings.get_model_override("/models/Other-Q4_K_M.gguf:Q4_K_M")[ + "max_seq_length" + ] == 4096 + + +def test_forget_of_a_repo_quant_key_derives_nothing(monkeypatch): + # Only a bare .gguf path derives a label. An id that already names a quant is + # the entry the caller meant, and a repo id never gets a filename read out of + # it, so neither reaches the extra removal. + import routes.settings as settings_route + + _mock_override_store(monkeypatch) + settings.set_model_override("unsloth/b-gguf:q4_k_m", max_seq_length = 8192) + settings_route.update_openai_auto_switch_override( + settings_route.ModelOverridePayload(model_id = "unsloth/B-GGUF", remove = True), + "tester", + ) + assert settings.get_model_override("unsloth/b-gguf:q4_k_m")["max_seq_length"] == 8192 + + def test_a_tag_that_names_no_quant_resolves_to_the_repo(monkeypatch): # A downloaded but unloaded GGUF asked for as org/model:latest missed the resolver, # so the switch could not load it (404ing on a quant that was never a quant with diff --git a/studio/frontend/src/features/hub/catalog/local-on-device-card.tsx b/studio/frontend/src/features/hub/catalog/local-on-device-card.tsx index 2a41152cdc..1a4bb50369 100644 --- a/studio/frontend/src/features/hub/catalog/local-on-device-card.tsx +++ b/studio/frontend/src/features/hub/catalog/local-on-device-card.tsx @@ -105,8 +105,14 @@ interface LocalOnDeviceCardProps { onEject?: () => void; onTrain?: () => void; onChange?: () => void; - /** Open settings for the quant this card is showing. */ - onOpenSettings?: (ggufVariant: string | null) => void; + /** + * Open settings for the quant this card is showing. + * + * ``quantIsUserPicked`` says whether that quant came from a pick in this + * card's selector or was derived from the resident model, which decides + * whether a fresher status read may override it. + */ + onOpenSettings?: (ggufVariant: string | null, quantIsUserPicked: boolean) => void; } function formatAdapterLabel( @@ -371,6 +377,14 @@ export function LocalOnDeviceCard({ )?.quant ?? sortedVariants?.[0]?.quant ?? null); + // Only the first branch above is a choice; the rest are fallbacks that read the + // store, and the store can be stale for as long as this window keeps focus. + const quantIsUserPicked = Boolean( + selectedVariantOverride && + sortedVariants?.some((variant) => + ggufVariantsMatch(variant.quant, selectedVariantOverride), + ), + ); const selectedVariant = sortedVariants?.find((variant) => ggufVariantsMatch(variant.quant, selectedQuant), @@ -587,7 +601,9 @@ export function LocalOnDeviceCard({ label={`Settings for ${repoId}`} // Pass the quant this card resolved, so the settings page edits the // variant on screen rather than the repo. - onClick={() => onOpenSettings(selectedQuant ?? null)} + onClick={() => + onOpenSettings(selectedQuant ?? null, quantIsUserPicked) + } /> )} {canUpdate && ( diff --git a/studio/frontend/src/features/hub/hub-page.tsx b/studio/frontend/src/features/hub/hub-page.tsx index 83032b9f02..2dd4569d37 100644 --- a/studio/frontend/src/features/hub/hub-page.tsx +++ b/studio/frontend/src/features/hub/hub-page.tsx @@ -125,11 +125,19 @@ import type { // handed: a repo cached outside the active HF cache loads by snapshot path, while // the chat picker and the auto-switch index key it by repo id, so saving under the // path strands the settings. Local rows are keyed by load id in both places. +// The row decides that, not the view it is being shown in: the same cached repo +// is reachable from Discover, where the kind is "discover" while the resource is +// still the cache row with its snapshot-path run id. Keying that off the kind +// stranded the settings for exactly the case this exists to handle. `hub_cache` +// is set only for a cached repo; a local row in the active HF cache is `hf_cache` +// and stays on its run id, as the Downloaded row keys it. function modelConfigIdentity( kind: SelectedModelView["kind"], resource: SelectedResourceRef, ): string { - if (kind !== "cache") return resource.runId; + if (kind !== "cache" && resource.source !== "hub_cache") { + return resource.runId; + } return resource.repoId ?? resource.runId; } @@ -1469,25 +1477,49 @@ export function ModelsPage() { // Opened from the detail view's on-device card, which passes in the quant it // resolved rather than making this re-derive it. const openSelectedModelSettings = useCallback( - (ggufVariant: string | null) => { + async (ggufVariant: string | null, quantIsUserPicked = false) => { if (!selectedModel) return; + // Share the sequence with openModelSettings: a pending variant lookup for + // another row must not land on top of this one. + const openSeq = ++settingsOpenSeq.current; + let variant = ggufVariant; + // The card derived this quant from the store's active variant, and nothing + // re-reads status while this window keeps focus, so an API-driven switch + // since the last focus event leaves it naming the model that switch + // displaced. A quant the user chose in the card is theirs and stands; a + // derived one defers to whatever a fresh read says is actually loaded. + if (!quantIsUserPicked) { + await refreshResidentModelStatus(); + if (settingsOpenSeq.current !== openSeq) return; + const settled = useChatRuntimeStore.getState(); + const settledCheckpoint = + settled.params.checkpoint && + !isExternalModelId(settled.params.checkpoint) + ? settled.params.checkpoint + : null; + // Every name this model answers to, as the row menu path matches them. + const aliases = [ + selectedModel.resource.runId, + selectedModel.resource.repoId, + selectedModel.resource.localPath, + ]; + if ( + settled.activeGgufVariant && + aliases.some((alias) => modelIdsMatch(alias, settledCheckpoint)) + ) { + variant = settled.activeGgufVariant; + } + } // The card passes null while its variant lookup is pending or after it failed, // so this needs the same guard openModelSettings applies: a model that needs a // quant cannot be configured without one. - if ( - !ggufVariant && - selectedModel.isGguf && - selectedModel.requiresVariant - ) { + if (!variant && selectedModel.isGguf && selectedModel.requiresVariant) { 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; } - // Share the sequence with openModelSettings: a pending variant lookup for - // another row must not land on top of this one. - settingsOpenSeq.current += 1; const id = selectedModel.resource.runId; const configId = modelConfigIdentity( selectedModel.kind, @@ -1497,8 +1529,8 @@ export function ModelsPage() { setSettingsTarget({ id, configId, - displayName: ggufVariant ? `${leaf} · ${ggufVariant}` : leaf, - ggufVariant, + displayName: variant ? `${leaf} · ${variant}` : leaf, + ggufVariant: variant, isGguf: selectedModel.isGguf, apiLoadable: selectedModel.isGguf && @@ -1506,14 +1538,14 @@ export function ModelsPage() { meta: { source: "local", isLora: selectedModel.modelFormat === "adapter", - ggufVariant: ggufVariant ?? undefined, + ggufVariant: variant ?? undefined, isGguf: selectedModel.isGguf, isDownloaded: selectedModel.isDownloaded, contextLength: null, }, }); }, - [selectedModel], + [selectedModel, refreshResidentModelStatus], ); // Whether the settings page is open on the model that is actually loaded, so it // can show the live launch config. A GGUF loaded from an inactive HF cache or diff --git a/tests/studio/test_model_picker_contracts.py b/tests/studio/test_model_picker_contracts.py index 293bd0abff..8056fd78c0 100644 --- a/tests/studio/test_model_picker_contracts.py +++ b/tests/studio/test_model_picker_contracts.py @@ -1150,7 +1150,9 @@ def test_detail_settings_need_a_resolved_quant(): bare-key fallback would apply it. openModelSettings already refuses; this entry point has to refuse the same way.""" src = " ".join(_read("features/hub/hub-page.tsx").split()) - guard = "if ( !ggufVariant && selectedModel.isGguf && selectedModel.requiresVariant )" + # `variant`, not the argument: a derived quant may have been replaced by the + # resident one first, and the guard has to judge what will actually be saved. + guard = "if (!variant && selectedModel.isGguf && selectedModel.requiresVariant) {" assert guard in src assert src.count("Couldn't determine which quant to configure.") == 2 @@ -1204,7 +1206,7 @@ def test_cached_repo_settings_are_keyed_by_the_repo_id(): assert "model_path: target.id," in config_page hub = " ".join(_read("features/hub/hub-page.tsx").split()) - assert 'if (kind !== "cache") return resource.runId;' in hub + assert 'if (kind !== "cache" && resource.source !== "hub_cache") {' in hub assert "return resource.repoId ?? resource.runId;" in hub # Both openers and the Hub's own load resolve through it. assert hub.count("modelConfigIdentity(") == 3 @@ -1555,3 +1557,29 @@ def test_settings_open_reads_status_before_resolving_the_quant(): assert "const refreshResidentModelStatus = useCallback((): Promise => {" in page assert "const [res] = await Promise.all([ listGgufVariants(" in page assert "refreshResidentModelStatus(), ]);" in page + + +def test_cached_repo_settings_key_follows_the_row_not_the_view(): + """A repo in an inactive HF cache loads by snapshot path while its settings + are keyed by repo id. The same row is reachable from Discover, where the view + kind is "discover" and only the resource says it is a cache row, so keying + off the kind stranded the settings for exactly the case the helper exists to + handle: Run from Discover looked up the snapshot path, found nothing, and + loaded with default context, GPU and template.""" + page = " ".join(_read("features/hub/hub-page.tsx").split()) + assert ( + 'if (kind !== "cache" && resource.source !== "hub_cache") {' in page + ) + + +def test_detail_settings_defers_a_derived_quant_to_a_fresh_status_read(): + """The on-device card resolves the quant it shows from the store's active + variant, and nothing re-reads status while the window keeps focus, so an + API-driven switch leaves that quant naming the model it displaced. A quant + the user picked in the card's selector is a choice and must survive; only a + derived one defers to the read.""" + page = " ".join(_read("features/hub/hub-page.tsx").split()) + card = " ".join(_read("features/hub/catalog/local-on-device-card.tsx").split()) + assert "if (!quantIsUserPicked) { await refreshResidentModelStatus();" in page + assert "variant = settled.activeGgufVariant;" in page + assert "onOpenSettings(selectedQuant ?? null, quantIsUserPicked)" in card