-
Notifications
You must be signed in to change notification settings - Fork 760
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
Add a --platform
argument to enable resolving against a target platform
#3111
Conversation
#[value(name = "aarch64-unknown-linux-musl")] | ||
Aarch64UnknownLinuxMusl, | ||
#[value(name = "x86_64-unknown-linux-musl")] | ||
X8664UnknownLinuxMusl, |
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 don't know that this is actually the representation we want to use, but it... kind of works well? And it lets us avoid inventing our own format.
Self::X8664UnknownLinuxGnu => Platform::new( | ||
Os::Manylinux { | ||
major: 2, | ||
minor: 17, |
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.
All of these values are based on the Tier 1 and Tier 2 definitions in the Rust project.
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 is also manylinux2014, so it should match what is often present
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 think there should be a way to override these for the user?
I just tried the --python-platform
feature (works great, thx!) but failed with packages built against a newer version of glibc
.
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 think we should add variants for the other manylinux versions. (By "failed", you mean, "didn't download those wheels"? Or, how did it fail?)
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.
e.g. open3d uses glibc
2.27 and it fails like this:
❯ uv pip compile requirements.in --python-platform=linux
x No solution found when resolving dependencies:
`-> Because open3d==0.18.0 is unusable because no wheels are available with a matching Python implementation
and you require open3d==0.18.0, we can conclude that the requirements are unsatisfiable.
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.
For completeness:
uv pip compile requirements.in --python-platform=macos
also fails for open3d
because the ARM64 wheels are only available for MacOS 13.0+ (or universal for 10.15+) while uv
specifies MacOS 11.0 for ARM64.
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.
In that case, should it be able to pick out the universal wheels? Wonder why it's not.
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.
Hmm good question, I think so?, because it would match the platform.
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'll look into that separately.
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.
Yes, with failed
I meant PEP 600 manylinux_x_y
packages not being selected during uv pip compile
due to a glibc version mismatch. Of course this behavior is expected, though an 11 year old version does effectively create constraints.
For us a few more allowed options for the python-platform
string would probably already work, thx 👍
c4ef696
to
9552ff3
Compare
Self::Aarch64AppleDarwin => "", | ||
Self::Aarch64UnknownLinuxGnu => "", | ||
Self::Aarch64UnknownLinuxMusl => "", | ||
Self::X8664UnknownLinuxMusl => "", |
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.
Per the spec, you're allowed to leave these empty to indicate "unknown".
It looks like this is sometimes used though: https://github.com/search?type=code&q=%22platform_release%22+path%3Arequirements.txt. E.g., sometimes as: sys_platform == "win32" and platform_release == "10"
.
Looks weirdly easy. I think this makes sense, will leave approval to one of the other too though. |
I think it actually is easy. Though there are probably some decisions around whether we want to allow users to provide more specific markers (like specific manylinux versions or whatever). |
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.
nice work!
crates/uv/src/cli.rs
Outdated
@@ -485,6 +486,12 @@ pub(crate) struct PipCompileArgs { | |||
#[arg(long, short)] | |||
pub(crate) python_version: Option<PythonVersion>, | |||
|
|||
/// The platform for which requirements should be resolved. | |||
/// | |||
/// Represented as a target triple (e.g., `x86_64-pc-windows-msvc`). |
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.
Can we link the list of target triples here?
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 think it would also be nice to elaborate on what is meant by "target triple" here. Coming from Rust/C/C++/whatever, the notion of the target triple makes intuitive sense. But in Python land, I might know that the things that make up a target are more involved than a standard target triple. I think calling that difference out in the docs will help understanding. And as a concrete example, it might help to answer the question of, "what if I want to specify a different Python implementation?" That isn't captured by the target triple. (And I think the suggestion in that case is to actually use the different Python implementation.)
Maybe a succinct way to explain things here is that the target triples are a heuristic to specify a part of the build parameters, but that they don't capture everything that can be in a "marker environment."
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 think "target triple" is the right abstraction to use?
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'm honestly not sure. I am skeptical. With that said, it does seem very useful. I feel like if we present it as a "shortcut" to specifying the most common parts of a marker environment that you might to specify, then it would be useful. (And we can say that there isn't yet a way to specify more parts of the marker environment than what the target triple does.)
I think the weird part of this is that the marker environment derived from the target triple is sort of a frankenstein monster. It's part static information fully determined by the triple and part dynamic information based on the current environment. I don't have enough experience to say how that's going to fall over, but it doesn't seem right to me. For example, target files for rustc don't have any dynamic information in them at all: https://github.com/rust-lang/rust/blob/eff958c59e8c07ba0515e164b825c9001b242294/tests/run-make/target-specs/my-x86_64-unknown-linux-gnu-platform.json
So I wonder if it makes sense to remove the dynamic parts here and, for example, hard-code cpython
. And for folks that want to use pypy (or whatever), then maybe in the future we can provide a way for end users to specify the complete marker environment.
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'm gonna run with it for now... Let's see where it takes us.
It's part static information fully determined by the triple and part dynamic information based on the current environment.
The only things that are dynamic are the Python-oriented tags: the Python version, the implementation version, etc. So it seems not-horrible to me.
Self::X8664UnknownLinuxGnu => Platform::new( | ||
Os::Manylinux { | ||
major: 2, | ||
minor: 17, |
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 is also manylinux2014, so it should match what is often present
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.
w00t! This is awesome.
@@ -7811,6 +7811,71 @@ fn no_version_for_direct_dependency() -> Result<()> { | |||
Ok(()) | |||
} | |||
|
|||
/// Compile against a dedicated platform, which may differ from the current platform. |
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.
It looks like the test here doesn't change based on the platform? Or maybe my eyes missed it. Can we add a more "exciting" test where there is a change based on the platform?
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.
It does, colorama gets included / excluded!
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'll add some more though. Do you have good examples from your work?
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.
Oh nice! I've mostly been working with anyio
, although I think that's mostly dependent on python_version
. Otherwise I made up my own packse scenarios.
crates/uv/src/cli.rs
Outdated
@@ -485,6 +486,12 @@ pub(crate) struct PipCompileArgs { | |||
#[arg(long, short)] | |||
pub(crate) python_version: Option<PythonVersion>, | |||
|
|||
/// The platform for which requirements should be resolved. | |||
/// | |||
/// Represented as a target triple (e.g., `x86_64-pc-windows-msvc`). |
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 think it would also be nice to elaborate on what is meant by "target triple" here. Coming from Rust/C/C++/whatever, the notion of the target triple makes intuitive sense. But in Python land, I might know that the things that make up a target are more involved than a standard target triple. I think calling that difference out in the docs will help understanding. And as a concrete example, it might help to answer the question of, "what if I want to specify a different Python implementation?" That isn't captured by the target triple. (And I think the suggestion in that case is to actually use the different Python implementation.)
Maybe a succinct way to explain things here is that the target triples are a heuristic to specify a part of the build parameters, but that they don't capture everything that can be in a "marker environment."
5cc156e
to
d0a2249
Compare
d0a2249
to
55b07a4
Compare
<!-- Thank you for contributing to uv! To help us out with reviewing, please consider the following: - Does this pull request include a summary of the change? (See below.) - Does this pull request include a descriptive title? - Does this pull request include references to any relevant issues? --> ## Summary The option `--platform` for `uv pip compile` was added in #3111 and was later renamed to `--python-platform` #3146. This PR updates the documentation such that the listed option is working again. ## Test Plan ```bash ❯ uv --version uv 0.1.37 (645d039 2024-04-23) ``` Before: ```bash ❯ uv pip compile requirements.in --platform=linux error: unexpected argument '--platform' found tip: to pass '--platform' as a value, use '-- --platform' Usage: uv pip compile <SRC_FILE>... For more information, try '--help'. ``` After: ```bash ❯ uv pip compile requirements.in --python-platform=linux Resolved 1 package in 215ms # This file was autogenerated by uv via the following command: # uv pip compile requirements.in --python-platform=linux uv==0.1.37 ```
Summary
I've wanted to try this for a long time, so decided to give it a shot. The basic idea is that you can provide a target triple (e.g.,
--platform x86_64-pc-windows-msvc
) and resolve against that platform, rather than the currently-running platform. It's functionally similar to--python-version
, though a bit simpler since there's no need to engage withRequires-Python
.Our infrastructure is well-setup for this and so, in the end, it's actually pretty straightforward: for each triple, we just need to override the markers and platform tags.