From d8b4193b9d70d179d7004aeac76d0366f7bde68f Mon Sep 17 00:00:00 2001 From: danielhanchen Date: Tue, 28 Jul 2026 20:23:57 +0000 Subject: [PATCH] Key one config per model, and stop the backfill replacing newer server state Five follow-ups on the per-model settings map. The one-time localStorage backfill read the override map once and then wrote each model in turn, so a save by another tab during that pass was replaced by this browser's older copy, against the migration's own "never overwrites" contract. Re-fetching per model would cost a round trip each; instead the PUT takes only_if_absent and the server tests and writes under one transaction. gpu_ids arrived unbounded and normalize_model_override de-duplicated it by scanning the list it was building, so a large authenticated array cost roughly 20x what the same work costs with a set (4.5s against 0.27s for a million entries). The payload now bounds the field to the number of ids the normalizer can store, and the dedupe uses a set. A settings target opened from the Chat model picker carried no apiLoadable, so the isGguf fallback mirrored an Ollama GGUF to the server. Ollama's blobs reach that picker as custom-folder GGUFs under a .studio_links / ollama_links dir, which local_model_resolver refuses to index, so the mirror advertised a load the API can never make. The picker, the sidebar editor and the backfill now all use the same classification. The Hub settings page compared the loaded model to settingsTarget.id, but a GGUF loaded from an inactive HF cache or straight off disk loads by path while /status reports the clean public id, so the page ignored the live launch config and showed saved or default values. It now also matches the settings identity and the public id the backend would report. A standalone .gguf gets a filename-derived format_variant from the inventory, so the Hub row menu stored its settings under :Q4_K_M while the Chat picker, the detail card and the backfill all used the bare path. The row menu now uses the bare path too. Tests: publicModelId / residentModelIdMatches / isOllamaLinkPath / settingsGgufVariantForRow in studio/frontend/tests, the create-only write and the gpu_ids bound in studio/backend/tests, and the wiring in tests/studio/test_model_picker_contracts.py. --- studio/backend/routes/settings.py | 23 ++- studio/backend/storage/studio_db.py | 22 ++- .../backend/tests/test_openai_auto_switch.py | 169 +++++++++++++++++- .../utils/openai_auto_switch_settings.py | 26 ++- studio/frontend/src/features/hub/hub-page.tsx | 33 ++-- studio/frontend/src/features/hub/index.ts | 3 + .../hub/inventory/settings-identity.ts | 26 +++ .../src/features/hub/lib/model-identity.ts | 98 ++++++++++ .../api/migrate-model-overrides.ts | 10 +- .../model-picker/api/model-overrides.ts | 22 ++- .../components/model-selector.tsx | 8 +- .../components/sidebar-model-config.tsx | 4 + .../model-config/model-identity.ts | 1 + .../tests/model-settings-identity.test.ts | 144 +++++++++++++++ studio/frontend/tsconfig.test.json | 4 +- tests/studio/test_model_picker_contracts.py | 97 ++++++++++ 16 files changed, 663 insertions(+), 27 deletions(-) create mode 100644 studio/frontend/src/features/hub/inventory/settings-identity.ts create mode 100644 studio/frontend/tests/model-settings-identity.test.ts diff --git a/studio/backend/routes/settings.py b/studio/backend/routes/settings.py index a5e3cab832..0a05525109 100644 --- a/studio/backend/routes/settings.py +++ b/studio/backend/routes/settings.py @@ -40,6 +40,7 @@ from utils.openai_auto_switch_settings import ( DEFAULT_AUTO_UNLOAD_KEEP_KV, DEFAULT_OPENAI_AUTO_DOWNLOAD_ENABLED, DEFAULT_OPENAI_AUTO_SWITCH_ENABLED, + MAX_GPU_ID, get_auto_unload_idle_seconds, get_auto_unload_keep_kv, get_model_overrides, @@ -142,6 +143,11 @@ _MAX_VARIANT_SUFFIX_LEN = 64 # sync while the local save succeeded. MAX_MODEL_OVERRIDE_KEY_LEN = 4096 + 1 + _MAX_VARIANT_SUFFIX_LEN +# normalize_model_override keeps ids 0..MAX_GPU_ID, so a longer list cannot name a +# device the normalizer would store; it only makes it walk more duplicates. Bound +# it here so an oversized array is rejected at the boundary instead of costing CPU. +MAX_GPU_IDS = MAX_GPU_ID + 1 + class ModelOverridePayload(BaseModel): """One model's saved launch config, applied when the API loads that model. @@ -172,10 +178,15 @@ class ModelOverridePayload(BaseModel): # -1 is Auto (llama.cpp --fit sizes the offload); the normalizer treats it as unset. 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 + gpu_ids: Optional[list[int]] = Field(default = None, max_length = MAX_GPU_IDS) # Explicit intent: an all-default save carries no fields, which is shape # identical to "forget this model". None keeps the legacy contract. remove: Optional[bool] = None + # Create, don't replace: the one-time localStorage backfill reads the map once + # and then writes each model in turn, so another tab saving during that pass + # would be overwritten by this browser's older copy. The server tests and + # writes under one transaction, which costs no extra round trip. + only_if_absent: bool = False @field_validator("chat_template_override") @classmethod @@ -365,11 +376,18 @@ def update_openai_auto_switch_override( from utils.openai_auto_switch_settings import get_model_override try: + if payload.only_if_absent and payload.remove is True: + # A create that is also a delete has no meaning, and silently picking one + # would either lose settings or resurrect them. + raise ValueError("only_if_absent cannot be combined with remove.") # Only model_id is the documented "remove". Otherwise omitted launch flags # carry over from the stored entry, since the settings UI cannot express them. requested_extra_args = payload.llama_extra_args + # only_if_absent is a write mode, not a saved field: leaving it in would make + # every payload look non-empty and break the legacy "no fields means remove". saved_fields = payload.model_dump( - exclude = {"model_id", "llama_extra_args", "remove"}, exclude_none = True + exclude = {"model_id", "llama_extra_args", "remove", "only_if_absent"}, + exclude_none = True, ) if payload.remove is not None: is_removal = payload.remove @@ -413,6 +431,7 @@ def update_openai_auto_switch_override( gpu_layers = payload.gpu_layers, n_cpu_moe = payload.n_cpu_moe, gpu_ids = payload.gpu_ids, + only_if_absent = payload.only_if_absent, ) except ValueError as exc: raise log_and_http_error( diff --git a/studio/backend/storage/studio_db.py b/studio/backend/storage/studio_db.py index e1e2953fe7..3934c0af27 100644 --- a/studio/backend/storage/studio_db.py +++ b/studio/backend/storage/studio_db.py @@ -2959,11 +2959,22 @@ def upsert_app_settings(settings: dict[str, Any]) -> dict[str, Any]: def upsert_app_setting_map_entry( - key: str, entry_key: str, entry_value: dict[str, Any] | None + key: str, + entry_key: str, + entry_value: dict[str, Any] | None, + *, + only_if_absent: bool = False, ) -> dict[str, Any]: """Set (or delete, when entry_value is falsy) one sub-entry of a dict-valued app setting, atomically under BEGIN IMMEDIATE so concurrent writers to other - sub-entries cannot drop each other's updates.""" + sub-entries cannot drop each other's updates. + + ``only_if_absent`` makes the write a create: an entry already there is left + exactly as it is, and nothing is ever deleted. The test and the write share + this transaction, so a caller that read the map earlier cannot replace a value + written since. Used by the one-time localStorage backfill, whose contract is + that the server copy is the newer authority. + """ conn = get_connection() try: conn.execute("BEGIN IMMEDIATE") @@ -2971,7 +2982,12 @@ def upsert_app_setting_map_entry( current = _json_loads(row["value_json"], {}) if row else {} if not isinstance(current, dict): current = {} - if entry_value: + if only_if_absent: + if not entry_value or entry_key in current: + conn.rollback() + return current + current[entry_key] = entry_value + elif entry_value: current[entry_key] = entry_value else: current.pop(entry_key, None) diff --git a/studio/backend/tests/test_openai_auto_switch.py b/studio/backend/tests/test_openai_auto_switch.py index 25bee8b98b..9e913de11f 100644 --- a/studio/backend/tests/test_openai_auto_switch.py +++ b/studio/backend/tests/test_openai_auto_switch.py @@ -701,9 +701,14 @@ def _mock_override_store(monkeypatch): store = {} - def _merge_entry(key, entry_key, entry_value): + def _merge_entry(key, entry_key, entry_value, *, only_if_absent = False): current = dict(store.get(key) or {}) - if entry_value: + if only_if_absent: + # Create only: an entry already there wins and nothing is deleted. + if not entry_value or entry_key in current: + return current + current[entry_key] = entry_value + elif entry_value: current[entry_key] = entry_value else: current.pop(entry_key, None) @@ -5179,3 +5184,163 @@ def test_abs_path_ids_are_recognised_in_either_platform_spelling(): # A repo id has no leading separator, drive or UNC prefix under either reading. for repo_id in ("org/Repo-GGUF", "Repo", "org/Repo-GGUF:Q4_K_M"): assert resolver._is_abs_path_id(repo_id) is False, repo_id + + +def test_only_if_absent_put_never_replaces_a_newer_server_entry(monkeypatch): + """The one-time localStorage backfill reads the override map once and then + writes each model in turn, so a save by another tab during that pass was + overwritten by this browser's older copy. only_if_absent makes the write a + create, so the entry already on the server wins.""" + import routes.settings as settings_route + + _mock_override_store(monkeypatch) + + # The other tab's save lands first. + newer = settings_route.ModelOverridePayload( + model_id = "unsloth/B-GGUF", max_seq_length = 8192 + ) + settings_route.update_openai_auto_switch_override(newer, "tester") + + # The backfill's write, carrying this browser's older localStorage value. + backfill = settings_route.ModelOverridePayload( + model_id = "unsloth/B-GGUF", max_seq_length = 2048, only_if_absent = True + ) + resp = settings_route.update_openai_auto_switch_override(backfill, "tester") + assert resp.overrides["unsloth/B-GGUF"]["max_seq_length"] == 8192 + + # With nothing stored it still creates, or the migration would never run. + fresh = settings_route.ModelOverridePayload( + model_id = "unsloth/C-GGUF", max_seq_length = 2048, only_if_absent = True + ) + resp2 = settings_route.update_openai_auto_switch_override(fresh, "tester") + assert resp2.overrides["unsloth/C-GGUF"]["max_seq_length"] == 2048 + + +def test_only_if_absent_matches_a_legacy_casing_and_never_deletes(monkeypatch): + """The stored key can carry the casing an older install typed, and it must not + be duplicated or emptied by a create for the folded spelling.""" + import routes.settings as settings_route + + _mock_override_store(monkeypatch) + + stored = settings_route.ModelOverridePayload( + model_id = "Unsloth/B-GGUF:Q4_K_M", max_seq_length = 8192 + ) + settings_route.update_openai_auto_switch_override(stored, "tester") + + folded = settings_route.ModelOverridePayload( + model_id = "unsloth/b-gguf:q4_k_m", max_seq_length = 2048, only_if_absent = True + ) + resp = settings_route.update_openai_auto_switch_override(folded, "tester") + assert list(resp.overrides) == ["Unsloth/B-GGUF:Q4_K_M"] + assert resp.overrides["Unsloth/B-GGUF:Q4_K_M"]["max_seq_length"] == 8192 + + # An all-default create is a no-op, not the "empty payload means forget" path. + empty = settings_route.ModelOverridePayload( + model_id = "Unsloth/B-GGUF:Q4_K_M", only_if_absent = True + ) + resp2 = settings_route.update_openai_auto_switch_override(empty, "tester") + assert resp2.overrides["Unsloth/B-GGUF:Q4_K_M"]["max_seq_length"] == 8192 + + # A create that is also a delete has no meaning. + with pytest.raises(HTTPException) as excinfo: + settings_route.update_openai_auto_switch_override( + settings_route.ModelOverridePayload( + model_id = "Unsloth/B-GGUF:Q4_K_M", remove = True, only_if_absent = True + ), + "tester", + ) + assert excinfo.value.status_code == 400 + + +def test_only_if_absent_does_not_break_the_empty_payload_removal(monkeypatch): + """only_if_absent is a write mode, not a saved field: leaving it in the dumped + payload would make every request look non-empty and silently retire the legacy + "a payload carrying only model_id forgets this model" contract.""" + import routes.settings as settings_route + + _mock_override_store(monkeypatch) + + stored = settings_route.ModelOverridePayload( + model_id = "unsloth/B-GGUF", max_seq_length = 4096 + ) + settings_route.update_openai_auto_switch_override(stored, "tester") + empty = settings_route.ModelOverridePayload(model_id = "unsloth/B-GGUF") + resp = settings_route.update_openai_auto_switch_override(empty, "tester") + assert "unsloth/B-GGUF" not in resp.overrides + + +def test_map_entry_create_tests_and_writes_in_one_transaction(tmp_path, monkeypatch): + """The real store, not the in-memory stand-in: the existence test has to share + the write's transaction, or a concurrent writer still slips between them.""" + import storage.studio_db as db + + monkeypatch.setenv("UNSLOTH_STUDIO_HOME", str(tmp_path)) + monkeypatch.setattr(db, "_schema_ready", False) + + key = "test_map_entry_create" + assert db.upsert_app_setting_map_entry(key, "a", {"v": 1}) == {"a": {"v": 1}} + # Present: left exactly as it is. + assert db.upsert_app_setting_map_entry(key, "a", {"v": 2}, only_if_absent = True) == { + "a": {"v": 1} + } + assert db.get_app_setting(key) == {"a": {"v": 1}} + # Absent: created. + assert db.upsert_app_setting_map_entry(key, "b", {"v": 3}, only_if_absent = True) == { + "a": {"v": 1}, + "b": {"v": 3}, + } + # A create never deletes, even with nothing to store. + assert db.upsert_app_setting_map_entry(key, "a", None, only_if_absent = True) == { + "a": {"v": 1}, + "b": {"v": 3}, + } + # The ordinary write still replaces and still removes. + db.upsert_app_setting_map_entry(key, "a", {"v": 9}) + db.upsert_app_setting_map_entry(key, "b", None) + assert db.get_app_setting(key) == {"a": {"v": 9}} + + +def test_gpu_ids_dedupe_is_not_a_scan_of_the_list_being_built(): + """gpu_ids arrives from an authenticated client and normalize_model_override + de-duplicates it. Testing membership against the growing list walks up to + MAX_GPU_ID entries per element; a set keeps the pass linear. Order, bounds and + the bool rejection all have to survive the change.""" + import time + + from utils.openai_auto_switch_settings import MAX_GPU_ID, normalize_model_override + + assert normalize_model_override({"gpu_ids": [3, 1, 3, 0, 1, 2]})["gpu_ids"] == [3, 1, 0, 2] + # bool is an int subclass; [True, False] must not pin GPUs 1 and 0. + assert normalize_model_override({"gpu_ids": [True, False]}) == {} + assert normalize_model_override({"gpu_ids": [MAX_GPU_ID + 1, -1, 2]})["gpu_ids"] == [2] + + ids = [index % (MAX_GPU_ID + 1) for index in range(200_000)] + started = time.perf_counter() + normalized = normalize_model_override({"gpu_ids": ids}) + elapsed = time.perf_counter() - started + assert len(normalized["gpu_ids"]) == MAX_GPU_ID + 1 + # The scan version took ~1s for this input on a dev box; a linear pass is ~50ms. + # The bound is loose so a slow CI runner does not redden it. + assert elapsed < 0.5, elapsed + + +def test_gpu_ids_payload_is_bounded(): + """A list longer than the number of ids the normalizer can store adds nothing + but work, so it is rejected at the boundary. A real device list is tiny.""" + import pydantic + + import routes.settings as settings_route + from utils.openai_auto_switch_settings import MAX_GPU_ID + + assert settings_route.MAX_GPU_IDS == MAX_GPU_ID + 1 + at_limit = settings_route.ModelOverridePayload( + model_id = "x", gpu_ids = list(range(settings_route.MAX_GPU_IDS)) + ) + assert len(at_limit.gpu_ids) == settings_route.MAX_GPU_IDS + with pytest.raises(pydantic.ValidationError): + settings_route.ModelOverridePayload( + model_id = "x", gpu_ids = [0] * (settings_route.MAX_GPU_IDS + 1) + ) + # The ordinary case is untouched. + assert settings_route.ModelOverridePayload(model_id = "x", gpu_ids = [0, 1]).gpu_ids == [0, 1] diff --git a/studio/backend/utils/openai_auto_switch_settings.py b/studio/backend/utils/openai_auto_switch_settings.py index 5cbdbfa2af..6958270ae3 100644 --- a/studio/backend/utils/openai_auto_switch_settings.py +++ b/studio/backend/utils/openai_auto_switch_settings.py @@ -287,6 +287,9 @@ VALID_GPU_MEMORY_MODES = frozenset({"auto", "manual"}) MAX_SEQ_LENGTH_CEILING = 1048576 MAX_CHAT_TEMPLATE_OVERRIDE_BYTES = 65_536 +# Highest device index a stored gpu_ids entry may name. Also bounds how many +# distinct ids one entry can hold, which is what the payload limit is built from. +MAX_GPU_ID = 1024 def _clean_str(value: Any, allowed: frozenset[str]) -> Optional[str]: @@ -378,11 +381,15 @@ def normalize_model_override(payload: dict[str, Any]) -> dict[str, Any]: gpu_ids = payload.get("gpu_ids") if isinstance(gpu_ids, (list, tuple)) and gpu_ids: # De-duplicate, preserving order: resolve_requested_gpu_ids rejects a repeat, - # so storing [0, 0] would 400 every later API load of this model. + # so storing [0, 0] would 400 every later API load of this model. Membership + # is a set, not a scan of the list being built: an id only has to be in + # 0..MAX_GPU_ID, so a long array walks that scan once per element. cleaned_ids: list[int] = [] + seen_ids: set[int] = set() for gid in gpu_ids: - parsed = _bounded_int(gid, minimum = 0, maximum = 1024) - if parsed is not None and parsed not in cleaned_ids: + parsed = _bounded_int(gid, minimum = 0, maximum = MAX_GPU_ID) + if parsed is not None and parsed not in seen_ids: + seen_ids.add(parsed) cleaned_ids.append(parsed) if cleaned_ids: entry["gpu_ids"] = cleaned_ids @@ -613,12 +620,18 @@ def set_model_override( model_id: str, llama_extra_args: Optional[list[str]] = None, max_seq_length: Optional[int] = None, + *, + only_if_absent: bool = False, **config: Any, ) -> dict: """Upsert one model's launch config; a config with no usable fields removes it. The two legacy parameters stay positional for existing callers; every other per-model field is passed by keyword and normalized together. + + ``only_if_absent`` turns the upsert into a create, leaving an entry already + stored untouched. Returns the normalized entry either way; read the map back + to see what is actually stored. """ if not model_id or not model_id.strip(): raise ValueError("model_id is required.") @@ -633,6 +646,11 @@ def set_model_override( from storage.studio_db import upsert_app_setting_map_entry # Atomic per-entry merge so two PUTs for different models can't drop each other. - upsert_app_setting_map_entry(MODEL_OVERRIDES_SETTING_KEY, model_id.strip(), entry or None) + upsert_app_setting_map_entry( + MODEL_OVERRIDES_SETTING_KEY, + model_id.strip(), + entry or None, + only_if_absent = only_if_absent, + ) _invalidate(MODEL_OVERRIDES_SETTING_KEY) return entry diff --git a/studio/frontend/src/features/hub/hub-page.tsx b/studio/frontend/src/features/hub/hub-page.tsx index 2417bec0d9..154d73644f 100644 --- a/studio/frontend/src/features/hub/hub-page.tsx +++ b/studio/frontend/src/features/hub/hub-page.tsx @@ -70,6 +70,7 @@ import { useHubModelVram } from "./hooks/use-hub-model-vram"; import { useModelsSelection } from "./hooks/use-models-selection"; import { useHubInventory } from "./inventory"; import { LOCAL_MODEL_SOURCE } from "./inventory/constants"; +import { settingsGgufVariantForRow } from "./inventory/settings-identity"; import { CHANNEL_TO_SECTION, type ChannelId, @@ -84,7 +85,11 @@ import { isHiddenModelId, } from "./lib/hidden-models"; import { inventoryRowMatches, tokenizeQuery } from "./lib/inventory-search"; -import { ggufVariantsMatch, modelIdsMatch } from "./lib/model-identity"; +import { + ggufVariantsMatch, + modelIdsMatch, + residentModelIdMatches, +} from "./lib/model-identity"; import { type ModelTypeFilter, matchesModelType, @@ -1271,7 +1276,7 @@ export function ModelsPage() { // repo with format_variant null). Opening with a null variant keys the config // to `repo::` while the loader reads `repo::Q4_K_M`, so it never applies and // the server mirror is wrong too. Resolve it as the on-device card does. - let ggufVariant = row.formatVariant?.trim() || null; + let ggufVariant = settingsGgufVariantForRow(row); if (!ggufVariant && row.isGguf && row.capabilities.requiresVariant) { const repoId = row.kind === "cache" ? row.repoId : (row.repoId ?? null); if (repoId) { @@ -1429,6 +1434,18 @@ export function ModelsPage() { }, [selectedModel], ); + // 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 + // straight off disk loads by path but is reported by its clean public id, so the + // row's path and its settings identity both have to be offered as aliases. + const settingsTargetIsResident = + settingsTarget !== null && + residentModelIdMatches( + activeCheckpoint, + settingsTarget.id, + settingsTarget.configId, + ) && + ggufVariantsMatch(activeGgufVariant, settingsTarget.ggufVariant); const handleSearchHub = useCallback( (next: string) => { const trimmed = next.trim(); @@ -1856,17 +1873,9 @@ export function ModelsPage() {
setSettingsTarget(null)} onRun={runSettingsTarget} diff --git a/studio/frontend/src/features/hub/index.ts b/studio/frontend/src/features/hub/index.ts index 7d9e5b74aa..c2925f5790 100644 --- a/studio/frontend/src/features/hub/index.ts +++ b/studio/frontend/src/features/hub/index.ts @@ -41,8 +41,11 @@ export { looksLikeLocalPath } from "./lib/local-path"; export { hubTokenHeader } from "./lib/hub-token-header"; export { ggufVariantsMatch, + isOllamaLinkPath, normalizeGgufVariantIdentity, normalizeModelIdentity, + publicModelId, + residentModelIdMatches, } from "./lib/model-identity"; export { formatBytes, formatRelativeShort } from "./lib/format"; export { ggufVariantDisplayLabel } from "./lib/gguf-variant-sort"; diff --git a/studio/frontend/src/features/hub/inventory/settings-identity.ts b/studio/frontend/src/features/hub/inventory/settings-identity.ts new file mode 100644 index 0000000000..820194dfa2 --- /dev/null +++ b/studio/frontend/src/features/hub/inventory/settings-identity.ts @@ -0,0 +1,26 @@ +// SPDX-License-Identifier: AGPL-3.0-only +// Copyright 2026-present the Unsloth AI Inc. team. All rights reserved. See /studio/LICENSE.AGPL-3.0 + +// How an inventory row maps onto the identity its saved settings are keyed by. + +import type { CachedInventoryRow, LocalInventoryRow } from "./types"; + +/** + * The GGUF variant a settings page should key this row's config by, before any + * per-repo quant lookup. + * + * A standalone `.gguf` has no quant to choose between, but the backend inventory + * still labels it from its filename (hub/services/models/common.py sets + * `format_variant` only when the scanned path is a single file). Adopting that + * label would key its settings to `:Q4_K_M` while the Chat model picker, the + * detail view's on-device card and the one-time backfill all use the bare path, + * leaving two surfaces editing two different configs for one file. + */ +export function settingsGgufVariantForRow( + row: CachedInventoryRow | LocalInventoryRow, +): string | null { + if (row.kind === "local" && row.path.toLowerCase().endsWith(".gguf")) { + return null; + } + return row.formatVariant?.trim() || null; +} diff --git a/studio/frontend/src/features/hub/lib/model-identity.ts b/studio/frontend/src/features/hub/lib/model-identity.ts index 488502ae28..8db1c015d1 100644 --- a/studio/frontend/src/features/hub/lib/model-identity.ts +++ b/studio/frontend/src/features/hub/lib/model-identity.ts @@ -63,3 +63,101 @@ export function ggufVariantsMatch( normalizeGgufVariantIdentity(left) === normalizeGgufVariantIdentity(right) ); } + +// Mirrors core/inference/model_ids.py _looks_like_path. +const PUBLIC_ID_PATH_PREFIX_RE = /^(?:[/\\]|\.{1,2}[\\/]|~)/; +const GGUF_SUFFIX_RE = /\.gguf$/i; +const BACKSLASHES_RE = /\\/g; +const TRAILING_SLASHES_RE = /\/+$/; + +function looksLikeModelPath(identifier: string): boolean { + if (GGUF_SUFFIX_RE.test(identifier)) { + return true; + } + if (PUBLIC_ID_PATH_PREFIX_RE.test(identifier)) { + return true; + } + if (identifier.length >= 2 && identifier[1] === ":") { + return true; + } + return identifier.split("/").length - 1 >= 2 || identifier.includes("\\"); +} + +/** `.../models--org--name/snapshots/` -> `org/name`, else null. */ +function hfCacheRepoId(path: string): string | null { + const parts = path.replace(BACKSLASHES_RE, "/").split("/"); + for (let index = 0; index < parts.length; index += 1) { + const part = parts[index]; + if (part.startsWith("models--") && parts[index + 1] === "snapshots") { + return part.slice("models--".length).replaceAll("--", "/"); + } + } + return null; +} + +/** + * The clean id the backend reports for a model loaded by path. + * + * Mirrors ``public_model_id`` in studio/backend/core/inference/model_ids.py, which + * is what ``/api/inference/status`` puts in ``active_model``: an HF cache snapshot + * becomes its repo id and any other local GGUF becomes its filename stem. Repo ids + * and already-clean names come back unchanged. + */ +export function publicModelId(identifier: string): string { + const trimmed = identifier.trim(); + if (!(trimmed && looksLikeModelPath(trimmed))) { + return trimmed; + } + const repoId = hfCacheRepoId(trimmed); + if (repoId) { + return repoId; + } + const slashPath = trimmed + .replace(BACKSLASHES_RE, "/") + .replace(TRAILING_SLASHES_RE, ""); + const name = slashPath.slice(slashPath.lastIndexOf("/") + 1); + return name.replace(GGUF_SUFFIX_RE, "") || trimmed; +} + +/** + * Whether the model the backend reports as loaded is one of *candidates*. + * + * A GGUF loaded from an inactive HF cache or straight off disk is loaded by path, + * but `/status` reports the clean public id, so an exact comparison against the + * catalog row's path says "not loaded" and the caller falls back to saved or + * default values instead of the live launch config. Candidates are compared + * literally first, then by the public id the backend would report for them. + */ +export function residentModelIdMatches( + activeModelId: string | null | undefined, + ...candidates: (string | null | undefined)[] +): boolean { + if (candidates.some((candidate) => modelIdsMatch(activeModelId, candidate))) { + return true; + } + const active = activeModelId?.trim(); + // A path-shaped active id is the raw identifier, which the literal pass covered. + if (!active || looksLikeModelPath(active)) { + return false; + } + return candidates.some((candidate) => { + const trimmed = candidate?.trim(); + return trimmed ? modelIdsMatch(active, publicModelId(trimmed)) : false; + }); +} + +// Ollama's blobs reach the picker through a ".studio_links"/"ollama_links" symlink +// directory. core/inference/local_model_resolver.py refuses to index anything under +// those (the scanner that creates them runs off the request path), so the API can +// never load one and mirroring its settings would advertise a load that cannot happen. +const OLLAMA_LINK_SEGMENTS = new Set([".studio_links", "ollama_links"]); + +export function isOllamaLinkPath(modelId: string | null | undefined): boolean { + if (!modelId) { + return false; + } + return modelId + .replace(BACKSLASHES_RE, "/") + .split("/") + .some((segment) => OLLAMA_LINK_SEGMENTS.has(segment)); +} 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 index 71a22dcd2f..1a82d09f0b 100644 --- a/studio/frontend/src/features/model-picker/api/migrate-model-overrides.ts +++ b/studio/frontend/src/features/model-picker/api/migrate-model-overrides.ts @@ -8,6 +8,7 @@ // API load uses app defaults, the exact bug the server-side map exists to fix. import { + isOllamaLinkPath, normalizeGgufVariantIdentity, normalizeModelIdentity, splitQuantSuffix, @@ -73,10 +74,13 @@ export async function backfillModelOverrides(): Promise { // A quant means GGUF, the only thing API auto-switch resolves, so backfilling a // safetensors config would claim behaviour that does not exist. A standalone // .gguf has no quant to select between and is stored with a null variant, so it - // needs the extra test or its settings stay browser-only for good. + // needs the extra test or its settings stay browser-only for good. An Ollama + // blob is GGUF but reached through a link dir the resolver skips, so it is not + // auto-switchable either. (entry) => (entry.ggufVariant != null || entry.modelId.toLowerCase().endsWith(".gguf")) && + !isOllamaLinkPath(entry.modelId) && !isDefaultConfig(entry.config), ); if (local.length === 0) { @@ -118,10 +122,14 @@ export async function backfillModelOverrides(): Promise { continue; } try { + // Create only. `known` is a snapshot from before this loop started, so a save + // by another tab during the pass is invisible here; the server does the test + // and the write together rather than this re-fetching once per model. await putModelOverride( current.modelId, current.ggufVariant, current.config, + { onlyIfAbsent: true }, ); } catch { failed = true; 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 0734962fdc..ffd5b39779 100644 --- a/studio/frontend/src/features/model-picker/api/model-overrides.ts +++ b/studio/frontend/src/features/model-picker/api/model-overrides.ts @@ -128,10 +128,23 @@ function toApiOverride(config: PerModelConfig | null): ApiModelOverride { // replace. Different models still overlap. const writesByKey = new Map>(); +export interface PutModelOverrideOptions { + /** + * Create only: leave an entry already on the server exactly as it is. + * + * The one-time backfill reads the map once and then writes each model in turn, + * so another tab saving during that pass would be overwritten by this browser's + * older localStorage copy. The server tests and writes under one transaction, + * which closes the window without a round trip per model. + */ + onlyIfAbsent?: boolean; +} + export async function putModelOverride( modelId: string, ggufVariant: string | null | undefined, config: PerModelConfig | null, + options?: PutModelOverrideOptions, ): Promise { // Keyed by the folded identity, not the literal spelling: the backfill sends a // legacy casing and a UI save the normalized one, and the backend resolves both @@ -144,7 +157,7 @@ export async function putModelOverride( const previous = writesByKey.get(key) ?? Promise.resolve(); const write = previous .catch(() => {}) - .then(() => sendModelOverride(modelId, ggufVariant, config)); + .then(() => sendModelOverride(modelId, ggufVariant, config, options)); writesByKey.set(key, write); try { await write; @@ -160,6 +173,7 @@ async function sendModelOverride( modelId: string, ggufVariant: string | null | undefined, config: PerModelConfig | null, + options?: PutModelOverrideOptions, ): Promise { const res = await authFetch(OVERRIDES_URL, { method: "PUT", @@ -167,6 +181,12 @@ async function sendModelOverride( body: JSON.stringify({ // biome-ignore lint/style/useNamingConvention: API schema model_id: modelOverrideKey(modelId, ggufVariant), + // Only sent when set, so an older backend that does not know the field is + // not handed an unexpected key by every ordinary save. + ...(options?.onlyIfAbsent + ? // biome-ignore lint/style/useNamingConvention: API schema + { only_if_absent: true } + : {}), // Say which operation this is: an all-default save carries no fields, which is // shape-identical to "forget this model", and guessing wrong wipes launch flags // the UI cannot show or restore. diff --git a/studio/frontend/src/features/model-picker/components/model-selector.tsx b/studio/frontend/src/features/model-picker/components/model-selector.tsx index 9ae7e59a1f..776b2677da 100644 --- a/studio/frontend/src/features/model-picker/components/model-selector.tsx +++ b/studio/frontend/src/features/model-picker/components/model-selector.tsx @@ -34,6 +34,7 @@ import { useRef, useState, } from "react"; +import { isOllamaLinkPath } from "../model-config/model-identity"; import { type PerModelConfig, resolveInitialConfig, @@ -480,11 +481,16 @@ function ModelSelectorContent({ const visibleConfigTarget = open ? configTarget : null; const openConfigPage = (id: string, meta: ModelSelectorChangeMeta) => { const leaf = id.includes("/") ? id.slice(id.lastIndexOf("/") + 1) : id; + const isGguf = meta.isGguf ?? Boolean(meta.ggufVariant); setConfigTarget({ id, displayName: meta.ggufVariant ? `${leaf} · ${meta.ggufVariant}` : leaf, ggufVariant: meta.ggufVariant ?? null, - isGguf: meta.isGguf ?? Boolean(meta.ggufVariant), + isGguf, + // Ollama's models list here as custom-folder GGUFs under a link dir the + // auto-switch resolver skips, so mirroring their settings to the server + // would advertise a load the API can never make. + apiLoadable: isGguf && !isOllamaLinkPath(id), meta, }); }; diff --git a/studio/frontend/src/features/model-picker/components/sidebar-model-config.tsx b/studio/frontend/src/features/model-picker/components/sidebar-model-config.tsx index 2d12c503a4..fe04363982 100644 --- a/studio/frontend/src/features/model-picker/components/sidebar-model-config.tsx +++ b/studio/frontend/src/features/model-picker/components/sidebar-model-config.tsx @@ -3,6 +3,7 @@ import { useMemo } from "react"; import { gpuFieldsSignature } from "../model-config/apply-per-model-config"; +import { isOllamaLinkPath } from "../model-config/model-identity"; import type { PerModelConfig } from "../model-config/per-model-config"; import { ModelConfigPage } from "./model-config-page"; import type { ModelPickTarget } from "./model-selector/types"; @@ -67,6 +68,9 @@ export function SidebarModelConfig({ displayName: ggufVariant ? `${leaf} · ${ggufVariant}` : leaf, ggufVariant, isGguf, + // An Ollama blob loads through a link dir the auto-switch resolver skips, + // so its settings must not be mirrored as if the API could load it. + apiLoadable: isGguf && !isOllamaLinkPath(modelId), meta: { source: "local", isLora: false, diff --git a/studio/frontend/src/features/model-picker/model-config/model-identity.ts b/studio/frontend/src/features/model-picker/model-config/model-identity.ts index ff2e510e82..764e823e53 100644 --- a/studio/frontend/src/features/model-picker/model-config/model-identity.ts +++ b/studio/frontend/src/features/model-picker/model-config/model-identity.ts @@ -7,6 +7,7 @@ import { } from "@/features/hub"; export { + isOllamaLinkPath, normalizeGgufVariantIdentity, normalizeModelIdentity, } from "@/features/hub"; diff --git a/studio/frontend/tests/model-settings-identity.test.ts b/studio/frontend/tests/model-settings-identity.test.ts new file mode 100644 index 0000000000..33c1fdf037 --- /dev/null +++ b/studio/frontend/tests/model-settings-identity.test.ts @@ -0,0 +1,144 @@ +// SPDX-License-Identifier: AGPL-3.0-only +// Copyright 2026-present the Unsloth AI Inc. team. All rights reserved. See /studio/LICENSE.AGPL-3.0 + +import assert from "node:assert/strict"; +import test from "node:test"; + +import { settingsGgufVariantForRow } from "../src/features/hub/inventory/settings-identity.ts"; +import type { + CachedInventoryRow, + LocalInventoryRow, +} from "../src/features/hub/inventory/types.ts"; +import { + isOllamaLinkPath, + modelIdsMatch, + publicModelId, + residentModelIdMatches, +} from "../src/features/hub/lib/model-identity.ts"; + +test("publicModelId mirrors what /status reports for a path-loaded model", () => { + // Mirrors public_model_id in studio/backend/core/inference/model_ids.py. + assert.equal( + publicModelId("/srv/models/Qwen3-8B-Q4_K_M.gguf"), + "Qwen3-8B-Q4_K_M", + ); + assert.equal( + publicModelId( + "/home/u/.cache/huggingface/hub/models--unsloth--Qwen3-8B-GGUF/snapshots/abc123", + ), + "unsloth/Qwen3-8B-GGUF", + ); + assert.equal(publicModelId("C:\\models\\Foo-Q4_K_M.gguf"), "Foo-Q4_K_M"); + assert.equal(publicModelId("~/models/Foo.gguf"), "Foo"); + assert.equal(publicModelId("/srv/models/repo/"), "repo"); + // A repo id and an already-clean name come back untouched. + assert.equal(publicModelId("unsloth/Qwen3-8B-GGUF"), "unsloth/Qwen3-8B-GGUF"); + assert.equal(publicModelId("Qwen3-8B-Q4_K_M"), "Qwen3-8B-Q4_K_M"); + // "models--" alone is not the cache layout; only the snapshots sibling is. + assert.equal(publicModelId("models--only--nosnapshots/blobs/x"), "x"); +}); + +test("a resident path-loaded model is matched by the id /status reports", () => { + // A loose .gguf: the catalog row is keyed by the path, /status by the stem. + assert.equal( + modelIdsMatch("Qwen3-8B-Q4_K_M", "/srv/models/Qwen3-8B-Q4_K_M.gguf"), + false, + ); + assert.equal( + residentModelIdMatches( + "Qwen3-8B-Q4_K_M", + "/srv/models/Qwen3-8B-Q4_K_M.gguf", + "/srv/models/Qwen3-8B-Q4_K_M.gguf", + ), + true, + ); + // A repo in an inactive HF cache loads by snapshot path but keeps the repo id + // as its settings identity, so the configId alias already covers it. + assert.equal( + residentModelIdMatches( + "unsloth/Qwen3-8B-GGUF", + "/mnt/old-cache/models--unsloth--Qwen3-8B-GGUF/snapshots/abc123", + "unsloth/Qwen3-8B-GGUF", + ), + true, + ); + // The raw identifier is still matched literally. + assert.equal( + residentModelIdMatches( + "/srv/models/Qwen3-8B-Q4_K_M.gguf", + "/srv/models/Qwen3-8B-Q4_K_M.gguf", + null, + ), + true, + ); + // Another model is still not the loaded one. + assert.equal( + residentModelIdMatches( + "Qwen3-8B-Q4_K_M", + "/srv/models/Llama-3-8B-Q4_K_M.gguf", + null, + ), + false, + ); + assert.equal( + residentModelIdMatches( + "unsloth/Qwen3-8B-GGUF", + "/mnt/old-cache/models--unsloth--Llama-3-GGUF/snapshots/abc123", + "unsloth/Llama-3-GGUF", + ), + false, + ); + assert.equal(residentModelIdMatches(null, "/srv/models/x.gguf"), false); + assert.equal(residentModelIdMatches("Qwen3-8B-Q4_K_M"), false); +}); + +test("Ollama link paths are recognised the way the resolver excludes them", () => { + // core/inference/local_model_resolver.py refuses any path with these segments. + assert.equal( + isOllamaLinkPath("/home/u/.ollama/models/.studio_links/q/qwen3-Q4_K_M.gguf"), + true, + ); + assert.equal( + isOllamaLinkPath("/home/u/.cache/unsloth/ollama_links/ab12/llama3.gguf"), + true, + ); + assert.equal( + isOllamaLinkPath("C:\\Users\\u\\.ollama\\models\\.studio_links\\q\\a.gguf"), + true, + ); + // Only those exact segments, not a directory that merely contains the name. + assert.equal(isOllamaLinkPath("/srv/studio_links_backup/a.gguf"), false); + assert.equal(isOllamaLinkPath("/srv/models/Qwen3-8B-Q4_K_M.gguf"), false); + assert.equal(isOllamaLinkPath("unsloth/Qwen3-8B-GGUF"), false); + assert.equal(isOllamaLinkPath(null), false); +}); + +test("a standalone gguf keeps one settings identity across surfaces", () => { + const loose = { + kind: "local", + path: "/srv/models/Qwen3-8B-Q4_K_M.gguf", + // What hub/services/models/common.py emits for a single scanned file. + formatVariant: "Q4_K_M", + } as LocalInventoryRow; + // The Chat picker opens the same file with no variant, so the Hub row must not + // adopt the filename-derived label or the two edit different configs. + assert.equal(settingsGgufVariantForRow(loose), null); + + // A GGUF directory still has a variant slot for the quant lookup to fill. + const repoDir = { + kind: "local", + path: "/srv/models/Qwen3-8B-GGUF", + formatVariant: null, + } as LocalInventoryRow; + assert.equal(settingsGgufVariantForRow(repoDir), null); + const lmStudioDir = { + kind: "local", + path: "/srv/lmstudio/Qwen3-8B-GGUF", + formatVariant: "Q8_0", + } as LocalInventoryRow; + assert.equal(settingsGgufVariantForRow(lmStudioDir), "Q8_0"); + + // Cached repo rows are unaffected (cache_inventory.py never sets one). + const cached = { kind: "cache", formatVariant: null } as CachedInventoryRow; + assert.equal(settingsGgufVariantForRow(cached), null); +}); diff --git a/studio/frontend/tsconfig.test.json b/studio/frontend/tsconfig.test.json index da6cdcc9ba..be53907fc7 100644 --- a/studio/frontend/tsconfig.test.json +++ b/studio/frontend/tsconfig.test.json @@ -3,7 +3,9 @@ "target": "ES2022", "lib": ["ES2022", "DOM", "DOM.Iterable"], "module": "ESNext", - "types": ["node"], + // vite/client so a test may import a src module that (transitively) reads + // import.meta.env; without it those reads fail to typecheck here only. + "types": ["node", "vite/client"], "skipLibCheck": true, "moduleResolution": "bundler", diff --git a/tests/studio/test_model_picker_contracts.py b/tests/studio/test_model_picker_contracts.py index 3abb8e3925..d80ff4af37 100644 --- a/tests/studio/test_model_picker_contracts.py +++ b/tests/studio/test_model_picker_contracts.py @@ -932,3 +932,100 @@ def test_the_settings_page_judges_the_config_storage_actually_keeps(): assert "export function normalizePerModelConfig(" in store assert "const normalized = normalize(config);" in store assert "if (isDefaultConfig(normalized)) {" in store, "the rule this mirrors" + + +def test_the_chat_picker_marks_ollama_targets_unloadable_by_the_api(): + """A settings target opened from the Chat model picker carried no apiLoadable, + so the `?? target.isGguf` fallback mirrored an Ollama GGUF to the server. + local_model_resolver.py refuses every path under a .studio_links/ollama_links + link dir, which is exactly how Ollama's blobs reach this picker, so the mirror + advertised a load the API can never make.""" + picker = " ".join(_read("features/model-picker/components/model-selector.tsx").split()) + assert "apiLoadable: isGguf && !isOllamaLinkPath(id)," in picker + sidebar = " ".join( + _read("features/model-picker/components/sidebar-model-config.tsx").split() + ) + assert "apiLoadable: isGguf && !isOllamaLinkPath(modelId)," in sidebar + # The same classification gates the one-time backfill, or a config saved before + # the upgrade still reaches the server on the next start. + backfill = " ".join(_read("features/model-picker/api/migrate-model-overrides.ts").split()) + assert "!isOllamaLinkPath(entry.modelId) &&" in backfill + + identity = _read("features/hub/lib/model-identity.ts") + assert 'new Set([".studio_links", "ollama_links"])' in identity + resolver = ( + WORKDIR / "studio" / "backend" / "core" / "inference" / "local_model_resolver.py" + ).read_text(encoding = "utf-8") + assert 'seg in (".studio_links", "ollama_links")' in resolver, "the rule this mirrors" + + +def test_the_backfill_writes_are_creates_not_replacements(): + """The backfill reads the override map once and then writes each model in turn, + so a save by another tab during that pass was overwritten by this browser's + older localStorage copy. The server tests and writes under one transaction + rather than this re-fetching per model.""" + backfill = " ".join(_read("features/model-picker/api/migrate-model-overrides.ts").split()) + assert "{ onlyIfAbsent: true }," in backfill + api = " ".join(_read("features/model-picker/api/model-overrides.ts").split()) + assert "onlyIfAbsent?: boolean;" in api + assert "options?.onlyIfAbsent ? { only_if_absent: true } : {}" in api.replace( + "// biome-ignore lint/style/useNamingConvention: API schema ", "" + ) + # Ordinary saves must stay unconditional, or a settings edit would never land. + assert "syncModelOverride" in api and "only_if_absent: true" in api + + route = (WORKDIR / "studio" / "backend" / "routes" / "settings.py").read_text( + encoding = "utf-8" + ) + assert "only_if_absent: bool = False" in route, "the rule this mirrors" + assert "only_if_absent = payload.only_if_absent," in route + # A write mode must not leak into the saved fields, or "only model_id means + # forget this model" stops working. + assert '"remove", "only_if_absent"' in route + + +def test_the_hub_settings_page_matches_a_resident_path_loaded_model(): + """A GGUF loaded from an inactive HF cache or straight off disk loads by path, + but /status reports the clean public id, so comparing it to settingsTarget.id + said "not loaded" and the page showed saved or default values instead of the + live launch config.""" + hub = " ".join(_read("features/hub/hub-page.tsx").split()) + assert ( + "residentModelIdMatches( activeCheckpoint, settingsTarget.id, settingsTarget.configId, )" + in hub + ) + assert "loadedConfig={settingsTargetIsResident ? activeModelConfig : null}" in hub + assert "settingsTargetIsResident ? activeGgufContextLength : null" in hub + # The alias is the backend's own public id rule, not a private heuristic. + identity = _read("features/hub/lib/model-identity.ts") + assert "export function publicModelId(" in identity + assert "models--" in identity and "snapshots" in identity + backend = ( + WORKDIR / "studio" / "backend" / "core" / "inference" / "model_ids.py" + ).read_text(encoding = "utf-8") + assert "def public_model_id(" in backend, "the rule this mirrors" + + +def test_a_standalone_gguf_has_one_settings_key(): + """The inventory labels a single scanned .gguf from its filename, so the Hub row + menu keyed its settings to `:Q4_K_M` while the Chat picker, the detail + card and the backfill all use the bare path: two surfaces, two configs.""" + hub = " ".join(_read("features/hub/hub-page.tsx").split()) + assert "let ggufVariant = settingsGgufVariantForRow(row);" in hub + assert "row.formatVariant" not in hub, "the row's raw label is not a settings key" + + helper = " ".join(_read("features/hub/inventory/settings-identity.ts").split()) + assert 'row.kind === "local" && row.path.toLowerCase().endsWith(".gguf")' in helper + + common = ( + WORKDIR + / "studio" + / "backend" + / "hub" + / "services" + / "models" + / "common.py" + ).read_text(encoding = "utf-8") + # The rule this mirrors: a variant is derived only for a single scanned file. + assert "extract_quant_label(gguf_files[0].name)" in common + assert "if scan_path.is_file() and len(gguf_files) == 1" in common