-
Notifications
You must be signed in to change notification settings - Fork 149
fix: resolve HF symlinks in from_hub via temp local_dir #1495
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -18,6 +18,7 @@ | |
| import contextlib | ||
| import pathlib | ||
| import re | ||
| import tempfile | ||
| import warnings | ||
| from typing import Literal, TypeAlias, TypeVar, cast | ||
|
|
||
|
|
@@ -751,8 +752,18 @@ def from_hub( | |
| ) -> list["EmbeddedIntrinsicAdapter"]: | ||
| """Load embedded adapters from a Granite Switch model on Hugging Face Hub. | ||
|
|
||
| Downloads `adapter_index.json` and the `io_configs/` directory, then | ||
| delegates to :meth:`from_model_directory`. | ||
| Downloads `adapter_index.json` and the `io_configs/` directory into a | ||
| self-contained local directory, then delegates to | ||
| :meth:`from_model_directory`. | ||
|
|
||
| `huggingface_hub.snapshot_download`'s default cache-backed snapshot | ||
| directory populates `io_configs/` with symlinks that resolve into a | ||
| sibling `blobs/` directory *outside* the snapshot root. That breaks the | ||
| contract `from_model_directory` expects (a self-contained model | ||
| directory) and trips its path-escape check. To satisfy that contract, | ||
| the files are downloaded directly into a temporary directory (via | ||
| `local_dir`) instead, so `io_configs/` contains real files rather than | ||
| symlinks escaping the directory. | ||
|
|
||
| Args: | ||
| repo_id (str): Hugging Face Hub repository ID | ||
|
|
@@ -780,22 +791,24 @@ def from_hub( | |
| 'Hugging Face Hub. Please install it with: pip install "mellea[switch]"' | ||
| ) from e | ||
|
|
||
| local_root = huggingface_hub.snapshot_download( | ||
| repo_id=repo_id, | ||
| allow_patterns=["adapter_index.json", "io_configs/**"], | ||
| cache_dir=cache_dir, | ||
| revision=revision, | ||
| ) | ||
| try: | ||
| return EmbeddedIntrinsicAdapter.from_model_directory( | ||
| local_root, intrinsic_name=intrinsic_name | ||
| with tempfile.TemporaryDirectory() as local_dir: | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Cache always cold. Each call is ~51 HTTP round-trips (repo_info + 25 HEAD + 25 GET) with zero reuse across calls. Measured on Hard offline failure: Suggested fix: Use a persistent directory under |
||
| downloaded_dir = huggingface_hub.snapshot_download( | ||
| repo_id=repo_id, | ||
| allow_patterns=["adapter_index.json", "io_configs/**"], | ||
| cache_dir=cache_dir, | ||
| local_dir=local_dir, | ||
| revision=revision, | ||
| ) | ||
| except ValueError as e: | ||
| if intrinsic_name is not None: | ||
| raise ValueError( | ||
| f"No adapter found for adapter function '{intrinsic_name}' in {repo_id}" | ||
| ) from e | ||
| raise ValueError(f"No adapters found in {repo_id}") from e | ||
| try: | ||
| return EmbeddedIntrinsicAdapter.from_model_directory( | ||
| downloaded_dir, intrinsic_name=intrinsic_name | ||
| ) | ||
| except ValueError as e: | ||
| if intrinsic_name is not None: | ||
| raise ValueError( | ||
| f"No adapter found for adapter function '{intrinsic_name}' in {repo_id}" | ||
| ) from e | ||
| raise ValueError(f"No adapters found in {repo_id}") from e | ||
|
|
||
| @staticmethod | ||
| def from_source( | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,7 +6,7 @@ | |
| import json | ||
| import os | ||
| import pathlib | ||
| from unittest.mock import MagicMock, patch | ||
| from unittest.mock import ANY, MagicMock, patch | ||
|
|
||
| import pytest | ||
| import yaml | ||
|
|
@@ -308,6 +308,7 @@ def test_downloads_and_delegates(self, model_dir): | |
| repo_id="ibm-granite/granite-switch-micro", | ||
| allow_patterns=["adapter_index.json", "io_configs/**"], | ||
| cache_dir="/tmp/test-cache", | ||
| local_dir=ANY, | ||
| revision="test-rev", | ||
| ) | ||
| assert len(adapters) == 2 | ||
|
|
@@ -324,11 +325,21 @@ def test_filter_single_intrinsic(self, model_dir): | |
| repo_id="ibm-granite/granite-switch-micro", | ||
| allow_patterns=["adapter_index.json", "io_configs/**"], | ||
| cache_dir=None, | ||
| local_dir=ANY, | ||
| revision="main", | ||
| ) | ||
| assert len(adapters) == 1 | ||
| assert adapters[0].intrinsic_name == "citations" | ||
|
|
||
| def test_from_hub_requests_local_dir(self, model_dir): | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Test gap: mock returns Suggested: a |
||
| """from_hub requests local_dir to avoid symlinks escaping model directory.""" | ||
| with patch( | ||
| "huggingface_hub.snapshot_download", return_value=str(model_dir) | ||
| ) as mock_dl: | ||
| EmbeddedIntrinsicAdapter.from_hub("ibm-granite/granite-switch-micro") | ||
| _, kwargs = mock_dl.call_args | ||
| assert "local_dir" in kwargs and kwargs["local_dir"] is not None | ||
|
|
||
| def test_missing_huggingface_hub_raises(self): | ||
| with patch.dict("sys.modules", {"huggingface_hub": None}): | ||
| with pytest.raises(ImportError, match="huggingface_hub is required"): | ||
|
|
@@ -377,6 +388,7 @@ def test_hub_passes_revision_and_cache(self, model_dir): | |
| repo_id="ibm-granite/granite-switch-micro", | ||
| allow_patterns=["adapter_index.json", "io_configs/**"], | ||
| cache_dir="/tmp/cache", | ||
| local_dir=ANY, | ||
| revision="v2", | ||
| ) | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NIT:
:meth:from_model_directoryis an RST cross-reference directive. AGENTS.md §5 bans RST markup in docstrings — should befrom_model_directory``. Fix all three occurrences in this docstring (lines 757, 782, 784) or leave for a sweep.