studio: address fourth review round on per-model settings
- Never case-fold a filesystem path when looking up an override. Two POSIX paths differing only in case are two different files, so a near miss must load defaults rather than replay another model's context and GPU pin. Repo ids still fold, which is what the migration needs. - Match a quant suffix against the loader's own quant pattern instead of a length heuristic. "/models/foo:bar.gguf" is one valid POSIX filename, and splitting it grafted /models/foo's launch flags onto an unrelated model. - Retry a load once without the saved gpu_ids when the loader rejects the pin. The pre-flight check cannot mirror every rule the loader applies (a Vulkan diffusion GGUF refuses GPU selection outright, and the rules move), so this stops chasing them one at a time: a stale placement preference must never be the reason a request cannot be served. - Make an explicit remove win over config fields sent in the same payload. - Seed only finished requests on the monitor's first snapshot. A request still running when Studio loads is traffic the user has not seen, not history. - Nudge the detail effect when the in-flight guard refuses a fetch. Nothing else changes its deps when the older fetch settles, so a terminal reply could stay truncated forever. - Invalidate pending row lookups when opening settings from a detail card, not just from a row. - Mark the covered detail pane inert so it leaves the focus order. - Refuse to open settings for a variant-required GGUF whose quant could not be resolved: the picker matches variants exactly and would never find the saved config, while the API falls back to the bare key and would apply it. - Mirror to the server only for GGUFs. The auto-switch resolver indexes GGUFs, so a safetensors config was being advertised as applied on API load when no API request could ever apply it.
This commit is contained in:
parent
0a49dfd047
commit
9c722b9059
9 changed files with 274 additions and 32 deletions
|
|
@ -3779,12 +3779,39 @@ async def _maybe_auto_switch_model(
|
|||
# Reuse the load impl so its dedup, tensor fallback, and threading
|
||||
# apply. Call the impl directly: we already hold the lifecycle gate
|
||||
# the /load route would otherwise take, so the route would deadlock.
|
||||
await _load_model_impl(
|
||||
LoadRequest(**load_kwargs),
|
||||
fastapi_request,
|
||||
current_subject,
|
||||
current_request_counted = True,
|
||||
)
|
||||
try:
|
||||
await _load_model_impl(
|
||||
LoadRequest(**load_kwargs),
|
||||
fastapi_request,
|
||||
current_subject,
|
||||
current_request_counted = True,
|
||||
)
|
||||
except HTTPException as exc:
|
||||
# The pre-flight check above cannot mirror every rule the
|
||||
# loader applies to gpu_ids (a Vulkan diffusion GGUF refuses
|
||||
# GPU selection outright, and the rules move). Rather than
|
||||
# duplicating them, retry once without the saved pin: a
|
||||
# stale placement preference must never be the reason an
|
||||
# API request cannot be served.
|
||||
if not (
|
||||
exc.status_code == 400
|
||||
and load_kwargs.get("gpu_ids")
|
||||
and "gpu" in str(exc.detail).lower()
|
||||
):
|
||||
raise
|
||||
logger.warning(
|
||||
"Retrying %s without saved gpu_ids %s: %s",
|
||||
override_id,
|
||||
load_kwargs.get("gpu_ids"),
|
||||
exc.detail,
|
||||
)
|
||||
load_kwargs.pop("gpu_ids", None)
|
||||
await _load_model_impl(
|
||||
LoadRequest(**load_kwargs),
|
||||
fastapi_request,
|
||||
current_subject,
|
||||
current_request_counted = True,
|
||||
)
|
||||
# Advertise the repo id (not the concrete load path) as the loaded
|
||||
# model's public id and override key for /v1/models and idle stash.
|
||||
get_llama_cpp_backend()._openai_advertised_id = override_id
|
||||
|
|
|
|||
|
|
@ -126,8 +126,10 @@ class OpenAIAutoSwitchResponse(BaseModel):
|
|||
auto_unload_keep_kv: bool = DEFAULT_AUTO_UNLOAD_KEEP_KV
|
||||
|
||||
|
||||
# A quant suffix, as modelOverrideKey builds it: no path separator and short.
|
||||
# Guards against splitting "C:\\models\\x.gguf", where the colon is a drive letter.
|
||||
# A quant suffix, as modelOverrideKey builds it. Matched against the loader's own
|
||||
# quant pattern rather than a length heuristic: a POSIX path may legitimately
|
||||
# contain a colon ("/models/foo:bar.gguf"), and treating "bar.gguf" as a quant
|
||||
# would graft an unrelated model's launch flags onto this one.
|
||||
_MAX_VARIANT_SUFFIX_LEN = 64
|
||||
|
||||
# A local model's id is its filesystem path, optionally with a quant suffix, and
|
||||
|
|
@ -342,11 +344,16 @@ def get_openai_auto_switch_overrides(
|
|||
|
||||
def _bare_model_id(model_id: str) -> Optional[str]:
|
||||
"""``repo`` for a ``repo:QUANT`` key, or None when there is no quant suffix."""
|
||||
from core.inference.llama_cpp import _GGUF_KNOWN_QUANT_RE
|
||||
|
||||
head, sep, tail = model_id.rpartition(":")
|
||||
if not sep or not head or not tail:
|
||||
return None
|
||||
if len(tail) > _MAX_VARIANT_SUFFIX_LEN or "/" in tail or "\\" in tail:
|
||||
return None
|
||||
# Must actually look like a quant, not just like a short path segment.
|
||||
if _GGUF_KNOWN_QUANT_RE.fullmatch(tail) is None:
|
||||
return None
|
||||
return head
|
||||
|
||||
|
||||
|
|
@ -382,21 +389,28 @@ def update_openai_auto_switch_override(
|
|||
if bare_id:
|
||||
requested_extra_args = get_model_override(bare_id).get("llama_extra_args")
|
||||
extra_args = validate_extra_args(requested_extra_args)
|
||||
set_model_override(
|
||||
payload.model_id,
|
||||
llama_extra_args = extra_args,
|
||||
max_seq_length = payload.max_seq_length,
|
||||
custom_context_length = payload.custom_context_length,
|
||||
kv_cache_dtype = payload.kv_cache_dtype,
|
||||
speculative_type = payload.speculative_type,
|
||||
spec_draft_n_max = payload.spec_draft_n_max,
|
||||
tensor_parallel = payload.tensor_parallel,
|
||||
chat_template_override = payload.chat_template_override,
|
||||
gpu_memory_mode = payload.gpu_memory_mode,
|
||||
gpu_layers = payload.gpu_layers,
|
||||
n_cpu_moe = payload.n_cpu_moe,
|
||||
gpu_ids = payload.gpu_ids,
|
||||
)
|
||||
if payload.remove is True:
|
||||
# An explicit remove wins over anything else in the payload: a stale
|
||||
# form field must not turn "forget this model" into an update that
|
||||
# keeps it. Only the explicit flag short-circuits; the legacy
|
||||
# inferred path still just gates launch-flag carry-over.
|
||||
set_model_override(payload.model_id, llama_extra_args = [], max_seq_length = None)
|
||||
else:
|
||||
set_model_override(
|
||||
payload.model_id,
|
||||
llama_extra_args = extra_args,
|
||||
max_seq_length = payload.max_seq_length,
|
||||
custom_context_length = payload.custom_context_length,
|
||||
kv_cache_dtype = payload.kv_cache_dtype,
|
||||
speculative_type = payload.speculative_type,
|
||||
spec_draft_n_max = payload.spec_draft_n_max,
|
||||
tensor_parallel = payload.tensor_parallel,
|
||||
chat_template_override = payload.chat_template_override,
|
||||
gpu_memory_mode = payload.gpu_memory_mode,
|
||||
gpu_layers = payload.gpu_layers,
|
||||
n_cpu_moe = payload.n_cpu_moe,
|
||||
gpu_ids = payload.gpu_ids,
|
||||
)
|
||||
except ValueError as exc:
|
||||
raise log_and_http_error(
|
||||
exc,
|
||||
|
|
|
|||
|
|
@ -4264,3 +4264,143 @@ def test_request_used_api_key_distinguishes_key_from_session():
|
|||
# A malformed request object must read as "not an API key", never raise, since
|
||||
# this runs on the hot path of every tracked request.
|
||||
assert inference_route._request_used_api_key(object()) is False
|
||||
|
||||
|
||||
def test_case_fallback_never_applies_to_a_posix_path(monkeypatch):
|
||||
# Two files that differ only in case are two different models on Linux, so a
|
||||
# near miss must load defaults rather than another model's context and GPU pin.
|
||||
_mock_override_store(monkeypatch)
|
||||
settings.set_model_override("/models/foo.gguf", max_seq_length = 8192, gpu_ids = [1])
|
||||
assert settings.get_model_override("/models/Foo.gguf") == {}
|
||||
assert settings.get_model_override("/models/foo.gguf")["max_seq_length"] == 8192
|
||||
|
||||
|
||||
def test_case_fallback_never_applies_to_a_windows_path(monkeypatch):
|
||||
_mock_override_store(monkeypatch)
|
||||
settings.set_model_override(r"C:\models\foo.gguf", max_seq_length = 8192)
|
||||
assert settings.get_model_override(r"C:\models\FOO.gguf") == {}
|
||||
|
||||
|
||||
def test_case_fallback_still_covers_repo_ids(monkeypatch):
|
||||
# The migration case this fallback exists for.
|
||||
_mock_override_store(monkeypatch)
|
||||
settings.set_model_override("unsloth/qwen3-8b-gguf:q4_k_m", max_seq_length = 8192)
|
||||
assert settings.get_model_override("unsloth/Qwen3-8B-GGUF:Q4_K_M")["max_seq_length"] == 8192
|
||||
|
||||
|
||||
def test_explicit_remove_wins_over_config_fields_in_the_same_payload(monkeypatch):
|
||||
# remove is the operation discriminator, so a stale form field alongside it
|
||||
# must not quietly turn "forget this model" into an update.
|
||||
import routes.settings as settings_route
|
||||
|
||||
_mock_override_store(monkeypatch)
|
||||
settings.set_model_override("unsloth/B-GGUF", max_seq_length = 4096)
|
||||
resp = settings_route.update_openai_auto_switch_override(
|
||||
settings_route.ModelOverridePayload(
|
||||
model_id = "unsloth/B-GGUF", remove = True, max_seq_length = 8192, tensor_parallel = True
|
||||
),
|
||||
"tester",
|
||||
)
|
||||
assert "unsloth/B-GGUF" not in resp.overrides
|
||||
|
||||
|
||||
def test_posix_colon_in_a_path_is_not_treated_as_a_quant(monkeypatch):
|
||||
# "/models/foo:bar.gguf" is one valid POSIX filename, not repo + quant.
|
||||
# Splitting it would graft /models/foo's launch flags onto a different model.
|
||||
import routes.settings as settings_route
|
||||
|
||||
_mock_override_store(monkeypatch)
|
||||
settings.set_model_override("/models/foo", llama_extra_args = ["--flash-attn"])
|
||||
resp = settings_route.update_openai_auto_switch_override(
|
||||
settings_route.ModelOverridePayload(model_id = "/models/foo:bar.gguf", max_seq_length = 4096),
|
||||
"tester",
|
||||
)
|
||||
assert "llama_extra_args" not in resp.overrides["/models/foo:bar.gguf"]
|
||||
|
||||
|
||||
def test_real_quant_suffix_on_a_path_still_carries_flags_over(monkeypatch):
|
||||
import routes.settings as settings_route
|
||||
|
||||
_mock_override_store(monkeypatch)
|
||||
settings.set_model_override("/models/x.gguf", llama_extra_args = ["--flash-attn"])
|
||||
resp = settings_route.update_openai_auto_switch_override(
|
||||
settings_route.ModelOverridePayload(model_id = "/models/x.gguf:Q4_K_M", max_seq_length = 4096),
|
||||
"tester",
|
||||
)
|
||||
assert resp.overrides["/models/x.gguf:Q4_K_M"]["llama_extra_args"] == ["--flash-attn"]
|
||||
|
||||
|
||||
def test_load_retries_without_gpu_ids_when_the_loader_rejects_the_pin(monkeypatch):
|
||||
# The pre-flight check cannot mirror every rule the loader applies (a Vulkan
|
||||
# diffusion GGUF refuses GPU selection outright). A stale placement preference
|
||||
# must never be the reason a request cannot be served.
|
||||
from fastapi import HTTPException
|
||||
|
||||
backend = _FakeBackend(None)
|
||||
rec = _LoadRecorder(backend)
|
||||
_wire(
|
||||
monkeypatch,
|
||||
enabled = True,
|
||||
resolves_to = ("unsloth/B-GGUF", "Q4_K_M", "unsloth/B-GGUF"),
|
||||
backend = backend,
|
||||
recorder = rec,
|
||||
)
|
||||
monkeypatch.setattr(
|
||||
settings, "get_model_override", lambda mid: {"gpu_ids": [0], "max_seq_length": 4096}
|
||||
)
|
||||
|
||||
async def _usable(ids):
|
||||
return True
|
||||
|
||||
monkeypatch.setattr(inference_route, "_override_gpu_ids_still_resolve", _usable)
|
||||
|
||||
calls = {"n": 0}
|
||||
|
||||
async def _load(request, *args, **kwargs):
|
||||
calls["n"] += 1
|
||||
if calls["n"] == 1:
|
||||
raise HTTPException(
|
||||
status_code = 400,
|
||||
detail = "GPU selection (gpu_ids) is not supported for a DiffusionGemma GGUF",
|
||||
)
|
||||
return await rec(request, *args, **kwargs)
|
||||
|
||||
monkeypatch.setattr(inference_route, "_load_model_impl", _load)
|
||||
|
||||
_run_hook("unsloth/B-GGUF")
|
||||
assert calls["n"] == 2
|
||||
served = rec.calls[-1]
|
||||
assert not served.gpu_ids
|
||||
assert served.max_seq_length == 4096
|
||||
|
||||
|
||||
def test_a_non_gpu_load_failure_is_not_retried(monkeypatch):
|
||||
from fastapi import HTTPException
|
||||
|
||||
backend = _FakeBackend(None)
|
||||
rec = _LoadRecorder(backend)
|
||||
_wire(
|
||||
monkeypatch,
|
||||
enabled = True,
|
||||
resolves_to = ("unsloth/B-GGUF", "Q4_K_M", "unsloth/B-GGUF"),
|
||||
backend = backend,
|
||||
recorder = rec,
|
||||
)
|
||||
monkeypatch.setattr(settings, "get_model_override", lambda mid: {"gpu_ids": [0]})
|
||||
|
||||
async def _usable(ids):
|
||||
return True
|
||||
|
||||
monkeypatch.setattr(inference_route, "_override_gpu_ids_still_resolve", _usable)
|
||||
|
||||
calls = {"n": 0}
|
||||
|
||||
async def _load(request, *args, **kwargs):
|
||||
calls["n"] += 1
|
||||
raise HTTPException(status_code = 400, detail = "Corrupt GGUF header")
|
||||
|
||||
monkeypatch.setattr(inference_route, "_load_model_impl", _load)
|
||||
|
||||
with pytest.raises(HTTPException):
|
||||
_run_hook("unsloth/B-GGUF")
|
||||
assert calls["n"] == 1
|
||||
|
|
|
|||
|
|
@ -426,6 +426,14 @@ def model_override_load_kwargs(override: dict[str, Any], *, is_gguf: bool) -> di
|
|||
return kwargs
|
||||
|
||||
|
||||
def _looks_like_filesystem_path(model_id: str) -> bool:
|
||||
"""True for an absolute path id, as the ./models and LM Studio scanners emit."""
|
||||
if model_id.startswith(("/", "\\")):
|
||||
return True
|
||||
# Windows drive letter, e.g. "C:\models\x.gguf".
|
||||
return len(model_id) >= 3 and model_id[1] == ":" and model_id[2] in ("\\", "/")
|
||||
|
||||
|
||||
def get_model_overrides() -> dict[str, dict]:
|
||||
"""Per-model launch configs keyed by model id (see normalize_model_override)."""
|
||||
raw = _cached_setting(MODEL_OVERRIDES_SETTING_KEY, None)
|
||||
|
|
@ -448,6 +456,11 @@ def get_model_override(model_id: str) -> dict:
|
|||
return override
|
||||
if not isinstance(model_id, str):
|
||||
return {}
|
||||
# Only repo-style ids fold. A POSIX path is case-sensitive and names a
|
||||
# different file, so matching "/models/Foo.gguf" against an entry saved for
|
||||
# "/models/foo.gguf" would replay another model's context and GPU pin.
|
||||
if _looks_like_filesystem_path(model_id):
|
||||
return {}
|
||||
folded = model_id.casefold()
|
||||
matches = [
|
||||
value
|
||||
|
|
|
|||
|
|
@ -168,8 +168,17 @@ export function ApiMonitorOverlay(): ReactElement | null {
|
|||
const ids = data.entries.map((entry) => entry.id);
|
||||
if (!seededRef.current) {
|
||||
seededRef.current = true;
|
||||
seenIdsRef.current = new Set(ids);
|
||||
return;
|
||||
// Seed finished requests only. A request that is still running when the
|
||||
// first snapshot lands started while Studio was loading, so it is live
|
||||
// traffic the user has not seen, not history to adopt silently.
|
||||
seenIdsRef.current = new Set(
|
||||
data.entries
|
||||
.filter((entry) => entry.status !== "running")
|
||||
.map((entry) => entry.id),
|
||||
);
|
||||
if (!data.entries.some((e) => e.via_api_key && e.status === "running")) {
|
||||
return;
|
||||
}
|
||||
}
|
||||
const seen = seenIdsRef.current;
|
||||
// Only API-key traffic counts. Studio's own chat goes through these same
|
||||
|
|
|
|||
|
|
@ -486,6 +486,7 @@ export function ApiMonitorPage(): ReactElement {
|
|||
const selectedUpdatedAt = selected?.updated_at ?? null;
|
||||
const selectedIsMissing = selectedId_ != null && details[selectedId_] == null;
|
||||
const lastFetchedRef = useRef<string | null>(null);
|
||||
const [retryTick, setRetryTick] = useState(0);
|
||||
useEffect(() => {
|
||||
if (selectedId_ == null) {
|
||||
return;
|
||||
|
|
@ -500,8 +501,21 @@ export function ApiMonitorPage(): ReactElement {
|
|||
// guard can refuse, and recording it anyway skips that revision for good.
|
||||
if (requestDetail(selectedId_)) {
|
||||
lastFetchedRef.current = revision;
|
||||
setRetryTick(0);
|
||||
} else {
|
||||
// Refused because an older fetch is still running. Nothing in this effect's
|
||||
// deps will change when that one settles, so without a nudge a revision
|
||||
// rejected here is never fetched, and a terminal reply stays truncated.
|
||||
const timer = window.setTimeout(() => setRetryTick((n) => n + 1), 250);
|
||||
return () => window.clearTimeout(timer);
|
||||
}
|
||||
}, [selectedId_, selectedUpdatedAt, selectedIsMissing, requestDetail]);
|
||||
}, [
|
||||
selectedId_,
|
||||
selectedUpdatedAt,
|
||||
selectedIsMissing,
|
||||
requestDetail,
|
||||
retryTick,
|
||||
]);
|
||||
|
||||
// The desktop webview's origin is tauri://, not the API server, and the
|
||||
// packaged app picks its port dynamically. Same source as the Agents tab.
|
||||
|
|
|
|||
|
|
@ -23,6 +23,7 @@ import {
|
|||
} from "@/features/model-picker";
|
||||
import { useDebouncedValue } from "@/hooks/use-debounced-value";
|
||||
import { useGpuInfo } from "@/hooks/use-gpu-info";
|
||||
import { toast } from "@/lib/toast";
|
||||
import { cn } from "@/lib/utils";
|
||||
import { useNavigate, useSearch } from "@tanstack/react-router";
|
||||
import {
|
||||
|
|
@ -1276,11 +1277,20 @@ export function ModelsPage() {
|
|||
downloaded[0]?.quant ??
|
||||
null;
|
||||
} catch {
|
||||
// Offline or an unreadable cache: fall through with no variant. The
|
||||
// settings page still works, it just cannot pin a specific quant.
|
||||
ggufVariant = null;
|
||||
}
|
||||
}
|
||||
if (!ggufVariant) {
|
||||
// A model that needs a quant cannot be configured without one: the
|
||||
// picker matches variants exactly and would never find the saved
|
||||
// config, while the API falls back to the bare key and would apply it.
|
||||
// Opening the editor here would quietly create that mismatch.
|
||||
toast.error("Couldn't determine which quant to configure.", {
|
||||
description:
|
||||
"Settings for this model are per quant. Check the connection or the model's cache, then try again.",
|
||||
});
|
||||
return;
|
||||
}
|
||||
}
|
||||
// The variant lookup above is async, so a second row opened while it was
|
||||
// pending would otherwise be overwritten by whichever call finished last.
|
||||
|
|
@ -1349,6 +1359,9 @@ export function ModelsPage() {
|
|||
const openSelectedModelSettings = useCallback(
|
||||
(ggufVariant: string | null) => {
|
||||
if (!selectedModel) return;
|
||||
// Share the sequence with openModelSettings: a row's variant lookup may
|
||||
// still be pending, and it must not land on top of this one.
|
||||
settingsOpenSeq.current += 1;
|
||||
const id = selectedModel.resource.runId;
|
||||
const leaf = id.split(/[\\/]/).filter(Boolean).pop() ?? id;
|
||||
setSettingsTarget({
|
||||
|
|
@ -1751,7 +1764,10 @@ export function ModelsPage() {
|
|||
|
||||
{splitMode ? (
|
||||
detailOpen ? (
|
||||
<div className="hub-canvas z-20 flex min-h-0 flex-col max-lg:absolute max-lg:inset-0 lg:relative lg:min-w-0 lg:flex-1">
|
||||
<div
|
||||
className="hub-canvas z-20 flex min-h-0 flex-col max-lg:absolute max-lg:inset-0 lg:relative lg:min-w-0 lg:flex-1"
|
||||
inert={settingsTarget !== null || undefined}
|
||||
>
|
||||
<HubDetailView
|
||||
model={selectedModel}
|
||||
isDataset={isDatasetMode}
|
||||
|
|
@ -1770,7 +1786,10 @@ export function ModelsPage() {
|
|||
)
|
||||
) : (
|
||||
detailOpen && (
|
||||
<div className="hub-canvas absolute inset-0 z-20 flex min-h-0 flex-col">
|
||||
<div
|
||||
className="hub-canvas absolute inset-0 z-20 flex min-h-0 flex-col"
|
||||
inert={settingsTarget !== null || undefined}
|
||||
>
|
||||
<HubDetailView
|
||||
model={selectedModel}
|
||||
isDataset={isDatasetMode}
|
||||
|
|
|
|||
|
|
@ -47,7 +47,10 @@ export async function backfillModelOverrides(): Promise<void> {
|
|||
return;
|
||||
}
|
||||
const local = listPerModelConfigs().filter(
|
||||
(entry) => !isDefaultConfig(entry.config),
|
||||
// A quant means it is a GGUF, which is the only thing API auto-switch
|
||||
// resolves. Backfilling a safetensors config would claim an API behaviour
|
||||
// that does not exist.
|
||||
(entry) => entry.ggufVariant != null && !isDefaultConfig(entry.config),
|
||||
);
|
||||
if (local.length === 0) {
|
||||
markRan();
|
||||
|
|
|
|||
|
|
@ -886,7 +886,10 @@ export function ModelConfigPage({
|
|||
// Skipped when the local write failed (quota, a future-schema entry): the
|
||||
// browser and the server would otherwise permanently disagree about this
|
||||
// model, with no way for the user to tell which one the next load used.
|
||||
if (!saveFailed) {
|
||||
// GGUF only: the API auto-switch resolver indexes GGUFs, so mirroring a
|
||||
// safetensors config to the server would advertise settings on the monitor's
|
||||
// "applied on API load" list that no API request can ever apply.
|
||||
if (!saveFailed && target.isGguf) {
|
||||
syncModelOverride(
|
||||
target.id,
|
||||
target.ggufVariant,
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue