Prefer direct conflicting causes when backtracking #12499
Conversation
notatallshaw
force-pushed
the
prefer-conflicting-causes
branch
2 times, most recently
from
b9ab7f5
to
e2e3f24
Compare
|
I have updated this PR based on the feedback in #12497 and updates in sarugaku/resolvelib#145 and In summary I added more detailed comments and docstrings, improved the naming, simplified some of the logic to make it more readable, and removed the commit related to moving over existing functionality to this potentially new API method from resolvelib (I will make a seperate PR for that) and now this PR just focuses on the issue described in #12498 |
notatallshaw
force-pushed
the
prefer-conflicting-causes
branch
from
e2e3f24
to
9aff0d0
Compare
There was a problem hiding this comment.
I didn't review the resolvelib changes, as I assume these are only temporary to get this patch working for now. They should be removed, and an updated version of resolvelib vendored in, before this PR is merged.
Also, can we have some tests for this code? It's complex enough that I don't love the idea of leaving it untested. Unit tests should be sufficient. (And reasonably-commented unit tests should help give a sense of what the code is doing).
|
|
||
| :params causes: An iterable of PreferenceInformation | ||
|
|
||
| Returns a set of strings, each representing the name of a requirement or |
There was a problem hiding this comment.
| Returns a set of strings, each representing the name of a requirement or | |
| Returns a set of strings, representing the name of a requirement and |
The code adds both the name and the parent's name if there's a parent. (There may be a better way of re-wording this, I don't think Github's "suggestion" mechanism allows for multi-line changes...)
| ) -> List["PreferenceInformation"]: | ||
| """ | ||
| Identifies causes that conflict because their parent package requirements | ||
| are not satisfied by another cause, or vice versa. |
There was a problem hiding this comment.
I'm still struggling to understand the longic here. I'd suggest 2 improvements.
- Clarify the above sentence - I don't really understand what "their parent package requirements are not satisfied by another cause" means, or why the parent package requirements should be satisfied by another cause.
- Describe the data structure of "causes" here, explaining the elements that we're using. Something like "each cause contains a requirement specifier, and a "parent", which is the candidate which has that requirement. The list of causes only contains elements that have been detected by the resolver to be in conflict somehow." (I'm assuming that explanation is at least in part wrong, as it's not enough for me to be able to make sense of the function code...)
|
|
||
| # If there are 2 or less causes then finding conflicts between | ||
| # them is not required as there will always be a minumum of two | ||
| # conflicts |
There was a problem hiding this comment.
I'd reword this as "unless there are more than 2 conflicts, there's nothing to simplify (because a conflict always involves at least 2 causes)".
| if len(backtrack_causes) < 3: | ||
| return identifiers | ||
|
|
||
| # First, try to resolve direct causes based on conflicting parent packages |
There was a problem hiding this comment.
| # First, try to resolve direct causes based on conflicting parent packages | |
| # If some of the causes have conflicting parents, focus on them. |
| # First, try to resolve direct causes based on conflicting parent packages | ||
| direct_causes = _causes_with_conflicting_parent(backtrack_causes) | ||
| if not direct_causes: | ||
| # If no conflicting parent packages found try to find some causes |
There was a problem hiding this comment.
| # If no conflicting parent packages found try to find some causes | |
| # Otherwise, try to find some causes |
| direct_causes = _causes_with_conflicting_parent(backtrack_causes) | ||
| if not direct_causes: | ||
| # If no conflicting parent packages found try to find some causes | ||
| # that share the same requirement name but no common candidate, |
There was a problem hiding this comment.
| # that share the same requirement name but no common candidate, | |
| # that share the same requirement name but no common candidate. |
| if not direct_causes: | ||
| # If no conflicting parent packages found try to find some causes | ||
| # that share the same requirement name but no common candidate, | ||
| # we take the first one of these as iterating through candidates |
There was a problem hiding this comment.
| # we take the first one of these as iterating through candidates | |
| # Stop after finding the first one of these as iterating through candidates |
| direct_causes = _first_causes_with_no_candidates( | ||
| backtrack_causes, candidates | ||
| ) | ||
| if direct_causes: |
There was a problem hiding this comment.
| if direct_causes: | |
| # If we found some causes worth focusing on, use them. | |
| if direct_causes: |
|
I am still working on this but I am marking it as a draft until I have addressed all of @pfmoore's comments and until resolvelib decides whether to accept sarugaku/resolvelib#145 and make a release |
Fixes: #12498
This PR introduces enhancements to Pip's dependency resolution logic. Below is a description of the three main functions implemented in this PR:
_causes_with_conflicting_parent_first_causes_with_no_candidatesnarrow_requirement_selectionget_preferenceswould lead to an adverse performance impact, introducing expensive new calls and potentially creating O(n^2) situations.