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