Make -U non-recursive by default, add --upgrade-recursive option #571

fdintino · 2012-06-08T03:40:43Z

I believe that this pull request fixes #304. I incorporated a suggestion from one of the comments in that issue and kept the current behavior with the command option --upgrade-recursive.

travisbot · 2012-06-11T23:59:09Z

This pull request passes (merged 8a38346 into 8d92fc2).

kennethreitz · 2012-07-02T03:01:43Z

This would be fantastic.

lepture · 2012-07-02T03:08:43Z

👍

kennethreitz · 2012-07-02T03:38:31Z

I'd also consider --upgrade-all instead of --upgrade-recursive

ejucovy · 2012-07-02T04:51:20Z

@kennethreitz, I think --upgrade-recursive is the better name; --upgrade-all might sound like it should upgrade all the currently installed packages. A google search for "pip upgrade all" turns up requests for that sort of feature, so calling this flag --upgrade-all would probably confuse people, and make it harder to give the intuitive name to a future "upgrade all" command.

kennethreitz · 2012-07-02T05:33:30Z

Ah, good point.

qwcode · 2012-07-02T05:42:40Z

travisbot passing all the tests (and seeing that the commit from #fdintino didn't add or alter any tests) would imply that there was no previous coverage on this feature.

tests/test_upgrade.py seems to be all about single distributions w/o dependencies.

this would certainly be the time to add some tests related to recursion.

ejucovy · 2012-07-02T06:23:27Z

I've forked fdintino's branch to add tests: ejucovy/pip@fdintino:fix-recursive-upgrades...ejucovy:fix-recursive-upgrades

The first new test, test_upgrade_without_unneeded_recursive_upgrades, will run and fail on pip presently. It passes on fdintino's branch.

The second new test passes in both cases. The third new test tests the new --upgrade-recursive feature.

ejucovy · 2012-07-02T13:51:33Z

Nice catch, @qwcode. You're right, the implementation in this pull request breaks pip install --upgrade reqs.txt. That command should upgrade all packages explicitly listed in reqs.txt, but with this patch it only upgrades the first package listed and ignores the subsequent lines.

I've added an additional test for this behavior on my fork of fdintino's branch: ejucovy@e192a41

The last assertion in my new test fails on the branch. The same assertion passes on develop, so this feature isn't ready to be merged.

fdintino · 2012-07-02T15:45:18Z

Thanks for the unit tests! I was getting concerned by the lack of feedback. I'll be away from a computer until this evening (EDT) but I'll fix this when I get back home.

Would the pip maintainers prefer if I merge commits where possible (having two commits, one from me and one from ejucovy) or does that not matter? I know that some projects prefer to have pull requests with as few commits as possible.

Thanks.

fdintino · 2012-07-03T20:10:50Z

I added an is_subrequirement property to InstallRequirement and a method is_upgradeable() to RequirementSet:

def is_upgradeable(self, requirement):
    if requirement.is_subrequirement and not requirement.is_extra:
        return self.upgrade_recursive
    else:
        return self.upgrade

InstallRequirement.is_subrequirement evaluates to True if InstallRequirement.comes_from is an instance of InstallRequirement. I assumed that we would want packages defined with the "extra" syntax (e.g. "packagefoo[packagebar]") to upgrade with the non-recursive command, so I added an is_extra attribute to InstallRequirement to keep track of whether a dependent requirement came from an extras definition.

travisbot · 2012-07-03T20:25:09Z

This pull request fails (merged daaff403 into 5562a6b).

travisbot · 2012-07-03T21:26:20Z

This pull request passes (merged ed95a70 into 5562a6b).

fdintino · 2012-07-04T00:36:31Z

In case anyone is confused by the above travisbot messages, the first test failed because of a timeout on the travis server, so I force pushed with a different commit timestamp to initiate another travis build.

fdintino · 2012-07-06T21:43:02Z

Let me know if there's anything else I can do to help get this pulled in.

carljm · 2012-07-10T00:06:38Z

pip/req.py

@@ -1096,6 +1113,12 @@ def prepare_files(self, finder, force_root_egg_info=False, bundle=False):
            finally:
                logger.indent -= 2

+    def is_upgradeable(self, requirement):
+        if requirement.is_subrequirement and not requirement.is_extra:


Why are extras treated differently from other dependencies (always recursively upgraded)?

At the time it made sense... I think my reasoning was that if you're supplying the extra alongside the package when issuing the upgrade command, then the expected behavior would be for the same upgrade logic to apply to both the base package and the extra packages, otherwise you wouldn't have used the extra specifier; e.g. why would you issue this command:

pip install -U jinja2[i18n]

and not this:

pip install -U jinja2

On the other hand, I'm not sure the extras make much sense in this context, so I'm not particularly attached to this line of reasoning and wouldn't be opposed to taking it out.

Is there a verdict on this? Keep it or not?

On further consideration, I think this is assuming too much. I'll take it out.

fdintino · 2012-07-13T22:38:47Z

I integrated the above comments about the tests and is_upgradeable into the latest pull request.

travisbot · 2012-07-13T22:46:48Z

This pull request fails (merged 679641d5 into c6789f6).

qwcode · 2012-07-13T23:02:11Z

@pnasrat , do you understand the scenario that leads to this kind of travis failure?

travisbot · 2012-07-13T23:05:08Z

This pull request passes (merged ba12e28 into c6789f6).

fdintino · 2012-07-13T23:07:56Z

I think it was because I force-pushed while it was starting to check it out, and the commit no longer existed for the merge. I had a typo in @ejucovy's author info

qwcode · 2012-07-13T23:26:14Z

btw, I was working on migrating tests last night for #598, and I'm pretty sure I found a bug that affects the current upgrade code pathway. I'll reconfirm and post a pull later this weekend, and reference this pull.

you're working hard on this. I'll look it over in detail over the weekend.

considering this is a compatibility breaking change, I think I might post something to the list requesting feedback.
I searched the list real quick and didn't see any recent conversation on that.
my sense is the most people would be in favor of this, but there may be people who argue strongly for keeping -U as it is, and adding something like --upgrade-nonrecursive

qwcode · 2012-07-16T06:43:27Z

@fdintino , ended up using what free time I had to sort thru #604. I'll look tomorrow. If it's real, I'm inclined to think #604 should block this pull, just because it shakes my confidence in some of the core logic underlying the upgrade.

qwcode · 2012-07-17T02:55:58Z

pip/req.py

                        req_to_install.conflicts_with = req_to_install.satisfied_by
                        req_to_install.satisfied_by = None
                    else:
                        install_needed = False
                if req_to_install.satisfied_by:
+                    if self.upgrade and not self.upgrade_recursive:


I don't follow this logic?
it seems like all we need to notify is this:
'Requirement already satisfied (use --upgrade to upgrade): %s' % req_to_install)
and if so, you don't need to pass in in upgrade as a property of RequirementSet, just upgrade_recursive

I believe this message displays after any installation where the requirement is currently satisfied, not just upgrades. So if you performed

pip install Django

and you had Django 1.2.3 installed, you would see:

Requirement already satisfied (use --upgrade to upgrade): Django in [...]

whereas if Django was a dependency in install_requires that was satisfied but would be force-upgraded with --upgrade-recursive, it would instead display:

Requirement already satisfied (use --upgrade-recursive to upgrade): Django>1.2 in [...]

hmm, I take that message just as an FYI to the user about how to upgrade an individual package, that was satisfied during the current installation, not as a tip on how to redo the last installation command to get certain packages upgraded.

That makes sense. I suppose the reason I did it this way is that with the previous behavior a pip install --upgrade never presented this message, whereas now it can and it struck me as confusing that the user would be directed to perform a --upgrade during a --upgrade (the source of the confusion here being the ambiguity of the argument to pip install)

An upside of presenting the (use --upgrade-recursive to upgrade) notice is that it might ease confusion for people expecting the previous pip install -U behavior.

yeah, I see your pt of view. maybe a 3rd person can help decide what is more intuitive in this case.

it would be nice to see the tests confirm the exptected notifications

fdintino · 2012-11-20T17:46:48Z

The pypy build failed on travis, but it doesn't look related to this commit. The message is a bit confusing, almost looks like a travis bug, but I'd need someone else to weigh in.

qwcode · 2012-11-20T18:34:04Z

pretty sure it's not related. I've seen that error pop up from time to time on other pypy tests. wish I knew how to guard against it once and for all.

qwcode · 2012-11-24T08:42:46Z

@pnasrat : any thoughts on this one? it's a merge candidate I think, but needs multiple maintainers giving it a go, since It changes the behavior of -U. Not something to take lightly.

qwcode · 2012-11-24T08:43:43Z

btw, I sent an email to the list months ago, asking if anyone had concerns with this change, and got no replies.

pnasrat · 2012-11-24T10:46:36Z

I'd say lets hold off on behaviour changes until we manage to review the command rework. Then we should come give some thought to the user experience and the right commands and options to make that happen. Then that would go towards a 2.0

fdintino · 2012-11-24T15:44:50Z

Just to make sure we're all on the same page, this isn't simply a reworking of the command, it fixes a long-standing bug with the behavior of recursive upgrades. The current behavior makes pip unworkable for certain workflows at The Atlantic, and judging from the ticket this is intended to fix we aren't alone in this regard.

qwcode · 2012-11-24T17:13:12Z

@fdintino , as for pip being "unworkable for certain workflows", I wrote up a gist a while back for your scenario.
see this comment: #304 (comment)
I remember we went back and forth on this, but I don't think you ever responded to my last iteration on this.
imo, pip can handle this workflow as is, but involves 2 steps that aren't obvious.

qwcode · 2012-11-24T17:21:33Z

@pnasrat , by "command rework", you're talking about #721? you think this should hinge/hold on that?

fdintino · 2012-11-24T21:00:57Z

@qwcode: I hadn't seen that. It's counterintuitive, but that is helpful. As a rule, if we're using the unpatched pip, we always use --no-deps when calling --upgrade, otherwise we're liable to end up with Django 1.5 alpha (because something has install_requires=['Django>1.2']), and who knows what else. Occasionally a developer will forget to use --no-deps, they start having issues with their install, and time is wasted until the problem is diagnosed back to having run --upgrade without --no-deps. This is why we decided it was worth spending company time working on fixing this issue.

I know you were just speculating, but to me #721 seems unrelated, since that seems to concern the apis in pip/commands/*, whereas this ticket is a modification to the behavior in pip/req.py.

fdintino · 2012-12-13T00:45:15Z

Updates?

fdintino · 2012-12-13T01:19:29Z

Specifically, I notice that #721 was closed, and this still passes the tests.

qwcode · 2012-12-13T23:14:32Z

@fdintino I appreciate your persistence on this. I think this pull is worthy, but it's going to need other maintainers giving it reviews and thumbs up, since it's a major change.

qwcode · 2012-12-17T02:08:51Z

@fdintino here's a thought. take your understanding you've gained from working on this, and use it towards a new pip upgrade command (a la #59), and leave the old pip install -U as it is for now.

usage:

pip upgrade <options> <package> ...

--recursive    //upgrade <package> recursively
--all                //upgrade everything  (lot of people wanting this)

qwcode · 2012-12-17T02:18:10Z

@fdintino note that "pip list" (#752) is soon to be merged, which has an option for showing you what's out of date.

qwcode · 2012-12-17T16:45:46Z

@fdintino , fyi, I'm gonna work on "pip upgrade", and just try to get it done.

fdintino · 2012-12-17T23:38:37Z

Okay, I'd be willing to take a stab at it if you would like.

qwcode · 2012-12-18T07:18:20Z

ok, thanks, I'll see how my time goes. I'll include you for review on anything I do.

qwcode · 2012-12-18T07:36:52Z

and btw, if "pip upgrade --recursive" makes it in under my pull , I'll give you and ejucovy mucho credit. : )

qwcode · 2013-02-10T04:28:37Z

hey, @fdintino, sorry, the reality of trying to get v1.3 tested and ready (and the latest effort to get ssl cert checking into pip) has stalled me on this. if you want to start on the pip upgrade idea, feel free and let me know.

I'm closing this in its current form, because I'm pretty keen on adding this via pip upgrade, and leave pip install --upgrade as it is, with an eventual deprecation.

btw, see the new current cookbook entry on how to do a non-recursive install using what pip offers now:
http://www.pip-installer.org/en/latest/cookbook.html#non-recursive-upgrades
If that seems wrong or confusing, let me know.

carljm reviewed Jul 10, 2012
View reviewed changes

qwcode mentioned this pull request Jul 16, 2012

fix version sort in PackageFinder #604

Closed

qwcode reviewed Jul 17, 2012
View reviewed changes

qwcode mentioned this pull request Dec 17, 2012

Add a "Upgrade all local installed packages" #313

Closed

qwcode closed this Feb 10, 2013

pv mentioned this pull request Mar 12, 2013

MAINT: add install_requires to setup.py scipy/scipy#465

Closed

fdintino referenced this pull request in theatlantic/django-ckeditor May 3, 2013

Smart, semantic exception handling for image resizing. refs #3379

835f1b2

qwcode mentioned this pull request Jun 30, 2013

pip API stability? #924

Closed

qwcode mentioned this pull request Jul 14, 2013

Should pip install -U (with no distributions mentioned) upgrade all out of date distributions? #1054

Closed

vbabiy mentioned this pull request Nov 22, 2013

Add "upgrade" and "upgrade-all" commands #59

Closed

rgommers mentioned this pull request Feb 23, 2014

Installation of scipy 0.13.3 via zc.buildout fails scipy/scipy#3379

Closed

gvalkov mentioned this pull request Mar 14, 2015

Add --install-options and --global-options to the requirements file parser. #2537

Merged

lock bot added the auto-locked Outdated issues that have been locked by automation label Jun 5, 2019

lock bot locked as resolved and limited conversation to collaborators Jun 5, 2019

Make -U non-recursive by default, add --upgrade-recursive option #571

Make -U non-recursive by default, add --upgrade-recursive option #571

Conversation

fdintino commented Jun 8, 2012

travisbot commented Jun 11, 2012

kennethreitz commented Jul 2, 2012

lepture commented Jul 2, 2012

kennethreitz commented Jul 2, 2012

ejucovy commented Jul 2, 2012

kennethreitz commented Jul 2, 2012

qwcode commented Jul 2, 2012

ejucovy commented Jul 2, 2012

ejucovy commented Jul 2, 2012

fdintino commented Jul 2, 2012

fdintino commented Jul 3, 2012

travisbot commented Jul 3, 2012

travisbot commented Jul 3, 2012

fdintino commented Jul 4, 2012

fdintino commented Jul 6, 2012

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fdintino commented Jul 13, 2012

travisbot commented Jul 13, 2012

qwcode commented Jul 13, 2012

travisbot commented Jul 13, 2012

fdintino commented Jul 13, 2012

qwcode commented Jul 13, 2012

qwcode commented Jul 16, 2012

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fdintino commented Nov 20, 2012

qwcode commented Nov 20, 2012

qwcode commented Nov 24, 2012

qwcode commented Nov 24, 2012

pnasrat commented Nov 24, 2012

fdintino commented Nov 24, 2012

qwcode commented Nov 24, 2012

qwcode commented Nov 24, 2012

fdintino commented Nov 24, 2012

fdintino commented Dec 13, 2012

fdintino commented Dec 13, 2012

qwcode commented Dec 13, 2012

qwcode commented Dec 17, 2012

qwcode commented Dec 17, 2012

qwcode commented Dec 17, 2012

fdintino commented Dec 17, 2012

qwcode commented Dec 18, 2012

qwcode commented Dec 18, 2012

qwcode commented Feb 10, 2013