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 reject of valid state in criteria compatibility check #111

Merged
Merged
Show file tree
Hide file tree
Changes from 21 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions news/91.bugfix.rst
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
Some valid states that were previously rejected are now accepted. This affects
states where multiple candidates for the same dependency conflict with each
other. The ``information`` argument passed to
``AbstractProvider.get_preference`` may now contain empty iterators. This has
always been allowed by the method definition but it was previously not possible
in practice.
41 changes: 41 additions & 0 deletions src/resolvelib/resolvers.py
Original file line number Diff line number Diff line change
Expand Up @@ -173,6 +173,31 @@ def _add_to_criteria(self, criteria, requirement, parent):
raise RequirementsConflicted(criterion)
criteria[identifier] = criterion

def _remove_information_from_criteria(self, criteria, parents):
"""Remove information from parents of criteria.

Concretely, removes all values from each criterion's ``information``
field that have one of ``parents`` as provider of the requirement.

:param criteria: The criteria to update.
:param parents: Identifiers for which to remove information from all criteria.
"""
if not parents:
return
for key, criterion in criteria.items():
criteria[key] = Criterion(
criterion.candidates,
[
information
for information in criterion.information
if (
information[1] is None
or self._p.identify(information[1]) not in parents
)
],
criterion.incompatibilities,
)

def _get_preference(self, name):
return self._p.get_preference(
identifier=name,
Expand Down Expand Up @@ -367,6 +392,11 @@ def resolve(self, requirements, max_rounds):
self._r.ending(state=self.state)
return self.state

# keep track of satisfied names to calculate diff after pinning
satisfied_names = set(self.state.criteria.keys()) - set(
unsatisfied_names
)

# Choose the most preferred unpinned criterion to try.
name = min(unsatisfied_names, key=self._get_preference)
failure_causes = self._attempt_to_pin_criterion(name)
Expand All @@ -383,6 +413,17 @@ def resolve(self, requirements, max_rounds):
if not success:
raise ResolutionImpossible(self.state.backtrack_causes)
else:
# discard as information sources any invalidated names
# (unsatisfied names that were previously satisfied)
newly_unsatisfied_names = {
key
for key, criterion in self.state.criteria.items()
if key in satisfied_names
and not self._is_current_pin_satisfying(key, criterion)
}
self._remove_information_from_criteria(
self.state.criteria, newly_unsatisfied_names
)
# Pinning was successful. Push a new state to do another pin.
self._push_new_state()

Expand Down
124 changes: 124 additions & 0 deletions tests/test_resolvers.py
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 packaging.requirements import Requirement

from resolvelib import (
AbstractProvider,
Expand All @@ -7,6 +21,12 @@
ResolutionImpossible,
Resolver,
)
from resolvelib.resolvers import Resolution # type: ignore
Copy link
Member

Choose a reason for hiding this comment

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

What errors does this ignore?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Resolution is not part of the public interface declared in src/resolvelib/resolvers.pyi. My reasoning was that for testing that does not matter. I meant to ask about this but I forgot about it. The error is as follows:

tests/test_resolvers.py:24: error: Module "resolvelib.resolvers" has no attribute "Resolution"; maybe "ResolutionError"?
Found 1 error in 1 file (checked 1 source file)

Copy link
Member

Choose a reason for hiding this comment

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

We can add Resolution to the type stubs; I think someone would need it at some point anyway.

Copy link
Contributor Author

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, thanks.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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():
Expand Down Expand Up @@ -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(requirement_or_candidate).name
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 candidate[1] in Requirement(requirement).specifier

# 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")