rez-pip: Assume pip provided by python package #757
Conversation
| @@ -40,6 +42,15 @@ def command(opts, parser, extra_arg_groups=None): | |||
| p.wait() | |||
| return | |||
|
|
|||
| if opts.pip_ver: | |||
| with warnings.catch_warnings(): | |||
There was a problem hiding this comment.
I feel like we should just trigger the warning here, and leave it up to elsewhere to set the filter. Since this is CLI code, we could safely just set to always in cli._main or something. The reason I say is because there are a bunch of other places where deprecation warnings should go, that aren't necessarily in CLi code. In those cases you would not be wanting to set the warnings filter - that would be up to the host application to set.
We might also consider wrapping this with a utils.deprecation_warning() func.
There was a problem hiding this comment.
ps - I also think we should do this in a separate PR though, so happy to leave this as-is
There was a problem hiding this comment.
My thinking was that if we always let warnings be printed out, it could cause some noise. That's nice when doing development and when running tests, but it's not so nice for a user that just wants to use rez without getting warnings from other libs or python itself for example. That's why I haven't enabled them globally.
I also thought about a deprecation warning function but I wasn't sure if you would have like it since right now there is no deprecation policy in Rez (For example, see the policy of pip https://pip.pypa.io/en/stable/development/release-process/#deprecation-policy), as everything as to stay backward compatible... Introducing deprecation warnings would mean we would start to really deprecate things and break backward compatibility in some places... So I wasn't sure. But if we are fine with this (I am fully fine with breaking compatibility when necessary and when well documented with proper error messages/warnings that points to the right documentation), I'm all for a common function.
|
Fair enough that enabling all warnings in rez cli tools may not be the best
idea. But yeah it'd also be a good idea to push into a utility function.
The problem I see is that, if you're in non-cli code then you shouldn't be
mucking about with warnings filters (same idea as python logging - apps
should control logging settings, not the libs doing the logging). And yet
at the same time, we _do_ want to force these warnings on when in cli, as
in your code above. But it would be nice to have just the one
deprecation_warning() function that works correctly regardless of where it
gets run from (ie via rez cli tool, or something else using the rez API).
In order to do that, we need some way of knowing if a rez cli tool is
currently being used. If it's not already there, it should be trivial to
add some env var (_REZ_CLI_TOOL or something), so then
deprecation_warning() can do what it needs to. Ie, always print the warning
if a CLI tool is active, or just call warnings.warn if not.
…On Tue, Oct 15, 2019 at 12:33 PM Jean-Christophe Morin < ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In src/rez/cli/pip.py
<#757 (comment)>:
> @@ -40,6 +42,15 @@ def command(opts, parser, extra_arg_groups=None):
p.wait()
return
+ if opts.pip_ver:
+ with warnings.catch_warnings():
My thinking was that if we always let warnings be printed out, it could
cause some noise. That's nice when doing development and when running
tests, but it's not so nice for a user that just wants to use rez without
getting warnings from other libs or python itself for example. That's why I
haven't enabled them globally.
I also thought about a deprecation warning function but I wasn't sure if
you would have like it since right now there is no deprecation policy in
Rez (For example, see the policy of pip
https://pip.pypa.io/en/stable/development/release-process/#deprecation-policy),
as everything as to stay backward compatible... Introducing deprecation
warnings would mean we would start to really deprecate things and break
backward compatibility in some places... So I wasn't sure. But if we are
fine with this (I am fully fine with breaking compatibility when necessary
and when well documented with proper error messages/warnings that points to
the right documentation), I'm all for a common function.
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub
<#757>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAMOUSW2MRAQXK6QNX5UJMTQOUMWLANCNFSM4I5T6SZQ>
.
|
JeanChristopheMorinPerso
changed the title
JeanChristopheMorinPerso
force-pushed
the
issue_706
branch
from
9faf32d to
4f62d85
Compare
|
@nerdvegas I added an entry in the changelog. You will just need to update the date for the release (I had to rebase too). |
|
GH Actions (from my fork): https://github.com/JeanChristopheMorinPerso/rez/runs/261669537 |
|
Though I haven't updated the version in |
|
@JeanChristopheMorinPerso I notice you're using python's |
|
@nerdvegas |
This PR closes #706 and closes #764
This PR adds a new logic to find which
pipwill be used to install pip packages.The logic is as follow:
pythonpackage specified with--python-version, orthe latest version if not specified;
pippackage specified with--pip-version,or latest version if not specified.
pipis found, use it;I also added a deprecation warning when
--pip-versionis used.Lastly, I added a wiki page to explain what is
rez-pipand how it really works under the hood. This, I hope, will save us some questions and will help us remember what are the good practices when it comes to pip + rez.I've tested this with most (if not all) scenarios, but I invite everybody to also try on their side to verify that it works for everybody in all sort of environment (windows, osx, linux, etc).