-
Notifications
You must be signed in to change notification settings - Fork 983
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
Allow users to mark platforms as "required" for wheel coverage #10067
base: main
Are you sure you want to change the base?
Conversation
Uhh trying Fork on... Add heuristic Gate to local versions Remove KnownMarkers Add required platform support
59b6690
to
eeb249d
Compare
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 like this!
@@ -196,6 +199,15 @@ impl Display for IncompatibleDist { | |||
IncompatibleWheel::RequiresPython(python, _) => { | |||
write!(f, "Python {python}") | |||
} | |||
IncompatibleWheel::MissingPlatform(marker) => { | |||
if let Some(platform) = KnownPlatform::from_marker(*marker) { | |||
write!(f, "no {platform}-compatible wheels") |
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
@@ -273,6 +273,36 @@ package = false | |||
|
|||
--- | |||
|
|||
### [`required-platforms`](#required-platforms) {: #required-platforms } |
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 might help if the docs specifically "compared and contrasted" required-platforms
with environments
. They do different things of course, but they take the same inputs and I could see end users being confused by them.
(What I tend to do in docs is to mention the other in the docs and contrast them. e.g., mention environments
in this section and required-platforms
in the environments
section. That way, users see the contrasting doc regardless of where they look.)
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 doing this would be helpful for naming. (as well as very helpful for users)
Another case that this could solve (but would require user intervention): #10085. Ideally, I think we should be able to give users a hint when the sync fails -- like "There are no available wheels for this platform, add |
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.
The feature is definitely needed and i like the approach, though the find_environments
if i haven't missed something fails to uphold the required platforms in certain conditions.
] | ||
"# | ||
)] | ||
pub required_platforms: Option<SupportedEnvironments>, |
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.
Should this be required_environments
or required_markers
? The other markers list is environments
, platforms
lacks symmetry
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 was somewhat intentional but I can change it.
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.
We might want to change the other one? I feel like environments
is a bit overloaded in the longterm.
|
||
// Find all dependencies on the current package. | ||
let mut marker = MarkerTree::FALSE; | ||
for index in incompatibilities { |
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.
Don't we add additional incompatibilities that target id
when we find a new package foo
with foo --[marker]--> id
? Could that lead to packages missing platform support because we could have a case like the following:
Required platform linux
Choose A 2, with A -> B; windows
, A -> C
Choose B 2. B 2 does have only a mac wheel but that's fine because it's only required on windows, so our required linux is coverd.
Choose C 2, with C -> B
Try to install on linux. We try to install A 2, C 2 and B 2, but B 2 fails since it has no wheel.
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.
@konstin -- I think this would actually work as-is, though for "bad" reasons. Basically, when we visit B
, we won't know that it's "only required for Windows", because we don't do "aggressive forking" today, so we'd still try to solve for Linux. It might get filtered out later? I can't quite remember.
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.
Could you add a test case covering this scenario? So it doesn't fail with future resolver changes.
Since this is specific to wheels should we name this differently? Like, if I encounter a build failure why would it be obvious that setting a |
@zanieb -- That's true, although I'd read that setting as "platforms on which building should be disabled", which isn't quite the same. We're not enforcing that all packages have wheels on these platforms. We're enforcing that all packages have wheels or source distributions on these platforms (and so, building is considered ok). |
I sort of prefer something that doesn't get into the source vs. binary distinction, and instead gets at the intent of the flag: to ensure that the lockfile supports the platforms you care about. |
Oh I guess I'm getting confused by the pull request title and body which say...
This sounds like we require wheels for the platforms? |
We require that there are compatible distributions for the platform. That could be a source distribution, or (for packages that lack source distributions) a wheel. |
That makes sense, it'd help to change the way this is presented in the title / body then. With this context, |
Summary
This PR revives #10017, which might be viable now that we don't enforce any platforms by default.
The basic idea here is that users can mark certain platforms as required (empty, by default). When resolving, we ensure that the specified platforms have wheel coverage, backtracking if not.
For example, to require that we include a version of PyTorch that supports Intel macOS:
Other than that, the forking is identical to past iterations of this PR.
This would give users a way to resolve the tail of issues in #9711, but with manual opt-in to supporting specific platforms.