Change install command's default behaviour to upgrade packages by default

[pradyunsg]

Resolves #3786.
Resolves #536.

Check-list:

• Hand-tested implementation.
  • Implement a resolution to the --target problem
• Update tests.
• Update documentation.
• Revisit the write-up written for issue 59 to update it with what has been implemented.
• Request code review and peer testing.
• Shout out on distutils-sig for views/comments.
• Resolve any new issues that come up.
  • Fix test failure caused by change in PyPI.
    • Rebase this work off the master once the fix merges.
  • Resolve pip should reinstall . #536 or push patch to keep current behaviour
  • Wait on resolution about how to address concerns regarding security raised by @rbtcollins.

---

This change is 

[pradyunsg]

/cc @pfmoore @dstufft

I see some failures in the tests due to HTTP 403s in the test-run. I think this is due to the recent change on PyPI to redirecting http urls to https ones. I also feel that the tests on master haven't been re-run since that happened.

If such is the case, could someone trigger a Travis CI re-run¹ of the tests on master to see if it is still in good shape? If not, that would have to be fixed.

¹ there's a button or something for that in their web-UI

(edited)

[pradyunsg]

Rebased work off fixed master.

[pradyunsg]

/cc @njsmith @xavfernandez @dstufft @pfmoore

---

This PR now implements a proposal based on what was discussed earlier over at #3786. Any further discussions may please be done here.

There also a new (read updated) write-up regarding the same, which I intend to link to when I announce this change on distutils-sig; still undecided on announcing on python-list.

I request someone to review the PR and the write-up. Let me know what you think! 😄

[pradyunsg]

Should we close Github issues where eager-upgrading was causing problems but non-eager upgrading won't?

This has mostly been done already, linking them to #59.

[pfmoore]

The new write-up looks good. I'll try to get some time to review the PR, but can't promise anything, as real life is busy at the moment.

[njsmith]

This whole section could possibly be deleted, since it only existed to give folks a workaround for the old confusing behavior, and after this pr is merged then pip will join most other package managers in not even having a concept of a "eager upgrade". We don't need to emphasize to people that it doesn't have eager upgrade, any more than we need to emphasize that it doesn't have four-wheel drive or email reading capabilities.

Alternatively, since people will often consult the latest manual even for old versions, and I think there are pages out there that link to this section, it might make sense to keep the old information and section title (since the section title determines the link anchor) but reframed as: "it used to be that pip did a weird thing with upgrades, and this section was here to tell you how to work around it. Starting with version X, it doesn't anymore. So the recommended way to work around it is to upgrade to version X or better. As a historical note, the way you used to work around this was by running pip ...."

[pradyunsg]

Keeping this as a historical note makes sense sounds good to me.

[njsmith]

In addition to my line comments above:

• There should probably be a test that checks that passing --upgrade and -U is legal.
• From one of the tests, I got the impression that you might have changed the behavior of pip install <URL> and pip install <PATH> so that they always reinstall, even if a nominally identical package is already installed. I think this is a great idea if so, but it's a behavior change that wasn't discussed in this thread before and has been somewhat controversial in the past, so I wanted to check. (a) is this true? (b) if so it needs to be mentioned in the release notes as a separate change.

I think that's everything -- overall it looks pretty good to me (though it would probably be good to get a quick pass over the core logic by someone who is more familiar with pip's core requirement logic). And thanks so much for pushing this forward!

[njsmith]

@pypa/pip-committers: BTW, in case it's any incentive ;-), I'm giving a talk at SciPy on July 15 about new stuff in pip/wheels/pypi/etc. I would be very happy to use that as a soapbox to tell people that this has been fixed so they should start putting correct dependency metadata into their packages -- but that only works if this is merged (or even better, released!) by then.

[pradyunsg]

There should probably be a test that checks that passing --upgrade and -U is legal.

On it.

(a) is this true?

I think if the version number of the passed item is same or newer, it would result in a reinstall. If it's older, no change takes place... I need to verify if it is indeed the case though.

(b) if so it needs to be mentioned in the release notes as a separate change.

I would like to have it to be discussed/debated and explicitly decided before we ship it out merge it in.

[pradyunsg]

And thanks so much for pushing this forward!

You're welcome! 😄

[pradyunsg]

@njsmith said:
From one of the tests, I got the impression that you might have changed the behavior of pip install and pip install so that they always reinstall, even if a nominally identical package is already installed.

@pradyunsg said:
[snip] I need to verify if it is indeed the case though.

@njsmith is correct. As it stands, this PR means pip does cause a reinstall on pip install <path> regardless of the version. This just entered #536's domain. I'll reignite the discussion there.

editted in: I don't think I want to cause this PR to change the behaviour.

I think this is a great idea if so, but it's a behavior change that wasn't discussed in this thread before and has been somewhat controversial in the past, so I wanted to check.

I had assumed that it would not have caused a reinstall of an older version over an existing version. Thanks for spotting this. I didn't notice that this was a consequence of the changes I made.

(editted)

[pfmoore]

As it stands, this PR means pip does cause a reinstall on pip install regardless of the version

But pip install <wheel/sdist/requirement> will not reinstall unless a later version will get installed, correct?

[pradyunsg]

But pip install <wheel/sdist/requirement> will not reinstall unless a later version will get installed, correct?

Incorrect. Should I add some guard code to stop doing this and defer it to #536?

$ ./pip/__init__.py install ./tests/data/packages/simple-1.0.tar.gz
Processing ./tests/data/packages/simple-1.0.tar.gz
Building wheels for collected packages: simple
  Running setup.py bdist_wheel for simple ... done
  Stored in directory: /home/pradyunsg/.cache/pip/wheels/12/5b/7e/d7de13ab46586f22fcf2282254ec6edfb1d44e715422299cfe
Successfully built simple
Installing collected packages: simple
Successfully installed simple-1.0

$ ./pip/__init__.py install ./tests/data/packages/simple-2.0.tar.gz
Processing ./tests/data/packages/simple-2.0.tar.gz
Building wheels for collected packages: simple
  Running setup.py bdist_wheel for simple ... done
  Stored in directory: /home/pradyunsg/.cache/pip/wheels/8c/30/2e/9d065a92dd9eac25b1b61d7d27ec22a143f10574fe1a04d644
Successfully built simple
Installing collected packages: simple
  Found existing installation: simple 1.0
    Uninstalling simple-1.0:
      Successfully uninstalled simple-1.0
Successfully installed simple-2.0

$ ./pip/__init__.py install ./tests/data/packages/simple-1.0.tar.gz
Processing ./tests/data/packages/simple-1.0.tar.gz
Building wheels for collected packages: simple
  Running setup.py bdist_wheel for simple ... done
  Stored in directory: /home/pradyunsg/.cache/pip/wheels/12/5b/7e/d7de13ab46586f22fcf2282254ec6edfb1d44e715422299cfe
Successfully built simple
Installing collected packages: simple
  Found existing installation: simple 2.0
    Uninstalling simple-2.0:
      Successfully uninstalled simple-2.0
Successfully installed simple-1.0

[pfmoore]

Hmm, is that the agreed behaviour? I'm not particularly happy that a simple pip install with no flags can downgrade an existing version.

I would have expected to need --ignore-installed to downgrade, and --force-reinstall to overwrite an identical version. The only exception is when installing from a local directory, where the use case of "make a change, then reinstall" argues for pip install . overwriting by default.

Specifically, I don't think that installing something with an explicit version number in the filename (i.e., a wheel or sdist) should reinstall or downgrade without needing a --force-reinstall or --ignore-installed respectively - in the same way as a requirement would be handled.

I just checked, and yes, --upgrade currently behaves the way a bare install will behave in future. But I'd view that as an unintended consequence rather than inherent in the proposal. At a minimum, can @dstufft and/or @njsmith (who argued for "install does upgrade" behaviour) confirm that they consider the above behaviour as expected? I'm willing to leave this as is, if it's only me that finds the current behaviour odd.

[njsmith]

@pfmoore: I definitely think we should consider this PR as written as making two changes: the intentional change to make pip install <package-name-in-index> do an upgrade if the given package is already installed, and the accidental change to make pip install <explicit file or URL> always install exactly the named file/URL.

The first change was extensively discussed in #3786 and friends, and I think we're all ok with it.

By an interesting coincidence, the second change actually is also one that has been proposed and discussed in #536. You can read the issue for the details, but the basic argument there was that if I tell pip that I want this file right here to be installed, then the only way pip can make that happen is by installing that file. This is somewhat similar to if I say pip install foo==1.2.1 where I am telling pip that I want some instance of foo to be installed that has exactly the given version number metadata -- note that this can also downgrade! The difference is that in the foo==1.2.1 case I'm leaving it up to pip to find a copy of foo that has the given metadata, so it's totally legitimate for pip to say "hey, you already have one installed that meets that requirement, so that's the one I'm giving you, no changes needed". But if I explicitly pass a file, then right now pip fetches that file, checks its version metadata, and then quits if I already have a package installed with the same name+version. This is incredibly frustrating, because in this case I didn't ask pip to give me "some package whose metadata is similar to this package here", I asked it to give me this package here. It would be OK if there really were a perfect 1-1 making between version numbers and package versions, but in practice this is just not true. E.g. you might be trying to replace a locally built numpy-1.11.0 with a numpy-1.11.0 that you downloaded from cgohlke -- they have the same version numbers, but maybe one works and the other doesn't. Or you might be trying to install a development snapshot like foo-1.2+dev, and pip's like "hey, you already have a development snapshot installed, so all is good" and you're like "wtf pip yes I have a development snapshot installed, but it's a different development snapshot from last week, and I don't care if they're both labeled foo-1.2+dev, you need to stop with the back talk and install the package that I literally just told you to install".

From my read of the #536 history, it looks like @dstufft, @xavfernandez, @rbtcollins, @njsmith, @ChrisBarker-NOAA, @rgommers were in favor of this change, @piotr-dobrogost against it, and @pfmoore hadn't made up his mind yet ;-).

So for this PR, options are:

• Keep it as is, with both changes together
• Split up the two changes so this only does the upgrade change, not the explicitly named location change

As a general rule I am a fan of splitting up PRs, but in this case I'm not so sure. There does seem to be widespread consensus in favor of both of the changes here, and "splitting it up" is not so easy, because the two behaviors are already coupled in the current code base. Basically we'd have to ask @pradyunsg to muck around in the internals more to explicitly disable the location handling behavior... and then I guess we'd have another PR immediately afterwards just to remove that work again? Seems like a lot of work for not much benefit.

So my preference would be to go ahead and merge this, closing both #3786 and #536 in one super efficient click :-).

OTOH if everyone wants to restart the argument about #536 again, then (a) do you really have to? :-(, (b) if you do then fine, but then we should probably add those hacks to this PR because we don't want it to get stalled out again (the bug's been open what, 4 years now?)

[rbtcollins]

Thanks for the tag. I'm pro #536; I'm not at all pro the proposed changes in #3786. The idea of pip install X upgrading X implicitly and dependencies only as required I'm fine with - is that what this does? The mucking around with other command line options like the meaning of -U is what concerns me. If this does that, I think it would be a mistake to merge it.

[pfmoore]

OTOH if everyone wants to restart the argument about #536 again, then (a) do you really have to?

Nope, thanks for the back-reference (there's been so much debate on this issue, that I've forgotten most of the history). I'm happy to defer to the majority.

[pradyunsg]

So, is this accidental change agreed upon and I maybe add a few more tests for it? If not, I'll be fine with going and diving into the internals and try to see what I can do to revert it, as much as I'll prefer not to.

Heads Up: I have to move to another city by 1st July. After that, I don't know whether I'll have the free time for this PR.

[pradyunsg]

I forgot to push the commits adding @njsmith's suggestions. 😅

[njsmith]

@pradyunsg: It sounds like (tentatively) everyone is fine with the "accidental change". It certainly needs to have a test -- but I think it might already? And it should be mentioned as in the release notes as a separate item from the install-now-upgrades change.

[pradyunsg]

It certainly needs to have a test -- but I think it might already?

I doubt it has a test. The change broke only one test which I modified to pass...

[pradyunsg]

I've just pushed 4 new tests for the behaviour change related to #536.

Could someone check that this is indeed the required behaviour?

---

I'll prefer to defer updating the changelog to someone else. It's got a section for 8.2.0 but I would like to see this in 9.0.0.

---

I'll wait for the go ahead from @njsmith and @pfmoore before announcing on the distutils-sig and python-list.

[pradyunsg]

One extra newline. One extra newline broke the build. :/

[pradyunsg]

(cross-posted) I'm still waiting on @pfmoore or @njsmith giving me the go-ahead that this PR is fine and we can announce the same on distutils-sig for comments from a larger audience.

[pfmoore]

I'm OK with you announcing it. It seems good enough to me. Holding off for perfection on the PR is not going to help (particularly if your free time is due to drop significantly soon!)

[pradyunsg]

Great! I just sent the mail. 😁

[pradyunsg]

Fixed the build.

[xavfernandez]

Hmm, is this still relevant after the merge of #3972 ?

[pradyunsg]

I think this can be closed.

If someone feels it's still relevant, we can reopen this.

[lock]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.