-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
[PEP 771] #13170
Draft
DEKHTIARJonathan
wants to merge
4
commits into
pypa:main
Choose a base branch
from
wheelnext:pep_771
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Draft
[PEP 771] #13170
Changes from all commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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, if PEP 771 is accepted, be aware you will not be able to submit this specific patch to resolvelib. Resolvelib is very generic and has no concept of extras, you will to solve this in someway in
src/pip/_internal/resolvelib
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.
@notatallshaw who is the best person to have a chat in the future about this PR ? Getting some directions ?
This PEP is a lot more difficult to implement than I anticipated and I'm not entirely sure I took the best approach.
I used the debugger to track up and down how
extras
are being managed, though it's far from being trivial and my overall confidence in this PR is not very high.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 not sure, as far as I'm aware all active pip maintainers volunteer personal time to do their maintenance duties and therefore it's extremely limited.
I see pip maintainers @pfmoore, @pradyunsg, and myself have all contributed or are actively involved in the PEP 771 discussion.
For my part I will review anything that touches the resolution code, but I can't currently help with larger architectural / mentoring tasks sorry.
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.
Fantastic thanks for your comment ;)
For now we don't need to worry about. Once a decision is taken on the PEP, I'll see what needs to be done.
And if I have specific questions I can always ask (and hope for the best ahaha)
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 happy to help out - I haven't yet taken a look at this PR mainly as it's in draft and I've had some RL issues to deal with.
@ichard26 is correct about vendoring, we'll need to get the patches in the source project(s) and wait for a release before we pick them up. We can carry the patches in the PR until that happens, though, so work here won't be blocked.
As for resolvelib, @notatallshaw is correct. There's code in the resolvelib "examples" directory that shows how to handle extras in resolvelib, if you want to see a simple example before diving into the full glory of pip's implementation.
The way pip handles this is that when you see
foo[xtra]
, you basically treat that as a completely separate "candidate", and inject an additional dependency that says thatfoo[xtra]
depends on the exact same version offoo
. The reason you need to do it like this is that candidates must have fixed dependencies in resolvelib, sofoo[xtra]
andfoo
must be different because they don't have the same dependencies. You're going to have to change this model a bit, as with default extras,foo
could include the default extra or not, depending on whetherfoo[actual_extra]
is requested as well. So when you see "foo", you don't actually know what candidate you need to tell resolvelib about...For a tricky example to think about, consider the following.
A depends on B. B depends on C. C depends on A[xtra]. Now, if you ask to install A, will the default extra of A get installed? I assume not, as down in the dependency tree, A[xtra] is requested, which "switches off" the default extra. But you can't know this when you process the initial requirement ("A") to pass it to resolvelib - so what dependencies do you say A has? Remember, you can't change your decision later...
Actually, this makes me wonder about the proposal itself. If A has a default extra of X, and X specifies a dependency of B, and B depends on A[Y], then if I install A, will X be installed? Because if it is, A[Y] will be installed, and the fact that you installed extra Y means that X (the default extra) shouldn't be installed. I don't know what the answer is here, but the PEP needs to specify, as you can't leave it to individual tools to pick something.