-
-
Notifications
You must be signed in to change notification settings - Fork 611
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
Fix Resolver unstability with extras: Use InstallRequirement.extras for the diff in the Resolver. #566
Merged
vphilippon
merged 1 commit into
jazzband:master
from
vphilippon:fix-extras-compare-in-resolver
Sep 26, 2017
Merged
Fix Resolver unstability with extras: Use InstallRequirement.extras for the diff in the Resolver. #566
vphilippon
merged 1 commit into
jazzband:master
from
vphilippon:fix-extras-compare-in-resolver
Sep 26, 2017
Conversation
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
The extras are combined in the InstallRequirement.extras, not in the Requirement.extras. Comparing the Requirement.extras could cause flakyness in the Resolver stabilisation, as the list of extras would vary based on the iteration order on the set of InstallRequirement.
vphilippon
force-pushed
the
fix-extras-compare-in-resolver
branch
from
September 25, 2017 16:22
892c559
to
08aed9d
Compare
davidovich
approved these changes
Sep 26, 2017
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.
LGTM
This was referenced Sep 27, 2017
This was referenced Sep 27, 2017
This was referenced Sep 27, 2017
This was referenced Nov 19, 2017
This was referenced Nov 30, 2017
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
The current diff of
RequirementSummary
would sometime (randomly) fail to stabilise, falsely detecting a requirement change on requirements constraints specifying extras (ex:requests[security]
)This would give this kind of output with
pip-compile -v
:Explanation:
The comparison of
RequirementSummary
would compareRequirement.extras
instead ofInstallRequirement.extras
, which are the one we actually combine in_group_constraints
.The reason for the "randomness" of the failure is a bit hard to explain clearly. It's that only the
Requirement.extras
from the firstInstallRequirement
of the combining iteration would be used for the diff, and which one is the first is random for each call (due to the usage ofset
and thekey
function not taking extras into account for the sort). This means that when computing the pre and post state of the round, one of them could combine and discard the extras, and the other one keep the extras, resulting in a fake difference, triggering another round. With enough bad luck, it could reach the 10 rounds cap.I have not good way of making a unit test for this. But I did a lot of testing and can confirm that the "fake change" is gone, and the resolving works fine.
Contributor checklist