Repository: pypa/pip
Pull request: Isolate, reuse PackageFinder best candidate logic
My take on #5175. This turns out to be more complicated than I imagined. I decided to make the outdated check follow how pip finds versions for requirements passed by the user:

* If `allow_all_prereleases` is True, consider prereleases. (not applicable for self version check)
* Try to find the latest stable version.
* If no stable versions are available, try to offer prereleases. (PEP 440)

I also tweaked the code in `find_requirement` a little to refactor out some duplicate formatting.

Fix #5175, close #5928.

Problem statement:
pip 9 offers upgrades to prereleases
* Pip version: 9.0.3
* Python version: 2.7.14
* Operating system: Ubuntu 17.10

### Description:

Any pip command says

```
You are using pip version 9.0.3, however version 10.0.0b2 is available.
You should consider upgrading via the 'pip install --upgrade pip' command.
```

I'm not sure it's a good idea to offer upgrades to pre-release versions?  Anyway, the suggested command doesn't work (because it needs --pre to allow upgrades to pre-release versions):

```
Requirement already up-to-date: pip in /home/mg/.venv/lib/python2.7/site-packages
You are using pip version 9.0.3, however version 10.0.0b2 is available.
You should consider upgrading via the 'pip install --upgrade pip' command.
```

Hints from issue discussion:
Thanks for reporting this! It's only likely to be an issue for a relatively short period (pip 10 final is due in just over a week) but it's good to have the problem recorded so that we can ensure it doesn't crop up for future betas.
https://github.com/pypa/pip/blob/0007825733aa862e76a66e5d1cfee121c29192ff/src/pip/_internal/utils/outdated.py#L135

Relevant line is this. We'd want to filter out pre-releases here.,

Commit message: Isolate, reuse PackageFinder best candidate logic

Split out how PackageFinder finds the best candidate, and reuse it in the
self version check, to avoid the latter duplicating (and incorrectly
implementing) the same logic.

Patch to review:
diff --git a/news/5175.bugfix b/news/5175.bugfix
new file mode 100644
index 00000000000..1ab90f4f35d
--- /dev/null
+++ b/news/5175.bugfix
@@ -0,0 +1 @@
+Make pip's self version check avoid recommending upgrades to prereleases if the currently-installed version is stable.
diff --git a/src/pip/_internal/index.py b/src/pip/_internal/index.py
index f251c4b58b6..173d90c3a8f 100644
--- a/src/pip/_internal/index.py
+++ b/src/pip/_internal/index.py
@@ -55,7 +55,8 @@
     BuildTag = Tuple[Any, ...]  # either empty tuple or Tuple[int, str]
     CandidateSortingKey = Tuple[int, _BaseVersion, BuildTag, Optional[int]]
 
-__all__ = ['FormatControl', 'PackageFinder']
+
+__all__ = ['FormatControl', 'FoundCandidates', 'PackageFinder']
 
 
 SECURE_ORIGINS = [
@@ -254,6 +255,63 @@ def _get_html_page(link, session=None):
     return None
 
 
+class FoundCandidates(object):
+    """A collection of candidates, returned by `PackageFinder.find_candidates`.
+
+    Arguments:
+
+    * `candidates`: A sequence of all available candidates found.
+    * `specifier`: Specifier to filter applicable versions.
+    * `prereleases`: Whether prereleases should be accounted. Pass None to
+        infer from the specifier.
+    * `sort_key`: A callable used as the key function when choosing the best
+        candidate.
+    """
+    def __init__(
+        self,
+        candidates,     # type: List[InstallationCandidate]
+        specifier,      # type: specifiers.BaseSpecifier
+        prereleases,    # type: Optional[bool]
+        sort_key,       # type: Callable[[InstallationCandidate], Any]
+    ):
+        # type: (...) -> None
+        self._candidates = candidates
+        self._specifier = specifier
+        self._prereleases = prereleases
+        self._sort_key = sort_key
+
+    @property
+    def all(self):
+        # type: () -> Iterable[InstallationCandidate]
+        return iter(self._candidates)
+
+    @property
+    def applicable(self):
+        # type: () -> Iterable[InstallationCandidate]
+        # Filter out anything which doesn't match our specifier.
+        versions = set(self._specifier.filter(
+            # We turn the version object into a str here because otherwise
+            # when we're debundled but setuptools isn't, Python will see
+            # packaging.version.Version and
+            # pkg_resources._vendor.packaging.version.Version as different
+            # types. This way we'll use a str as a common data interchange
+            # format. If we stop using the pkg_resources provided specifier
+            # and start using our own, we can drop the cast to str().
+            [str(c.version) for c in self._candidates],
+            prereleases=self._prereleases,
+        ))
+        # Again, converting to str to deal with debundling.
+        return (c for c in self._candidates if str(c.version) in versions)
+
+    @property
+    def best(self):
+        # type: () -> Optional[InstallationCandidate]
+        try:
+            return max(self.applicable, key=self._sort_key)
+        except ValueError:  # Raised by max() if self.applicable is empty.
+            return None
+
+
 class PackageFinder(object):
     """This finds packages.
 
@@ -628,6 +686,27 @@ def find_all_candidates(self, project_name):
         # This is an intentional priority ordering
         return file_versions + find_links_versions + page_versions
 
+    def find_candidates(self, project_name, specifier):
+        """Find matches for the given project and specifier.
+
+        `specifier` should implement `filter` to allow version filtering (e.g.
+        ``packaging.specifiers.SpecifierSet``).
+
+        Returns a FoundCandidates instance that contains the following
+        properties:
+
+        * `all`: All candidates matching `project_name` found on the index.
+        * `applicable`: Candidates matching `project_name` and `specifier`.
+        * `best`: The best candidate available. This may be None if no
+            applicable candidates are found.
+        """
+        return FoundCandidates(
+            self.find_all_candidates(project_name),
+            specifier=specifier,
+            prereleases=(self.allow_all_prereleases or None),
+            sort_key=self._candidate_sort_key,
+        )
+
     def find_requirement(self, req, upgrade):
         # type: (InstallRequirement, bool) -> Optional[Link]
         """Try to find a Link matching req
@@ -636,35 +715,8 @@ def find_requirement(self, req, upgrade):
         Returns a Link if found,
         Raises DistributionNotFound or BestVersionAlreadyInstalled otherwise
         """
-        all_candidates = self.find_all_candidates(req.name)
-
-        # Filter out anything which doesn't match our specifier
-        compatible_versions = set(
-            req.specifier.filter(
-                # We turn the version object into a str here because otherwise
-                # when we're debundled but setuptools isn't, Python will see
-                # packaging.version.Version and
-                # pkg_resources._vendor.packaging.version.Version as different
-                # types. This way we'll use a str as a common data interchange
-                # format. If we stop using the pkg_resources provided specifier
-                # and start using our own, we can drop the cast to str().
-                [str(c.version) for c in all_candidates],
-                prereleases=(
-                    self.allow_all_prereleases
-                    if self.allow_all_prereleases else None
-                ),
-            )
-        )
-        applicable_candidates = [
-            # Again, converting to str to deal with debundling.
-            c for c in all_candidates if str(c.version) in compatible_versions
-        ]
-
-        if applicable_candidates:
-            best_candidate = max(applicable_candidates,
-                                 key=self._candidate_sort_key)
-        else:
-            best_candidate = None
+        candidates = self.find_candidates(req.name, req.specifier)
+        best_candidate = candidates.best
 
         if req.satisfied_by is not None:
             installed_version = parse_version(req.satisfied_by.version)
@@ -678,7 +730,7 @@ def find_requirement(self, req, upgrade):
                 req,
                 ', '.join(
                     sorted(
-                        {str(c.version) for c in all_candidates},
+                        {str(c.version) for c in candidates.all},
                         key=parse_version,
                     )
                 )
@@ -710,21 +762,24 @@ def find_requirement(self, req, upgrade):
                 )
             return None
 
+        compatible_version_display = ", ".join(
+            str(v) for v in sorted({c.version for c in candidates.applicable})
+        ) or "none"
+
         if best_installed:
             # We have an existing version, and its the best version
             logger.debug(
                 'Installed version (%s) is most up-to-date (past versions: '
                 '%s)',
                 installed_version,
-                ', '.join(sorted(compatible_versions, key=parse_version)) or
-                "none",
+                compatible_version_display,
             )
             raise BestVersionAlreadyInstalled
 
         logger.debug(
             'Using version %s (newest of versions: %s)',
             best_candidate.version,
-            ', '.join(sorted(compatible_versions, key=parse_version))
+            compatible_version_display,
         )
         return best_candidate.location
 
diff --git a/src/pip/_internal/utils/outdated.py b/src/pip/_internal/utils/outdated.py
index f7692761186..7d7d50cfdef 100644
--- a/src/pip/_internal/utils/outdated.py
+++ b/src/pip/_internal/utils/outdated.py
@@ -8,6 +8,7 @@
 
 from pip._vendor import lockfile, pkg_resources
 from pip._vendor.packaging import version as packaging_version
+from pip._vendor.packaging.specifiers import SpecifierSet
 
 from pip._internal.index import PackageFinder
 from pip._internal.utils.compat import WINDOWS
@@ -129,12 +130,10 @@ def pip_version_check(session, options):
                 trusted_hosts=options.trusted_hosts,
                 session=session,
             )
-            all_candidates = finder.find_all_candidates("pip")
-            if not all_candidates:
+            candidate = finder.find_candidates("pip", SpecifierSet()).best
+            if candidate is None:
                 return
-            pypi_version = str(
-                max(all_candidates, key=lambda c: c.version).version
-            )
+            pypi_version = str(candidate.version)
 
             # save that we've performed a check
             state.save(pypi_version, current_time)
diff --git a/tests/unit/test_unit_outdated.py b/tests/unit/test_unit_outdated.py
index 31b5a7fa384..0696b71c7a4 100644
--- a/tests/unit/test_unit_outdated.py
+++ b/tests/unit/test_unit_outdated.py
@@ -1,3 +1,4 @@
+import collections
 import datetime
 import os
 import sys
@@ -28,8 +29,10 @@ class MockPackageFinder(object):
     def __init__(self, *args, **kwargs):
         pass
 
-    def find_all_candidates(self, project_name):
-        return self.INSTALLATION_CANDIDATES
+    def find_candidates(self, project_name, specifier):
+        return collections.namedtuple("FoundCandidates", "best")(
+            best=self.INSTALLATION_CANDIDATES[0],
+        )
 
 
 class MockDistribution(object):


Code review comments:
diff_hunk: @@ -254,6 +255,63 @@ def _get_html_page(link, session=None):
     return None
 
 
+class FoundCandidates(object):
+    """A collection of candidates, returned by `PackageFinder.find_candidates`.
+
+    Arguments:
+
+    * `candidates`: A sequence of all available candidates found.
+    * `specifier`: Specifier to filter applicable versions.
+    * `prereleases`: Whether prereleases should be accounted. Pass None to
+        infer from the specifier.
+    * `sort_key`: A callable used as the key function when choosing the best
+        candidate.
+    """
+    def __init__(
+        self,
+        candidates,     # type: List[InstallationCandidate]
+        specifier,      # type: specifiers.BaseSpecifier
+        prereleases,    # type: Optional[bool]
+        sort_key,       # type: Callable[[InstallationCandidate], Any]
+    ):
+        # type: (...) -> None
+        self._candidates = candidates
+        self._specifier = specifier
+        self._prereleases = prereleases
+        self._sort_key = sort_key
+
+    @property
+    def all(self):
+        # type: () -> Iterable[InstallationCandidate]
+        return iter(self._candidates)
+
+    @property
+    def applicable(self):
+        # type: () -> Iterable[InstallationCandidate]
+        # Filter out anything which doesn't match our specifier.
+        versions = set(self._specifier.filter(
+            # We turn the version object into a str here because otherwise
+            # when we're debundled but setuptools isn't, Python will see
+            # packaging.version.Version and
+            # pkg_resources._vendor.packaging.version.Version as different
+            # types. This way we'll use a str as a common data interchange
+            # format. If we stop using the pkg_resources provided specifier
+            # and start using our own, we can drop the cast to str().
+            [str(c.version) for c in self._candidates],
+            prereleases=self._prereleases,
+        ))
+        # Again, converting to str to deal with debundling.
+        return (c for c in self._candidates if str(c.version) in versions)
+
+    @property
+    def best(self):
+        # type: () -> Optional[InstallationCandidate]
+        try:
+            return max(self.applicable, key=self._sort_key)
+        except ValueError:  # Raised by max() if self.applicable is empty.
path: src/pip/_internal/index.py
text: @user1:
I think you should check whether `self.applicable` is non-empty before calling `max()` (like the original code did) because `_sort_key` can be an arbitrary function. If a `ValueError` occurs, you don't know if it was raised by `max()` or if it's coming from somewhere in the sort key function. If the latter, we'd want that to bubble up. This is similar to the point I made before about `ValueError`.

diff_hunk: @@ -628,6 +686,27 @@ def find_all_candidates(self, project_name):
         # This is an intentional priority ordering
         return file_versions + find_links_versions + page_versions
 
+    def find_candidates(self, project_name, specifier):
+        """Find matches for the given project and specifier.
+
+        `specifier` should implement `filter` to allow version filtering (e.g.
+        ``packaging.specifiers.SpecifierSet``).
+
+        Returns a FoundCandidates instance that contains the following
+        properties:
+
+        * `all`: All candidates matching `project_name` found on the index.
path: src/pip/_internal/index.py
text: @user1:
If a docstring says an object of a certain type is being returned, I don't think the documentation of the class should be repeated. One can always look at the class directly. Also, if the documentation is repeated, it's liable to get out of sync.

diff_hunk: @@ -129,12 +130,10 @@ def pip_version_check(session, options):
                 trusted_hosts=options.trusted_hosts,
                 session=session,
             )
-            all_candidates = finder.find_all_candidates("pip")
-            if not all_candidates:
+            candidate = finder.find_candidates("pip", SpecifierSet()).best
path: src/pip/_internal/utils/outdated.py
text: @user1:
Hmm, I'm still not convinced the caller should have to pass / know about the `specifier` argument here. If the default value was something non-trivial, I would agree with you. But here the default value is "no filter" / "don't filter by anything," so the caller shouldn't have to pass anything to get that behavior. This is similar to the argument for why it's preferable to invoke `SpecifierSet()` instead of `SpecifierSet(specifiers='', prereleases=None)`.


@author:
Oops, somehow my previous reply was lost after rebasing. I asked whether this would be acceptable:

```python
finder.find_candidates("pip", specifier=None)
```

This would make the intention of not filtering explicit without requiring SpecifierSet. (Requiring a specifier string would be fine as well, in this case empty).

diff_hunk: @@ -254,6 +255,63 @@ def _get_html_page(link, session=None):
     return None
 
 
+class FoundCandidates(object):
+    """A collection of candidates, returned by `PackageFinder.find_candidates`.
+
+    Arguments:
+
+    * `candidates`: A sequence of all available candidates found.
+    * `specifier`: Specifier to filter applicable versions.
+    * `prereleases`: Whether prereleases should be accounted. Pass None to
+        infer from the specifier.
+    * `sort_key`: A callable used as the key function when choosing the best
+        candidate.
+    """
+    def __init__(
+        self,
+        candidates,     # type: List[InstallationCandidate]
+        specifier,      # type: specifiers.BaseSpecifier
+        prereleases,    # type: Optional[bool]
+        sort_key,       # type: Callable[[InstallationCandidate], Any]
+    ):
+        # type: (...) -> None
+        self._candidates = candidates
+        self._specifier = specifier
+        self._prereleases = prereleases
+        self._sort_key = sort_key
+
+    @property
+    def all(self):
path: src/pip/_internal/index.py
text: @user1:
I think each of these would be more appropriate as methods rather than properties, as calling them constructs a new iterator object. And in the case of `best`, it's doing an involved computation. If on the other hand, these were pre-computed and passed to the `FoundCandidates` constructor, then it could be a simple attribute lookup..