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