From ddbc8fd7c15dc642854cccf099f5632119fc2055 Mon Sep 17 00:00:00 2001 From: Paul Moore Date: Wed, 13 May 2020 17:52:09 +0100 Subject: [PATCH 1/4] Split find_matches into generation and sorting --- .../resolution/resolvelib/candidates.py | 8 ++ .../resolution/resolvelib/factory.py | 79 +++++++----------- .../resolution/resolvelib/provider.py | 81 ++++++++++++++++++- .../resolution/resolvelib/resolver.py | 19 +++-- tests/unit/resolution_resolvelib/conftest.py | 3 +- 5 files changed, 127 insertions(+), 63 deletions(-) diff --git a/src/pip/_internal/resolution/resolvelib/candidates.py b/src/pip/_internal/resolution/resolvelib/candidates.py index c4772c33ff8..794d14026ce 100644 --- a/src/pip/_internal/resolution/resolvelib/candidates.py +++ b/src/pip/_internal/resolution/resolvelib/candidates.py @@ -101,6 +101,14 @@ def make_install_req_from_dist(dist, parent): return ireq +def is_already_installed(cand): + # type: (Candidate) -> bool + # For an ExtrasCandidate, we check the base + if isinstance(cand, ExtrasCandidate): + cand = cand.base + return isinstance(cand, AlreadyInstalledCandidate) + + class _InstallRequirementBackedCandidate(Candidate): def __init__( self, diff --git a/src/pip/_internal/resolution/resolvelib/factory.py b/src/pip/_internal/resolution/resolvelib/factory.py index b3cbb22f17b..79343da32bd 100644 --- a/src/pip/_internal/resolution/resolvelib/factory.py +++ b/src/pip/_internal/resolution/resolvelib/factory.py @@ -42,8 +42,6 @@ class Factory(object): - _allowed_strategies = {"eager", "only-if-needed", "to-satisfy-only"} - def __init__( self, finder, # type: PackageFinder @@ -52,11 +50,9 @@ def __init__( force_reinstall, # type: bool ignore_installed, # type: bool ignore_requires_python, # type: bool - upgrade_strategy, # type: str py_version_info=None, # type: Optional[Tuple[int, ...]] ): # type: (...) -> None - assert upgrade_strategy in self._allowed_strategies self.finder = finder self.preparer = preparer @@ -64,9 +60,6 @@ def __init__( self._make_install_req_from_spec = make_install_req self._force_reinstall = force_reinstall self._ignore_requires_python = ignore_requires_python - self._upgrade_strategy = upgrade_strategy - - self.root_reqs = set() # type: Set[str] self._link_candidate_cache = {} # type: Cache[LinkCandidate] self._editable_candidate_cache = {} # type: Cache[EditableCandidate] @@ -118,23 +111,27 @@ def _make_candidate_from_link( return ExtrasCandidate(base, extras) return base - def _eligible_for_upgrade(self, dist_name): - # type: (str) -> bool - if self._upgrade_strategy == "eager": - return True - elif self._upgrade_strategy == "only-if-needed": - return (dist_name in self.root_reqs) - return False - def iter_found_candidates(self, ireq, extras): # type: (InstallRequirement, Set[str]) -> Iterator[Candidate] name = canonicalize_name(ireq.req.name) - if not self._force_reinstall: - installed_dist = self._installed_dists.get(name) - can_upgrade = self._eligible_for_upgrade(name) - else: - installed_dist = None - can_upgrade = False + seen_versions = set() + + # Yield the installed version, if it matches, unless the user + # specified `--force-reinstall`, when we want the version from + # the index instead. + if not self._force_reinstall and name in self._installed_dists: + installed_dist = self._installed_dists[name] + installed_version = installed_dist.parsed_version + if ireq.req.specifier.contains( + installed_version, + prereleases=True + ): + seen_versions.add(installed_version) + yield self._make_candidate_from_dist( + dist=installed_dist, + extras=extras, + parent=ireq, + ) found = self.finder.find_best_candidate( project_name=ireq.req.name, @@ -142,40 +139,18 @@ def iter_found_candidates(self, ireq, extras): hashes=ireq.hashes(trust_internet=False), ) for ican in found.iter_applicable(): - if (installed_dist is not None and - installed_dist.parsed_version == ican.version): - if can_upgrade: - yield self._make_candidate_from_dist( - dist=installed_dist, - extras=extras, - parent=ireq, - ) - continue - yield self._make_candidate_from_link( - link=ican.link, - extras=extras, - parent=ireq, - name=name, - version=ican.version, - ) - - # Return installed distribution if it matches the specifier. This is - # done last so the resolver will prefer it over downloading links. - if can_upgrade or installed_dist is None: - return - installed_version = installed_dist.parsed_version - if ireq.req.specifier.contains(installed_version, prereleases=True): - yield self._make_candidate_from_dist( - dist=installed_dist, - extras=extras, - parent=ireq, - ) + if ican.version not in seen_versions: + seen_versions.add(ican.version) + yield self._make_candidate_from_link( + link=ican.link, + extras=extras, + parent=ireq, + name=name, + version=ican.version, + ) def make_requirement_from_install_req(self, ireq): # type: (InstallRequirement) -> Requirement - if ireq.is_direct and ireq.name: - self.root_reqs.add(canonicalize_name(ireq.name)) - if ireq.link: # TODO: Get name and version from ireq, if possible? # Specifically, this might be needed in "name @ URL" diff --git a/src/pip/_internal/resolution/resolvelib/provider.py b/src/pip/_internal/resolution/resolvelib/provider.py index 226dc3687d3..2dbcc873573 100644 --- a/src/pip/_internal/resolution/resolvelib/provider.py +++ b/src/pip/_internal/resolution/resolvelib/provider.py @@ -3,14 +3,35 @@ from pip._internal.utils.typing import MYPY_CHECK_RUNNING +from .candidates import is_already_installed + if MYPY_CHECK_RUNNING: - from typing import Any, Dict, Optional, Sequence, Tuple, Union + from typing import Any, Dict, Optional, Sequence, Set, Tuple, Union from pip._internal.req.req_install import InstallRequirement + from pip._vendor.packaging.version import _BaseVersion from .base import Requirement, Candidate from .factory import Factory +# Notes on the relationship between the provider, the factory, and the +# candidate and requirement classes. +# +# The provider is a direct implementation of the resolvelib class. Its role +# is to deliver the API that resolvelib expects. +# +# Rather than work with completely abstract "requirement" and "candidate" +# concepts as resolvelib does, pip has concrete classes implementing these two +# ideas. The API of Requirement and Candidate objects are defined in the base +# classes, but essentially map fairly directly to the equivalent provider +# methods. In particular, `find_matches` and `is_satisfied_by` are +# requirement methods, and `get_dependencies` is a candidate method. +# +# The factory is the interface to pip's internal mechanisms. It is stateless, +# and is created by the resolver and held as a property of the provider. It is +# responsible for creating Requirement and Candidate objects, and provides +# services to those objects (access to pip's finder and preparer). + class PipProvider(AbstractProvider): def __init__( @@ -18,11 +39,66 @@ def __init__( factory, # type: Factory constraints, # type: Dict[str, SpecifierSet] ignore_dependencies, # type: bool + upgrade_strategy, # type: str + roots, # type: Set[str] ): # type: (...) -> None self._factory = factory self._constraints = constraints self._ignore_dependencies = ignore_dependencies + self._upgrade_strategy = upgrade_strategy + self.roots = roots + + def sort_matches(self, matches): + # type: (Sequence[Candidate]) -> Sequence[Candidate] + + # The requirement is responsible for returning a sequence of potential + # candidates, one per version. The provider handles the logic of + # deciding the order in which these candidates should be passed to + # the resolver. + + # The `matches` argument is a sequence of candidates, one per version, + # which are potential options to be installed. The requirement will + # have already sorted out whether to give us an already-installed + # candidate or a version from PyPI (i.e., it will deal with options + # like --force-reinstall and --ignore-installed). + + # We now work out the correct order. + # + # 1. If no other considerations apply, later versions take priority. + # 2. An already installed distribution is preferred over any other, + # unless the user has requested an upgrade. + # Upgrades are allowed when: + # * The --upgrade flag is set, and + # - The project was specified on the command line, or + # - The project is a dependency and the "eager" upgrade strategy + # was requested. + + def _eligible_for_upgrade(name): + # type: (str) -> bool + if self._upgrade_strategy == "eager": + return True + elif self._upgrade_strategy == "only-if-needed": + print(name, self.roots) + return (name in self.roots) + return False + + def keep_installed(c): + # type: (Candidate) -> int + """Give priority to an installed version?""" + if not is_already_installed(c): + return 0 + + if _eligible_for_upgrade(c.name): + return 0 + + return 1 + + def key(c): + # type: (Candidate) -> Tuple[int, _BaseVersion] + return (keep_installed(c), c.version) + + return sorted(matches, key=key) def get_install_requirement(self, c): # type: (Candidate) -> Optional[InstallRequirement] @@ -45,7 +121,8 @@ def get_preference( def find_matches(self, requirement): # type: (Requirement) -> Sequence[Candidate] constraint = self._constraints.get(requirement.name, SpecifierSet()) - return requirement.find_matches(constraint) + matches = requirement.find_matches(constraint) + return self.sort_matches(matches) def is_satisfied_by(self, requirement, candidate): # type: (Requirement, Candidate) -> bool diff --git a/src/pip/_internal/resolution/resolvelib/resolver.py b/src/pip/_internal/resolution/resolvelib/resolver.py index 5c94d3dc057..b363f303232 100644 --- a/src/pip/_internal/resolution/resolvelib/resolver.py +++ b/src/pip/_internal/resolution/resolvelib/resolver.py @@ -32,6 +32,8 @@ class Resolver(BaseResolver): + _allowed_strategies = {"eager", "only-if-needed", "to-satisfy-only"} + def __init__( self, preparer, # type: RequirementPreparer @@ -47,6 +49,9 @@ def __init__( py_version_info=None, # type: Optional[Tuple[int, ...]] ): super(Resolver, self).__init__() + + assert upgrade_strategy in self._allowed_strategies + self.factory = Factory( finder=finder, preparer=preparer, @@ -54,23 +59,17 @@ def __init__( force_reinstall=force_reinstall, ignore_installed=ignore_installed, ignore_requires_python=ignore_requires_python, - upgrade_strategy=upgrade_strategy, py_version_info=py_version_info, ) self.ignore_dependencies = ignore_dependencies + self.upgrade_strategy = upgrade_strategy self._result = None # type: Optional[Result] def resolve(self, root_reqs, check_supported_wheels): # type: (List[InstallRequirement], bool) -> RequirementSet - # The factory should not have retained state from any previous usage. - # In theory this could only happen if self was reused to do a second - # resolve, which isn't something we do at the moment. We assert here - # in order to catch the issue if that ever changes. - # The persistent state that we care about is `root_reqs`. - assert len(self.factory.root_reqs) == 0, "Factory is being re-used" - constraints = {} # type: Dict[str, SpecifierSet] + roots = set() requirements = [] for req in root_reqs: if req.constraint: @@ -82,6 +81,8 @@ def resolve(self, root_reqs, check_supported_wheels): else: constraints[name] = req.specifier else: + if req.is_direct and req.name: + roots.add(canonicalize_name(req.name)) requirements.append( self.factory.make_requirement_from_install_req(req) ) @@ -90,6 +91,8 @@ def resolve(self, root_reqs, check_supported_wheels): factory=self.factory, constraints=constraints, ignore_dependencies=self.ignore_dependencies, + upgrade_strategy=self.upgrade_strategy, + roots=roots, ) reporter = BaseReporter() resolver = RLResolver(provider, reporter) diff --git a/tests/unit/resolution_resolvelib/conftest.py b/tests/unit/resolution_resolvelib/conftest.py index 41c479bf350..3327d9cc009 100644 --- a/tests/unit/resolution_resolvelib/conftest.py +++ b/tests/unit/resolution_resolvelib/conftest.py @@ -55,7 +55,6 @@ def factory(finder, preparer): force_reinstall=False, ignore_installed=False, ignore_requires_python=False, - upgrade_strategy="to-satisfy-only", py_version_info=None, ) @@ -66,4 +65,6 @@ def provider(factory): factory=factory, constraints={}, ignore_dependencies=False, + upgrade_strategy="to-satisfy-only", + roots=set(), ) From 0db022fc425112cc9d6f334262e218df0801bd2d Mon Sep 17 00:00:00 2001 From: Paul Moore Date: Wed, 13 May 2020 18:04:20 +0100 Subject: [PATCH 2/4] Remove left-over print from debugging --- src/pip/_internal/resolution/resolvelib/provider.py | 1 - 1 file changed, 1 deletion(-) diff --git a/src/pip/_internal/resolution/resolvelib/provider.py b/src/pip/_internal/resolution/resolvelib/provider.py index 2dbcc873573..b8e7413f96b 100644 --- a/src/pip/_internal/resolution/resolvelib/provider.py +++ b/src/pip/_internal/resolution/resolvelib/provider.py @@ -79,7 +79,6 @@ def _eligible_for_upgrade(name): if self._upgrade_strategy == "eager": return True elif self._upgrade_strategy == "only-if-needed": - print(name, self.roots) return (name in self.roots) return False From 9cf1bed78d7d4fa556b8a93274516eb07100f172 Mon Sep 17 00:00:00 2001 From: Paul Moore Date: Thu, 14 May 2020 11:33:30 +0100 Subject: [PATCH 3/4] Address review comments --- .../_internal/resolution/resolvelib/base.py | 5 ++ .../resolution/resolvelib/candidates.py | 20 ++++---- .../resolution/resolvelib/factory.py | 6 +++ .../resolution/resolvelib/provider.py | 46 +++++++++++-------- .../resolution/resolvelib/requirements.py | 4 ++ .../resolution/resolvelib/resolver.py | 6 +-- tests/unit/resolution_resolvelib/conftest.py | 2 +- 7 files changed, 57 insertions(+), 32 deletions(-) diff --git a/src/pip/_internal/resolution/resolvelib/base.py b/src/pip/_internal/resolution/resolvelib/base.py index 3ccc48e1fea..eacdf8ecc8e 100644 --- a/src/pip/_internal/resolution/resolvelib/base.py +++ b/src/pip/_internal/resolution/resolvelib/base.py @@ -44,6 +44,11 @@ def version(self): # type: () -> _BaseVersion raise NotImplementedError("Override in subclass") + @property + def is_installed(self): + # type: () -> bool + raise NotImplementedError("Override in subclass") + def get_dependencies(self): # type: () -> Sequence[Requirement] raise NotImplementedError("Override in subclass") diff --git a/src/pip/_internal/resolution/resolvelib/candidates.py b/src/pip/_internal/resolution/resolvelib/candidates.py index 794d14026ce..ed6e71bab57 100644 --- a/src/pip/_internal/resolution/resolvelib/candidates.py +++ b/src/pip/_internal/resolution/resolvelib/candidates.py @@ -101,15 +101,10 @@ def make_install_req_from_dist(dist, parent): return ireq -def is_already_installed(cand): - # type: (Candidate) -> bool - # For an ExtrasCandidate, we check the base - if isinstance(cand, ExtrasCandidate): - cand = cand.base - return isinstance(cand, AlreadyInstalledCandidate) - - class _InstallRequirementBackedCandidate(Candidate): + # These are not installed + is_installed = False + def __init__( self, link, # type: Link @@ -279,6 +274,8 @@ def _prepare_abstract_distribution(self): class AlreadyInstalledCandidate(Candidate): + is_installed = True + def __init__( self, dist, # type: Distribution @@ -400,6 +397,11 @@ def version(self): # type: () -> _BaseVersion return self.base.version + @property + def is_installed(self): + # type: () -> _BaseVersion + return self.base.is_installed + def get_dependencies(self): # type: () -> Sequence[Requirement] factory = self.base._factory @@ -436,6 +438,8 @@ def get_install_requirement(self): class RequiresPythonCandidate(Candidate): + is_installed = False + def __init__(self, py_version_info): # type: (Optional[Tuple[int, ...]]) -> None if py_version_info is not None: diff --git a/src/pip/_internal/resolution/resolvelib/factory.py b/src/pip/_internal/resolution/resolvelib/factory.py index 79343da32bd..2dc3a6ac9ec 100644 --- a/src/pip/_internal/resolution/resolvelib/factory.py +++ b/src/pip/_internal/resolution/resolvelib/factory.py @@ -114,6 +114,12 @@ def _make_candidate_from_link( def iter_found_candidates(self, ireq, extras): # type: (InstallRequirement, Set[str]) -> Iterator[Candidate] name = canonicalize_name(ireq.req.name) + + # We use this to ensure that we only yield a single candidate for + # each version (the finder's preferred one for that version). The + # requirement needs to return only one candidate per version, so we + # implement that logic here so that requirements using this helper + # don't all have to do the same thing later. seen_versions = set() # Yield the installed version, if it matches, unless the user diff --git a/src/pip/_internal/resolution/resolvelib/provider.py b/src/pip/_internal/resolution/resolvelib/provider.py index b8e7413f96b..f74fcae2f29 100644 --- a/src/pip/_internal/resolution/resolvelib/provider.py +++ b/src/pip/_internal/resolution/resolvelib/provider.py @@ -3,8 +3,6 @@ from pip._internal.utils.typing import MYPY_CHECK_RUNNING -from .candidates import is_already_installed - if MYPY_CHECK_RUNNING: from typing import Any, Dict, Optional, Sequence, Set, Tuple, Union @@ -40,16 +38,16 @@ def __init__( constraints, # type: Dict[str, SpecifierSet] ignore_dependencies, # type: bool upgrade_strategy, # type: str - roots, # type: Set[str] + user_requested, # type: Set[str] ): # type: (...) -> None self._factory = factory self._constraints = constraints self._ignore_dependencies = ignore_dependencies self._upgrade_strategy = upgrade_strategy - self.roots = roots + self.user_requested = user_requested - def sort_matches(self, matches): + def _sort_matches(self, matches): # type: (Sequence[Candidate]) -> Sequence[Candidate] # The requirement is responsible for returning a sequence of potential @@ -76,28 +74,36 @@ def sort_matches(self, matches): def _eligible_for_upgrade(name): # type: (str) -> bool + """Are upgrades allowed for this project? + + This checks the upgrade strategy, and whether the project was one + that the user specified in the command line, in order to decide + whether we should upgrade if there's a newer version available. + + (Note that we don't need access to the `--upgrade` flag, because + an upgrade strategy of "to-satisfy-only" means that `--upgrade` + was not specified). + """ if self._upgrade_strategy == "eager": return True elif self._upgrade_strategy == "only-if-needed": - return (name in self.roots) + return (name in self.user_requested) return False - def keep_installed(c): - # type: (Candidate) -> int - """Give priority to an installed version?""" - if not is_already_installed(c): - return 0 - - if _eligible_for_upgrade(c.name): - return 0 + def sort_key(c): + # type: (Candidate) -> Tuple[int, _BaseVersion] + """Return a sort key for the matches. - return 1 + The highest priority should be given to installed candidates that + are not eligible for upgrade. We use the integer value in the first + part of the key to sort these before other candidates. + """ + if c.is_installed and not _eligible_for_upgrade(c.name): + return (1, c.version) - def key(c): - # type: (Candidate) -> Tuple[int, _BaseVersion] - return (keep_installed(c), c.version) + return (0, c.version) - return sorted(matches, key=key) + return sorted(matches, key=sort_key) def get_install_requirement(self, c): # type: (Candidate) -> Optional[InstallRequirement] @@ -121,7 +127,7 @@ def find_matches(self, requirement): # type: (Requirement) -> Sequence[Candidate] constraint = self._constraints.get(requirement.name, SpecifierSet()) matches = requirement.find_matches(constraint) - return self.sort_matches(matches) + return self._sort_matches(matches) def is_satisfied_by(self, requirement, candidate): # type: (Requirement, Candidate) -> bool diff --git a/src/pip/_internal/resolution/resolvelib/requirements.py b/src/pip/_internal/resolution/resolvelib/requirements.py index c32187c6c5f..5c7d00c2a4a 100644 --- a/src/pip/_internal/resolution/resolvelib/requirements.py +++ b/src/pip/_internal/resolution/resolvelib/requirements.py @@ -76,6 +76,10 @@ def name(self): def find_matches(self, constraint): # type: (SpecifierSet) -> Sequence[Candidate] + + # We should only return one candidate per version, but + # iter_found_candidates does that for us, so we don't need + # to do anything special here. return [ c for c in self._factory.iter_found_candidates( diff --git a/src/pip/_internal/resolution/resolvelib/resolver.py b/src/pip/_internal/resolution/resolvelib/resolver.py index b363f303232..34c74ef95e6 100644 --- a/src/pip/_internal/resolution/resolvelib/resolver.py +++ b/src/pip/_internal/resolution/resolvelib/resolver.py @@ -69,7 +69,7 @@ def resolve(self, root_reqs, check_supported_wheels): # type: (List[InstallRequirement], bool) -> RequirementSet constraints = {} # type: Dict[str, SpecifierSet] - roots = set() + user_requested = set() requirements = [] for req in root_reqs: if req.constraint: @@ -82,7 +82,7 @@ def resolve(self, root_reqs, check_supported_wheels): constraints[name] = req.specifier else: if req.is_direct and req.name: - roots.add(canonicalize_name(req.name)) + user_requested.add(canonicalize_name(req.name)) requirements.append( self.factory.make_requirement_from_install_req(req) ) @@ -92,7 +92,7 @@ def resolve(self, root_reqs, check_supported_wheels): constraints=constraints, ignore_dependencies=self.ignore_dependencies, upgrade_strategy=self.upgrade_strategy, - roots=roots, + user_requested=user_requested, ) reporter = BaseReporter() resolver = RLResolver(provider, reporter) diff --git a/tests/unit/resolution_resolvelib/conftest.py b/tests/unit/resolution_resolvelib/conftest.py index 3327d9cc009..45deca10947 100644 --- a/tests/unit/resolution_resolvelib/conftest.py +++ b/tests/unit/resolution_resolvelib/conftest.py @@ -66,5 +66,5 @@ def provider(factory): constraints={}, ignore_dependencies=False, upgrade_strategy="to-satisfy-only", - roots=set(), + user_requested=set(), ) From 96b3377cd79e5d946f4d2b07e1cb8f8f3676850e Mon Sep 17 00:00:00 2001 From: Paul Moore Date: Fri, 15 May 2020 10:57:07 +0100 Subject: [PATCH 4/4] Type annotations --- src/pip/_internal/resolution/resolvelib/factory.py | 2 +- src/pip/_internal/resolution/resolvelib/resolver.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/pip/_internal/resolution/resolvelib/factory.py b/src/pip/_internal/resolution/resolvelib/factory.py index 2dc3a6ac9ec..99e20c35511 100644 --- a/src/pip/_internal/resolution/resolvelib/factory.py +++ b/src/pip/_internal/resolution/resolvelib/factory.py @@ -120,7 +120,7 @@ def iter_found_candidates(self, ireq, extras): # requirement needs to return only one candidate per version, so we # implement that logic here so that requirements using this helper # don't all have to do the same thing later. - seen_versions = set() + seen_versions = set() # type: Set[_BaseVersion] # Yield the installed version, if it matches, unless the user # specified `--force-reinstall`, when we want the version from diff --git a/src/pip/_internal/resolution/resolvelib/resolver.py b/src/pip/_internal/resolution/resolvelib/resolver.py index 34c74ef95e6..7cfded74ca6 100644 --- a/src/pip/_internal/resolution/resolvelib/resolver.py +++ b/src/pip/_internal/resolution/resolvelib/resolver.py @@ -69,7 +69,7 @@ def resolve(self, root_reqs, check_supported_wheels): # type: (List[InstallRequirement], bool) -> RequirementSet constraints = {} # type: Dict[str, SpecifierSet] - user_requested = set() + user_requested = set() # type: Set[str] requirements = [] for req in root_reqs: if req.constraint: