Studio: fix the permanent GGUF "update available" on no-symlink caches (#7113)
* Studio: fix the permanent GGUF "update available" on no-symlink caches Without the symlink privilege (the default on Windows with Developer Mode OFF), hf_hub_download MOVES the downloaded blob into snapshots/ instead of symlinking it out of blobs/, so blobs/ is left empty and scan_cache_dir reports blob_path = the snapshot file itself. Path(blob_path).name is then the GGUF FILENAME, not the file's etag. _repo_gguf_blob_map recorded that filename as the file's local blob hash, so _variant_update_available_from_requirement's `remote_sha256 in local_set` test could never match and every cached GGUF reported "update available" forever. Re-downloading could not clear it: the same file is rewritten, still with no blob. Only treat Path(blob_path).name as a hash when the file really lives in the cache's blobs/ dir; otherwise record a size identity so the file still appears in the map (dropping it would make the update check read it as absent and report the same phantom update). The comparison falls back to the remote ExpectedFile.size only when the cached file carries no blob hash, so the blob-hash path is unchanged wherever HF does produce blobs. A remote requant that keeps the byte size identical is not detected in that layout; re-hashing multi-GB GGUFs on the inventory hot path is the only stricter option. Fixes #7060 * Studio: match GGUF update checks by manifest sha256, closing the equal-size requant blind spot (#7060) On a no-symlink cache (Windows without Developer Mode) blobs/ is empty, so the update check falls back to comparing byte size. A Studio download records each file's sha256 in its manifest, so feed that into the local identity set: the check can then match by hash and detect an equal-size requant. The size fallback now applies only when no real hash is present, so a manifest hash that differs is still reported as a genuine update. Adds regression tests. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Revert the manifest-sha256 identity merge; keep the size-identity fallback (#7060) The download manifest is written before the transfer with the expected remote hashes, so it records download intent, not verified on-disk content, and completion is checked by size only. Merging those hashes into the local identity set could clear the update badge for an interrupted equal-size update that left the old bytes on disk. The accompanying all-size-identity gate also suppressed the size fallback whenever an older revision contributed a real blob hash, which re-showed a false update on mixed hash and size caches. Restoring the plain size-identity fallback keeps the fix without those regressions. * Studio: don't delete no-symlink GGUFs during stale-variant reclaim (#7060) On a no-symlink cache (Windows without Developer Mode) the downloaded file is moved into snapshots/ and scan_cache_dir reports its blob_path as that snapshot file, whose name is the filename, not an etag. reclaim_replaced_gguf_variant treated that name as a blob hash, which never matches the current hashes to keep, so it unlinked the freshly downloaded file. Only extract a deletable hash when the blob path is a real cache blob under the repo blobs directory, and keep any file we cannot identify; stale no-symlink revisions leak rather than risk removing the current file. Adds a regression test. * Studio: anchor GGUF blob-hash detection to the repo cache blobs dir (#7060) The inventory update check and the stale-variant reclaim both decide whether a scanned file is a real cache blob (name is the etag) or a moved no-symlink snapshot file (name is the filename). Both now share one _is_real_cache_blob helper that anchors to the repo cache blobs directory instead of matching any parent folder named blobs, so a repo that ships GGUFs under its own blobs subdir is no longer misread as the cache blob store. Threads repo_path through _repo_gguf_blob_map. Adds a regression test. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Studio: tighten comments in the GGUF update-check helpers (#7060) Post-review comment pass: shorten the internal blob-identity and size-fallback docstrings. Comments only, no behavior change. --------- Co-authored-by: Lee Jackson <130007945+Imagineer99@users.noreply.github.com> Co-authored-by: Daniel Han <danielhanchen@gmail.com> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
This commit is contained in:
parent
1e0d5ec6a9
commit
eb31d1e5ca
4 changed files with 256 additions and 17 deletions
|
|
@ -49,6 +49,10 @@ _REPO_SIZE_NEG_TTL = 60.0
|
|||
_MODEL_METADATA_TIMEOUT_SECONDS = 5.0
|
||||
_repo_size_cache_lock = threading.Lock()
|
||||
|
||||
# Identity for a cached file with no HF blob (Windows without Developer Mode: hf
|
||||
# moves the blob into snapshots/ and leaves blobs/ empty).
|
||||
_LOCAL_SIZE_IDENTITY_PREFIX = "size:"
|
||||
|
||||
|
||||
def get_repo_snapshot_metadata_cached(
|
||||
repo_id: str, hf_token: Optional[str] = None
|
||||
|
|
@ -135,23 +139,52 @@ def _cached_repo_file_name(file_obj) -> str:
|
|||
return str(getattr(file_obj, "file_name", "")).replace("\\", "/")
|
||||
|
||||
|
||||
def _is_real_cache_blob(blob: Optional[Path], repo_dir: Optional[Path]) -> bool:
|
||||
"""True only for a real cache blob at ``<repo_dir>/blobs/<etag>``.
|
||||
|
||||
A no-symlink ``snapshots/`` file (name is the filename, not an etag) or a
|
||||
repo's own ``blobs/`` subdir is not the cache blob store.
|
||||
"""
|
||||
if blob is None or repo_dir is None:
|
||||
return False
|
||||
try:
|
||||
return blob.parent.resolve(strict = False) == (repo_dir / "blobs").resolve(strict = False)
|
||||
except OSError:
|
||||
return False
|
||||
|
||||
|
||||
def _cached_blob_hash(blob_path, repo_path = None) -> Optional[str]:
|
||||
"""The cache blob hash (etag) for a cached file, or None when there is no blob.
|
||||
|
||||
Only a real blob under the repo's ``blobs/`` dir has name == hash; a moved
|
||||
no-symlink ``snapshots/`` file is "no blob", so the caller uses a size identity.
|
||||
"""
|
||||
path = Path(blob_path)
|
||||
repo_dir = Path(repo_path) if repo_path is not None else None
|
||||
return path.name if _is_real_cache_blob(path, repo_dir) else None
|
||||
|
||||
|
||||
def local_size_identity(size: int) -> str:
|
||||
"""Identity for a cached file whose blob hash is unknowable: its size.
|
||||
|
||||
Re-hashing multi-GB GGUFs on the inventory hot path is not viable, and a
|
||||
``size:`` token never collides with a hex hash.
|
||||
"""
|
||||
return f"{_LOCAL_SIZE_IDENTITY_PREFIX}{int(size)}"
|
||||
|
||||
|
||||
def _repo_gguf_blob_map(repo_info, *, include_companions: bool = False) -> dict[str, set[str]]:
|
||||
"""Map each cached GGUF file's repo-relative name to the SET of its local
|
||||
blob hashes across all cached revisions.
|
||||
identities across all revisions.
|
||||
|
||||
HF names each local cache blob FILE by the file's etag (lfs.sha256 else
|
||||
blob_id), so a local file's blob hash == ``Path(blob_path).name``. An updated
|
||||
repo keeps BOTH the old and new revision snapshots until HF garbage-collects
|
||||
them, so the same file resolves to several blobs; collecting them ALL (not
|
||||
just the first one seen, since ``repo_info.revisions`` is a frozenset and
|
||||
yields them in arbitrary order) lets the remote-vs-local diff treat the file
|
||||
as current when the remote (``main``) blob is present in any cached revision.
|
||||
Mirrors the ``cached_blob_ids`` membership test in routes/models.py.
|
||||
|
||||
By default this keeps the historical MAIN-GGUF-only behavior. GGUF update
|
||||
checks opt into companions so a shared mmproj/MTP blob can be compared too.
|
||||
An identity is the file's blob hash, or a size identity when the cache holds no
|
||||
blob (Windows without Developer Mode). BOTH old and new revision blobs are kept
|
||||
(a set), so the diff treats the file as current when the remote ``main`` blob is
|
||||
in any cached revision. Main GGUF only by default; update checks opt into
|
||||
companions to compare a shared mmproj/MTP blob too.
|
||||
"""
|
||||
blob_map: dict[str, set[str]] = {}
|
||||
repo_path = getattr(repo_info, "repo_path", None)
|
||||
for revision in repo_info.revisions:
|
||||
for f in revision.files:
|
||||
if include_companions:
|
||||
|
|
@ -163,7 +196,13 @@ def _repo_gguf_blob_map(repo_info, *, include_companions: bool = False) -> dict[
|
|||
if not blob_path:
|
||||
continue
|
||||
name = _cached_repo_file_name(f)
|
||||
blob_map.setdefault(name, set()).add(Path(blob_path).name)
|
||||
identity = _cached_blob_hash(blob_path, repo_path)
|
||||
if identity is None:
|
||||
size = int(getattr(f, "size_on_disk", 0) or 0)
|
||||
if size <= 0:
|
||||
continue
|
||||
identity = local_size_identity(size)
|
||||
blob_map.setdefault(name, set()).add(identity)
|
||||
return blob_map
|
||||
|
||||
|
||||
|
|
|
|||
|
|
@ -408,7 +408,13 @@ def reclaim_replaced_gguf_variant(
|
|||
and extract_quant_label(name).lower() == variant_key,
|
||||
)
|
||||
for snap, blob, name in matches:
|
||||
blob_hash = _blob_hash_from_path(blob) if blob is not None else None
|
||||
# Prune only a file we can identify as a real, stale cache blob. A
|
||||
# no-symlink snapshot file has no identifiable blob hash, so keep it.
|
||||
blob_hash = (
|
||||
_blob_hash_from_path(blob)
|
||||
if cache_inventory._is_real_cache_blob(blob, repo_dir)
|
||||
else None
|
||||
)
|
||||
if blob_hash is None or blob_hash in keep_main_hashes:
|
||||
continue
|
||||
stale_matches.append((snap, blob, name))
|
||||
|
|
|
|||
|
|
@ -337,6 +337,22 @@ def _local_main_gguf_blobs_by_quant(repo_id: str) -> dict[str, dict[str, set[str
|
|||
return result
|
||||
|
||||
|
||||
def _size_identity_matches(local_set: set[str], remote_size: int) -> bool:
|
||||
"""Whether a cached file with NO blob hash is current, judged by size.
|
||||
|
||||
A size token only lands in ``local_set`` for a file the cache has no blob for,
|
||||
so it never loosens the hash comparison for a normal file. Tradeoff: an
|
||||
equal-size requant is missed, versus the status quo where every no-blob GGUF
|
||||
shows a phantom update that no re-download clears.
|
||||
"""
|
||||
size = int(remote_size or 0)
|
||||
if size <= 0:
|
||||
return False
|
||||
from hub.services.models import cache_inventory
|
||||
|
||||
return cache_inventory.local_size_identity(size) in local_set
|
||||
|
||||
|
||||
def _variant_update_available_from_requirement(
|
||||
local_blobs: dict[str, set[str]], requirement: Optional[_GgufVariantRequirement], variant: str
|
||||
) -> bool:
|
||||
|
|
@ -355,8 +371,13 @@ def _variant_update_available_from_requirement(
|
|||
if not remote_blob:
|
||||
continue
|
||||
local_set = local_by_posix.get(path)
|
||||
if not local_set or remote_blob not in local_set:
|
||||
if not local_set:
|
||||
return True
|
||||
if remote_blob in local_set:
|
||||
continue
|
||||
if _size_identity_matches(local_set, expected.size):
|
||||
continue
|
||||
return True
|
||||
return False
|
||||
|
||||
|
||||
|
|
|
|||
|
|
@ -382,14 +382,148 @@ def test_repo_gguf_blob_map_collects_all_revision_blobs():
|
|||
"""Every cached revision's blob for a gguf file is kept as a set, not
|
||||
collapsed to one arbitrary blob."""
|
||||
repo_info = SimpleNamespace(
|
||||
repo_path = "/", # real blobs live at <repo_path>/blobs/<etag>
|
||||
revisions = [
|
||||
_rev(("lfm2-350m-q4_k_m.gguf", "OLDsha")),
|
||||
_rev(("lfm2-350m-q4_k_m.gguf", "NEWsha")),
|
||||
]
|
||||
],
|
||||
)
|
||||
assert CI._repo_gguf_blob_map(repo_info) == {"lfm2-350m-q4_k_m.gguf": {"OLDsha", "NEWsha"}}
|
||||
|
||||
|
||||
# ── no-symlink (Windows without Developer Mode) GGUF update detection ──
|
||||
#
|
||||
# Regression for the phantom "Update available" that NEVER clears (#7060). Without
|
||||
# the symlink privilege, hf_hub_download MOVES the blob into snapshots/ instead of
|
||||
# symlinking it, so blobs/ is empty and scan_cache_dir reports blob_path = the
|
||||
# snapshot file. Its name is the FILENAME, not an etag, so a remote-vs-local sha256
|
||||
# comparison can never match and every cached GGUF reports an update forever --
|
||||
# which re-downloading cannot fix, since the same file is rewritten with no blob.
|
||||
|
||||
|
||||
def _rev_no_symlink(*files):
|
||||
"""A revision whose GGUFs were MOVED into snapshots/ (no blobs/ entry)."""
|
||||
return SimpleNamespace(
|
||||
files = [
|
||||
SimpleNamespace(
|
||||
file_name = name,
|
||||
blob_path = f"/hf/models--org--repo/snapshots/{'a' * 40}/{name}",
|
||||
size_on_disk = size,
|
||||
)
|
||||
for name, size in files
|
||||
]
|
||||
)
|
||||
|
||||
|
||||
def _requirement(*expected):
|
||||
from hub.utils.download_manifest import ExpectedFile
|
||||
from hub.utils.gguf_plan import GgufVariantPlan
|
||||
|
||||
expected_files = tuple(
|
||||
ExpectedFile(path = path, size = size, sha256 = sha) for path, size, sha in expected
|
||||
)
|
||||
return GgufVariantPlan(
|
||||
main_filenames = frozenset(e.path for e in expected_files),
|
||||
target_filenames = tuple(e.path for e in expected_files),
|
||||
main_hashes = frozenset(e.sha256 for e in expected_files if e.sha256),
|
||||
required_hashes = frozenset(e.sha256 for e in expected_files if e.sha256),
|
||||
companion_hashes = frozenset(),
|
||||
mmproj_filenames = frozenset(),
|
||||
mmproj_hashes = frozenset(),
|
||||
expected_files = expected_files,
|
||||
main_size_bytes = sum(e.size for e in expected_files),
|
||||
download_size_bytes = sum(e.size for e in expected_files),
|
||||
)
|
||||
|
||||
|
||||
def test_repo_gguf_blob_map_uses_size_identity_when_cache_has_no_blob():
|
||||
"""A snapshot-resident GGUF (no blobs/ entry) must NOT be recorded under its
|
||||
filename as if that were a hash -- it gets a size identity instead."""
|
||||
repo_info = SimpleNamespace(
|
||||
repo_path = "/hf/models--org--repo",
|
||||
revisions = [_rev_no_symlink(("model-Q4_K_M.gguf", 4096))],
|
||||
)
|
||||
|
||||
assert CI._repo_gguf_blob_map(repo_info) == {
|
||||
"model-Q4_K_M.gguf": {CI.local_size_identity(4096)}
|
||||
}
|
||||
|
||||
|
||||
def test_repo_gguf_blob_map_skips_snapshot_file_with_unknown_size():
|
||||
"""No blob and no readable size means no identity at all, rather than a
|
||||
filename masquerading as a hash."""
|
||||
repo_info = SimpleNamespace(
|
||||
repo_path = "/hf/models--org--repo",
|
||||
revisions = [_rev_no_symlink(("model-Q4_K_M.gguf", 0))],
|
||||
)
|
||||
|
||||
assert CI._repo_gguf_blob_map(repo_info) == {}
|
||||
|
||||
|
||||
def test_repo_gguf_blob_map_ignores_repo_blobs_subdir_on_no_symlink():
|
||||
"""A repo that ships a GGUF under its own blobs/ subdir lands at
|
||||
snapshots/<rev>/blobs/model.gguf on a no-symlink cache. Its parent is named
|
||||
'blobs' but it is NOT the cache blob store, so it gets a size identity rather
|
||||
than having its filename recorded as a hash (which would show a phantom update)."""
|
||||
repo_path = "/hf/models--org--repo"
|
||||
repo_info = SimpleNamespace(
|
||||
repo_path = repo_path,
|
||||
revisions = [
|
||||
SimpleNamespace(
|
||||
files = [
|
||||
SimpleNamespace(
|
||||
file_name = "model-Q4_K_M.gguf",
|
||||
blob_path = f"{repo_path}/snapshots/{'a' * 40}/blobs/model-Q4_K_M.gguf",
|
||||
size_on_disk = 4096,
|
||||
)
|
||||
]
|
||||
)
|
||||
],
|
||||
)
|
||||
|
||||
assert CI._repo_gguf_blob_map(repo_info) == {
|
||||
"model-Q4_K_M.gguf": {CI.local_size_identity(4096)}
|
||||
}
|
||||
|
||||
|
||||
def test_no_symlink_cache_matching_remote_size_reports_no_update():
|
||||
"""The #7060 repro: a GGUF stored directly in snapshots/ whose size matches the
|
||||
remote is CURRENT, and must not show a phantom 'update available'."""
|
||||
local_blobs = {"model-Q4_K_M.gguf": {CI.local_size_identity(4096)}}
|
||||
requirement = _requirement(("model-Q4_K_M.gguf", 4096, "REMOTEsha256"))
|
||||
|
||||
assert (
|
||||
GV._variant_update_available_from_requirement(local_blobs, requirement, "Q4_K_M") is False
|
||||
)
|
||||
|
||||
|
||||
def test_no_symlink_cache_with_different_remote_size_still_reports_update():
|
||||
"""A genuine upstream change is still detected in the no-symlink layout."""
|
||||
local_blobs = {"model-Q4_K_M.gguf": {CI.local_size_identity(4096)}}
|
||||
requirement = _requirement(("model-Q4_K_M.gguf", 8192, "REMOTEsha256"))
|
||||
|
||||
assert GV._variant_update_available_from_requirement(local_blobs, requirement, "Q4_K_M") is True
|
||||
|
||||
|
||||
def test_symlinked_cache_with_stale_blob_still_reports_update():
|
||||
"""The blob-hash path is untouched: a real blob that does not match the remote
|
||||
sha256 is still stale, and a size-identity fallback must not rescue it."""
|
||||
local_blobs = {"model-Q4_K_M.gguf": {"OLDsha"}}
|
||||
requirement = _requirement(("model-Q4_K_M.gguf", 4096, "NEWsha"))
|
||||
|
||||
assert GV._variant_update_available_from_requirement(local_blobs, requirement, "Q4_K_M") is True
|
||||
|
||||
|
||||
def test_symlinked_cache_with_current_blob_reports_no_update():
|
||||
"""The blob-hash path is untouched: a matching blob is current."""
|
||||
local_blobs = {"model-Q4_K_M.gguf": {"OLDsha", "NEWsha"}}
|
||||
requirement = _requirement(("model-Q4_K_M.gguf", 4096, "NEWsha"))
|
||||
|
||||
assert (
|
||||
GV._variant_update_available_from_requirement(local_blobs, requirement, "Q4_K_M") is False
|
||||
)
|
||||
|
||||
|
||||
def test_reclaim_replaced_gguf_variant_prunes_old_revision_only(monkeypatch, tmp_path):
|
||||
"""After a verified update, stale same-variant files/blobs are removed while
|
||||
the freshly downloaded hash and sibling variants remain cached."""
|
||||
|
|
@ -463,3 +597,42 @@ def test_reclaim_replaced_gguf_variant_prunes_old_revision_only(monkeypatch, tmp
|
|||
assert sibling_snap.exists() is True
|
||||
assert sibling_blob.exists() is True
|
||||
assert invalidated == [True]
|
||||
|
||||
|
||||
def test_reclaim_replaced_gguf_variant_keeps_no_symlink_current_file(monkeypatch, tmp_path):
|
||||
"""No-symlink cache (Windows without Developer Mode): the moved GGUF lives
|
||||
directly in snapshots/ and blobs/ is empty, so scan_cache_dir reports
|
||||
blob_path == the snapshot file and its name is the FILENAME, not an etag.
|
||||
Reclaim must NOT mistake that filename for a stale hash and delete the
|
||||
freshly-downloaded current file."""
|
||||
repo_id = "org/repo-GGUF"
|
||||
repo_path = tmp_path / "models--org--repo-GGUF"
|
||||
snap = repo_path / "snapshots" / ("a" * 40) / "model-Q4_K_M.gguf"
|
||||
snap.parent.mkdir(parents = True, exist_ok = True)
|
||||
snap.write_bytes(b"current-download")
|
||||
(repo_path / "blobs").mkdir(parents = True, exist_ok = True) # empty: moved, not linked
|
||||
|
||||
repo_info = SimpleNamespace(
|
||||
repo_id = repo_id,
|
||||
repo_type = "model",
|
||||
repo_path = repo_path,
|
||||
revisions = [
|
||||
SimpleNamespace(
|
||||
files = [
|
||||
SimpleNamespace(
|
||||
file_name = "model-Q4_K_M.gguf",
|
||||
file_path = str(snap),
|
||||
blob_path = str(snap), # no-symlink: blob_path == the snapshot file
|
||||
)
|
||||
]
|
||||
)
|
||||
],
|
||||
)
|
||||
monkeypatch.setattr(CI, "all_hf_cache_scans", lambda: [SimpleNamespace(repos = [repo_info])])
|
||||
monkeypatch.setattr(CI, "invalidate_hf_cache_scans", lambda: None)
|
||||
|
||||
result = D.reclaim_replaced_gguf_variant(repo_id, "Q4_K_M", frozenset({"REMOTEsha256"}))
|
||||
|
||||
assert snap.exists() is True # the current file must survive
|
||||
assert result["removed_snapshots"] == 0
|
||||
assert result["deleted_blobs"] == 0
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue