-
Notifications
You must be signed in to change notification settings - Fork 3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add an upgrade-strategy option #3972
Conversation
@@ -224,6 +236,14 @@ def run(self, options, args): | |||
if options.build_dir: | |||
options.build_dir = os.path.abspath(options.build_dir) | |||
|
|||
allowed_strategies = ["eager", "non-eager"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we'll ever add any new ones but this list should probably be kept close to the code actually performing the upgrade-or-not decision and be visible from here.
Please suggest what would be the correct/appropriate place to put this list of allowed upgrade strategies?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could instead just set add choices=["eager", "non-eager"]
to the add_option
call above. It should handle the error handling for you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes! I was wondering if I'm missing something obvious there...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we'll ever add any new ones
--no-deps
can also be considered an upgrade strategy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
--no-deps
can also be considered an upgrade strategy.
For the sake of simplicity, not in this PR. ;P Maybe add a new issue for this?
Travis failed on nightly for reasons I don't understand and pep8 because the test names are too long. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like it's pretty close to being able to land.
@@ -224,6 +236,14 @@ def run(self, options, args): | |||
if options.build_dir: | |||
options.build_dir = os.path.abspath(options.build_dir) | |||
|
|||
allowed_strategies = ["eager", "non-eager"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could instead just set add choices=["eager", "non-eager"]
to the add_option
call above. It should handle the error handling for you.
cmd_opts.add_option( | ||
'--upgrade-strategy', | ||
dest='upgrade_strategy', | ||
default='non-eager', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I mentioned in #3871 (comment) I think we'll want this to default to eager
for this time and we'll make sure to file an issue to swap the default for the next X+1 release.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll push a commit for the same.
You can ignore the Python 3.6/nightly errors. That's not a problem with this branch. I'm talking to the Travis folks about getting an upgraded version of that available. Worst case I'll disable the matrix line. |
@dstufft Thanks for the review. I've made the changes that you suggested. PS: Considering the size of the patch, I really think this should be squashed and merged... |
Oh, and if this merges today, I have some time everyday for a week free to tackle any random issue. Any that comes to your head? |
I generally squash merge changes anyways yea. |
I need to get the 3.6 stuff fixed, going to poke at that now. If you have a chance to write a change log entry for this that'd be great. Otherwise I'll do it before I merge. |
I think that the build is fixed. Should I rebase? |
Go for it! |
Associate an upgrade-strategy with `pip install --upgrade`. The upgrade- strategy determines how upgrading of the dependencies. Provides 2 upgrade-strategies "eager" and "non-eager". "eager" upgrades dependencies regardless of whether they currently satisfy the new parent version-specification. "non-eager" upgrades dependencies if and only if they do not satisfy the new parent version-specification.
7c962d6
to
09fe699
Compare
Done! Build is passing. |
'--upgrade-strategy', | ||
dest='upgrade_strategy', | ||
default='eager', | ||
choices=['non-eager', 'eager'], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You may want to consider whether "non-eager" is clear to a user. The original proposal of "only-if-needed" is not much longer. That's what the pip docs call it now: https://pip.pypa.io/en/latest/user_guide/#only-if-needed-recursive-upgrade
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense. I'll make the change.
if best_installed: | ||
skip_reason = 'already up-to-date' | ||
elif self.upgrade_strategy == "non-eager": | ||
skip_reason = 'not upgraded as not directly required' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This gets appended to "Requirement already" instead of to "Requirement". Now it looks a bit weird: Requirement already not upgraded as not directly required
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and same two lines above, gives Requirement already already up-to-date
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I missed that line. 😅 I thought I had changed it...
elif self.upgrade_strategy == "non-eager": | ||
skip_reason = 'not upgraded as not directly required' | ||
else: | ||
skip_reason = 'already satisfied' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can't happen - if the above two clauses are false, then the latest version is not installed and strategy is eager. Hence can't skip but need to upgrade.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now that I think about it, yes. This is a need-to-upgrade. I don't know what reasoning I was applying earlier for this...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah... I did that because of the assert on line 484. And because I could never hit that branch in any of my testing earlier, for the previous PR.
Played around with this a little bit - behaves as expected. LGTM |
Thanks for the review @rgommers! |
The build broke for reasons not related to these changes (some 404 on hitting the web for generating docs). |
I restarted the doc build |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, one minor nit noted, which is fine left as is.
# For link based requirements we have to pull the | ||
# tree down and inspect to assess the version #, so | ||
# its handled way down. | ||
if not (self.force_reinstall or req_to_install.link): | ||
try: | ||
finder.find_requirement(req_to_install, self.upgrade) | ||
finder.find_requirement( | ||
req_to_install, upgrade_allowed) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Won't upgrade_allowed
always be true here (line 414)? I guess the same was true of self.upgrade
before, though...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wondered the same initially. Then I decided to keep it as it was with upgrade.
Weird, I reran the doc build twice, but it failed both times. I can fetch the offending file manually, though. Travis weirdness? |
Thanks for the review @pfmoore!
I think so... |
Nice ! Many thanks for this. |
Would it be useful to update a section of the documentation as updated here: https://github.com/pypa/pip/pull/3806/files#diff-0fcaa96196be65c5d918fd5099ea19a8 to mention this new option? |
that makes sense to me. Later on (1-2 years?) the comments on history can be removed again, but for now they will be helpful |
Made the PR: #4232 |
To prevent unnecessary upgrades of libraries. See pypa/pip#3972
To prevent unnecessary upgrades of libraries. See pypa/pip#3972
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. |
Resolves #3871
Associate an upgrade-strategy with
pip install --upgrade
. The upgrade-strategy determines how upgrading of the dependencies.
Provides 2 upgrade-strategies "eager" and "non-eager". "eager" upgrades
dependencies regardless of whether they currently satisfy the new parent
version-specification. "non-eager" upgrades dependencies if and only if
they do not satisfy the new parent version-specification.
PS: I've allowed write access to maintainers (there was this checkbox) on this branch but I don't think that would be necessary.
Pinging (sorry if this is noise): @njsmith @xavfernandez @pfmoore @dstufft @rbtcollins @rgommers @dholth
As before, I would like it if we discussed all the implementation related stuff on this PR and use #3871 for all the other related discussions.