-
Notifications
You must be signed in to change notification settings - Fork 29
PoC: allow users to lock the kernel revisions #8
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
Conversation
This change allows Python projects that use kernels to lock the kernel revisions on a project-basis. For this to work, the user only has to include `hf-kernels` as a build dependency. During the build, a lock file is written to the package's pkg-info. During runtime we can read it out and use the corresponding revision. When the kernel is not locked, the revision that is provided as an argument is used.
| with open(pyproject, "rb") as f: | ||
| data = tomllib.load(f) | ||
|
|
||
| kernel_versions = _get_nested_attr(data, ["tool", "kernels", "dependencies"]) |
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: I don't like those helper functions (they are super easy to screw up).
data.get("tool", {}).get("kernels", {}).get("dependencies", None)Is more readable to me than delegating to another function.
| locked_revision = _get_caller_locked_kernel(repo_id) | ||
| if locked_revision is not None: | ||
| revision = locked_revision |
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.
This clashes with the argument.
IMHO:
We should have EITHER revision be used, but never both, meaning:
if we can have a proper lockfile, then we should ALWAYS use the lockfile.
If there are side effects that may get no lockfile, we should ALWAYS ask for the version.
Making where the information comes from is much easier to debug afterwards imho.
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.
Yeah, agreed. How should be go about this? Something like revision=None means lock file, otherwise get the specified revision? (I only Python had sum types 😁 )
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.
I suggest we just support one.
I'm fine looking for the lockfile always if it works (and if people don´t use setuptools we crash first, figure it out later)
| revision = locked_revision | ||
|
|
||
| filename = hf_hub_download( | ||
| repo_id, "build.toml", local_files_only=True, revision=revision |
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.
Maybe we should just use "main" all the time since we're not going to use revision as a versionning system.
| return [importlib.metadata.distribution(dist_name) for dist_name in dist_names] | ||
|
|
||
|
|
||
| def _get_caller_module() -> Optional[ModuleType]: |
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.
Pretty tricky.
I can see some potential screw ups in the resoluion there (like dependency injection, callback feeding etc..).
But in general I think it's good enough to get started.
Worst case we simply inspect the entire callstack, we're bound to find our caller somewhere.
|
Abandoning this PR for one with real lock files. |
This change allows Python projects that use kernels to lock the kernel revisions on a project-basis. For this to work, the user only has to include
hf-kernelsas a build dependency. During the build, a lock file is written to the package's pkg-info. During runtime we can read it out and use the corresponding revision. When the kernel is not locked, the revision that is provided as an argument is used.