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

Carry all incompatibilities during backtracking #60

Closed
wants to merge 1 commit into from

Conversation

uranusjr
Copy link
Member

@uranusjr uranusjr commented Nov 4, 2020

Consider this situation:

  • Candidate Ax depends on B
  • Candidate B1 depends on C==1 D==1 E==1
  • Candidate B2 depends on C==2 D==1 E==1
  • Candidate C1 depends on D==1
  • Candidate C2 depends on D==1
  • Candidate D1 depends on E!=1

In the previous implementation, the conflict on E is discovered after we resolved Ax-B1-C1-D1. D1 is marked as an incompatibility, we backtrack the C1 pin. But now we don’t have an available C, and need to also backtrack the B1 pin. At this point, however, the previous implementation would fail to “remember” that D1 is also marked as incompatible, and proceed to try B2. That would eventually fail, we backtrack to the same point, and end up trying B1 again. Now we are stuck.

This fix uses a list to remember all the candidates marked as incompatible along the whole backtrack process, and “re-mark” them in parent states. This makes the resolver aware, when it backtracks B1, that B2 is also not viable, and avoid hitting it.

@uranusjr uranusjr force-pushed the backtrack–carry-all-incompatibilities branch from 54464a4 to a5ad9c1 Compare November 4, 2020 12:07
@uranusjr uranusjr force-pushed the backtrack–carry-all-incompatibilities branch from a5ad9c1 to 44dd815 Compare November 4, 2020 12:28
Consider this situation:

* Candidate Ax depends on B
* Candidate B1 depends on C==1, D==1, E==1
* Candidate B2 depends on C==2, D==1, E==1
* Candidate C1 depends on D==1
* Candidate C2 depends on D==1
* Candidate D1 depends on E!=1

In the previous implementation, the conflict on E is discovered after we
resolved Ax-B1-C1-D1. D1 is marked as an incompatibility, we backtrack
the C1 pin. But now we don't have an available C, and need to also
backtrack the B1 pin. At this point, however, the previous implemen-
tation would fail to "remember" that D1 is also marked as incompatible,
and proceed to try B2. That would eventually fail, we backtrack to the
same point, are stuck trying B1 and B2 repeatedly.

This fix uses a list to remember all the candidates marked as
incompatible along the whole backtrack process, and "re-mark" them in
parent states. This makes the resolver aware, when it backtracks B1,
that B2 is also not viable, and avoid hitting it.
@uranusjr uranusjr force-pushed the backtrack–carry-all-incompatibilities branch from 44dd815 to 1521ccb Compare November 4, 2020 12:37
@webknjaz
Copy link
Contributor

webknjaz commented Nov 4, 2020

The solution sounds legit. Do you have a regression test case for this?

@uranusjr
Copy link
Member Author

uranusjr commented Nov 4, 2020

I’m still trying to come up with a good, real world, test case. Both cases available in pypa/pip#9011 are quite big and I want to trim them down to a managable input that contains only PyPI wheels.

@pradyunsg
Copy link
Contributor

pradyunsg commented Nov 4, 2020

TBH, a synthetic test case (as described in OP) should also be sufficient -- we'd want to check calls to make sure we're not repeating the same things multiple times + add a timeout.

@uranusjr
Copy link
Member Author

uranusjr commented Nov 5, 2020

I tried to create a test case from the abstract example I described above, and… it passed on master. I think my diagnosis is on the right trck, and the fix could actually be correct (it does fix the real-world case?), but it’s very difficult to find the right input that hits the exact code path to trigger the issue. Maybe we need more candidates on a package? More packages hitting the conflict? A conflict deeper in the graph? All or some combination of the above? I don’t know. 😟

@brainwane
Copy link

Per a meeting today, as developers created a substantive and working test case, they found significant problems with their original approach, and so this issue needs more investigation and discussion to figure this out. This is a blocker (as is pypa/pip#9011) to the pip 20.3 release.

I believe @uranusjr is poking at this a bit in his volunteer time -- is that correct? Tzu-ping?

@pradyunsg plans to finish the other remaining things on his 20.3 pypa/pip#8936 checklist first, so he can make use of TP's investigations, finish and merge a fix, then immediately cut the 20.3 release.

@uranusjr
Copy link
Member Author

Yes, I’ve successfully reproduced the issue in #61 (which this PR would not fix) and plan to continue investigation and work on a fix there.

@uranusjr
Copy link
Member Author

Closing in favour of #61.

@uranusjr uranusjr closed this Nov 20, 2020
@uranusjr uranusjr deleted the backtrack–carry-all-incompatibilities branch November 28, 2020 07:49
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.

4 participants