studio: address third review round on per-model settings
- Fall back to a case-insensitive override lookup. The browser normalizes ids to lowercase before storing, so the backfill wrote "org/model:q4_k_m" while the resolver asked for "Org/Model:Q4_K_M" and never matched, which made the whole migration a no-op. Exact match still wins and an ambiguous fallback matches nothing, so two POSIX paths differing only in case stay distinct. - Record a detail revision only when a fetch actually started. requestDetail's in-flight guard can refuse, and recording anyway meant a revision that landed during an earlier fetch was skipped for good once updated_at stopped moving, leaving a terminal request showing a stale running payload. - Discard superseded settings opens. The GGUF variant lookup is async, so opening a second row while the first was pending let whichever finished last win, and the page could then save or load settings for the wrong model. - Raise the model_id cap to PATH_MAX plus a quant suffix. A local model's id is its filesystem path and LoadRequest.model_path is unbounded, so the old 512 limit 422d the server sync while the local save succeeded, leaving the UI showing settings the API would never apply.
This commit is contained in:
parent
04e8beec62
commit
8f322aa627
6 changed files with 82 additions and 14 deletions
|
|
@ -126,6 +126,17 @@ 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.
|
||||
_MAX_VARIANT_SUFFIX_LEN = 64
|
||||
|
||||
# A local model's id is its filesystem path, optionally with a quant suffix, and
|
||||
# LoadRequest.model_path is unbounded. A limit under PATH_MAX would 422 the server
|
||||
# sync while the local save succeeded, leaving the UI showing settings the API
|
||||
# never applies.
|
||||
MAX_MODEL_OVERRIDE_KEY_LEN = 4096 + 1 + _MAX_VARIANT_SUFFIX_LEN
|
||||
|
||||
|
||||
class ModelOverridePayload(BaseModel):
|
||||
"""One model's saved launch config, applied when the API loads that model.
|
||||
|
||||
|
|
@ -136,7 +147,7 @@ class ModelOverridePayload(BaseModel):
|
|||
mode) are left to it, since their valid sets follow the llama.cpp build.
|
||||
"""
|
||||
|
||||
model_id: str = Field(..., min_length = 1, max_length = 512)
|
||||
model_id: str = Field(..., min_length = 1, max_length = MAX_MODEL_OVERRIDE_KEY_LEN)
|
||||
# None means "leave the stored value alone": the settings UI has no control
|
||||
# for launch flags, so a save from it must not wipe flags set through this
|
||||
# API. An explicit [] clears them (that is how "forget this model" arrives).
|
||||
|
|
@ -329,11 +340,6 @@ def get_openai_auto_switch_overrides(
|
|||
return ModelOverridesResponse(overrides = get_model_overrides())
|
||||
|
||||
|
||||
# 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.
|
||||
_MAX_VARIANT_SUFFIX_LEN = 64
|
||||
|
||||
|
||||
def _bare_model_id(model_id: str) -> Optional[str]:
|
||||
"""``repo`` for a ``repo:QUANT`` key, or None when there is no quant suffix."""
|
||||
head, sep, tail = model_id.rpartition(":")
|
||||
|
|
|
|||
|
|
@ -4220,3 +4220,30 @@ def test_remove_false_with_real_fields_saves_normally(monkeypatch):
|
|||
"tester",
|
||||
)
|
||||
assert resp.overrides["unsloth/B-GGUF"]["max_seq_length"] == 8192
|
||||
|
||||
|
||||
def test_override_lookup_falls_back_to_case_insensitive(monkeypatch):
|
||||
# The browser lowercases ids before storing them, so the backfill writes
|
||||
# "unsloth/qwen3-8b-gguf:q4_k_m" while the resolver asks for the repo's real
|
||||
# casing. Without this fallback every migrated entry is invisible.
|
||||
_mock_override_store(monkeypatch)
|
||||
settings.set_model_override("unsloth/qwen3-8b-gguf:q4_k_m", max_seq_length = 8192)
|
||||
got = settings.get_model_override("unsloth/Qwen3-8B-GGUF:Q4_K_M")
|
||||
assert got["max_seq_length"] == 8192
|
||||
|
||||
|
||||
def test_exact_override_match_beats_a_case_variant(monkeypatch):
|
||||
_mock_override_store(monkeypatch)
|
||||
settings.set_model_override("/models/foo.gguf", max_seq_length = 1024)
|
||||
settings.set_model_override("/models/Foo.gguf", max_seq_length = 8192)
|
||||
assert settings.get_model_override("/models/Foo.gguf")["max_seq_length"] == 8192
|
||||
assert settings.get_model_override("/models/foo.gguf")["max_seq_length"] == 1024
|
||||
|
||||
|
||||
def test_ambiguous_case_fallback_matches_nothing(monkeypatch):
|
||||
# Two POSIX paths differing only in case are two different files. Guessing
|
||||
# between them would apply one model's settings to another.
|
||||
_mock_override_store(monkeypatch)
|
||||
settings.set_model_override("/models/foo.gguf", max_seq_length = 1024)
|
||||
settings.set_model_override("/models/FOO.gguf", max_seq_length = 8192)
|
||||
assert settings.get_model_override("/models/Foo.gguf") == {}
|
||||
|
|
|
|||
|
|
@ -433,9 +433,28 @@ def get_model_overrides() -> dict[str, dict]:
|
|||
|
||||
|
||||
def get_model_override(model_id: str) -> dict:
|
||||
"""The launch override applied when auto-switch loads ``model_id`` (or empty)."""
|
||||
override = get_model_overrides().get(model_id)
|
||||
return override if isinstance(override, dict) else {}
|
||||
"""The launch override applied when auto-switch loads ``model_id`` (or empty).
|
||||
|
||||
Falls back to a case-insensitive match when nothing matches exactly. Repo ids
|
||||
and quants are case-insensitive in practice ("Q4_K_M" and "q4_k_m" name one
|
||||
file), and the browser normalizes them to lowercase before storing, so an
|
||||
exact-only lookup misses entries written from that side. Exact still wins, and
|
||||
an ambiguous fallback matches nothing, so two POSIX paths differing only in
|
||||
case stay distinct.
|
||||
"""
|
||||
overrides = get_model_overrides()
|
||||
override = overrides.get(model_id)
|
||||
if isinstance(override, dict):
|
||||
return override
|
||||
if not isinstance(model_id, str):
|
||||
return {}
|
||||
folded = model_id.casefold()
|
||||
matches = [
|
||||
value
|
||||
for key, value in overrides.items()
|
||||
if isinstance(key, str) and key.casefold() == folded and isinstance(value, dict)
|
||||
]
|
||||
return matches[0] if len(matches) == 1 else {}
|
||||
|
||||
|
||||
def set_model_override(
|
||||
|
|
|
|||
|
|
@ -496,8 +496,11 @@ export function ApiMonitorPage(): ReactElement {
|
|||
if (!selectedIsMissing && lastFetchedRef.current === revision) {
|
||||
return;
|
||||
}
|
||||
lastFetchedRef.current = revision;
|
||||
requestDetail(selectedId_);
|
||||
// Only remember the revision when a fetch really started; the in-flight
|
||||
// guard can refuse, and recording it anyway skips that revision for good.
|
||||
if (requestDetail(selectedId_)) {
|
||||
lastFetchedRef.current = revision;
|
||||
}
|
||||
}, [selectedId_, selectedUpdatedAt, selectedIsMissing, requestDetail]);
|
||||
|
||||
// The desktop webview's origin is tauri://, not the API server, and the
|
||||
|
|
|
|||
|
|
@ -168,7 +168,7 @@ interface UseApiMonitorResult {
|
|||
/** Full prompt/reply for entries the user expanded, keyed by entry id. */
|
||||
details: Record<string, ApiMonitorEntry>;
|
||||
loadingDetails: ReadonlySet<string>;
|
||||
requestDetail: (id: string) => void;
|
||||
requestDetail: (id: string) => boolean;
|
||||
}
|
||||
|
||||
/**
|
||||
|
|
@ -247,9 +247,12 @@ export function useApiMonitor({
|
|||
};
|
||||
}, [paused, intervalMs]);
|
||||
|
||||
const requestDetail = useCallback((id: string): void => {
|
||||
// Returns whether a fetch actually started. Callers that remember "I have
|
||||
// fetched revision N" must not record it when the in-flight guard turned them
|
||||
// away, or that revision is skipped for good once updated_at stops moving.
|
||||
const requestDetail = useCallback((id: string): boolean => {
|
||||
if (inFlightDetails.current.has(id)) {
|
||||
return;
|
||||
return false;
|
||||
}
|
||||
inFlightDetails.current.add(id);
|
||||
setLoadingDetails((prev) => new Set(prev).add(id));
|
||||
|
|
@ -275,6 +278,7 @@ export function useApiMonitor({
|
|||
return next;
|
||||
});
|
||||
});
|
||||
return true;
|
||||
}, []);
|
||||
|
||||
const clear = useCallback(async (): Promise<void> => {
|
||||
|
|
|
|||
|
|
@ -1226,8 +1226,12 @@ export function ModelsPage() {
|
|||
const [settingsTarget, setSettingsTarget] = useState<ModelPickTarget | null>(
|
||||
null,
|
||||
);
|
||||
// Bumped per open so a slow variant lookup for a row the user has moved on
|
||||
// from cannot land on top of the row they actually chose.
|
||||
const settingsOpenSeq = useRef(0);
|
||||
const openModelSettings = useCallback(
|
||||
async (row: CachedInventoryRow | LocalInventoryRow) => {
|
||||
const openSeq = ++settingsOpenSeq.current;
|
||||
// loadId is what the loader accepts; repoId is only a display/API alias.
|
||||
const id = row.loadId;
|
||||
// Whether the loaded model is this row, under any of the names it goes by.
|
||||
|
|
@ -1278,6 +1282,11 @@ export function ModelsPage() {
|
|||
}
|
||||
}
|
||||
}
|
||||
// The variant lookup above is async, so a second row opened while it was
|
||||
// pending would otherwise be overwritten by whichever call finished last.
|
||||
if (settingsOpenSeq.current !== openSeq) {
|
||||
return;
|
||||
}
|
||||
const leaf = id.split(/[\\/]/).filter(Boolean).pop() ?? id;
|
||||
setSettingsTarget({
|
||||
id,
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue