-
Notifications
You must be signed in to change notification settings - Fork 544
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
feat: Introduce an experimental uv
toolchain
#1989
Conversation
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 decided that I needed to refresh my memory on bzlmod now that it seems more stable and functional. I definitely have a lot to learn. Anyway, so this only works with bzlmod. Maybe it's not much work to do similar in WORKSPACE. Does compile_pip_requirements even work in bzlmod?
Yes it does :)
I decided to fetch the uv binaries from PyPI just in case we want to launch it with Python in future e.g. python -m uv pip compile, but I actually just punted and launched it with the worst Bash script for now.
I decided to manually unpack the wheel as a filegroup and grab the rust binary from the wheel because all of the whl_library stuff looks too complicated and overkill given that Im not running it via Python
As I mentioned, I think we should probably use toolchains and use the binaries directly from GH releases page of uv
. It is a more direct way of having the platform binary available to you.
I've always liked the idea of having "pinning" be a concern of the "pip" extension / repository. They are intrinsically linked in that to retrieve third-party dependencies, you need a "pinned" lockfile for the relevant platforms. So why not extend "pip" to do it? Anyway, that's why I started here, but it obviously isn't vendoring the lockfiles into the right locations right now. It's just a simple way to launch "uv"
I like the idea of a pin
target. I am wondering about what we can do here. I think in the ideal case we should have:
- A rule (not a macro) that does
uv pip compile
and uses auv
toolchain and thepython
toolchain to get the tools. This allows us to pass the right interpreter touv
all the time. We should haveexec
configuration on the rule for the toolchains. - The output file should be created by the rule and that output file can be synched using the same technique we have in
examples/pip_parse_vendored
. That way we can reuse the existing techniques to write back to the system. - I like the idea of keeping the wrapping of
uv
as minimal as possible.
Bikeshedding:
- Is
pin
the best name? Should we instead uselock
? - Should we accept
requirements.in
orpyproject.toml
as input to thepip.parse
so that the user does not need to specify it using in thepin
target? I would probably say yes to this. - Should we automatically generate targets for pinning for all of the platform targets that the user wants to support? I would also probably say yes to that.
Having an interface like:
pip.parse(
hub_name = "foo",
python_versions = [], # means all
platforms = ["windows_x86_64", "linux_x86_64", "mac_aarch64"], # could be default
src = "pyproject.toml", # could be default
lockfile_dir = "", # default, means the root of the workspace
)
maybe could be feasible?
Other things I would love to fix the following issues as part of this redesign:
- Users needing to create a requirements_lock.txt file to store the output of the pinning. The error is unintuitive and I think we could do some stuff there - aspects write_source_file does not fail if the destination file is not present.
I might have forgotten something, so if I remember I'll update this lengthy prose.
Thanks for the initial review @aignas
Yes. I like the look of it. We will keep iterating on it. The interface you have there is almost an exact match of what I have in mind.
Yes. That's aligned with my thinking and helps with the migration path and onboarding. In addition, we should also support a simple bazel list of requirements specifiers `requirement_specifiers = ["requests > 2.0.0", "click > 7.0.0", ]
Yes. That's aligned with my thinking too.
Yes. I'd like to fix it too. I think in your interface sketch, as long as we use a directory with deterministic named files, then it should be reasonable to do. I think that would let the target run on initial bootstrap without failing 🤞 |
Yes, agreed. I think a toolchain makes sense. I avoided in this quick hack because every time I get involved with toolchains, I confuse myself 😂 I think |
re: toolchains: Yeah, toolchains are exactly for this sort of thing. uv is basically a "compiler" for the requirements.in "source code". Here's the basic for a toolchain-based implementation. T
That should be the gist of it. The above allows An alternative might be to add it to the exec_tools toolchain type, too, directly or indirectly. In order to have something like |
You're probably right. I use them interchangeably a lot of the time, but it's probably better to be precise here. I'm not sure if there is a good write-up anywhere on the topic that clearly describes the difference between pinning vs locking? To me, here's the difference:
I think if we align on those, then I think |
CHANGELOG: - (bzlmod) Added `uv` to handle requirements files internally within rules_python.
I had some code laying around with using the uv binaries directly, so I took the liberty to push that here. |
Thanks both! I'll hopefully have some time today and I think my goal will be to add a simple toolchain. It's likely that it could be merged independently even if unused while we work on the rest of the interface and functionality. Some additional thoughts I've had:
|
First of all, thank you for looking into that! I've been personally struggling with maintaining One thought for consideration: how does it align with #1360 ? If |
They're unrelated for the most part. This PR is to assist with a better implementation of compile_pip_requirements. That issue was more how we could align to bring some code across for another initiative that we likely won't pursue at the moment. We did vendor some of the code at the time https://github.com/bazelbuild/rules_python/tree/main/third_party/rules_pycross We can probably close that issue #1360 tbh. If pycross is working for you, that's great! There's no real reason to favor rules_python. Use what works best for you. We keep close eyes on the standards, and I'm sure one day a lockfile standard will emerge. However, I must say that it's not that important that a lockfile is standardized as long as the input dependency specifiers are standardized. Which fortunately they are and have been for a long time. So largely anyone using a PEP621 pyproject.toml can relock dependencies and I would largely expect the lockfiles to be similar. The important thing is whether a resolver can find a valid solution, not that all resolvers produce identical solutions. |
I am fully on the same page. I might have been unclear in that bit as I was primarily looking at this PR as a way to make multi-platform like easier, but I wasn't concerned with that. I am not even convinced a lockfile standard is needed at all. My sentiment was about using something like pmd or poetry that generates platform-agnostic lockfiles given that there was an intention to integrate Either way, if |
Creating cross-platform uv.lock file is in the works, is that going to be considered as well |
We are aware of that. The PR is still in development and discussions with the maintainers, so it's too early to commit to anything. The lockfile format itself is really an implementation detail in this design though. I'm curious though: what difference does it make to the functionality what the lockfile format is? As I mentioned in other comments, the lockfile format itself is not the most important thing.
|
I will say that a lock file that includes the URLs of the wheels/sdist, has the advantage of utilizing the bazel downloader directly without an extra step of fetching the metadata. Also having a single lock file for all platforms is a plus. |
FYI, @groodt, I have created #2006 to move the remaining of the PyPI related
The gist is that we can keep implementation specific names in
There are many projects that have very different requirements, so its hard to As for needing to call PyPI before fetching - it is done only needed when If you would like to consider |
uv
toolchainuv
toolchain
Ok. You've convinced me. Also some issues I encountered with rules_jsonnet and some passing conversations with people at work also led me to discover this: bazelbuild/bazel#22024 (reply in thread) I tend to agree with you. The naming doesn't matter and it's cleaner to use I know I implemented the numeric prefixes in earlier iterations, but I've removed it for now. I found it to be a bit too arcane and unnecessary right now. It can be added back later once we're further along. |
Ok @aignas @rickeylev This is probably ready for another pass. I think I've incorporated the majority of the requested changes. I've also updated the original PR description with some items to follow-up with #1989 (comment) so please have a read of that as well and let me know if there is more to add. |
uv
toolchainuv
toolchain
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.
Nothing blocking. Left a few comments that would be good address (other than the bikeshedding one) and feel free to merge this one and create small PRs or create issues to not forget to create PRs later. You could also just address the comments here if you wish.
@@ -41,6 +42,7 @@ filegroup( | |||
"//python/pip_install:distribution", | |||
"//python/private:distribution", | |||
"//python/runfiles:distribution", | |||
"//python/uv:distribution", |
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 am not sure if having python/uv/private
is better than python/private/uv
but I don't we can discuss/bikeshed it later, we can merge it as is right now.
url = url, | ||
sha256 = UV_TOOL_VERSIONS[repository_ctx.attr.uv_version][repository_ctx.attr.platform].sha256, | ||
stripPrefix = strip_prefix, |
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, in order to ensure that this works correctly with internal indexes and the bazel downloader, it would be good to pass the auth
headers, similar to how it is done in whl_library.bzl
.
# Marked manual so that `bazel test //...` passes | ||
# even if no toolchain is registered. | ||
tags = ["manual"], | ||
# EXPERIMENTAL: Visibility is restricted to allow for changes. |
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.
Do you want to factor visibility into a variable? I just mention it in case you plan to patch-out the visibility restrictions to experiment with. It'd be easier to patch the variable than all the various visibility lines
Follows: #1989 Addresses the following: * Removes usage of `maybe` * Pretty-print renders some generated *.bazel * Shorter default_repo_names
} | ||
|
||
# From: https://github.com/astral-sh/uv/releases | ||
UV_TOOL_VERSIONS = { |
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.
FYI @groodt you may be interested in https://github.com/aspect-build/rules_lint/blob/main/lint/mirror_ruff.sh which auto-updates our mirror of Ruff, another tool distributed in the same way by astral-sh. We have GitHub Actions send the update PRs so no humans need to do anything.
Context
This PR introduces a toolchain for uv and a module extension that can install it. It will be followed by some rules that make use of the toolchain for things like dependency locking.
Relates to #1975
Future enhancements (in follow-up PRs):
Notes
Try it out:
cd examples/bzlmod bazel run @rules_python//python/uv:current_toolchain
I was able to produce a windows lockfile from my
osx_x86_64
:Click me