From 5aa018370680845f306478115146aeb86138b422 Mon Sep 17 00:00:00 2001 From: shimmyshimmer Date: Thu, 23 Jul 2026 18:58:24 -0700 Subject: [PATCH] Studio autoload: retry next folder quant after a skip; dedupe by load target only Third round of review follow-ups: - Quant resolution for a local GGUF folder now returns the smallest quant that is not already in the skipped set, so one corrupt or blocked file cannot sink a folder that still has other complete quants. - The cached/local dedupe keys on actual load targets and on-disk paths only. A local copy that merely shares a repo model_id is a distinct set of files and stays available when the cached copy fails or has no usable quant. Contract tests updated and extended for both. --- .../src/features/chat/api/chat-adapter.ts | 35 +++++++++++++------ tests/studio/test_model_picker_contracts.py | 31 ++++++++++++---- 2 files changed, 49 insertions(+), 17 deletions(-) diff --git a/studio/frontend/src/features/chat/api/chat-adapter.ts b/studio/frontend/src/features/chat/api/chat-adapter.ts index 2b32ac89da..6620541904 100644 --- a/studio/frontend/src/features/chat/api/chat-adapter.ts +++ b/studio/frontend/src/features/chat/api/chat-adapter.ts @@ -1523,6 +1523,7 @@ function localRowToCandidate( async function resolveLocalRowCandidate( row: LocalModelInfo, rememberedVariant: string | null = null, + isSkippedCandidate?: (candidate: AutoLoadCandidate) => boolean, ): Promise { const isGguf = row.model_format === "gguf"; if (row.capabilities?.requires_variant === true) { @@ -1544,8 +1545,14 @@ async function resolveLocalRowCandidate( entry.downloaded && !entry.partial && isAutoLoadableGgufVariant(entry), ) .sort((a, b) => a.size_bytes - b.size_bytes); - if (downloaded.length === 0) return null; - return localRowToCandidate(row, downloaded[0].quant); + // Smallest first, skipping quants that already failed or were + // blocked, so one bad file cannot sink a folder with other quants. + for (const entry of downloaded) { + const candidate = localRowToCandidate(row, entry.quant); + if (isSkippedCandidate?.(candidate)) continue; + return candidate; + } + return null; } } return localRowToCandidate(row, isGguf ? rememberedVariant : null); @@ -1944,8 +1951,10 @@ export async function autoLoadOnDeviceModel(): Promise<{ const ggufRepos = allGgufRepos.filter(isAutoLoadableCachedRepo); const modelRepos = allModelRepos.filter(isAutoLoadableCachedRepo); const localRows = allLocalRows.filter(isAutoLoadableLocalRow); - // Dedupe candidates that appear in both the cached and the local - // inventory (e.g. a custom scan folder pointing into an HF cache). + // Dedupe candidates that resolve to the SAME load target (e.g. a custom + // scan folder pointing into an HF cache). Keyed on load targets and + // on-disk paths only: a shared model_id does not mean the same files, and + // a distinct local copy must stay available when the cached copy fails. const seenLoadTargets = new Set(); const markSeen = (...values: (string | null | undefined)[]): void => { for (const value of values) { @@ -1997,7 +2006,7 @@ export async function autoLoadOnDeviceModel(): Promise<{ } else if (lastLoaded.kind === "gguf") { const repo = findCachedRepo(ggufRepos, lastLoaded.id); if (repo && lastLoaded.ggufVariant) { - markSeen(repo.repo_id, repo.load_id, repo.cache_path); + markSeen(repo.load_id || repo.repo_id, repo.cache_path); try { const variants = await listGgufVariants(repo.repo_id, undefined, { preferLocalCache: true, @@ -2042,7 +2051,7 @@ export async function autoLoadOnDeviceModel(): Promise<{ } else { const repo = findCachedRepo(modelRepos, lastLoaded.id); if (repo) { - markSeen(repo.repo_id, repo.load_id, repo.cache_path); + markSeen(repo.load_id || repo.repo_id, repo.cache_path); try { toast("Loading last used model…", { id: toastId, @@ -2130,7 +2139,7 @@ export async function autoLoadOnDeviceModel(): Promise<{ if (loadAttempts >= MAX_AUTO_LOAD_ATTEMPTS) break; if (candidate.type === "cached-gguf") { const repo = candidate.repo; - markSeen(repo.repo_id, repo.load_id, repo.cache_path); + markSeen(repo.load_id || repo.repo_id, repo.cache_path); try { const variants = await listGgufVariants(repo.repo_id, undefined, { preferLocalCache: true, @@ -2172,7 +2181,7 @@ export async function autoLoadOnDeviceModel(): Promise<{ } if (candidate.type === "cached-model") { const repo = candidate.repo; - markSeen(repo.repo_id, repo.load_id, repo.cache_path); + markSeen(repo.load_id || repo.repo_id, repo.cache_path); if ( skippedAutoLoadCandidates.has( autoLoadCandidateKey("model", repo.repo_id), @@ -2201,12 +2210,16 @@ export async function autoLoadOnDeviceModel(): Promise<{ continue; } const row = candidate.row; - if (isSeen(row.load_id, row.id, row.path, row.model_id)) { + if (isSeen(row.load_id, row.id, row.path)) { continue; } - markSeen(row.load_id, row.id, row.path, row.model_id); + markSeen(row.load_id, row.id, row.path); try { - const localCandidate = await resolveLocalRowCandidate(row); + const localCandidate = await resolveLocalRowCandidate(row, null, (c) => + skippedAutoLoadCandidates.has( + autoLoadCandidateKey(c.kind, c.id, c.ggufVariant), + ), + ); if (!localCandidate) { continue; } diff --git a/tests/studio/test_model_picker_contracts.py b/tests/studio/test_model_picker_contracts.py index 4e420b5704..1c18ca39ea 100644 --- a/tests/studio/test_model_picker_contracts.py +++ b/tests/studio/test_model_picker_contracts.py @@ -565,13 +565,32 @@ def test_autoload_remembers_last_model_across_all_sources(): def test_autoload_deduplicates_cached_and_local_candidates(): - """A model visible in both the cached lists and the local inventory - (e.g. a custom scan folder pointing into an HF cache) must not be tried - twice.""" + """Candidates resolving to the same load target (e.g. a custom scan + folder pointing into an HF cache) must not be tried twice, but the + dedupe must key on actual load targets/paths only: a local copy that + merely shares a repo model_id is a distinct set of files and must stay + available when the cached copy fails or has no usable quant.""" auto_load = _autoload_section() assert "const seenLoadTargets = new Set()" in auto_load - assert "markSeen(repo.repo_id, repo.load_id, repo.cache_path)" in auto_load - assert "isSeen(row.load_id, row.id, row.path, row.model_id)" in auto_load + assert "markSeen(repo.load_id || repo.repo_id, repo.cache_path)" in auto_load + assert "isSeen(row.load_id, row.id, row.path)" in auto_load + # The repo-id-based dedupe that shadowed distinct local copies is gone. + assert "isSeen(row.load_id, row.id, row.path, row.model_id)" not in auto_load + assert "markSeen(repo.repo_id," not in auto_load + + +def test_local_quant_resolution_skips_failed_quants(): + """When a folder's smallest quant already failed or was blocked, the + resolver must return the next complete quant instead of abandoning the + whole folder (which made Send falsely report no model).""" + src = _read("features/chat/api/chat-adapter.ts") + resolve_fn = src.split("async function resolveLocalRowCandidate", 1)[1] + resolve_fn = resolve_fn.split("\nfunction ", 1)[0] + assert "for (const entry of downloaded)" in resolve_fn + assert "if (isSkippedCandidate?.(candidate)) continue;" in resolve_fn + # The fallback loop feeds the skip set into resolution. + auto_load = _autoload_section() + assert "await resolveLocalRowCandidate(row, null, (c) =>" in auto_load def test_autoload_trust_guard_still_blocks_background_loads(): @@ -652,7 +671,7 @@ def test_directory_gguf_rows_resolve_variant_like_picker(): # The cascade must keep directory GGUF rows as candidates. auto_load = src.split("async function autoLoadOnDeviceModel", 1)[1] assert 'row.model_format === "gguf" ||' in auto_load - assert "await resolveLocalRowCandidate(row)" in auto_load + assert "await resolveLocalRowCandidate(row, null, (c) =>" in auto_load def test_remembered_local_failure_does_not_block_folder_fallback():