Conversation
This change sets up the kernel resolver data structures and functions. These will be used to recursively solve kernel dependencies in subsequent PRs. A revolver is class with a method that returns where the kernel can be found (`LocalKernel` or `RemoteKernel`) given a `KernelDependency`. The current resolvers are: - `HubResolver`: resolves a kernel from the Hub. - `HubCacheResolver`: resolves a kernel from the local Hub cache. - `LockedHubResolver`: resolves a kernel from the Hub using a lock file. - `LockedHubCacheResolver`: resolves a kernel from the local Hub cache using a lock file. - `RepoPathsResolver`: resolves kernels from a repo ID -> local path mapping. - `KernelPathsResolver`: resolves kernels from a kernel dependency -> local path mapping. - `SequentialResolver`: tries a list of resolvers in order until one resolves. - `NoopResolver`: noop resolver
|
The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update. |
drbh
left a comment
There was a problem hiding this comment.
wooo the new resolvers are great! thanks for adding 🙏
|
|
||
| if version is not None: | ||
| revision = resolve_version_spec_as_ref(repo_id, version).name | ||
| revision = resolve_version_spec_as_ref(repo_id, version, local_files_only=constants.HF_HUB_OFFLINE).name |
There was a problem hiding this comment.
Just a note on these: this was determined locally in the functions, but this kind of global state makes it hard to reason about code, harder to test, and is really incompatible with the new resolvers. So switching to explicit passing here.
|
the changes may be skipping the trusted publisher check? this small test fails to throw on a untrusted publisher.. might be because we need to make a remote call to check the trusted publisher status? may be worth investigating further? diff --git a/kernels/tests/test_resolver.py b/kernels/tests/test_resolver.py
index 3ccb3d3..ed65d71 100644
--- a/kernels/tests/test_resolver.py
+++ b/kernels/tests/test_resolver.py
@@ -205,6 +205,32 @@ def test_hub_resolver_blocks_untrusted_org(api):
)
+def test_hub_cache_resolver_blocks_untrusted_org(monkeypatch, api):
+ class UntrustedOrg:
+ trustedKernelPublisher = False
+
+ class FakeApi:
+ def get_organization_overview(self, publisher):
+ return UntrustedOrg()
+
+ monkeypatch.setattr("kernels.hf_hub._get_hf_api", lambda: FakeApi())
+ monkeypatch.setattr(
+ "kernels.resolver.resolve_kernel_version",
+ lambda *args, **kwargs: "a" * 40,
+ )
+ monkeypatch.setattr(
+ "kernels.resolver.resolve_hub_cache_kernel",
+ lambda *args, **kwargs: _local_kernel("/tmp/cached"),
+ )
+
+ with pytest.raises(ValueError, match="not from a trusted publisher"):
+ HubCacheResolver(trust_remote_code=False).resolve(
+ api=api,
+ backend="cpu",
+ kernel=_dep("untrusted-org/kernel"),
+ )
+
+
@pytest.mark.cuda_only
def test_hub_resolver_trust_remote_code_bypasses_check(api):
location = HubResolver(trust_remote_code=True).resolve(
************UPDATE: this is reasonable since we only need to check trusted publishers are fetch time |
|
I have run the tests in various combinations locally. Merging now since we don't seem to get runners. Will fix any fallout 🤞 . |
This change sets up the kernel resolver data structures and functions. These will be used to recursively solve kernel dependencies in subsequent PRs. A revolver is class with a method that returns where the kernel can be found (
LocalKernelorRemoteKernel) given aKernelDependency. The current resolvers are:HubResolver: resolves a kernel from the Hub.HubCacheResolver: resolves a kernel from the local Hub cache.LockedHubResolver: resolves a kernel from the Hub using a lock file.LockedHubCacheResolver: resolves a kernel from the local Hub cache using a lock file.RepoPathsResolver: resolves kernels from a repo ID -> local path mapping.KernelPathsResolver: resolves kernels from a kernel dependency -> local path mapping.SequentialResolver: tries a list of resolvers in order until one resolves.NoopResolver: noop resolver