-
Notifications
You must be signed in to change notification settings - Fork 98
feat: make kernel loading work when offline mode ie enabled. #580
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
Merged
+207
−41
Merged
Changes from all commits
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
8fa5009
feat: make kernel loading work when offline mode ie enabled.
sayakpaul cd50dd7
Merge branch 'main' into offline-kernels
sayakpaul c449588
Apply suggestions from code review
sayakpaul c47341f
Merge branch 'main' into offline-kernels
sayakpaul c831159
address reviewer comments.
sayakpaul dafe225
up
sayakpaul 91def05
Apply suggestions from code review
sayakpaul e8a9715
Merge branch 'main' into offline-kernels
sayakpaul File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,9 +3,11 @@ | |
| import pytest | ||
| import torch | ||
| import torch.nn.functional as F | ||
| from huggingface_hub import constants | ||
| from huggingface_hub.errors import HfHubHTTPError | ||
|
|
||
| from kernels import get_kernel, get_local_kernel, has_kernel, install_kernel | ||
| from kernels._versions import resolve_version_spec_as_ref, select_revision_or_version | ||
|
|
||
|
|
||
| @pytest.fixture | ||
|
|
@@ -243,6 +245,67 @@ def test_trust_remote_code_flag_allows_untrusted(): | |
| get_kernel("kernels-test-untrusted/ci-test-kernel", version=1, trust_remote_code=True) | ||
|
|
||
|
|
||
| def test_install_kernel_offline_with_revision(monkeypatch, local_kernel_path): | ||
|
Member
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. Love these tests! |
||
| """install_kernel should resolve a cached snapshot when HF_HUB_OFFLINE=1.""" | ||
| expected_path = local_kernel_path | ||
| monkeypatch.setattr(constants, "HF_HUB_OFFLINE", True) | ||
|
|
||
| path = install_kernel("kernels-community/relu", revision="v1") | ||
| assert path == expected_path | ||
|
|
||
|
|
||
| def test_install_kernel_offline_avoids_network(monkeypatch, local_kernel_path): | ||
| """When HF_HUB_OFFLINE=1, install_kernel must not make any Hub requests.""" | ||
| expected_path = local_kernel_path | ||
|
|
||
| class _NoNetwork(RuntimeError): | ||
| pass | ||
|
|
||
| def _fail(*_args, **_kwargs): | ||
| raise _NoNetwork("Hub access attempted in offline test") | ||
|
|
||
| monkeypatch.setattr("huggingface_hub.hf_api.get_session", _fail) | ||
|
|
||
| # Online path must touch the Hub via get_session and therefore fail. | ||
| with pytest.raises(_NoNetwork): | ||
| install_kernel("kernels-community/relu", revision="v1") | ||
|
|
||
| # Offline mode resolves entirely from the local cache, so get_session is | ||
| # never called. | ||
| monkeypatch.setattr(constants, "HF_HUB_OFFLINE", True) | ||
| path = install_kernel("kernels-community/relu", revision="v1") | ||
| assert path == expected_path | ||
|
|
||
|
|
||
| def test_install_kernel_offline_with_version(monkeypatch, local_kernel_path): | ||
| """get_kernel(version=) should resolve via local refs when HF_HUB_OFFLINE=1.""" | ||
| expected_path = local_kernel_path | ||
| monkeypatch.setattr(constants, "HF_HUB_OFFLINE", True) | ||
|
|
||
| commit = select_revision_or_version("kernels-community/relu", revision=None, version=1) | ||
| path = install_kernel("kernels-community/relu", revision=commit) | ||
| assert path == expected_path | ||
|
|
||
|
|
||
| def test_install_kernel_offline_uncached_revision(monkeypatch): | ||
| """install_kernel should fail with a helpful error when offline and uncached.""" | ||
| monkeypatch.setattr(constants, "HF_HUB_OFFLINE", True) | ||
|
|
||
| with pytest.raises(FileNotFoundError, match=r"local snapshot"): | ||
| install_kernel( | ||
| "kernels-test/this-repo-should-not-exist", | ||
| revision="0000000000000000000000000000000000000000", | ||
| ) | ||
|
|
||
|
|
||
| def test_version_resolution_offline_missing(monkeypatch): | ||
| """resolve_version_spec_as_ref should raise a clear error when offline and no cache.""" | ||
| monkeypatch.setattr(constants, "HF_HUB_OFFLINE", True) | ||
|
|
||
| with pytest.raises(ValueError, match=r"offline mode"): | ||
| resolve_version_spec_as_ref("kernels-test/this-repo-should-not-exist", 1) | ||
|
|
||
|
|
||
| def silu_and_mul_torch(x: torch.Tensor): | ||
| d = x.shape[-1] // 2 | ||
| return F.silu(x[..., :d]) * x[..., d:] | ||
Oops, something went wrong.
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.
Uh oh!
There was an error while loading. Please reload this page.
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.
Can't we
load_kernelhere and keep the body of_resolve_local_variant_pathinsideload_kernel? It would more clearly express the relation between the functions.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.
Thought about it a bit but
install_kernelreturnsPath, whereasload_kernelreturnsModuleType. To recover the path from a module return byload_kernel(), we will have to do some kind of parsing.Then the
_import_from_path()utility also needs some changes.The flow would become
get_kernel->install_kernel>load_kernel->_import_from_path(repo_info=None)— that cachesLoadedKernel(repo_info=None).Then
get_kerneldoes its own_import_from_path(variant_path, repo_info=RepoInfo(...)), hits the cache, and returns the module while silently discarding the actualrepo_info.As a result,
get_loaded_kernels()would reportrepo_info=Nonefor kernels loaded viaget_kernelin offline mode which would be a regression, if not handled in_import_from_path.I think the shared utility solution is a cleaner approach here.
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.
Ah, got it, thanks!