Repository navigation
fix: verify the cached model files and re-fetch a corrupt one - #642
Conversation
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthroughHuggingFace cache lookup now matches normalized repository-relative paths and checks canonical and differently cased cache locations. It raises a corruption error when a required model file has a recorded size mismatch, while auxiliary-file mismatches remain warning-only. The download flow forces the next HuggingFace attempt after that corruption error. Google Cloud Storage retrieval now stages downloads, validates archives, and cleans up staging files. The tests cover corrupt model files, including nested paths, and tolerated auxiliary-file drift. Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Bug fix Suggested reviewers: Merge Risk: ⚪ Minimal · up to No actionable issue is established from the supplied evidence; the change is mergeable after normal checks. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The recovery path improves detection of truncated model files, but an incomplete older cache can interfere with recovery of the current cache. The observed impact is confined to model availability and cache validation; no new remote access or privilege path was established. Retained concerns
Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@fastembed/common/model_management.py`:
- Around line 475-479: The call currently passes force_download=True and also
**kwargs which can contain force_download, causing a TypeError; update the
download_model call site (in fastembed.common.model_management, function
download_model) to ensure force_download is set in the kwargs map before
expanding it (e.g., remove any existing force_download key and then set
kwargs['force_download']=True or overwrite it) so only one value is supplied to
the underlying function.
- Around line 262-268: The call to list_repo_tree(...) incorrectly uses
recursive=True; remove that unsupported kwarg and instead call
list_repo_tree(hf_source_repo, revision=repo_revision, repo_type="model",
path=...) or use expand=True (depending on API) to retrieve directory entries,
then perform client-side recursion/filtering to collect nested files into
repo_tree; update the code around repo_tree and the filtering that uses
allowed_extensions to walk subdirectories (by repeatedly calling list_repo_tree
with sub-paths or expanding entries) so that nested model files are discovered
without passing recursive=True to huggingface_hub.list_repo_tree.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 04ae48e3-578d-4d6a-aa44-89abf81e7057
📒 Files selected for processing (2)
fastembed/common/model_management.pytests/test_model_management.py
| repo_tree = list( | ||
| list_repo_tree( | ||
| hf_source_repo, revision=repo_revision, repo_type="model", recursive=True | ||
| ) | ||
| ) | ||
|
|
||
| allowed_extensions = {".json", ".onnx", ".txt"} | ||
| allowed_extensions = {".json", ".onnx", ".txt", ".onnx_data", ".npy", ".vocab"} |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
huggingface_hub list_repo_tree recursive parameter
💡 Result:
In huggingface_hub, HfApi.list_repo_tree() does not provide a recursive parameter. Instead, you control what is listed via its other arguments (notably path/subdirectory scope) and you can get expanded metadata depending on the expand flag. Hugging Face staff also note that listing files in a repo is costly and that “it is not possible to filter them server-side when listing files from a repo… best solution is still to do the filtering client-side”; however, you can limit the request by only listing files from a subdirectory [1]. Practical implication for your question: if you want a recursive listing, you should treat list_repo_tree() as the API meant for listing repo trees, and use the supported arguments to restrict scope (e.g., target a subdirectory path). Don’t look for a recursive parameter; it isn’t part of the documented method signature [1]. References: Hugging Face issue discussion explaining server-side filtering limits for list_repo_tree [1]; and the official HfApi documentation entry point (for method signatures and related API usage) [2].
Citations:
- 1: Support server-side filtering for list_repo_tree huggingface/huggingface_hub#2676
- 2: https://huggingface.co/docs/huggingface_hub/en/package_reference/hf_api
Fix list_repo_tree call: remove unsupported recursive=True.
fastembed/common/model_management.py (lines 262-268) passes recursive=True to huggingface_hub.list_repo_tree(), but HfApi.list_repo_tree() has no recursive parameter; this will raise a TypeError at runtime. Rework the listing to use supported arguments (e.g., subdirectory scoping via path and/or expand, then filter client-side) to collect nested model files.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@fastembed/common/model_management.py` around lines 262 - 268, The call to
list_repo_tree(...) incorrectly uses recursive=True; remove that unsupported
kwarg and instead call list_repo_tree(hf_source_repo, revision=repo_revision,
repo_type="model", path=...) or use expand=True (depending on API) to retrieve
directory entries, then perform client-side recursion/filtering to collect
nested files into repo_tree; update the code around repo_tree and the filtering
that uses allowed_extensions to walk subdirectories (by repeatedly calling
list_repo_tree with sub-paths or expanding entries) so that nested model files
are discovered without passing recursive=True to huggingface_hub.list_repo_tree.
There was a problem hiding this comment.
list_repo_tree does take recursive. It is in the signature (recursive: bool = False) and has been since the method landed in huggingface_hub 0.23.0, which is also the floor this function already needs, since it calls list_repo_tree regardless of this PR. So there is no new version constraint here. The linked #2676 is about server-side filtering on large repos, not whether the parameter exists. Verified on 1.17.0: the call returns the nested files (onnx/model.onnx plus the quantized variants) without raising, so I have left recursive=True as is.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Resolve the coderabbit review comment on the PR
gh api graphql -f query='
{
repository(owner: "qdrant", name: "fastembed") {
pullRequest(number: 642) {
reviewThreads(first: 50) {
nodes {
id
isResolved
comments(first: 5) {
nodes {
author { login }
body
url
}
}
}
}
}
}
}'Length of output: 8970
🏁 Script executed:
#!/bin/bash
# Resolve the incorrect review thread
gh api graphql -f query='
mutation {
resolveReviewThread(input: { threadId: "PRRT_kwDOJ7ZVjc6F90mS" }) {
thread {
id
isResolved
}
}
}'Length of output: 717
@Dmheath1, thanks for the thorough clarification — my initial flag was wrong and I've noted the correction. Unfortunately I don't have permission to resolve the thread programmatically in this repo, so you'd need to resolve it manually. The recursive=True usage is correct and the review comment can be dismissed.
(ᵔ ᵕ ᵔ)🐇
🧬 Code Graph Analysis Results
fastembed/common/model_management.py
Lines 11-12 (imports list_repo_tree):
from huggingface_hub import snapshot_download, model_info, list_repo_tree
from huggingface_hub.hf_api import RepoFileLines 261-277 (calls list_repo_tree(..., recursive=True) and builds repo_files from the returned tree):
repo_revision = model_info(hf_source_repo).sha
repo_tree = list(
list_repo_tree(
hf_source_repo, revision=repo_revision, repo_type="model", recursive=True
)
)
allowed_extensions = {".json", ".onnx", ".txt", ".onnx_data", ".npy", ".vocab"}
repo_files = (
[
f
for f in repo_tree
if isinstance(f, RepoFile) and Path(f.path).suffix in allowed_extensions
]
if repo_tree
else []
)✏️ Learnings added
Learnt from: Dmheath1
Repo: qdrant/fastembed PR: 642
File: fastembed/common/model_management.py:262-268
Timestamp: 2026-05-31T19:37:43.899Z
Learning: `huggingface_hub.list_repo_tree()` does support a `recursive: bool = False` parameter — it has been part of the method signature since huggingface_hub 0.23.0 and works correctly on 1.17.0. Passing `recursive=True` returns all files in nested subdirectories (e.g., `onnx/model.onnx`) without raising a TypeError. GitHub issue huggingface/huggingface_hub#2676 is about server-side filtering performance on large repos, not the absence of the `recursive` parameter. Do not flag `recursive=True` in `list_repo_tree` calls as invalid.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
An interrupted or externally truncated download can leave a corrupt file in the Hugging Face cache. On the next load the offline-first probe sees the file on disk and returns it, so the model fails with INVALID_PROTOBUF and the only fix is to delete the cache by hand. This makes the cache self-heal. The probe now raises when a model file's size does not match the recorded metadata, so download_model falls through to an online retry with force_download=True (huggingface_hub's cache check is existence-only and will not re-fetch a present-but-truncated blob). Verification also walks the repo tree recursively and matches files by their repo-relative path. Without that, weights kept in a subdirectory (onnx/model.onnx, more than half the models) were never recorded in the metadata, so neither the new check nor the existing one ever looked at them. A mismatch on an auxiliary config file is still left to best-effort loading, as before.
If a caller passes force_download through kwargs, the explicit force_download=True on the recovery retry would raise a duplicate keyword error. Merge it into kwargs so the recovery value wins without the collision.
b0a2f5f to
60e62b2
Compare
joein
changed the title
|
Thanks for this, David. The metadata gap is a good find: with the non-recursive tree listing, weights under onnx/ were never recorded, so the download integrity check has effectively been a no-op for 29 of our 52 models. I rebased onto main, which has moved since you opened this. #732 rewrote the same offline block into a loop over cache candidates, so your check now sits inside that loop, and a corrupt entry falls through to the next candidate instead of raising. That way a differently cased copy left by an older fastembed still loads offline. I also narrowed force_download. It was firing on any probe failure, so an otherwise intact cache missing one newly added file re-fetched the weights as well, and because it deletes the partial .incomplete file, each retry restarted a large download from zero. It now fires only on actual corruption, and only for the first attempt. Take a look and push back if you disagree with any of it. |
What this fixes
If a model download is interrupted, or the cache is truncated later (a partial copy into a Docker image, a CI cache restore, a disk issue), fastembed leaves a short or empty file in the HF cache. The next time you load that model, the offline-first probe sees the file on disk and returns its path. onnxruntime then throws
INVALID_PROTOBUF/ModelProto does not have a graph, and the only way out today is to delete the cache by hand.Reproduce (on main)
The fix
download_modelalready wraps that probe in a try/except, so the raise just routes to the existing online retry.force_download=True. huggingface_hub's cache check is existence-only, so a plainsnapshot_downloadkeeps a present-but-truncated blob;force_downloadis the documented way to re-fetch it (the same idiom transformers and sentence-transformers use).onnx/model.onnx, more than half the supported models) were never recorded in the metadata, so neither the new self-heal nor the existing download-integrity check ever looked at them.Behavior
Tested locally against root-level (bge), subdirectory (snowflake arctic, jina reranker) and external-weight (e5, minicoil) layouts, plus the existing suite.