* fix(studio/hub): apply repo_id length limit per segment, not whole string is_valid_repo_id() applied the 96-char limit to the full "namespace/repo_name" string, so a repo with a valid (<=96 char) name but a long combined id was falsely rejected. Match huggingface_hub.validate_repo_id by checking the length per segment instead. Fixes #6946. * Fix long repo id state filenames * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --------- Co-authored-by: Etherll <61019402+Etherll@users.noreply.github.com> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
This commit is contained in:
parent
fcb1152c76
commit
d0c8d550a6
3 changed files with 82 additions and 6 deletions
|
|
@ -105,6 +105,10 @@ def test_repo_id_validation_accepts_hf_repo_id_contract(repo_id):
|
|||
assert paths.is_valid_repo_id(repo_id)
|
||||
|
||||
|
||||
def test_repo_id_validation_accepts_max_length_namespaced_repo():
|
||||
assert paths.is_valid_repo_id(f"{'a' * 96}/{'b' * 96}")
|
||||
|
||||
|
||||
@pytest.mark.parametrize(
|
||||
"repo_id",
|
||||
[
|
||||
|
|
@ -121,6 +125,48 @@ def test_repo_id_validation_rejects_unsafe_or_invalid_ids(repo_id):
|
|||
assert not paths.is_valid_repo_id(repo_id)
|
||||
|
||||
|
||||
def test_download_state_preserves_readable_keys_when_safe(monkeypatch, tmp_path):
|
||||
monkeypatch.setattr(state_dir, "cache_root", lambda: tmp_path)
|
||||
|
||||
path = state_dir.marker_path("model", "Owner/Repo", "Q4_K_M")
|
||||
|
||||
assert path is not None
|
||||
assert path.name == "models--owner--repo--variant--q4_k_m.json"
|
||||
|
||||
|
||||
@pytest.mark.parametrize("variant", ["bad variant with spaces", "q" * 64])
|
||||
def test_download_state_bounds_long_repo_variant_filenames(monkeypatch, tmp_path, variant):
|
||||
monkeypatch.setattr(state_dir, "cache_root", lambda: tmp_path)
|
||||
repo_id = f"{'a' * 96}/{'b' * 96}"
|
||||
|
||||
assert paths.is_valid_repo_id(repo_id)
|
||||
assert download_manifest.write_cancel_marker("model", repo_id, variant, "http")
|
||||
assert download_manifest.write_manifest(
|
||||
"model",
|
||||
repo_id,
|
||||
variant,
|
||||
[download_manifest.ExpectedFile(path = "model.gguf", size = 1)],
|
||||
"http",
|
||||
)
|
||||
|
||||
marker_path = state_dir.marker_path("model", repo_id, variant)
|
||||
manifest_path = state_dir.manifest_path("model", repo_id, variant)
|
||||
|
||||
assert marker_path is not None
|
||||
assert manifest_path is not None
|
||||
assert "--sha256-" in marker_path.name
|
||||
assert len(marker_path.name.encode("utf-8")) <= 255
|
||||
assert len(f".{marker_path.name}.tmp-00000000".encode("utf-8")) <= 255
|
||||
assert download_manifest.has_cancel_marker("model", repo_id, variant)
|
||||
assert download_manifest.read_manifest("model", repo_id, variant) is not None
|
||||
assert list(download_manifest.iter_variant_markers("model", repo_id)) == [
|
||||
(variant, marker_path)
|
||||
]
|
||||
assert list(download_manifest.iter_variant_manifests("model", repo_id)) == [
|
||||
(variant, manifest_path)
|
||||
]
|
||||
|
||||
|
||||
class _RecordingLogger:
|
||||
def __init__(self):
|
||||
self.warnings = []
|
||||
|
|
|
|||
|
|
@ -181,15 +181,20 @@ def is_valid_repo_id(repo_id: str) -> bool:
|
|||
"""Validate Hugging Face ``repo_name`` or ``namespace/repo_name`` IDs."""
|
||||
if not repo_id or repo_id != repo_id.strip():
|
||||
return False
|
||||
if len(repo_id) > _MAX_REPO_ID_LENGTH or repo_id.endswith(".git"):
|
||||
if repo_id.endswith(".git"):
|
||||
return False
|
||||
if "--" in repo_id or ".." in repo_id:
|
||||
return False
|
||||
segments = repo_id.split("/")
|
||||
if len(segments) not in (1, 2):
|
||||
return False
|
||||
# Match huggingface_hub.validate_repo_id: the 96-char limit applies per
|
||||
# segment (repo name / namespace), not to the whole "namespace/repo_name"
|
||||
# string, so long-but-valid repo names are not falsely rejected.
|
||||
return all(
|
||||
segment not in ("", ".", "..") and _VALID_REPO_ID_SEGMENT.fullmatch(segment) is not None
|
||||
segment not in ("", ".", "..")
|
||||
and len(segment) <= _MAX_REPO_ID_LENGTH
|
||||
and _VALID_REPO_ID_SEGMENT.fullmatch(segment) is not None
|
||||
for segment in segments
|
||||
)
|
||||
|
||||
|
|
|
|||
|
|
@ -11,8 +11,9 @@ cache lifecycle. Two subdirectories:
|
|||
manifests/ <key>.json per-download expected-files manifest
|
||||
cancelled/ <key>.json per-download cancel marker
|
||||
|
||||
The ``<key>`` mirrors HF's cache dir naming so a state file can be
|
||||
eyeballed next to the on-disk repo it describes:
|
||||
The ``<key>`` mirrors HF's cache dir naming while the resulting manifest,
|
||||
cancel-marker, and atomic-write temp filenames fit common filesystem basename
|
||||
limits. Very long repo IDs use a stable hash in the state key:
|
||||
|
||||
models--<owner>--<name> full snapshot
|
||||
models--<owner>--<name>--variant--<variant> GGUF variant
|
||||
|
|
@ -49,6 +50,11 @@ _MANIFESTS_SUBDIR = "manifests"
|
|||
_CANCELLED_SUBDIR = "cancelled"
|
||||
_WORKERS_SUBDIR = "workers"
|
||||
_SAFE_VARIANT_FRAGMENT = re.compile(r"^[a-z0-9._-]{1,64}$")
|
||||
_MAX_STATE_BASENAME_BYTES = 255
|
||||
_STATE_EXTENSION = ".json"
|
||||
# _atomic_write_json writes ".<target>.tmp-<8hex>" beside the final file.
|
||||
_ATOMIC_WRITE_TMP_OVERHEAD = len(".") + len(".tmp-") + 8
|
||||
_MAX_VARIANT_FRAGMENT_LENGTH = 64
|
||||
|
||||
|
||||
def state_root() -> Optional[Path]:
|
||||
|
|
@ -84,16 +90,35 @@ def repo_cache_basename(repo_type: RepoType, repo_id: str) -> str:
|
|||
return f"{repo_type}s--{repo_id.replace('/', '--')}".lower()
|
||||
|
||||
|
||||
def _filename_bytes(name: str) -> int:
|
||||
return len(name.encode("utf-8"))
|
||||
|
||||
|
||||
def _state_filename_fits(entry_key: str) -> bool:
|
||||
filename = f"{entry_key}{_STATE_EXTENSION}"
|
||||
return _filename_bytes(filename) + _ATOMIC_WRITE_TMP_OVERHEAD <= _MAX_STATE_BASENAME_BYTES
|
||||
|
||||
|
||||
def _state_repo_key(repo_type: RepoType, repo_id: str) -> str:
|
||||
base = repo_cache_basename(repo_type, repo_id)
|
||||
variant_prefix = f"{base}--variant--"
|
||||
longest_variant_key = f"{variant_prefix}{'x' * _MAX_VARIANT_FRAGMENT_LENGTH}"
|
||||
if _state_filename_fits(longest_variant_key):
|
||||
return base
|
||||
digest = hashlib.sha256(base.encode("utf-8")).hexdigest()[:32]
|
||||
return f"{repo_type}s--sha256-{digest}"
|
||||
|
||||
|
||||
def variant_filename_prefix(repo_type: RepoType, repo_id: str) -> str:
|
||||
"""Lowercased prefix every variant-keyed state file for this repo shares.
|
||||
|
||||
The single source the download_manifest enumerators match against, so the
|
||||
scheme in :func:`_entry_key` cannot drift from them silently."""
|
||||
return f"{repo_cache_basename(repo_type, repo_id)}--variant--"
|
||||
return f"{_state_repo_key(repo_type, repo_id)}--variant--"
|
||||
|
||||
|
||||
def _entry_key(repo_type: RepoType, repo_id: str, variant: Optional[str]) -> str:
|
||||
base = repo_cache_basename(repo_type, repo_id)
|
||||
base = _state_repo_key(repo_type, repo_id)
|
||||
if not variant:
|
||||
return base
|
||||
normalized_variant = variant.strip().lower()
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue