feat: support HF_ENDPOINT environment variable for model downloads - #667
Open
pangwangshu wants to merge 6 commits into
Open
feat: support HF_ENDPOINT environment variable for model downloads#667pangwangshu wants to merge 6 commits into
pangwangshu wants to merge 6 commits into
Conversation
…d_files_from_huggingface Co-authored-by: pangwangshu <1851324+pangwangshu@users.noreply.github.com>
Co-authored-by: pangwangshu <1851324+pangwangshu@users.noreply.github.com>
…ctions model_info and list_repo_tree, as imported from huggingface_hub, are bound methods of a module-level HfApi() singleton and don't accept an `endpoint` kwarg — only snapshot_download does. Passing `endpoint=` to them raised TypeError at runtime whenever HF_ENDPOINT was set. Construct an HfApi instance with the endpoint instead, and use autospec=True in the tests so mocks are checked against the real huggingface_hub signatures. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus 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. 📝 WalkthroughWalkthrough
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to The change is merge-ready after normal checks and review; no actionable merge-blocking risk remains. Suggested reviewers: 🚥 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: 3
🧹 Nitpick comments (1)
tests/test_common.py (1)
64-68: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the local-only endpoint path.
The helper always uses the default
local_files_only=False. The tests therefore do not execute the local-onlysnapshot_downloadcall, so a regression in that endpoint forwarding would pass this suite.Add a helper parameter and a test that invokes
local_files_only=True.🤖 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 `@tests/test_common.py` around lines 64 - 68, Add a local_files_only parameter to the relevant test helper around ModelManagement.download_files_from_huggingface, forward it to the download call, and add coverage invoking the helper with local_files_only=True. Preserve the existing default behavior for tests that omit the parameter.
🤖 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 218-219: Normalize the HF_ENDPOINT value in the model-management
flow before constructing HfApi or calling snapshot_download: treat an empty
environment value as unset and pass None or omit the endpoint, or raise a clear
configuration error. Update the hf_endpoint handling adjacent to
HfApi(endpoint=hf_endpoint), preserving configured non-empty endpoints.
- Around line 235-236: Remove endpoint from kwargs before forwarding download
arguments in the model download flow, resolve that value once, and reuse it
consistently when constructing HfApi and in both snapshot_download calls.
Preserve other kwargs unchanged to avoid passing endpoint twice.
In `@tests/test_common.py`:
- Around line 37-55: Update _run_download_with_mocks to use the autospecced
mock_hf_api_cls.return_value directly instead of replacing it with a plain
MagicMock via return_value. Configure model_info and list_repo_tree on that
autospecced instance while preserving the HfApi patch with autospec=True, so
invalid method calls still raise TypeError.
---
Nitpick comments:
In `@tests/test_common.py`:
- Around line 64-68: Add a local_files_only parameter to the relevant test
helper around ModelManagement.download_files_from_huggingface, forward it to the
download call, and add coverage invoking the helper with local_files_only=True.
Preserve the existing default behavior for tests that omit the parameter.
🪄 Autofix
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 Plus
Run ID: 6c06c512-85b0-437b-8f1d-f040eb518d79
📒 Files selected for processing (2)
fastembed/common/model_management.pytests/test_common.py
Addresses CodeRabbit review on PR qdrant#667: - Treat an empty-string HF_ENDPOINT as unset instead of forwarding "" as the endpoint. - Pop endpoint out of kwargs before resolving hf_endpoint, so callers who pass endpoint= directly no longer collide with the explicit endpoint= kwarg passed to snapshot_download. - Stop overwriting the autospecced HfApi instance mock with a plain MagicMock, so method calls on it stay signature-checked. - Add test coverage for the above plus the local_files_only download path. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@tests/test_common.py`:
- Line 48: Update the environment patching setup in the test around patch.dict
so HF_ENDPOINT is removed whenever it is absent from extra_env, while preserving
any explicitly provided endpoint value. Ensure the unset-endpoint test does not
inherit HF_ENDPOINT from the runner environment.
🪄 Autofix
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 Plus
Run ID: cf4a5dc1-5100-41db-a06c-b4c9def58256
📒 Files selected for processing (2)
fastembed/common/model_management.pytests/test_common.py
🚧 Files skipped from review as they are similar to previous changes (1)
- fastembed/common/model_management.py
Addresses CodeRabbit review on PR qdrant#667. patch.dict(os.environ, extra_env) only adds/overrides keys, it never removes ones already present. If HF_ENDPOINT happens to be set in the runner's environment, tests calling _run_download_with_mocks with an extra_env that omits it would silently inherit that value instead of exercising the unset case. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Both branches added independent tests to the same file; kept both sets of imports and test functions.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes #502. Adds support for the
HF_ENDPOINTenvironment variable sofastembedcan download models from a custom/mirrored Hugging Faceendpoint (e.g. a Nexus repository, per the linked feature request).
ModelManagement.download_files_from_huggingfacenow readsHF_ENDPOINTfrom the environment and forwards it to
snapshot_download. Formodel_infoandlist_repo_tree, forwarding is done via anHfApi(endpoint=...)instance rather than the module-level functions, since those are bound
methods of a default
HfApi()singleton that doesn't accept anendpointkwarg directly.
HF_ENDPOINTis unset, behavior is unchanged (endpoint resolves tohuggingface_hub's default).
Test plan
test_hf_endpoint_forwarded_to_hub_calls/test_no_hf_endpoint_no_extra_kwargin
tests/test_common.py, usingautospec=Trueso the mocks arechecked against the real
huggingface_hubcall signatures.pytest tests/test_common.pypasses.ruff check/ruff format --checkclean on changed files.huggingface_hub(v1.26.1) thatHfApi(endpoint=...).model_info()succeeds where the naivemodel_info(..., endpoint=...)approach raisesTypeError.