Fork version selection based on wheel Python version tags#18587
Fork version selection based on wheel Python version tags#18587charliermarsh wants to merge 6 commits intomainfrom
Conversation
Merging this PR will not alter performance
Performance Changes
Comparing |
350f7ef to
8e8f698
Compare
| description = "State-of-the-art Machine Learning for JAX, PyTorch and TensorFlow" | ||
| keywords = ["NLP", "vision", "speech", "deep", "learning", "transformer", "pytorch", "tensorflow", "jax", "BERT", "GPT-2", "Wav2Vec2", "ViT"] | ||
| requires-python = ">=3.9.0" | ||
| requires-python = ">=3.9.0,<3.12" |
There was a problem hiding this comment.
This is a bit unfortunate, but tensorflow>=2.6,<2.16 legitimately doesn't have a wheel for Python 3.12...
There was a problem hiding this comment.
Is this new upper bound required, or did you it for some other reason? Cause I wouldn't want to make it required when we otherwise recommend against upper bounds.
There was a problem hiding this comment.
It's required, because the resolution below ends up forking on Python version during the course of solving, so we have a branch with >=3.12 that tries to include tensorflow>=2.6,<2.16, which doesn't have a valid wheel in that range, so it's a broken solution.
We could instead add this as a supported environments field rather than bounding requires-python. Or we could decide we don't want these semantics, and we could limit this behavior to the user-defined requires-python range (e.g., we need wheels at least for Python 3.9).
fd5df8a to
841e923
Compare
crates/uv/tests/it/pip_compile.rs
Outdated
| torchvision==0.15.1 ; (python_full_version < '3.12' and platform_machine == 'aarch64' and platform_python_implementation == 'CPython' and sys_platform == 'linux') or sys_platform == 'darwin' | ||
| # via -r requirements.in | ||
| torchvision==0.15.1+rocm5.4.2 ; platform_machine == 'x86_64' and sys_platform != 'darwin' | ||
| torchvision==0.15.1+cu118 ; (python_full_version >= '3.12' and sys_platform == 'linux') or (platform_machine != 'aarch64' and sys_platform == 'linux') or (platform_python_implementation != 'CPython' and sys_platform == 'linux') or (sys_platform != 'darwin' and sys_platform != 'linux') |
There was a problem hiding this comment.
Why did we switch accelerator here?
841e923 to
28894da
Compare
|
Is this related to #15932 ? |
|
Yeah, I think it's a similar idea. I am wondering if we should change this PR to only enforce the minimum Python version, though, to avoid problems like the "Tensorflow doesn't have a 3.12 wheel" issue. |
|
I filed mhammond/pywin32#2726 cause the combination of no source dist and no requires-python metadata is rare, most projects have |
|
I mean, I think we should still support some version of this? It's a bug, and requires-python is not the same thing. |
|
How would requires-python and an inferred bound from Python version tags act different here? Maybe I'm missing what this PR is trying to do. Or do we do want to support it with an upper bound? This would then be a design decision about whether we want upper-bounds in requires-python in user packages when we ignore then for dependency packages. |
|
It's plausible that you could publish a package that supports Python 3.8 and later (e.g., The goal of this PR is that we would not accept that version as a valid solution for Python 3.8 through Python 3.11 (unless the user's own project only supports Python 3.12 and later). |
|
That's seem like an odd combination, how would the package be compatible with 3.8, when there's no option to install it before 3.12? To me, this sounds more like a mistake in the metadata annotations. I know we had this case with an ML package before (torch or tensorflow), but there it was a mistake on their side and they fixed the bound to match the published wheels in the next release. |
|
I don't think it's an incorrect combination though. The code can support Python versions for which they don't publish wheels. |
|
I don't understand why we wouldn't want to handle that case correctly? |
| @@ -1532,43 +1503,43 @@ wheels = [ | |||
|
|
|||
| [[package]] | |||
| name = "h5py" | |||
| version = "3.11.0" | |||
| version = "3.11.[X]" | |||
There was a problem hiding this comment.
That looks like a too broad redaction.
|
I agree that this is correct logic-wise, but I'm worried about the added complexity in the already complex logic around implied/requested platform support and forking.
Do we have packages for which this is the case? |
|
PyTorch 2.5.1: #14836 |
|
If we only care about enforcing the minimum requires-python for the user's project, I suspect we can simplify the change. Right now, this attempts to enforce it in every fork. |
28894da to
c07e523
Compare
|
Okay, I pushed a commit that significantly simplifies things by only requiring that each chosen release has a wheel that's compatible with the minimum version in your |
c07e523 to
896ea11
Compare
Summary
In #15069, the latest version of
pywin32doesn't include a Python 3.7 wheel. With this PR, we fork if a wheel-only distribution has a lower-bound below ourrequires-pythonbound so that each fork can choose a version with a compatible wheel.Closes #15069.