Skip to content
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

Conversation

vphilippon
Copy link
Member

@vphilippon vphilippon commented Sep 25, 2017

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:

                           ROUND 1
<... Snip, a few rounds, resolving>
                           ROUND 4
<... Snip, everything stable except requests>
New dependencies found in this round:
   adding [u'requests', '<3.0,>=2.7', "['security']"]
Removed dependencies in this round:
   removing [u'requests', '<3.0,>=2.7', '[]']
------------------------------------------------------------
Result of round 4: not stable

                           ROUND 5
<... Snip, no dependency change, but falsely detecting a requirements of requests without the 'security' extra, even if giving a specification to explicitly **not** use an extra is impossible.>
New dependencies found in this round:
   adding [u'requests', '<3.0,>=2.7', "[]"]
Removed dependencies in this round:
   removing [u'requests', '<3.0,>=2.7', '['security']']
------------------------------------------------------------
Result of round 5: not stable

                           ROUND 6
<... Snip, no dependency change, but the 'security' extra gets added again.>
New dependencies found in this round:
   adding [u'requests', '<3.0,>=2.7', "['security']"]
Removed dependencies in this round:
   removing [u'requests', '<3.0,>=2.7', '[]']
------------------------------------------------------------
Result of round 6: not stable

<... Snip, bad luck, this keeps going up to round 10>
------------------------------------------------------------
Result of round 10: not stable
Traceback (most recent call last):
< ... Snip, trace>
RuntimeError: No stable configuration of concrete packages could be found for the given constraints after 10 rounds of resolving.
This is likely a bug.

Explanation:
The comparison of RequirementSummary would compare Requirement.extras instead of InstallRequirement.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 first InstallRequirement 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 of set and the key 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
  • Provided the tests for the changes
  • Added the changes to CHANGELOG.md
  • Requested (or received) a review from another contributor

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.
Copy link
Contributor

@davidovich davidovich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@vphilippon vphilippon merged commit b3c6a11 into jazzband:master Sep 26, 2017
@vphilippon vphilippon added this to the 1.10 milestone Sep 26, 2017
@vphilippon vphilippon deleted the fix-extras-compare-in-resolver branch September 27, 2017 19:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants