diff --git a/studio/backend/routes/inference.py b/studio/backend/routes/inference.py index f951f91c04..61b3520d19 100644 --- a/studio/backend/routes/inference.py +++ b/studio/backend/routes/inference.py @@ -4357,22 +4357,30 @@ async def _maybe_auto_switch_model( # Apply this model's saved launch config so an API swap loads # it exactly as the picker would. Try variant-qualified keys # first (two quants of one repo can differ), then bare ids, and - # both the repo id and the load path (a local folder or a - # non-active HF cache is configured against its path). - # A standalone .gguf resolves with variant=None, but the picker - # keys it by the quant label derived from the filename, so its - # settings live under ":LABEL" and no bare key reaches them. + # within each pair the concrete load path before the advertised + # id. The settings UI keys every local row (a folder, an LM + # Studio dir, a non-active HF cache, a loose .gguf) by that path, + # while override_id is a derived alias -- the /v1/models name a + # hand-written overrides PUT uses, and for a loose file only its + # filename stem. Reading the alias first let an older entry under + # it shadow the settings the user just saved, for good. A cached + # repo is keyed by its repo id, which is override_id, and no path + # entry exists for it, so it still resolves on the second try. + # A standalone .gguf resolves with variant=None; an early build + # of this feature keyed it by the quant label derived from the + # filename, so read ":LABEL" too, after the bare path the + # picker writes today. file_variant = None if not variant and target_id.lower().endswith(".gguf"): from hub.utils.gguf import extract_quant_label file_variant = extract_quant_label(os.path.basename(target_id)) override = {} for override_key in ( - f"{override_id}:{variant}" if variant else None, f"{target_id}:{variant}" if variant else None, + f"{override_id}:{variant}" if variant else None, + target_id, f"{target_id}:{file_variant}" if file_variant else None, override_id, - target_id, ): if not override_key: continue diff --git a/studio/backend/routes/settings.py b/studio/backend/routes/settings.py index 0a05525109..c2faad2011 100644 --- a/studio/backend/routes/settings.py +++ b/studio/backend/routes/settings.py @@ -182,11 +182,14 @@ class ModelOverridePayload(BaseModel): # 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 + # Fill in, 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 + # would be overwritten by this browser's older copy. The server reads and writes + # under one transaction, which costs no extra round trip. Field level, because an + # install upgraded from a release that stored only llama_extra_args and + # max_seq_length holds an entry the browser has the rest of, and skipping the + # whole entry would strand exactly the settings this migration exists to carry. + fill_absent_fields: bool = False @field_validator("chat_template_override") @classmethod @@ -376,17 +379,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 + if payload.fill_absent_fields and payload.remove is True: + # A fill 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.") + raise ValueError("fill_absent_fields 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". + # fill_absent_fields 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", "only_if_absent"}, + exclude = {"model_id", "llama_extra_args", "remove", "fill_absent_fields"}, exclude_none = True, ) if payload.remove is not None: @@ -396,13 +400,19 @@ def update_openai_auto_switch_override( key: value for key, value in saved_fields.items() if key != "tensor_parallel" } if requested_extra_args is None and not is_removal: - requested_extra_args = get_model_override(payload.model_id).get("llama_extra_args") - if requested_extra_args is None: - # First per-quant save for flags stored under the bare repo id. - # Auto-switch prefers the qualified entry, so carry them over. - bare_id = _bare_model_id(payload.model_id) - if bare_id: - requested_extra_args = get_model_override(bare_id).get("llama_extra_args") + stored = get_model_override(payload.model_id) + # A fill leaves every stored value alone, so an entry that is already + # there keeps its flags without this echoing them back through + # validation: one accepted when it was saved but denylisted since would + # 400 the one-time migration, which then retries on every start. + if not (payload.fill_absent_fields and stored): + requested_extra_args = stored.get("llama_extra_args") + if requested_extra_args is None: + # First per-quant save for flags stored under the bare repo id. + # Auto-switch prefers the qualified entry, so carry them over. + bare_id = _bare_model_id(payload.model_id) + if bare_id: + requested_extra_args = get_model_override(bare_id).get("llama_extra_args") # Not validated on an explicit remove: nothing is stored, so a 400 would only # leave the override in place. A stale flag must not block forgetting. extra_args = [] if payload.remove is True else validate_extra_args(requested_extra_args) @@ -431,7 +441,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, + fill_absent_fields = payload.fill_absent_fields, ) 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 3934c0af27..46488698cd 100644 --- a/studio/backend/storage/studio_db.py +++ b/studio/backend/storage/studio_db.py @@ -2963,17 +2963,20 @@ def upsert_app_setting_map_entry( entry_key: str, entry_value: dict[str, Any] | None, *, - only_if_absent: bool = False, + fill_absent_fields: 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. - ``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. + ``fill_absent_fields`` writes only what is missing: the entry is created when + it is not there, and otherwise gains the fields it does not already hold while + every stored value is left exactly as it is. Nothing is ever deleted. The read + 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: an + upgraded install can hold an entry with only the fields an older release knew, + while this browser holds the rest, and entry-level skipping would strand them. """ conn = get_connection() try: @@ -2982,11 +2985,20 @@ def upsert_app_setting_map_entry( current = _json_loads(row["value_json"], {}) if row else {} if not isinstance(current, dict): current = {} - if only_if_absent: - if not entry_value or entry_key in current: + if fill_absent_fields: + if not entry_value: conn.rollback() return current - current[entry_key] = entry_value + stored = current.get(entry_key) + if isinstance(stored, dict): + # Stored values win field by field, so this only ever adds. + merged = {**entry_value, **stored} + if merged == stored: + conn.rollback() + return current + current[entry_key] = merged + else: + current[entry_key] = entry_value elif entry_value: current[entry_key] = entry_value else: diff --git a/studio/backend/tests/test_openai_auto_switch.py b/studio/backend/tests/test_openai_auto_switch.py index 42ea6ee957..11759278cf 100644 --- a/studio/backend/tests/test_openai_auto_switch.py +++ b/studio/backend/tests/test_openai_auto_switch.py @@ -706,14 +706,18 @@ def _mock_override_store(monkeypatch): entry_key, entry_value, *, - only_if_absent = False, + fill_absent_fields = False, ): current = dict(store.get(key) or {}) - if only_if_absent: - # Create only: an entry already there wins and nothing is deleted. - if not entry_value or entry_key in current: + if fill_absent_fields: + # Fill only: every stored value wins and nothing is deleted. + if not entry_value: return current - current[entry_key] = entry_value + stored = current.get(entry_key) + if isinstance(stored, dict): + current[entry_key] = {**entry_value, **stored} + else: + current[entry_key] = entry_value elif entry_value: current[entry_key] = entry_value else: @@ -4343,9 +4347,11 @@ def test_override_found_under_a_concrete_path_with_variant(monkeypatch): assert rec.calls[0].max_seq_length == 8192 -def test_repo_qualified_override_beats_path_qualified(monkeypatch): - # Most specific first, and the public repo id is what the picker configured - # against. +def test_path_qualified_override_beats_repo_qualified(monkeypatch): + # Most specific first: the settings page keys a local row (a folder, an LM Studio + # dir, a loose file) by the path being loaded, while the repo id is the advertised + # alias that a second copy of the same repo, or a hand-written overrides PUT, is + # configured under. The row the user actually edited wins. backend = _FakeBackend(None) rec = _LoadRecorder(backend) _wire( @@ -4362,7 +4368,7 @@ def test_repo_qualified_override_beats_path_qualified(monkeypatch): monkeypatch.setattr(settings, "get_model_override", lambda mid: stored.get(mid, {})) _run_hook("unsloth/B-GGUF") - assert rec.calls[0].max_seq_length == 8192 + assert rec.calls[0].max_seq_length == 1024 def test_first_quant_save_keeps_legacy_bare_repo_launch_flags(monkeypatch): @@ -5192,11 +5198,11 @@ def test_abs_path_ids_are_recognised_in_either_platform_spelling(): 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): +def test_fill_absent_fields_put_never_replaces_a_newer_server_value(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.""" + overwritten by this browser's older copy. fill_absent_fields writes only what + the entry lacks, so every value already on the server wins.""" import routes.settings as settings_route _mock_override_store(monkeypatch) @@ -5207,22 +5213,72 @@ def test_only_if_absent_put_never_replaces_a_newer_server_entry(monkeypatch): # 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 + model_id = "unsloth/B-GGUF", max_seq_length = 2048, fill_absent_fields = 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 + model_id = "unsloth/C-GGUF", max_seq_length = 2048, fill_absent_fields = 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): +def test_fill_absent_fields_carries_the_browser_only_settings_into_a_legacy_entry(monkeypatch): + """Codex P1: the override map shipped before the browser mirror did, storing only + llama_extra_args and max_seq_length. An upgraded install holds such an entry while + localStorage holds the context, KV cache, speculative and GPU settings, and an + entry-level skip would strand exactly what the migration exists to carry.""" + import routes.settings as settings_route + + store = _mock_override_store(monkeypatch) + + legacy = settings_route.ModelOverridePayload( + model_id = "unsloth/B-GGUF:Q4_K_M", + llama_extra_args = ["--flash-attn"], + max_seq_length = 8192, + ) + settings_route.update_openai_auto_switch_override(legacy, "tester") + + backfill = settings_route.ModelOverridePayload( + model_id = "unsloth/B-GGUF:Q4_K_M", + # The browser's own copy of a field the server already has, plus the ones + # only it holds. + max_seq_length = 2048, + custom_context_length = 32768, + kv_cache_dtype = "q8_0", + speculative_type = "ngram", + gpu_ids = [0, 1], + fill_absent_fields = True, + ) + resp = settings_route.update_openai_auto_switch_override(backfill, "tester") + entry = resp.overrides["unsloth/B-GGUF:Q4_K_M"] + # The server's own values survive untouched. + assert entry["max_seq_length"] == 8192 + assert entry["llama_extra_args"] == ["--flash-attn"] + # The browser-only settings are now there, so an API load applies them. + assert entry["custom_context_length"] == 32768 + assert entry["kv_cache_dtype"] == "q8_0" + assert entry["speculative_type"] == "ngram" + assert entry["gpu_ids"] == [0, 1] + # One entry, not two: the fill resolves onto the key a load reads. + assert list(store[settings.MODEL_OVERRIDES_SETTING_KEY]) == ["unsloth/B-GGUF:Q4_K_M"] + + # An ordinary save is still a replacement, or a settings edit could never + # clear a field. + edit = settings_route.ModelOverridePayload( + model_id = "unsloth/B-GGUF:Q4_K_M", max_seq_length = 4096 + ) + resp2 = settings_route.update_openai_auto_switch_override(edit, "tester") + assert resp2.overrides["unsloth/B-GGUF:Q4_K_M"]["max_seq_length"] == 4096 + assert "kv_cache_dtype" not in resp2.overrides["unsloth/B-GGUF:Q4_K_M"] + + +def test_fill_absent_fields_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.""" + be duplicated or emptied by a fill for the folded spelling.""" import routes.settings as settings_route _mock_override_store(monkeypatch) @@ -5233,32 +5289,32 @@ def test_only_if_absent_matches_a_legacy_casing_and_never_deletes(monkeypatch): 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 + model_id = "unsloth/b-gguf:q4_k_m", max_seq_length = 2048, fill_absent_fields = 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. + # An all-default fill 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 + model_id = "Unsloth/B-GGUF:Q4_K_M", fill_absent_fields = 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. + # A fill 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 + model_id = "Unsloth/B-GGUF:Q4_K_M", remove = True, fill_absent_fields = 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 +def test_fill_absent_fields_does_not_break_the_empty_payload_removal(monkeypatch): + """fill_absent_fields 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 @@ -5272,9 +5328,9 @@ def test_only_if_absent_does_not_break_the_empty_payload_removal(monkeypatch): 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.""" +def test_map_entry_fill_reads_and_writes_in_one_transaction(tmp_path, monkeypatch): + """The real store, not the in-memory stand-in: the read 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)) @@ -5282,19 +5338,23 @@ def test_map_entry_create_tests_and_writes_in_one_transaction(tmp_path, monkeypa 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) == { + # Present: the stored value stays, and a field it lacks is added. + assert db.upsert_app_setting_map_entry(key, "a", {"v": 2}, fill_absent_fields = True) == { "a": {"v": 1} } assert db.get_app_setting(key) == {"a": {"v": 1}} + assert db.upsert_app_setting_map_entry( + key, "a", {"v": 2, "w": 7}, fill_absent_fields = True + ) == {"a": {"v": 1, "w": 7}} + assert db.get_app_setting(key) == {"a": {"v": 1, "w": 7}} # Absent: created. - assert db.upsert_app_setting_map_entry(key, "b", {"v": 3}, only_if_absent = True) == { - "a": {"v": 1}, + assert db.upsert_app_setting_map_entry(key, "b", {"v": 3}, fill_absent_fields = True) == { + "a": {"v": 1, "w": 7}, "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}, + # A fill never deletes, even with nothing to store. + assert db.upsert_app_setting_map_entry(key, "a", None, fill_absent_fields = True) == { + "a": {"v": 1, "w": 7}, "b": {"v": 3}, } # The ordinary write still replaces and still removes. @@ -5346,3 +5406,148 @@ def test_gpu_ids_payload_is_bounded(): ) # The ordinary case is untouched. assert settings_route.ModelOverridePayload(model_id = "x", gpu_ids = [0, 1]).gpu_ids == [0, 1] + + +# ── codex round: the concrete key beats the advertised alias ────────── + + +def _switch_with_overrides(monkeypatch, resolves_to, stored, requested): + """Run the auto-switch hook against a real override map and return the load.""" + backend = _FakeBackend(None) + rec = _LoadRecorder(backend) + _wire(monkeypatch, enabled = True, resolves_to = resolves_to, backend = backend, recorder = rec) + _mock_override_store(monkeypatch) + for key, max_seq_length in stored.items(): + settings.set_model_override(key, max_seq_length = max_seq_length) + _run_hook(requested) + assert len(rec.calls) == 1 + return rec.calls[0] + + +def test_a_loose_gguf_prefers_its_path_keyed_settings_over_the_alias(monkeypatch): + """Codex: the settings UI keys a standalone .gguf by its bare path, while + override_id is the filename stem /v1/models advertises and an overrides PUT can + be written against. Reading the alias first let it shadow the saved settings for + good, so an API load kept applying the old flags.""" + path = "/srv/models/Qwen3-8B-Q4_K_M.gguf" + alias = "Qwen3-8B-Q4_K_M" + req = _switch_with_overrides( + monkeypatch, + resolves_to = (path, None, alias), + stored = {alias: 2048, path: 32768}, + requested = alias, + ) + assert req.max_seq_length == 32768 + + # The alias is still read when it is the only key, so an entry written against + # the advertised id keeps working. + req2 = _switch_with_overrides( + monkeypatch, + resolves_to = (path, None, alias), + stored = {alias: 2048}, + requested = alias, + ) + assert req2.max_seq_length == 2048 + + +def test_the_filename_label_key_no_longer_shadows_the_bare_path(monkeypatch): + """An early build of this feature keyed a standalone .gguf by the quant label + derived from its filename. Those entries stay readable, but the bare path the + picker writes today comes first.""" + path = "/srv/models/Qwen3-8B-Q4_K_M.gguf" + alias = "Qwen3-8B-Q4_K_M" + req = _switch_with_overrides( + monkeypatch, + resolves_to = (path, None, alias), + stored = {f"{path}:Q4_K_M": 2048, path: 32768}, + requested = alias, + ) + assert req.max_seq_length == 32768 + + req2 = _switch_with_overrides( + monkeypatch, + resolves_to = (path, None, alias), + stored = {f"{path}:Q4_K_M": 2048}, + requested = alias, + ) + assert req2.max_seq_length == 2048 + + +def test_a_variant_qualified_path_key_beats_the_same_quant_under_the_alias(monkeypatch): + """An LM Studio dir or a non-active HF cache is configured against its path, so + a same-quant entry under the repo id (another copy of the same repo, or a + hand-written PUT) must not win over the row the user actually edited.""" + path = "/srv/lmstudio/publisher/Qwen3-8B-GGUF" + repo = "publisher/Qwen3-8B-GGUF" + req = _switch_with_overrides( + monkeypatch, + resolves_to = (path, "Q4_K_M", repo), + stored = {f"{repo}:Q4_K_M": 2048, f"{path}:Q4_K_M": 32768}, + requested = f"{repo}:Q4_K_M", + ) + assert req.max_seq_length == 32768 + + +def test_a_cached_repo_still_resolves_by_its_repo_id(monkeypatch): + """The Hub keys a cached repo row by its repo id, which is the advertised id, + and no path entry exists for it, so it still resolves on the second try.""" + snapshot = "/mnt/old-cache/models--unsloth--Qwen3-8B-GGUF/snapshots/abc123" + repo = "unsloth/Qwen3-8B-GGUF" + req = _switch_with_overrides( + monkeypatch, + resolves_to = (snapshot, "Q4_K_M", repo), + stored = {f"{repo}:Q4_K_M": 32768}, + requested = f"{repo}:Q4_K_M", + ) + assert req.max_seq_length == 32768 + + # A bare entry under the repo id keeps working too. + req2 = _switch_with_overrides( + monkeypatch, + resolves_to = (snapshot, "Q4_K_M", repo), + stored = {repo: 16384}, + requested = f"{repo}:Q4_K_M", + ) + assert req2.max_seq_length == 16384 + + +def test_a_fill_does_not_replay_a_stored_flag_through_validation(monkeypatch): + """The migration now writes for entries it used to skip, and an omitted + llama_extra_args is normally carried over from the stored entry. Replaying a + flag that has been denylisted since it was saved would 400 the one-time + migration, which then retries on every start. A fill keeps the stored flags + without sending them back.""" + import routes.settings as settings_route + from core.inference import llama_server_args + + store = _mock_override_store(monkeypatch) + settings.set_model_override("unsloth/B-GGUF:Q4_K_M", llama_extra_args = ["--flash-attn"]) + + # The flag is refused from now on, as a later release's denylist would. + real_validate = llama_server_args.validate_extra_args + + def _reject_flash_attn(args): + if args and "--flash-attn" in args: + raise ValueError("--flash-attn is managed by the server.") + return real_validate(args) + + monkeypatch.setattr(llama_server_args, "validate_extra_args", _reject_flash_attn) + + fill = settings_route.ModelOverridePayload( + model_id = "unsloth/B-GGUF:Q4_K_M", custom_context_length = 32768, fill_absent_fields = True + ) + resp = settings_route.update_openai_auto_switch_override(fill, "tester") + entry = resp.overrides["unsloth/B-GGUF:Q4_K_M"] + assert entry["llama_extra_args"] == ["--flash-attn"] + assert entry["custom_context_length"] == 32768 + assert list(store[settings.MODEL_OVERRIDES_SETTING_KEY]) == ["unsloth/B-GGUF:Q4_K_M"] + + # An ordinary save still validates what it is handed. + with pytest.raises(HTTPException) as excinfo: + settings_route.update_openai_auto_switch_override( + settings_route.ModelOverridePayload( + model_id = "unsloth/C-GGUF", llama_extra_args = ["--flash-attn"] + ), + "tester", + ) + assert excinfo.value.status_code == 400 diff --git a/studio/backend/utils/openai_auto_switch_settings.py b/studio/backend/utils/openai_auto_switch_settings.py index 6958270ae3..936da15707 100644 --- a/studio/backend/utils/openai_auto_switch_settings.py +++ b/studio/backend/utils/openai_auto_switch_settings.py @@ -621,7 +621,7 @@ def set_model_override( llama_extra_args: Optional[list[str]] = None, max_seq_length: Optional[int] = None, *, - only_if_absent: bool = False, + fill_absent_fields: bool = False, **config: Any, ) -> dict: """Upsert one model's launch config; a config with no usable fields removes it. @@ -629,9 +629,9 @@ def set_model_override( 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. + ``fill_absent_fields`` writes only what is missing: an entry already stored + keeps every field it holds and gains only the ones it lacks. 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.") @@ -650,7 +650,7 @@ def set_model_override( MODEL_OVERRIDES_SETTING_KEY, model_id.strip(), entry or None, - only_if_absent = only_if_absent, + fill_absent_fields = fill_absent_fields, ) _invalidate(MODEL_OVERRIDES_SETTING_KEY) return entry diff --git a/studio/frontend/src/features/chat/index.ts b/studio/frontend/src/features/chat/index.ts index cfeb8fff0f..884502a67e 100644 --- a/studio/frontend/src/features/chat/index.ts +++ b/studio/frontend/src/features/chat/index.ts @@ -35,6 +35,7 @@ export type { GgufVariantDetail, InferenceStatusResponse, } from "./types/api"; +export { resolveInferenceCheckpointId } from "./lib/apply-inference-status-to-store"; export { ChatSettingsPanel, ParamSlider, diff --git a/studio/frontend/src/features/hub/hub-page.tsx b/studio/frontend/src/features/hub/hub-page.tsx index 154d73644f..ee310f6e62 100644 --- a/studio/frontend/src/features/hub/hub-page.tsx +++ b/studio/frontend/src/features/hub/hub-page.tsx @@ -6,6 +6,7 @@ import { getInferenceStatus, isExternalModelId, listGgufVariants, + resolveInferenceCheckpointId, useChatModelRuntime, useChatRuntimeStore, } from "@/features/chat"; @@ -384,16 +385,23 @@ export function ModelsPage() { void getInferenceStatus() .then((status) => { if (cancelled || !status.active_model) return; + // The loadable identifier, as every other status reader records it: a GGUF + // from a non-active HF cache or straight off disk loads by path, while + // active_model is the clean public id (an HF snapshot's repo id, any other + // file's filename stem). Two files that share a stem collapse onto one id, + // so storing that would make the catalog row for one of them look loaded. + const checkpointId = resolveInferenceCheckpointId(status); + if (!checkpointId) return; const store = useChatRuntimeStore.getState(); if ( !isExternalModelId(store.params.checkpoint) && - (!modelIdsMatch(store.params.checkpoint, status.active_model) || + (!modelIdsMatch(store.params.checkpoint, checkpointId) || !ggufVariantsMatch( store.activeGgufVariant, status.gguf_variant ?? null, )) ) { - store.setCheckpoint(status.active_model, status.gguf_variant ?? null); + store.setCheckpoint(checkpointId, status.gguf_variant ?? null); } }) .catch(() => undefined); diff --git a/studio/frontend/src/features/hub/lib/model-identity.ts b/studio/frontend/src/features/hub/lib/model-identity.ts index 8db1c015d1..01cc33632b 100644 --- a/studio/frontend/src/features/hub/lib/model-identity.ts +++ b/studio/frontend/src/features/hub/lib/model-identity.ts @@ -122,11 +122,19 @@ export function publicModelId(identifier: string): string { /** * 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. + * A GGUF loaded from an inactive HF cache is loaded by path, but a caller holding + * only the public id would read an exact comparison against the catalog row's path + * as "not loaded" and fall back to saved or default values instead of the live + * launch config. Candidates are compared literally first, then by the public id. + * + * That second pass only accepts an identity that can name one model: an HF cache + * snapshot collapses onto its repo id, which is globally unique, while every other + * path collapses onto a filename or directory stem that two models can share + * (`/models/alpha/model.gguf` and `/models/beta/model.gguf` are both "model"). + * Accepting a stem would mark the wrong row resident, seeding its editor with + * another model's live config and saving it under this model's key. Callers with + * the loadable identifier (`/status`'s `model_identifier`) pass it as the active + * id, and the literal pass answers exactly. */ export function residentModelIdMatches( activeModelId: string | null | undefined, @@ -142,7 +150,12 @@ export function residentModelIdMatches( } return candidates.some((candidate) => { const trimmed = candidate?.trim(); - return trimmed ? modelIdsMatch(active, publicModelId(trimmed)) : false; + if (!trimmed) { + return false; + } + const publicId = publicModelId(trimmed); + // Unambiguous only when the collapse produced a namespaced repo id. + return publicId.includes("/") && modelIdsMatch(active, publicId); }); } 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 1a82d09f0b..74fb36c8c9 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 @@ -17,10 +17,12 @@ import { isDefaultConfig, listPerModelConfigs, } from "../model-config/per-model-config"; +import type { ApiModelOverride } from "./model-overrides"; import { fetchModelOverrides, modelOverrideKey, putModelOverride, + toApiOverride, } from "./model-overrides"; const DONE_FLAG = "unsloth_model_overrides_backfilled_v1"; @@ -62,9 +64,32 @@ function normalizedOverrideKey(key: string): string { } /** - * Push local configs the server has never seen. Never deletes and never overwrites: - * an entry already there is the newer authority, and losing a setting would be - * worse than leaving one unmigrated. + * The fields *config* would contribute that the stored entry does not hold. + * + * A malformed entry (nothing constrains what an older install wrote into + * app_settings) counts as holding nothing, so the migration still runs. + */ +function absentFields( + stored: ApiModelOverride, + config: Parameters[0], +): string[] { + const fields = Object.keys(toApiOverride(config)); + if (typeof stored !== "object" || stored === null) { + return fields; + } + return fields.filter((field) => !(field in stored)); +} + +/** + * Push local settings the server does not hold. Never deletes and never overwrites: + * a value already there is the newer authority, and losing a setting would be worse + * than leaving one unmigrated. + * + * Field by field, not entry by entry. The override map shipped before this browser + * mirror did, storing only llama_extra_args and max_seq_length, so an upgraded + * install can hold an entry for a model whose context, KV cache, speculative and GPU + * settings live only here. Treating the key as done would skip exactly the settings + * this migration exists to carry and then mark it complete. */ export async function backfillModelOverrides(): Promise { if (alreadyRan()) { @@ -97,7 +122,10 @@ export async function backfillModelOverrides(): Promise { return; } - const known = new Set(Object.keys(existing).map(normalizedOverrideKey)); + const known = new Map(); + for (const [storedKey, storedEntry] of Object.entries(existing)) { + known.set(normalizedOverrideKey(storedKey), storedEntry); + } let failed = false; for (const entry of local) { @@ -106,9 +134,6 @@ export async function backfillModelOverrides(): Promise { const key = normalizedOverrideKey( modelOverrideKey(entry.modelId, entry.ggufVariant), ); - if (known.has(key)) { - continue; - } // Re-read rather than trusting the snapshot from before the fetch: this write // is queued behind the interactive one and commits last, so a save or forget // during the round trip would be undone by it. @@ -121,15 +146,20 @@ export async function backfillModelOverrides(): Promise { if (!current || isDefaultConfig(current.config)) { continue; } + const stored = known.get(key); + // Nothing this browser could add, so skip the round trip entirely. + if (stored && absentFields(stored, current.config).length === 0) { + 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. + // Fills the gaps only. `known` is a snapshot from before this loop started, so + // a save by another tab during the pass is invisible here; the server reads and + // writes together rather than this re-fetching once per model. await putModelOverride( current.modelId, current.ggufVariant, current.config, - { onlyIfAbsent: true }, + { fillAbsentFields: 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 ffd5b39779..3f58bbc66b 100644 --- a/studio/frontend/src/features/model-picker/api/model-overrides.ts +++ b/studio/frontend/src/features/model-picker/api/model-overrides.ts @@ -78,8 +78,11 @@ export async function fetchModelOverrides(): Promise { * Only fields the user set are sent: the backend reads an absent field as "app * default", so nulls would pin defaults and stop the model following later global * changes. A `null` config means "no saved settings", which clears the entry. + * + * Exported so the one-time backfill can ask what this config would contribute and + * compare it against the entry already on the server, field by field. */ -function toApiOverride(config: PerModelConfig | null): ApiModelOverride { +export function toApiOverride(config: PerModelConfig | null): ApiModelOverride { if (!config) { return {}; } @@ -130,14 +133,16 @@ const writesByKey = new Map>(); export interface PutModelOverrideOptions { /** - * Create only: leave an entry already on the server exactly as it is. + * Fill in only what is missing: every value already on the server stays 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. + * 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 reads and writes under one transaction, which + * closes the window without a round trip per model. Field level, so an entry an + * older release stored with only its two fields still gains the browser-only ones + * without the server losing anything it holds. */ - onlyIfAbsent?: boolean; + fillAbsentFields?: boolean; } export async function putModelOverride( @@ -183,9 +188,9 @@ async function sendModelOverride( 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 + ...(options?.fillAbsentFields ? // biome-ignore lint/style/useNamingConvention: API schema - { only_if_absent: true } + { fill_absent_fields: 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 diff --git a/studio/frontend/tests/model-settings-identity.test.ts b/studio/frontend/tests/model-settings-identity.test.ts index 33c1fdf037..04311ec902 100644 --- a/studio/frontend/tests/model-settings-identity.test.ts +++ b/studio/frontend/tests/model-settings-identity.test.ts @@ -39,14 +39,15 @@ test("publicModelId mirrors what /status reports for a path-loaded model", () => }); 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. + // A loose .gguf: the catalog row is keyed by the path, and the Hub page records + // the loadable identifier (status.model_identifier), so the literal pass answers. 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", "/srv/models/Qwen3-8B-Q4_K_M.gguf", ), @@ -92,6 +93,37 @@ test("a resident path-loaded model is matched by the id /status reports", () => assert.equal(residentModelIdMatches("Qwen3-8B-Q4_K_M"), false); }); +test("a shared filename or folder name never marks a row resident", () => { + // Two loose GGUFs with the same filename in different folders collapse onto one + // public id, so a stem can only say "one of these", never which. + const loaded = "/srv/models/alpha/model.gguf"; + const other = "/srv/models/beta/model.gguf"; + assert.equal(publicModelId(loaded), publicModelId(other)); + assert.equal(residentModelIdMatches(publicModelId(loaded), other, other), false); + // The loadable identifier names exactly one of them. + assert.equal(residentModelIdMatches(loaded, loaded, loaded), true); + assert.equal(residentModelIdMatches(loaded, other, other), false); + + // Same collapse one level up: two model directories sharing a basename. + const loadedDir = "/srv/lmstudio/publisher-a/Llama-3-8B-GGUF"; + const otherDir = "/srv/models/publisher-b/Llama-3-8B-GGUF"; + assert.equal(publicModelId(loadedDir), publicModelId(otherDir)); + assert.equal( + residentModelIdMatches(publicModelId(loadedDir), otherDir, otherDir), + false, + ); + + // A cache snapshot still collapses onto its repo id, which names one model. + assert.equal( + residentModelIdMatches( + "unsloth/Qwen3-8B-GGUF", + "/mnt/old-cache/models--unsloth--Qwen3-8B-GGUF/snapshots/abc123", + null, + ), + true, + ); +}); + 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( diff --git a/tests/studio/test_model_picker_contracts.py b/tests/studio/test_model_picker_contracts.py index ca8c8bfc9a..7ab8642e16 100644 --- a/tests/studio/test_model_picker_contracts.py +++ b/tests/studio/test_model_picker_contracts.py @@ -695,8 +695,8 @@ def test_backfill_compares_server_keys_by_normalized_identity(): src = " ".join(_read("features/model-picker/api/migrate-model-overrides.ts").split()) assert "function normalizedOverrideKey(" in src # Folded on both sides: the older `id::variant` local keys are not. - assert "const known = new Set(Object.keys(existing).map(normalizedOverrideKey));" in src - assert "if (known.has(key)) { continue; }" in src + assert "known.set(normalizedOverrideKey(storedKey), storedEntry);" in src + assert "const stored = known.get(key);" in src # A quant-aware split, so a Windows drive letter is not read as a separator. assert "const split = splitQuantSuffix(key);" in src # Repo ids fold and POSIX paths do not, which is what these do. @@ -957,27 +957,42 @@ def test_the_chat_picker_marks_ollama_targets_unloadable_by_the_api(): assert 'seg in (".studio_links", "ollama_links")' in resolver, "the rule this mirrors" -def test_the_backfill_writes_are_creates_not_replacements(): +def test_the_backfill_fills_in_fields_rather_than_skipping_known_keys(): """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.""" + older localStorage copy. The server reads and writes under one transaction + rather than this re-fetching per model, and it does so field by field: the + override map shipped before this browser mirror did, holding only + llama_extra_args and max_seq_length, so an entry-level skip would strand the + context, KV cache, speculative and GPU settings this migration exists to carry.""" backfill = " ".join(_read("features/model-picker/api/migrate-model-overrides.ts").split()) - assert "{ onlyIfAbsent: true }," in backfill + assert "{ fillAbsentFields: true }," in backfill + # Key presence alone is not "done": what the server lacks decides. + assert "const stored = known.get(key);" in backfill + assert "if (stored && absentFields(stored, current.config).length === 0) { continue; }" in backfill + assert "const fields = Object.keys(toApiOverride(config));" in backfill + assert "return fields.filter((field) => !(field in stored));" 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( + assert "fillAbsentFields?: boolean;" in api + assert "options?.fillAbsentFields ? { fill_absent_fields: 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 + assert "syncModelOverride" in api and "fill_absent_fields: 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 + assert "fill_absent_fields: bool = False" in route, "the rule this mirrors" + assert "fill_absent_fields = payload.fill_absent_fields," 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 + assert '"remove", "fill_absent_fields"' in route + + # The merge is the server's, under the write's own transaction: a client-side + # read-modify-write would reopen the race the conditional write closed. + db = (WORKDIR / "studio" / "backend" / "storage" / "studio_db.py").read_text(encoding = "utf-8") + assert "merged = {**entry_value, **stored}" in db + assert "BEGIN IMMEDIATE" in db def test_the_hub_settings_page_matches_a_resident_path_loaded_model(): @@ -992,10 +1007,21 @@ def test_the_hub_settings_page_matches_a_resident_path_loaded_model(): ) assert "loadedConfig={settingsTargetIsResident ? activeModelConfig : null}" in hub assert "settingsTargetIsResident ? activeGgufContextLength : null" in hub + # The loadable identifier, as every other status reader records it. active_model + # is the clean public id, and two files sharing a filename collapse onto one, so + # storing it would let the wrong catalog row look loaded. + assert "const checkpointId = resolveInferenceCheckpointId(status);" in hub + assert "store.setCheckpoint(checkpointId, status.gguf_variant ?? null);" in hub + assert "setCheckpoint(status.active_model" not in hub + chat = " ".join(_read("features/chat/lib/apply-inference-status-to-store.ts").split()) + assert "return status.model_identifier ?? status.active_model;" in chat, "the rule this mirrors" # 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 + # Only a namespaced repo id names one model; a filename or directory stem does + # not, so it must never stand in for the loaded model's identity. + assert 'return publicId.includes("/") && modelIdsMatch(active, publicId);' in identity backend = (WORKDIR / "studio" / "backend" / "core" / "inference" / "model_ids.py").read_text( encoding = "utf-8" )