Fix for #770: pip install -U shouldn't look at pypi if not needed

[msabramo]

If an exact version is specified for install, and that version is
already installed, then there is no point going to pypi as no install
is needed.

This is a rework of PR #771, because that PR is old and has merge
conflicts that were easier to fix by applying the changes manually.

I would recommend closing #771.

[msabramo]

Please add label: needstest

[xavfernandez]

self.satisfied_by does not seem to be defined, I guess this should be req_to_install.satisfied_by.version
and yes, this needs tests ;)

[xavfernandez]

the not (a == b) could be replaced by a != b

[msabramo]

A unit test could mock PackageFinder and assert that no calls are made on find_requirements.

[xavfernandez]

same here for the not (a == b) vs a != b

[msabramo]

Hmmm, I agree that it seems like that would be better, but when I tried it, it seems to break:

As-is:

$ pip install -U wheel==0.24.0
Requirement already satisfied (use --upgrade to upgrade): wheel==0.24.0 in /Users/marca/python/virtualenvs/pip/lib/python2.7/site-packages

but if I do this:

--- a/pip/req/req_set.py
+++ b/pip/req/req_set.py
@@ -230,8 +230,8 @@ class RequirementSet(object):
                 if req_to_install.satisfied_by:
                     # check if we are upgrading and that we don't already have
                     # an exact version match
-                    if self.upgrade and not (
-                            req_to_install.satisfied_by.as_requirement() ==
+                    if self.upgrade and (
+                            req_to_install.satisfied_by.as_requirement() !=
                             req_to_install.req):
                         if not self.force_reinstall and not req_to_install.url:
                             try:

then I get a different and incorrect behavior:

$ pip install -U wheel==0.24.0
processing ~/.pdbrc.py
[5] > /Users/marca/dev/git-repos/pip/pip/index.py(284)find_requirement()
-> def mkurl_pypi_url(url):

Note that now it's calling PackageFinder.find_requirement, whereas before it wasn't.

I admit to being mystified by this. Any ideas why this is happening?

[msabramo]

I broke logic. Now I have no faith in anything.

[xavfernandez]

what is your version of setuptools ?

[msabramo]

Hmmm, pip._vendor.pkg_resources.Requirement overrides __eq__ and does not override __ne__... Suspicious....

[xavfernandez]

(Pdb) req_to_install.satisfied_by.as_requirement() == req_to_install.req
True
(Pdb) req_to_install.satisfied_by.as_requirement() != req_to_install.req
True

[msabramo]

Adding the following fixes the problem:

    def __ne__(self, other):
        return not self == other

which of course if this is a legitimate bug, should be fixed in setuptools proper and not in the vendored copy of it.

But is this a bug or was there some intentional reason for omitting __ne__? @jaraco: Maybe you could shed some light? Let me know if you want me to send a PR to setuptools...

[msabramo]

In order to make the code nicer and to workaround what I think is a setuptools problem, I changed it to this:

--- a/pip/req/req_set.py
+++ b/pip/req/req_set.py
@@ -228,7 +228,12 @@ class RequirementSet(object):
             if not self.ignore_installed and not req_to_install.editable:
                 req_to_install.check_if_exists()
                 if req_to_install.satisfied_by:
-                    if self.upgrade:
+                    # check if we are upgrading and that we don't already have
+                    # an exact version match
+                    satisfied = (
+                        req_to_install.satisfied_by.as_requirement() ==
+                        req_to_install.req)
+                    if self.upgrade and not satisfied:
                         if not self.force_reinstall and not req_to_install.url:
                             try:
                                 url = finder.find_requirement(

[msabramo]

I decided to send a PR to setuptools since it seems likely that it's a bug and I don't want it to get lost in the shuffle:

https://bitbucket.org/pypa/setuptools/pull-request/125/add-__ne__-method-to-requirement-class/diff

[msabramo]

Please restart Travis CI for just py33, as it failed with an error trying to hit launchpad (ugh):

bzr: ERROR: Invalid http response for 
http://bazaar.launchpad.net/~django-wikiapp/django-wikiapp/release-0.1/.bzr/branch/format: 
Unable to handle http code 503: Service Temporarily Unavailable
============== 1 failed, 228 passed, 3 skipped in 271.72 seconds ===============

[msabramo]

Please add label: needstest

[msabramo]

OK, I just added a test called test_upgrade_no_look_at_pypi_if_exact_version_installed in 1da69c8, so forget about adding the needstest label.

[msabramo]

The Travis CI build passed. Ready for review...

[msabramo]

Contains merge conflicts now. I'll take a look...

[msabramo]

Fixed merge conflicts with d641ee1

[msabramo]

The Travis CI build passed. Ready for review...

[msabramo]

How does it look now, @xavfernandez?

[xavfernandez]

I must say I'm not comfortable with the req_to_install.satisfied_by.as_requirement() == req_to_install.req test which rely too much on setuptools' Requirement implementation.

The fact that:

In [1]: from pip.req.req_install import InstallRequirement
In [2]: ir=InstallRequirement.from_line('pip===6.0.8')
In [3]: ir.check_if_exists()
Out[3]: True
In [4]: ir.satisfied_by.as_requirement() == ir.req
Out[4]: False

does not help.
(yes, it is a far-fetched example, an other bad one could be: 'pip==6.0.8,<7')

[msabramo]

@xavfernandez: What would you change it to?

[xavfernandez]

Good question :)
I'd be tempted to use:

req_to_install.req.specifier.contains(req_to_install.satisfied_by.version)

[msabramo]

Updated. Thanks!

[msabramo]

@xavfernandez:

req_to_install.req.specifier.contains(req_to_install.satisfied_by.version)

Hmmm, tests are failing with that. I guess the meaning is different than what we had before?

[msabramo]

Maybe once we figure out what the right logic is, we should make it a exact_match method of InstallRequirement...

[xavfernandez]

My bad, specifier.contains will indeed prevent upgrade: pkg 0.5 does satisfy pkg>=0.4 but we are still expected to search for an upgrade if upgrade is specified.

Checking if the current specifier is satisfied by the current version is useless, the if req_to_install.satisfied_by: test already checked it. What we want is just to check whether the specifier is stric or not. This could be performed via:

diff --git a/pip/req/req_set.py b/pip/req/req_set.py
index 4aa541d..880e0aa 100644
--- a/pip/req/req_set.py
+++ b/pip/req/req_set.py
@@ -225,12 +225,11 @@ class RequirementSet(object):
             if not self.ignore_installed and not req_to_install.editable:
                 req_to_install.check_if_exists()
                 if req_to_install.satisfied_by:
-                    # check if we are upgrading and that we don't already have
-                    # an exact version match
-                    satisfied = (
-                        req_to_install.satisfied_by.as_requirement() ==
-                        req_to_install.req)
-                    if self.upgrade and not satisfied:
+                    # check that we don't already have an exact version match
+                    # i.e. with at least one strict req operator
+                    strict_req = set(('==', '===')) & set(
+                        op for op, _ in req_to_install.req.specs)
+                    if self.upgrade and not strict_req:
                         if not (self.force_reinstall or req_to_install.link):
                             try:
                                 link = finder.find_requirement

[msabramo]

@xavfernandez: Thanks for the new idea. Just updated it. Crossing fingers...

[xavfernandez]

I did test my patch ;)

[msabramo]

and Travis CI passed so looking good...

[dstufft]

This breaks --force-reinstall, it'd be great to get this fixed so that --force-reinstall works, and ideally add a test to ensure it continues to work.

[msabramo]

Fixed --force-reinstall and added a test: test_upgrade_look_at_pypi_if_exact_version_installed_and_force

[piotr-dobrogost]

Couldn't this work also for <= requirements?

[xavfernandez]

I was wondering if it was a good idea with the == specifier.

With a ==1.0 specifier, one could expect pip install -U to install 1.0+toto if the latter is available on a private pypi...
or maybe not ?

[dstufft]

Hey @pypa/pip-developers (@ncoghlan probably has an opinion too) I merged this 16 days ago, but there's an open question about it..

Right now it assumes that if you do pip install foo==1.0 that you don't need to reach out and check PyPI (because you already have 1.0 installed). However there might be multiple things called 1.0 if someone has multiple local versions available. How do we want to handle that? Do we want to revert this because of that case? Do we want to mandate --force-reinstall in that case?

I also notice this has a problem where it won't do the right thing for ==1.* either, but that's more easily fixed.

[msabramo]

I would think that if all of the versions are 1.0 then they are the exact same bits. Obviously, someone can violate this, but that seems to be very against the concept of semantic versioning and such. I'd hate to penalize the more common case where people play by the rules because of an unusual case. But I'll change this is folks think it is necessary.

[pfmoore]

@msabramo The issue here is that all of 1.0, 1.0+local1 and 1.0+local2 are ==1.0, but they are explicitly not the same bits. The only actual PEP 440 operation that guarantees "exact same bits" is ===, and that operator is "highly discouraged" for general use.

It's an unusual situation - and I don't have any personal need for it - but if you have a local variant (available via --find-links or --extra-index-url) 1.0+patched1 installed, then you could quite reasonably want to do pip install -U foo==1.0 to upgrade to 1.0+patched2 without grabbing a newer version such as 2.0 from PyPI. It seems to me that this patch breaks that usage without offering any workaround.

The complaint in the original issue #770 was that if you ran pip install -U -r reqs.txt and the requirements were already satisfied, then pip shouldn't "check PyPI". But why not? That's what -U is explicitly asking for. The user's expectation was that because they were using == version matches, there's no point in checking - but that's not true once you take into account that == can be satisfied by multiple versions, some of which may be newer than the installed one. About the only thing you can say is that PyPI can be excluded from the search, because local versions can't be uploaded to PyPI. If this change removed PyPI from the search list, we wouldn't have this issue now.

I'm inclined to say that this PR was over-enthusiastic, and should be modified to "not check PyPI" rather than not do any searches at all. Obviously that's a lot harder, though, and may even be sufficiently hard to mean that the change is no longer justifiable.

But again, this is only a theoretical concern for me. In practice, the issue doesn't affect me either way.

(Actually, the original issue quoted "1000x" slower, under pip 1.2.1. It's quite possible that the caching in recent pip versions will have improved that significantly, maybe even to the extent that this change is no longer needed...)

[dstufft]

I'm considering if we should just revert it all together.

Short circuiting the index check for == is an optimization. An optimization that breaks "valid" things is generally not a very good optimization.

A few things have changed since that original ticket to increase the speed of checking for an update:

• PyPI is behind a CDN now, so the common case is much faster for fetching the /simple/ page.
• pip no longer scans random external links nor does it attempt to look at versioned urls, so what could previously take many HTTP requests (some projects over a 100) now takes a single HTTP request.
• PyPI and pip now work together to reduce HTTP requests, previously pip install django would get /simple/django/ (1 HTTP request) then get redirected to /simple/Django/ (another HTTP request), but now PyPI uses the "normalized" name for the URL and pip defaults to that, so you'll only get that single /simple/django/ HTTP request.
• pip caches the /simple/ pages, although only for a maximum of 10 minutes, so unless you're doing it multiple times in that time period this won't matter as much.

In that time period a few things have changed in the meaning of ==:

• Previously foo==1.0 would only match something versioned as 1.0, however now adays this will match local versions as well.
• Previously there was no ability to do foo==1.*, however now there is.

I do feel like "don't check PyPI" is a bad way of implementing this, either it should not check at all or it should do the normal checks. I'm leaning heavily towards reverting this and just saying that --upgrade checks PyPI and that's how it works and that we've made sufficient improvements since that ticket that it's now "fast enough" without needing to rely on a short circuiting of the == logic.

[pfmoore]

Yeah, the fact that it's intended to be an optimisation but changes behaviour seems wrong to me. I'd support reverting.

[msabramo]

Yeah, reverting seems reasonable to me.