Pip improvements #667

nerdvegas · 2019-07-19T05:57:28Z

Our existing pip integration had some fairly major shortcomings, hopefully this PR addresses many of them.

Main issues addressed:

Conversion of pip version specifiers to equivalent rez Requirement is more robust
Variant requirements are correctly constructed based on env markers if present
Extras are properly managed
Requirements are properly filtered out based on extras and env markers
More robustly manages mix of PEP426 and PEP528 requirements specs
Utility pip-related code has been moved to a separate utils/pip.py file

-much better pip->rez requirement conversion -much better pip->rez specifier conversion -much better management of extras and env markers -more accurate variants, which include reqs that they should -'packaging' vendor lib imports updated (were broken) -'packaging' lib used more widely -correctly manage differing cases of extras usage

instinct-vfx · 2019-07-19T08:19:27Z

I am working on a test-suite to test with lots of packages. Already found a kind of peculiar issue.
Installing pytest fails because the variant contains < which is one of the redirect characters in cmd.exe.
My main question would be the actual variant though as it creates a single variant with the following packages:

variants = \
   [['python-2.7',
     'funcsigs-1.0+',
     'pathlib2-2.2.0+',
     'more_itertools-4.0.0+<6.0.0']]

While the resulting path is only invalid on windows it seems, the variant still does not really make sense to me on first sight. Needs more investigation. The shell error may be solved by hashed variants i guess?

instinct-vfx · 2019-07-19T08:42:40Z

Looking at the package (thanks @lambdaclan for the support on that one!) It seems the variant is indeed correct.
i just gave hashed variants a try by adding pkg.hashed_variants = True to rez pip here: https:/nerdvegas/rez/blob/61e0a641b91b68fb8dca449c69c196c9f354328c/src/rez/pip.py#L324

That seems to work fine for pytest (and also for using the resulting package).

lambdaclan · 2019-07-19T08:46:13Z

Adding some info for reference (pytest)

dist-info metadata

Requires-Python: >=2.7, !=3.0., !=3.1., !=3.2., !=3.3.
Requires-Dist: py (>=1.5.0)
Requires-Dist: six (>=1.10.0)
Requires-Dist: packaging
Requires-Dist: attrs (>=17.4.0)
Requires-Dist: atomicwrites (>=1.0)
Requires-Dist: pluggy (<1.0,>=0.12)
Requires-Dist: importlib-metadata (>=0.12)
Requires-Dist: wcwidth
Requires-Dist: funcsigs (>=1.0) ; python_version < "3.0"
Requires-Dist: pathlib2 (>=2.2.0) ; python_version < "3.6"
Requires-Dist: more-itertools (<6.0.0,>=4.0.0) ; python_version <= "2.7"
Requires-Dist: more-itertools (>=4.0.0) ; python_version > "2.7"
Requires-Dist: colorama ; sys_platform == "win32"
Provides-Extra: testing
Requires-Dist: argcomplete ; extra == 'testing'
Requires-Dist: hypothesis (>=3.56) ; extra == 'testing'
Requires-Dist: nose ; extra == 'testing'
Requires-Dist: requests ; extra == 'testing'
Requires-Dist: mock ; (python_version == "2.7") and extra == 'testing'

Rez pip now handles these:

Requires-Dist: funcsigs (>=1.0) ; python_version < "3.0"
Requires-Dist: pathlib2 (>=2.2.0) ; python_version < "3.6"
Requires-Dist: more-itertools (<6.0.0,>=4.0.0) ; python_version <= "2.7"
Requires-Dist: more-itertools (>=4.0.0) ; python_version > "2

and creates the variants accordingly but it leads to invalid paths unless hashed variants are used.

setup.py

INSTALL_REQUIRES = [
   "py>=1.5.0",
   "packaging",
   "attrs>=17.4.0",
   "more-itertools>=4.0.0",
   "atomicwrites>=1.0",
   'pathlib2>=2.2.0;python_version<"3.6"',
   'colorama;sys_platform=="win32"',
   "pluggy>=0.12,<1.0",
   "importlib-metadata>=0.12",
   "wcwidth",
]

generated package

# -- coding: utf-8 --
name = u'pytest'
version = '4.6.4'
description = \
   """
   pytest: simple powerful testing with Python
   """
tools = [
   u'py.test',
   u'pytest'
]
requires = [
   'atomicwrites-1.0+',
   'attrs-17.4.0+',
   'wcwidth',
   'six-1.10.0+',
   'packaging',
   'importlib_metadata-0.12+',
   'py-1.5.0+',
   'pluggy-0.12+<1.0'
]
variants = \
   [['python-2.7',
     'funcsigs-1.0+',
     'pathlib2-2.2.0+',
     'more_itertools-4.0.0+<6.0.0']]
def commands():
   env.PYTHONPATH.append('{root}/python')
   env.PATH.append('{root}/bin')
timestamp = 1563523950
format_version = 2

nerdvegas · 2019-07-19T09:44:38Z

Yeah I expected this, didn't get a chance to mention. So, hashed variants do work currently. The only downside is that the variant 'handle' used to define the variant is still integer-based, but that needs to be updated so it's based on the hash instead. It's still ok-ish as it is today, but it'd be more robust under all conditions if it were hash-based. This is a feature I need to get to soon. I wonder if we should just enabled hashed variants on rez-pipified packages. Thorsten if you've tested this and it seems to work, perhaps it's good to go. It would solve the variant problem.. A

…

On Fri, Jul 19, 2019 at 6:46 PM Ira ***@***.***> wrote: Adding the dist-info metadata for reference (pytest) Requires-Python: >=2.7, !=3.0., !=3.1., !=3.2., !=3.3. Requires-Dist: py (>=1.5.0) Requires-Dist: six (>=1.10.0) Requires-Dist: packaging Requires-Dist: attrs (>=17.4.0) Requires-Dist: atomicwrites (>=1.0) Requires-Dist: pluggy (<1.0,>=0.12) Requires-Dist: importlib-metadata (>=0.12) Requires-Dist: wcwidth Requires-Dist: funcsigs (>=1.0) ; python_version < "3.0" Requires-Dist: pathlib2 (>=2.2.0) ; python_version < "3.6" Requires-Dist: more-itertools (<6.0.0,>=4.0.0) ; python_version <= "2.7" Requires-Dist: more-itertools (>=4.0.0) ; python_version > "2.7" Requires-Dist: colorama ; sys_platform == "win32" Provides-Extra: testing Requires-Dist: argcomplete ; extra == 'testing' Requires-Dist: hypothesis (>=3.56) ; extra == 'testing' Requires-Dist: nose ; extra == 'testing' Requires-Dist: requests ; extra == 'testing' Requires-Dist: mock ; (python_version == "2.7") and extra == 'testing' Rez pip now handles these: Requires-Dist: funcsigs (>=1.0) ; python_version < "3.0" Requires-Dist: pathlib2 (>=2.2.0) ; python_version < "3.6" Requires-Dist: more-itertools (<6.0.0,>=4.0.0) ; python_version <= "2.7" Requires-Dist: more-itertools (>=4.0.0) ; python_version > "2 and creates the variants accordingly but it leads to invalid paths unless hashed variants are used. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#667>, or mute the thread <https:/notifications/unsubscribe-auth/AAMOUSXI7CTTWGLP22GFPRLQAF5NRANCNFSM4IFCTS2A> .

instinct-vfx · 2019-07-19T10:01:59Z

I am in the progress of testing this against a bigger set of pip packages, will report on methodology asap. I do see another weird error that i can not reliably reproduce or track down. When installing packages i sometimes get the following:

ERROR: importlib-metadata 0.18 requires configparser>=3.5; python_version < "3", which is not installed.
ERROR: pytest 4.6.3 requires colorama; sys_platform == "win32", which is not installed.
ERROR: arrow 0.14.1 requires backports.functools-lru-cache>=1.2.1; python_version == "2.7", which is not installed.

So far it has always been these three (or a subset). I am not always getting it or at least fail to find the packages calling this. Every time i call rez-pip only on the packages that seem to call this it works :(

If anyone has ideas or pointers that would be really welcome. (Windows of course).

nerdvegas · 2019-07-19T10:45:17Z

Note to self:

hey, so I found a problem. There was something odd in the original rez-pip code and I didn't know why it was there... now I know
8:32 PM
it seems that pip pkg names are non-case-sensitive... and there are cases where a package will list a required pkg with a name that is a different case to the pkg that pip downloads
8:32 PM
eg, Flask requires click, but the pkg pip downloads is actually Click

# Conflicts: # src/rez/utils/_version.py

nerdvegas · 2019-07-20T06:34:32Z

Fix for case sensitivity issues added

src/rez/utils/pip.py

nerdvegas · 2019-07-22T10:57:48Z

So I've been thinking about it.. unless there are any objections, I think I'm going to just enable hashed variants on all rez-pipified packages, since that solves the varianting issue (ie ranges such as >N ending up in the variant root path, now that we're correctly constructing variants).

I'll also add some custom attributes such as from_pip and is_pure_python, that's useful info that we may as well keep intact in the rez package.

instinct-vfx · 2019-07-22T11:02:05Z

That will also give hashed variants some thorough testing! I agree.

nerdvegas · 2019-07-22T12:14:04Z

You know it also just struck me that it's a bit of a feat that simply setting pkg.hashed_variants=True works at all. We have a completely different case where PackageMaker is getting used to create a package (rez-pip, whereas the main one is of course rez-build)... and yet everything is working the same. That's fortunate given what we're trying to do.

-use hashed variants in rez-pip -added some pip-specific metadata to package.py

JeanChristopheMorinPerso

So as promised, here is my code review. Hope my comments make sense.

JeanChristopheMorinPerso · 2019-07-20T17:39:09Z

src/rez/utils/pip.py

+ rez_req_str = pip_to_rez_package_name(packaging_req.name)
+
+ if packaging_req.specifier:
+ range_ = pip_specifier_to_rez_requirement(str(packaging_req.specifier))


packaging.requirements.Requirements.specifier returns a SpecifierSet that can be list'ed. That would save you the split at line https:/nerdvegas/rez/pull/667/files#diff-7dd5a0607b04e979c35f51b0122edf36R212

JeanChristopheMorinPerso · 2019-08-13T00:22:47Z

src/rez/pip.py

- requirements.extend(_get_dependencies(requirement, distributions))
-
+ # get list of package and dependencies
+ distributions = list(distribution_path.get_distributions())


This is already done at line 236. Or does it have to do with the "moving bin folder to expected relative location as per wheel RECORD files" ?

I think this was just missed after code reorg (moving stuff into utils.pip). Will fix

JeanChristopheMorinPerso · 2019-08-13T00:47:18Z

src/rez/utils/pip.py

+ if pkg_version.release:
+ # the components of the release segment excluding epoch or any
+ # prerelease/development/postrelease suffixes
+ rez_version += '.'.join(str(i) for i in pkg_version.release)


I think you can use the base_version attribute here, like pkg_version.base_version.

>>> packaging.version.parse('1.0.3rc2.post345.dev456+ubuntu-1').base_version '1.0.3'

JeanChristopheMorinPerso · 2019-08-13T01:20:08Z

src/rez/utils/pip.py

+ `VersionRange`: Equivalent rez version range.
+ """
+ ranges = []
+ parts = specifier.split(',')


If you passed the SpecifierSet as mentioned lower, you could simply do list(specifier). packaging will take care of removing all spaces.

Also, by passing the specifier set object, I think we could benefit from the packaging API here. For example, list on a SpecifierSet returns specifiers, from which you can get the operator with .operator and the version with .version. From the version we can do packaging.version.Version(v) and then use the methods to know if it's a pre/post release for example. We might still have to do some endswith but at least we will do way less than we do right now.

I'm getting to this soon - yeah I'll redo this using native SpecifierSet as it looks like that'll simplify things

JeanChristopheMorinPerso · 2019-08-13T01:55:49Z

src/rez/utils/pip.py

+ if "Windows" in platform.system():
+ # https:/nerdvegas/rez/pull/659
+ ver_str = subprocess.check_output(
+ pip_exe + " -V",


We might want to do python -m pip instead. That way we could be sure which pip will be used. I would even go one step further and use sys.executable to get the path to the correct python (only in the case where we use the system's pip of course).

This code needs to be updated actually.. currently it causes pip to be run from outside of its rez context (if rezified pip was found), which is completely wrong. I'm gonna fix this quite soon so ignore this for now. I'll make it part of this PR though.

On that note, sys.executable would only get used if no rezified pip packages was found (but yes I agree that it should be used in that case, and that pip exe shouldn't be used directly).

JeanChristopheMorinPerso · 2019-08-13T02:02:46Z

src/rez/utils/pip.py

+
+ for req in reqs:
+ # skip if env marker is present and doesn't evaluate
+ if req.marker and not req.marker.evaluate():


I have to admit that I'm confused here. Tell me if I have understood everything:

At this point, the extras have been removed from the marker. So the marker should only contain a python version and a platform?

The marker can refer to a number of things - platform, python, os (see https://www.python.org/dev/peps/pep-0508/#id24). I'm not sure what you are getting at though? The marker might specify python-3, and we might be running python-2.7, in which case the req should be skipped.

In any case, I think I see a problem - the marker is going to get evaluated wrt current python interpreter, even though a rezified pip may have been used (and thus, a potentially different python version). We do avoid a lot of potential problems if the system pip (ie the one rez gets installed with) is only ever used, but then you're restricted to rez-pipping only for that version of python.

We could override the python version all the time with the one we are running under or the one specified when rez-pip was invoked. So basically always do:

req.marker.evaluate( environment={ "python_full_version": str(python_version), "python_version": str(python_version.trim(2)), "implementation_version": str(python_version) } )

Yeah you read my mind, was hoping it was possible to do this way and looks like it is, cheers.

JeanChristopheMorinPerso · 2019-08-13T02:07:51Z

src/rez/utils/pip.py

+ if "os" in sys_requires:
+ sys_variant_requires.append("os-%s" % _system.os)
+
+ if "python" in sys_requires:


Is this if necessary?

I think it's cleaner to have it here, even though python is always a requirement. Then all the code that deals with converting a simple list of system requirements, into a more specific list, is all in the one place. Especially as later, it may get more complex and may not just default to python-MAJOR.MINOR.

JeanChristopheMorinPerso · 2019-08-15T00:19:03Z

src/rez/utils/pip.py

+
+ rez_version = ""
+
+ if pkg_version.release:


This condition cannot be false right?

I believe it can be, for versions like (eg) 1.a2, which is a pre-release, not a release.

Actually, this can only happen if the version is a legacy version, like a2 or 2.0b1pl0. If it's 1.a2, the release would be 1. But that also means it would need the allow_legacy=True, which mean it would never reach this line. See https:/pypa/packaging/blob/master/packaging/version.py#L92

ah yup I see, removing

JeanChristopheMorinPerso · 2019-08-15T01:06:39Z

src/rez/utils/pip.py

+
+ raise PackageRequestError(
+ "Don't know how to convert PEP440 specifier %r into rez equivalent",
+ specifier


This gives an error: TypeError: __init__() takes at most 2 arguments (3 given). It looks like you use the "lazy logger" technic her, which doesn't work for exceptions 😛 .

Fixed in #693

JeanChristopheMorinPerso · 2019-08-19T02:13:59Z

src/rez/utils/pip.py

+
+ # the components of the release segment excluding epoch or any
+ # prerelease/development/postrelease suffixes
+ rez_version = pkg_version.base_version


Umm, I'm sorry but that introduces a bug. I thought the base version would be enough, but it includes the epoch... So we will need to go back to the previous version of the code. Sorry about that.

nerdvegas · 2019-08-19T02:18:00Z

Ah cheers, haven't rerun the tests yet, thx

…

On Mon, Aug 19, 2019 at 12:14 PM Jean-Christophe Morin < ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/rez/utils/pip.py <#667 (comment)>: > + if isinstance(pkg_version, packaging_LegacyVersion): + if allow_legacy: + print_warning( + "Invalid PEP440 version detected: %r. Reverting to legacy mode.", + pkg_version + ) + # this will always be the entire version string + return pkg_version.base_version.lower() + else: + raise packaging_InvalidVersion( + "Version: {} is not compatible with PEP440.".format(dist_version) + ) + + # the components of the release segment excluding epoch or any + # prerelease/development/postrelease suffixes + rez_version = pkg_version.base_version Umm, I'm sorry but that introduces a bug. I thought the base version would be enough, but it includes the epoch... So we will need to go back to the previous version of the code. Sorry about that. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#667>, or mute the thread <https:/notifications/unsubscribe-auth/AAMOUSSEJYGIHP5RJ6IXYA3QFH6WVANCNFSM4IFCTS2A> .

JeanChristopheMorinPerso · 2019-08-19T02:18:08Z

src/rez/utils/pip.py

+ "sys.platform": [_plat],
+ "sys_platform": [_plat],
+
+ "os.name": [_os],


Thinking about it even more, os.name can return posix (Linux, OSX, etc), nt (Windows) or java (Jython). We can't do much about Jython, but for the others, it's basically the platform, not the OS (OS would add a dependency on the arch).

Do you know of a reference anywhere that lists valid os.name values? I'll
update this with a comment.

They are, here: https://docs.python.org/3/library/os.html#os.name
https://docs.python.org/3/library/platform.html#platform.system

These are the values I know that are mentioned in the docs.

nerdvegas · 2019-08-19T02:22:43Z

Do you know of a reference anywhere that lists valid os.name values? I'll update this with a comment.

…

On Mon, Aug 19, 2019 at 12:18 PM Jean-Christophe Morin < ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/rez/utils/pip.py <#667 (comment)>: > + _os = "os" + + sys_requires_lookup = { + # TODO There is no way to associate a python version with its implementation + # currently (ie CPython etc). When we have "package features", we may be + # able to manage this; ignore for now + "implementation_name": [_py], + "implementation_version": [_py], + "platform_python_implementation": [_py], + "platform.python_implementation": [_py], + "python_implementation": [_py], + + "sys.platform": [_plat], + "sys_platform": [_plat], + + "os.name": [_os], Thinking about it even more, os.name can return posix (Linux, OSX, etc), nt (Windows) or java (Jython). We can't do much about Jython, but for the others, it's basically the platform, not the OS (OS would add a dependency on the arch). — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#667>, or mute the thread <https:/notifications/unsubscribe-auth/AAMOUSSMPBXALZWUI3LG3LTQFH7GFANCNFSM4IFCTS2A> .

nerdvegas · 2019-08-19T02:34:13Z

Pls ignore.. I see that it maps to python os.name. A

…

On Mon, Aug 19, 2019 at 12:22 PM Allan Johns ***@***.***> wrote: Do you know of a reference anywhere that lists valid os.name values? I'll update this with a comment. On Mon, Aug 19, 2019 at 12:18 PM Jean-Christophe Morin < ***@***.***> wrote: > ***@***.**** commented on this pull request. > ------------------------------ > > In src/rez/utils/pip.py > <#667 (comment)>: > > > + _os = "os" > + > + sys_requires_lookup = { > + # TODO There is no way to associate a python version with its implementation > + # currently (ie CPython etc). When we have "package features", we may be > + # able to manage this; ignore for now > + "implementation_name": [_py], > + "implementation_version": [_py], > + "platform_python_implementation": [_py], > + "platform.python_implementation": [_py], > + "python_implementation": [_py], > + > + "sys.platform": [_plat], > + "sys_platform": [_plat], > + > + "os.name": [_os], > > Thinking about it even more, os.name can return posix (Linux, OSX, etc), > nt (Windows) or java (Jython). We can't do much about Jython, but for > the others, it's basically the platform, not the OS (OS would add a > dependency on the arch). > > — > You are receiving this because you were mentioned. > Reply to this email directly, view it on GitHub > <#667>, > or mute the thread > <https:/notifications/unsubscribe-auth/AAMOUSSMPBXALZWUI3LG3LTQFH7GFANCNFSM4IFCTS2A> > . >

…ip-improvements

-minor code redundancy removal

# Conflicts: # src/rez/utils/_version.py

nerdvegas · 2019-08-19T03:25:53Z

Ok, I've merged in those tests and updated wrt PR changes here. Everything looks good. Give it a whirl and let me know how you go. If there aren't any problems ten I'll merge to master tomorrow.

Comprehensive tests btw! Would also be good to have a rez-pip install happen from the test installed_distributions dir also; you mention this should be possible but I don't know the details just yet.

Thx
A

instinct-vfx · 2019-08-19T14:43:05Z

I have a first issue here. Have yet to investigate, but if i rez-pip install pip as a package rez-pip failes afterwards:

16:33:16 INFO     Using pip-19.2.2 (c:\users\tkaufmann\rez_packages_local\pip\19.2.2\package.py[0])
Traceback (most recent call last):
  File "C:\Python27\Lib\runpy.py", line 174, in _run_module_as_main
    "__main__", fname, loader, pkg_name)
  File "C:\Python27\Lib\runpy.py", line 72, in _run_code
    exec code in run_globals
  File "c:\ProgramData\Mackevision\rez_local_development\rez\pip-improvements2\platform-windows\Scripts\rez\rez-pip.exe\__main__.py", line 7, in <module>
  File "c:\programdata\mackevision\rez_local_development\rez\pip-improvements2\platform-windows\lib\site-packages\rez\cli\_entry_points.py", line 187, in run_rez_pip
    return run("pip")
  File "c:\programdata\mackevision\rez_local_development\rez\pip-improvements2\platform-windows\lib\site-packages\rez\cli\_main.py", line 148, in run
    returncode = run_cmd()
  File "c:\programdata\mackevision\rez_local_development\rez\pip-improvements2\platform-windows\lib\site-packages\rez\cli\_main.py", line 140, in run_cmd
    return opts.func(opts, opts.parser, extra_arg_groups)
  File "c:\programdata\mackevision\rez_local_development\rez\pip-improvements2\platform-windows\lib\site-packages\rez\cli\pip.py", line 47, in command
    release=opts.release)
  File "c:\programdata\mackevision\rez_local_development\rez\pip-improvements2\platform-windows\lib\site-packages\rez\pip.py", line 194, in pip_install_package
    py_exe, context = find_pip(pip_version, python_version)
  File "c:\programdata\mackevision\rez_local_development\rez\pip-improvements2\platform-windows\lib\site-packages\rez\pip.py", line 115, in find_pip
    if int(pip_major) < 19:
ValueError: invalid literal for int() with base 10: ''

JeanChristopheMorinPerso · 2019-08-19T14:51:42Z

For me, having pip as a rez package should be prohibited as it's actually searching for quite a lot of trouble...

But anyway. Did you get this error while running rez-pip install pip or it worked when you did rez-pip install pip?

instinct-vfx · 2019-08-19T18:49:19Z

rez-pip install pip worked fine, but all subsequent calls to rez-pip then fail. It tries to find_pip and fails when pip is present as a rez package it seems.

I wonder though: Don't we need pip as a rez package in order to allow installing python-3.x packages with rez-pip?

nerdvegas · 2019-08-19T22:50:23Z

Hmm, so I've rez-pip'd pip before, and used the resulting pip, and it's worked. I'll have a look at this. Wrt whether this makes sense - absolutely it does. Whatever version of python/pip rez itself has been installed with should be incidental, it should have no bearing on how other packages are getting built. So whilst it's convenient to pip-install packages using python-2.7, and rez's 2.7-based pip, it's also completely feasible that you'd want to pip-install a python-3.x package... and vice versa. A

…

On Tue, Aug 20, 2019 at 4:49 AM Thorsten Kaufmann ***@***.***> wrote: rez-pip install pip worked fine, but all subsequent calls to rez-pip then fail. It tries to find_pip and fails when pip is present as a rez package it seems. I wonder though: Don't we need pip as a rez package in order to allow installing python-3.x packages with rez-pip? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#667>, or mute the thread <https:/notifications/unsubscribe-auth/AAMOUSXNWTBLBMQSP43GCGLQFLTLDANCNFSM4IFCTS2A> .

nerdvegas · 2019-08-19T23:15:52Z

I can `rez-pip -i pip`, then do further `rez-pip -i X`. It uses the rezified pip and everything seems fine. Perhaps it's a Windows issue... what happens specifically? A

…

On Tue, Aug 20, 2019 at 8:50 AM Allan Johns ***@***.***> wrote: Hmm, so I've rez-pip'd pip before, and used the resulting pip, and it's worked. I'll have a look at this. Wrt whether this makes sense - absolutely it does. Whatever version of python/pip rez itself has been installed with should be incidental, it should have no bearing on how other packages are getting built. So whilst it's convenient to pip-install packages using python-2.7, and rez's 2.7-based pip, it's also completely feasible that you'd want to pip-install a python-3.x package... and vice versa. A On Tue, Aug 20, 2019 at 4:49 AM Thorsten Kaufmann < ***@***.***> wrote: > rez-pip install pip worked fine, but all subsequent calls to rez-pip > then fail. It tries to find_pip and fails when pip is present as a rez > package it seems. > > I wonder though: Don't we need pip as a rez package in order to allow > installing python-3.x packages with rez-pip? > > — > You are receiving this because you were mentioned. > Reply to this email directly, view it on GitHub > <#667>, > or mute the thread > <https:/notifications/unsubscribe-auth/AAMOUSXNWTBLBMQSP43GCGLQFLTLDANCNFSM4IFCTS2A> > . >

instinct-vfx · 2019-08-20T07:19:42Z

I did a bit of research. We have a central python that is added to path through a wrapper. What happens is that rez falls back to this python executable when in a context. So if it finds the package, it somehow picks up the wrong python on the PATH. I will investigate further. If i wipe this central python from PATH it works as expected (note though that the central python is on path AFTER the local one).

instinct-vfx · 2019-08-20T11:09:47Z

Okay, so the problem is related to system PATH vs. user PATH on windows. This only breaks if there is an additional python on the system path it seems. So for THIS ticket it can be considered out of scope. I will make an issue to track this down as it seems to be more of a general cmd issue.

JeanChristopheMorinPerso · 2019-08-20T13:34:02Z

I wonder though: Don't we need pip as a rez package in order to allow installing python-3.x packages with rez-pip?

In a normal python environment, pip is installed per python installation, either with --ensurepip (for python 2) when compiling python, by default in python 3 there is already pip installed, or by using the get-pip.py bootstrap file which by default will install in the python installation folder. All three options will also install setuptools and wheel and all three will install straight in the install directory by default. Of course, you can use get-pip.py with the --user option, though this can create problems if you have pip already installed in the python installation (corner cases, but still).

So to answer your question, if you would want to install python 3 packages, you would just have to call python3.X -m pip install ..., where python3.X is the version you specified with rez-pip --python-version 3.X install <package>. Doing this guarantees you which version of pip you use.

So whilst it's convenient to pip-install packages using python-2.7, and rez's
2.7-based pip, it's also completely feasible that you'd want to pip-install
a python-3.x package... and vice versa.

When I'm saying that we shouldn't need a pip rez package, I'm not saying that we shouldn't use rez-pip to install a python X package even if rez was installed with python Y. I'm just saying that it is entirely possible to do without a pip rez package and that I wouldn't recommend having pip as a rez package just because we cannot guarantee that it will work (since pip is not supposed to be installed with the --target option. Yes it's possible to do it, but it's a little bit an offroad track).

instinct-vfx · 2019-08-20T20:55:01Z

That would mean that rez-pip depends on specifically locally installed python interpreters that also need to have ensure pip. Wouldn't it be more stable/predictable to have a dependency on a standard pip package as installed by rez-pip itself?

instinct-vfx · 2019-08-20T21:04:10Z

I have finished my testing and have good news and bad news. Good news first:
Of the 1000 most downloaded pypi packages, 942 installed fine with rez-pip. I threw the remaining packages at pip directly and the majority also failed with pip itself (for various reasons, like requiring python 3.x or not providing binary wheels (at least for python27).

Here are the remaining ones that did work with pip but not rez-pip:

mysql-python
pycrypto
pyinotify
suds-jurko
tensorflow-serving-api
ujson

I have not done thorough investigations yet, but mysql-python is giving me a headache straight away. I called it the same way that rez-pip does (python -m pip install --use-pep517 --target PATH --no-deps) but rez-pip pulls a source release whereas pip pulls a wheel.

maxnbk · 2019-08-21T02:02:39Z

I can probably help with some suggestions for why some of these might fail.

Most likely, in some of these cases, you're seeing compilations fail, which are going to depend on finding an appropriate compiler on the host system, or appropriate flags to compile against other rezified pip or unrelated packages. Some also do not declare a dependency on a system-level devel library being installed, which is unfortunate.

There are also several packages which, in their setup.py, declare additional dependencies not a part of the pip-fetchable metadata. I can get a larger list of examples of these later, but a typical example is a need for vcversioner or setuptools_scm or pytest_runner.

In addition, we have a tool internally that has (at least until this work here can be more directly finished) supplanted rez-pip, which can be supplied an argument like "--links" that specifies packages to which CXXFLAGS / CFLAGS / LDFLAGS are meant to be appended at compile time, so that, for example, a rezified scipy can find and link to a rezified numpy at compile time, when building from a tarball and not just relying on wheels (The vast majority of our packages are compiled from source. Probably 99%+ of all packages that provide an sdist, as a direct result of this ability).

I have more ideas, but this is a good starter list of the most common reasons.

instinct-vfx · 2019-08-21T07:29:03Z

I am fine if they fail in pip, then failure of rez-pip is a given. Just the ones that fail in rez-pip but work in pip are a concern.

I could check a few more of the list. I started the test in a different shell and forgot to wipe PIP_INDEX_URL which made it pull internal wheels for some of these packages. With these off the list rez-pip installed all but 2 packages (!!),

The only ones that fail with rez-pip are

suds-jurko
tensorflow-serving-api

JeanChristopheMorinPerso · 2019-08-21T13:29:23Z

src/rez/pip.py

- context = None
- else:
- raise
+ # fall back on system pip


Before falling back to system (so virtualenv's python), I would try to get the latest python version (if python was not provided) available as a rez package and assume that there might be pip installed into it. If this fails too, then fallback to rez's virtualenv pip.

Would that make sense?

I believe my latest comment addresses this - as in yes we would change things to be expecting python package to provided pip, and would be checking for this before falling back to rezified pip, then to system pip (and rezified pip just for backwards compatibility - current users could very well have python without pip, and rezified pip).

nerdvegas · 2019-08-25T04:24:46Z

I wonder though: Don't we need pip as a rez package in order to allow installing python-3.x packages with rez-pip?

In a normal python environment, pip is installed per python installation, either with --ensurepip (for python 2) when compiling python, by default in python 3 there is already pip installed, or by using the get-pip.py bootstrap file which by default will install in the python installation folder. All three options will also install setuptools and wheel and all three will install straight in the install directory by default. Of course, you can use get-pip.py with the --user option, though this can create problems if you have pip already installed in the python installation (corner cases, but still).

So to answer your question, if you would want to install python 3 packages, you would just have to call python3.X -m pip install ..., where python3.X is the version you specified with rez-pip --python-version 3.X install <package>. Doing this guarantees you which version of pip you use.

So whilst it's convenient to pip-install packages using python-2.7, and rez's
2.7-based pip, it's also completely feasible that you'd want to pip-install
a python-3.x package... and vice versa.

When I'm saying that we shouldn't need a pip rez package, I'm not saying that we shouldn't use rez-pip to install a python X package even if rez was installed with python Y. I'm just saying that it is entirely possible to do without a pip rez package and that I wouldn't recommend having pip as a rez package just because we cannot guarantee that it will work (since pip is not supposed to be installed with the --target option. Yes it's possible to do it, but it's a little bit an offroad track).

Ah yes that makes sense.

So what I would propose is to first assume that python itself provides pip, and only fall back to searching for a rezified pip if that is not the case. Really the reason to do this is just for backwards compatibility.

I think I'd rather do this in a separate PR though, as this one is dragging on, and the current behavior already (erroneously as @JeanChristopheMorinPerso points out) expects rezified pip. I would aim to do this directly after merging this PR, and probably will not do a release in the meantime.

How does that sound?

JeanChristopheMorinPerso · 2019-08-25T22:25:15Z

I think I'd rather do this in a separate PR though, as this one is dragging on, and the current behavior already (erroneously as @JeanChristopheMorinPerso points out) expects rezified pip. I would aim to do this directly after merging this PR, and probably will not do a release in the meantime.

That's good for me.

ajohns added 3 commits July 16, 2019 20:39

-fixed vendored 'packaging' imports

cf6064e

Merge branch 'master' into pip-improvements

7d008f3

nerdvegas mentioned this pull request Jul 19, 2019

[Fix] Package schema and request errors #658

Closed

2 tasks

ajohns added 3 commits July 20, 2019 15:43

Merge branch 'master' into pip-improvements

e44be42

# Conflicts: # src/rez/utils/_version.py

-moved pip version detection to util pip module

cc6f206

fix for wrong-cased requirements names in rez-pip

6f8de75

ajohns added 2 commits July 20, 2019 16:47

docstring updates

1e16494

removed some leftover unused code

d8c4d65

JeanChristopheMorinPerso reviewed Jul 20, 2019

View reviewed changes

src/rez/utils/pip.py Outdated Show resolved Hide resolved

lambdaclan mentioned this pull request Jul 22, 2019

[Feature] Add native not operator (!) support to version ranges #670

Closed

1 task

nerdvegas mentioned this pull request Jul 22, 2019

hashed variant shortlinks aren't create with rez-pip #672

Open

-unused import removed

ab8a2d5

-use hashed variants in rez-pip -added some pip-specific metadata to package.py

nerdvegas mentioned this pull request Jul 29, 2019

Add multi-package support to rez-pip #682

Open

JeanChristopheMorinPerso reviewed Aug 13, 2019

View reviewed changes

JeanChristopheMorinPerso reviewed Aug 15, 2019

View reviewed changes

nerdvegas mentioned this pull request Aug 19, 2019

[WIP-FIX] Remove the shell=True globally from Windows Python commands (pip bind, version check) #681

Closed

1 task

JeanChristopheMorinPerso reviewed Aug 19, 2019

View reviewed changes

updated to work with package.SpecifierSet rather than str

90fec45

ajohns added 5 commits August 19, 2019 12:38

fix for os.name env marker

acd31da

merged conflicts

6e96c44

Merge branch 'JeanChristopheMorinPerso-pip-improvements-tests' into p…

a8052ad

…ip-improvements

-moved pip tests so rez-selftest cli works

4edcbee

-minor code redundancy removal

Merge branch 'master' into pip-improvements

02ef3c1

# Conflicts: # src/rez/utils/_version.py

JeanChristopheMorinPerso reviewed Aug 21, 2019

View reviewed changes

nerdvegas merged commit 02ef3c1 into master Aug 30, 2019

bpabel deleted the pip-improvements branch January 19, 2023 20:33

Pip improvements #667

Pip improvements #667

Conversation

nerdvegas commented Jul 19, 2019

instinct-vfx commented Jul 19, 2019

instinct-vfx commented Jul 19, 2019

lambdaclan commented Jul 19, 2019 • edited Loading

dist-info metadata

setup.py

generated package

nerdvegas commented Jul 19, 2019 via email

instinct-vfx commented Jul 19, 2019

nerdvegas commented Jul 19, 2019

nerdvegas commented Jul 20, 2019

nerdvegas commented Jul 22, 2019

instinct-vfx commented Jul 22, 2019

nerdvegas commented Jul 22, 2019

JeanChristopheMorinPerso left a 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

Choose a reason for hiding this comment

JeanChristopheMorinPerso Aug 13, 2019 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JeanChristopheMorinPerso Aug 19, 2019 • edited Loading

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

nerdvegas commented Aug 19, 2019 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nerdvegas commented Aug 19, 2019 via email

nerdvegas commented Aug 19, 2019 via email

nerdvegas commented Aug 19, 2019

instinct-vfx commented Aug 19, 2019 • edited Loading

JeanChristopheMorinPerso commented Aug 19, 2019

instinct-vfx commented Aug 19, 2019

nerdvegas commented Aug 19, 2019 via email

nerdvegas commented Aug 19, 2019 via email

instinct-vfx commented Aug 20, 2019

instinct-vfx commented Aug 20, 2019

JeanChristopheMorinPerso commented Aug 20, 2019

instinct-vfx commented Aug 20, 2019

instinct-vfx commented Aug 20, 2019

maxnbk commented Aug 21, 2019

instinct-vfx commented Aug 21, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nerdvegas commented Aug 25, 2019

JeanChristopheMorinPerso commented Aug 25, 2019

lambdaclan commented Jul 19, 2019 •

edited

Loading

JeanChristopheMorinPerso Aug 13, 2019 •

edited

Loading

JeanChristopheMorinPerso Aug 19, 2019 •

edited

Loading

instinct-vfx commented Aug 19, 2019 •

edited

Loading