Merge pull request #8234 from pfmoore/refactor_find_matches

Split find_matches into generation and sorting

Author: pfmoore

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

src/pip/_internal/resolution/resolvelib/candidates.py

@@ -102,6 +102,9 @@ def make_install_req_from_dist(dist, parent):
 
 
 class _InstallRequirementBackedCandidate(Candidate):
+    # These are not installed
+    is_installed = False
+
     def __init__(
         self,
         link,          # type: Link
@@ -271,6 +274,8 @@ def _prepare_abstract_distribution(self):
 
 
 class AlreadyInstalledCandidate(Candidate):
+    is_installed = True
+
     def __init__(
         self,
         dist,  # type: Distribution
@@ -392,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
@@ -428,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:

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,21 +50,16 @@ 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
         self._python_candidate = RequiresPythonCandidate(py_version_info)
         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,64 +111,52 @@ 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
+
+        # 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()  # type: Set[_BaseVersion]
+
+        # 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,
             specifier=ireq.req.specifier,
             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"

src/pip/_internal/resolution/resolvelib/provider.py

@@ -4,25 +4,106 @@
 from pip._internal.utils.typing import MYPY_CHECK_RUNNING
 
 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__(
         self,
         factory,  # type: Factory
         constraints,  # type: Dict[str, SpecifierSet]
         ignore_dependencies,  # type: bool
+        upgrade_strategy,  # type: 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.user_requested = user_requested
+
+    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
+            """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.user_requested)
+            return False
+
+        def sort_key(c):
+            # type: (Candidate) -> Tuple[int, _BaseVersion]
+            """Return a sort key for the matches.
+
+            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)
+
+            return (0, c.version)
+
+        return sorted(matches, key=sort_key)
 
     def get_install_requirement(self, c):
         # type: (Candidate) -> Optional[InstallRequirement]
@@ -45,7 +126,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

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(

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,30 +49,27 @@ 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,
             make_install_req=make_install_req,
             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]
+        user_requested = set()  # type: Set[str]
         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:
+                    user_requested.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,
+            user_requested=user_requested,
         )
         reporter = BaseReporter()
         resolver = RLResolver(provider, reporter)

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",
+        user_requested=set(),
     )