-
Notifications
You must be signed in to change notification settings - Fork 34
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 reject of valid state in criteria compatibility check #111
Changes from 16 commits
8931c6d
0d8faa9
568fc6d
525b8c2
47f1e6c
7586575
dabe77d
1809154
8232a17
469e1e5
41ceb33
8e4d2c4
a25f3bc
85c9ce9
65fa410
70c9567
5e85621
7e841f8
cc1c220
ad9eaca
20df2c8
6e9cf49
546a11f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -42,6 +42,7 @@ lint = | |
mypy | ||
isort | ||
types-requests | ||
types-setuptools | ||
test = | ||
commentjson | ||
packaging | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,18 @@ | ||
from typing import ( | ||
Any, | ||
Iterable, | ||
Iterator, | ||
List, | ||
Mapping, | ||
Sequence, | ||
Set, | ||
Tuple, | ||
Union, | ||
) | ||
|
||
import pytest | ||
from packaging.version import Version | ||
from pkg_resources import Requirement | ||
uranusjr marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
from resolvelib import ( | ||
AbstractProvider, | ||
|
@@ -7,6 +21,12 @@ | |
ResolutionImpossible, | ||
Resolver, | ||
) | ||
from resolvelib.resolvers import Resolution # type: ignore | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What errors does this ignore? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can add There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. All right, I'll do that, thanks. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. I did have to add another more specific type ignore because the test case monkeypatches a private method. I think that should be acceptable? |
||
from resolvelib.resolvers import ( | ||
Criterion, | ||
RequirementInformation, | ||
RequirementsConflicted, | ||
) | ||
|
||
|
||
def test_candidate_inconsistent_error(): | ||
|
@@ -143,3 +163,107 @@ def run_resolver(*args): | |
backtracking_causes = run_resolver([("a", {1, 2}), ("b", {1})]) | ||
exception_causes = run_resolver([("a", {2}), ("b", {1})]) | ||
assert exception_causes == backtracking_causes | ||
|
||
|
||
def test_pin_conflict_with_self(monkeypatch, reporter): | ||
# type: (Any, BaseReporter) -> None | ||
""" | ||
Verify correct behavior of attempting to pin a candidate version that conflicts | ||
with a previously pinned (now invalidated) version for that same candidate (#91). | ||
""" | ||
Candidate = Tuple[ | ||
str, Version, Sequence[str] | ||
] # name, version, requirements | ||
all_candidates = { | ||
"parent": [("parent", Version("1"), ["child<2"])], | ||
"child": [ | ||
("child", Version("2"), ["grandchild>=2"]), | ||
("child", Version("1"), ["grandchild<2"]), | ||
("child", Version("0.1"), ["grandchild"]), | ||
], | ||
"grandchild": [ | ||
("grandchild", Version("2"), []), | ||
("grandchild", Version("1"), []), | ||
], | ||
} # type: Mapping[str, Sequence[Candidate]] | ||
|
||
class Provider(AbstractProvider): # AbstractProvider[int, Candidate, str] | ||
def identify(self, requirement_or_candidate): | ||
# type: (Union[str, Candidate]) -> str | ||
result = ( | ||
Requirement.parse(requirement_or_candidate).key | ||
if isinstance(requirement_or_candidate, str) | ||
else requirement_or_candidate[0] | ||
) | ||
assert result in all_candidates, "unknown requirement_or_candidate" | ||
return result | ||
|
||
def get_preference(self, identifier, *args, **kwargs): | ||
# type: (str, *object, **object) -> str | ||
# prefer child over parent (alphabetically) | ||
return identifier | ||
|
||
def get_dependencies(self, candidate): | ||
# type: (Candidate) -> Sequence[str] | ||
return candidate[2] | ||
|
||
def find_matches( | ||
self, | ||
identifier, # type: str | ||
requirements, # type: Mapping[str, Iterator[str]] | ||
incompatibilities, # type: Mapping[str, Iterator[Candidate]] | ||
): | ||
# type: (...) -> Iterator[Candidate] | ||
return ( | ||
candidate | ||
for candidate in all_candidates[identifier] | ||
if all( | ||
self.is_satisfied_by(req, candidate) | ||
for req in requirements[identifier] | ||
) | ||
if candidate not in incompatibilities[identifier] | ||
) | ||
|
||
def is_satisfied_by(self, requirement, candidate): | ||
# type: (str, Candidate) -> bool | ||
return str(candidate[1]) in Requirement.parse(requirement) | ||
|
||
# patch Resolution._get_updated_criteria to collect rejected states | ||
rejected_criteria = [] # type: List[Criterion] | ||
get_updated_criteria_orig = Resolution._get_updated_criteria | ||
|
||
def get_updated_criteria_patch(self, candidate): | ||
try: | ||
return get_updated_criteria_orig(self, candidate) | ||
except RequirementsConflicted as e: | ||
rejected_criteria.append(e.criterion) | ||
raise | ||
|
||
monkeypatch.setattr( | ||
Resolution, "_get_updated_criteria", get_updated_criteria_patch | ||
) | ||
|
||
resolver = Resolver( | ||
Provider(), reporter | ||
) # type: Resolver[str, Candidate, str] | ||
result = resolver.resolve(["child", "parent"]) | ||
|
||
def get_child_versions(information): | ||
# type: (Iterable[RequirementInformation[str, Candidate]]) -> Set[str] | ||
return { | ||
str(inf.parent[1]) | ||
for inf in information | ||
if inf.parent is not None and inf.parent[0] == "child" | ||
} | ||
|
||
# verify that none of the rejected criteria are based on more than one candidate for | ||
# child | ||
assert not any( | ||
len(get_child_versions(criterion.information)) > 1 | ||
for criterion in rejected_criteria | ||
) | ||
|
||
assert set(result.mapping) == {"parent", "child", "grandchild"} | ||
assert result.mapping["parent"][1] == Version("1") | ||
assert result.mapping["child"][1] == Version("1") | ||
assert result.mapping["grandchild"][1] == Version("1") |
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.
Empty information is technically allowed but likely unexpected, since it records where a criterion came from, and if a criterion comes from nowhere, should it not be removed?
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.
My reasoning was that we're dropping the constraint provided by a parent because we're dropping the pin on that parent. But if the new pin on the parent happens to have the same constraint (the common case), we might want to keep
incompatibilities
, in order to prevent backtracking over the same thing again.But as I type this I realize that I'm really not yet confident in my understanding of how incompatibilities are gathered. So if you are confident that we can safely drop the criterion when no information remains I'll apply that change. Otherwise I'll have to sink my teeth in the incompatibilities some more.
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.
If the intent is to keep
incompatibilities
in the graph, I think it’s OK to keep a criterion with empty information (it’s basically an orphan node). We should describe this in the release note (by adding a release note fragment file innews
) so provider/reporter implementers are aware of the possibility thatinformation
could be empty (and what that means), it’s likely no-one would be particular troubled by this since an orphan node can simply be ignored in the final graph anyway.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.
All right, I'll do that. I think
pip
will need a minor change, but there seems to already be a breaking change in master compared to the last release anyway so I guess that is acceptable.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 think the orphan nodes will be excluded automatically by
build_result()
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.
Indeed they will for the final result, but not for intermediate calls to the provider, see here: #91 (comment).
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've made an attempt at the news fragment but I'm not entirely sure about your change entry conventions.