-
Notifications
You must be signed in to change notification settings - Fork 238
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: build[uv] #1856
feat: build[uv] #1856
Conversation
Opened an issue about missing URI support in uv: astral-sh/uv#4124 |
7410da3
to
f1a6af4
Compare
26511bf
to
7923ecd
Compare
Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com>
Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com>
Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com>
Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com>
Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com>
Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com>
7923ecd
to
09bd9ba
Compare
I marked this ready for review. I think this + #1854, combined with Pyodide, would be a good new minor release. I chose |
Even just moving some (not all!) of the build tests and none of the pip tests still makes a noticeable impact on our test time:
|
I cancelled the Travis CI build to get manylinux to build. |
@@ -176,6 +177,7 @@ messages_control.disable = [ | |||
"wrong-import-position", | |||
"unused-argument", # Handled by Ruff | |||
"broad-exception-raised", # Could be improved eventually | |||
"consider-using-in", # MyPy can't narrow "in" |
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.
Is there a related issue opened in MyPy ?
I agree with both pros you're mentioning but I'd just like to mention some cons (I might be wrong, please do correct me):
That being said, I'm fine with the current state and we can decide later on if this needs to be updated. Am I wrong in thinking there are no tests for macOS tests with this new frontend ?
|
Does it work if we don't add the Actually, I think |
It's working locally, let's see how it goes in CI. |
I don't know what you asked exactly but my guess would be yes - even though it might not be tested.
|
I might do a) in my fork just to see the improvement we'd get. |
Probably could ask @ned-deily for the intel suffixed free-threading, it would be nice if user could just use uv directly with the environment we give them, rather than always having to add the platform target flag. Also wonder why there's no Regarding a), we probably could use uv automatically for all the installs we do (including pip), though we can't rely on it, so it's strictly a speed up, not a simplification. I don't think we can move any user installs implicitly to uv, since uv isn't a guaranteed drop-in replacement; things like dependencies that require dev versions will fail with uv. But on things we control, I think it's safe to use it if present. But we could always do that later. We can also use That's one nice benefit of this: Regarding d), this would increase the usage of the build backend, and moving the default (to build without uv? The fact it's a little slower than pip due to less setup is annoying, though, without uv; using uv for our internal installs would fix that, I think) would be something interesting, maybe even for a 3.0 release. |
Sure, we can add both of those additional names. Please open an issue for that on the cpython tracker. |
There's one last item before this can be merge I think.
|
Got the following error when testing a universal2 wheel:
|
Thanks for the report @njzjz, int can be reproduced in tests, I'll commit the test failure. I'll propose a fix later today. |
I don't know why the updated test does not fail in CI. Edit: I can't build pillow from sources locally but CI can. |
Rather than just removing this environment variable from the test environment, I propose we use the current version where tests are running. It could be useful in the edge case where a native dependency needs to be built in order to run the tests. |
IMO, I think it would be better to remove MACOSX_DEPLOYMENT_TARGET in the tests environment, as that is more likely to mimic what a user will have, and be a bit less surprising overall. Libraries should still build, though users should hopefully not be building binaries in the test step. In general, it would be nice if we could validate this - a library should not build for a lower macOS deployment target than its dependencies. Unfortunately, validating it is hard, as test dependencies might not count (could be optional dependencies), for example. I thought about the action a bit, I think it's best to just expect someone to setup uv in a previous step for now. I've been using - name: Setup uv
uses: yezz123/setup-uv@v4
- name: Build wheels
uses: henryiii/cibuildwheel@henryiii/feat/builduv Longer term, maybe the action could check cibuildwheel's config and detect if |
After reading a bit more on the usage of I'd rather set Once we can use
This plan sounds good.
Well, while I do agree with general idea, I think probably not doable and there always will be someone that does want to use an up-to-date version of something with outdated versions of dependencies (maybe just by using |
Okay, sounds good as is then.
That’s what I meant by hard. At best we might come up with a way to add a warning. Good to go then? |
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.
Good to go.
@henryiii, I'll let you merge if you don't have any last minute remarks.
I'm curious, did this ever happen? If not, is it worth writing it up in an issue? |
This adds support for setting the build backend to
build[uv]
, which will use the build backend with the uv installer option. Using this will also move all installs over to uv if possible. Python <3.8 will still use pip.There's one workaround we need to fix first; uv doesn't seem to support URI's. Edit: fixed upstream in astral-sh/uv#4145, but we pulled our existing workaround out into a function, so going with that for now, can be updated once a new uv is out and common.
This just uses the system uv everywhere (except on linux), which is fastest and gives the user control when installing it.
Also checking the tests by setting this as the default in the CI sample job; this actually built without isolation before, so it's a little slower, but much faster than if we enabled isolation any other way. Our build tests use it, which actually is faster, except for a few that expected pip to be present. The uv backend doesn't need to be backward compatible with anything, so it installs as little as possible (also faster).
Build example at https://github.com/henryiii/deflate/actions/workflows/build.yml. Speedup is visible in the images below.
pip
build[uv]