Skip to content
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

WIP Implements #612, Useful pip #614

Closed

Conversation

mottosso
Copy link
Contributor

@mottosso mottosso commented Apr 24, 2019

A first draft of #612.

$ rez-pip --install six  # no variant
$ rez-pip --install urllib3 --variant python-2.7
$ rez-pip --install PySide --variant platform-windows --variant arch-AMD64 --variant python-2.7

Also works in conjunction with --python-version in case you've got multiple.

$ rez-pip --install urllib3 --variant python-2.7 --python-version 2.7

Also works (better) with #599 or #602. This makes no assumptions about the variant being installed.

Testing this out, not yet ready to merge.

mottosso and others added 5 commits April 18, 2019 16:05
They aren't supported on Windows, and result in read-only file permissions which cannot be overwritten and breaks building of variants amongst others, see https://docs.python.org/2/library/os.html#os.chmod
Cosmetics
@mottosso mottosso force-pushed the feature/useful-pip branch from 16639f1 to 870cb01 Compare April 24, 2019 13:18
TypeError: join() takes at least 1 argument (0 given)
@mottosso
Copy link
Contributor Author

That last commit is due to this line which in the case of not passing --variant throws this exception, rather than evaluating to False.

@mottosso
Copy link
Contributor Author

Discovered an issue with this approach, and the rez pip approach in general, which has to do with dependencies of the pip package.

E.g. packageA is pure-Python, but depends on PyQt5. So when calling:

$ rez pip --install packageA

You'll end up with both packageA and PyQt5 in a variant-less directory. Likewise, if you call:

$ rez pip --install packageA --variant platform-windows --python-3.6

You'll end up with PyQt5 in the right place, but packageA overly specified.

Solution

For that, I would probably do:

  1. rez pip --install somePackage
  2. If somePackage have no dependencies, or all dependencies are already installed, proceed
  3. Else error out, with a message detailing which dependencies need installing first

That way, installing packageA becomes a two-step process.

$ rez pip --install packageA
# ...
# ERROR: Must install PyQt5 first
$ rez pip --install PyQt5 --variant platform-windows --variant python-3.6
$ rez pip --install packageA
# Success

More tedious, but unavoidable I think, without a lot of assumptions.

Thoughts?

Before, it would add variants = [[]] which threw an exception when trying to generate a path to it.
@mottosso
Copy link
Contributor Author

mottosso commented Apr 28, 2019

Another issue with rez pip in general that I'll attempt to solve in this PR.

  • If the package already exists relative the Python site-package/, then rez pip won't make a package out of it.

For example, if python isn't a package when calling rez pip, then Rez will pick up the system-wide Python and carry on. If that system Python then already has e.g. mkdocs in its site-packages/, then a helpful (but hidden) message will kindly tell you how it's skipped.

rez pip --install mkdocs==1.0.4
13:44:49 INFO     Using python-3.6 (C:\Users\manima\Dropbox\dev\anima\github\mottosso\rez-for-projects\local_packages_path\python\3.6\package.py[])
c:\python36\lib\site-packages\pip\_internal\commands\install.py:244: UserWarning: Disabling all use of wheels due to the use of --build-options / --global-options / --install-options.
  cmdoptions.check_install_build_global(options)
Requirement already satisfied: mkdocs==1.0.4 in c:\python36\lib\site-packages (1.0.4)
Requirement already satisfied: PyYAML>=3.10 in c:\python36\lib\site-packages (from mkdocs==1.0.4) (3.13)
Requirement already satisfied: tornado>=5.0 in c:\python36\lib\site-packages (from mkdocs==1.0.4) (5.1.1)

As a result, the command will finish successfully, but not have made the packages you expected it to.

Edit: Maybe resolve dependencies up-front, with e.g. https://github.com/wimglenn/johnnydep?

mottosso and others added 8 commits April 28, 2019 17:16
For some packages, there is no distribution name other than what is passed in, resulting in None being returned which is never OK
As it turns out, no one is actually using rez pip, so breaking backwards compatibility is no big deal. Now it's actually useful for anyone wanting to use it, woho!

# Conflicts:
#	src/rez/cli/pip.py
Now variants are dynamically added to resulting packages. E.g. if a pip package requires Python 3, then it will be installed with a python-3 dependency. If a package requires Linux, then a platform-linux variant is added, and so forth.

# Conflicts:
#	src/rez/cli/pip.py
It doesn't really make sense.
@mottosso
Copy link
Contributor Author

I've included #599 into this PR as it didn't make much sense without it.

I've now also addressed the above issues.

  1. Auto-variants Dependencies having variants of their own, via variant auto-detection. The classifiers section of each PyPI package is parsed for clues on whether a distribution requires Python 2 or 3. I'd also like for it to figure out whether the platform is important, i.e. whether a library is binary or not
  2. Ignore Existing Package already installed to site-package, now those are simply ignored.

The solution for (1) was not sufficient however; as some packages may support both Python 2 and 3, but have separate distributions for each. As such, the classifiers wouldn't be enough. What should happen is that it should look towards the "python_tag" of the bdist_wheel for an exact match.

For example PyYAML-5.1-cp27-cp27m-win32.whl means python-2.

Now automatically find accurate platform and python version based on wheel metadata
@mottosso
Copy link
Contributor Author

Magic, the wheel contains information on whether a package is binary and needs the platform- variant, and the Python version for the python- variant.

All is looking well, looking out for trouble now..

@mottosso mottosso mentioned this pull request May 9, 2019
@mottosso
Copy link
Contributor Author

The automatic detection of Python version for a given package works great, except it currently only includes the major version, e.g. python-2 or python-3, whereas some packages depend on the minor version as well, such as PySide which requires python-2.6 or python-2.7.

Probably safe to always using the minor version too, which is already available here but simply not used.

It didn't work without --release
@mottosso
Copy link
Contributor Author

mottosso commented May 24, 2019

With a few weeks of use, requirements are clearing up.

Package Properties Description Example
Qt.py pure python, any version Should install without variant ~/packages/Qt.py/1.0.0
PyYAML pure python Should install with major version variant ~/packages/PyYAML/1.0.0/python-2
PySide compiled Should install with platform and major.minor version variant ~/packages/PySide/1.0.0/platform-windows/python-2.7

Need to figure out whether and what to include for os, as packages built for Ubuntu may not work for OpenBSD, but anything built for ubuntu.18.0.13151 will run on ubuntu.18.0.13152; so a balance of sorts.

I'd also like for the --platform option to pip to be made available, so that you can install Linux packages from Windows and vice versa; something only possible for wheels, which isn't everything but every little helps.

$ rez pip --platform linux --install PySide2
$ rez pip --platform windows --install PySide2

Also with this PR you can pass multiple packages to --install, but you can't continue passing arguments which is unexpected.

$ rez pip --install rez --python-version 3
# "--python-version" and "3" are considered pip packages

What's missing at the moment is those two, and the minor version for compiled packages, resulting in PySide being installed for python-2, despite working for 2.7 specifically.

And collaborate with rez env for dependencies
@mottosso
Copy link
Contributor Author

That last test involved writing to an existing package.py, which doesn't work on Windows without feature/windows-fileperm #598 so I included that here.

@mottosso
Copy link
Contributor Author

mottosso commented May 28, 2019

I've added a --index-url option to account for studios using a custom index, and are also checking for a minimum required pip version, to avoid unexpected error.

$ rez env python-2 pip-18 -- rez wheel --install six
ERROR: Requires pip>=19

The next thing I'd like to "fix" is preserving the case on names like PySide. I thought it would be a good idea to lowercase everything, but turns out it causes more trouble than benefit; primarily opening the doors for two packages named PySide and pyside.

mottosso added 3 commits May 29, 2019 15:39
It still isn't all roses; the abspath for source files is rather awkwardly formatted and too dependent on the exact layout of the InstalledDistribution class of distutils. The _get_dependencies is also really ugly and need a pass with a comb and some tweezers.
There's no need to enforce lowercase
The files are fetched not from the destination directory, but from the setup.py of a package, so if the package author included a file that isn't actually part of the physical install, this warning will sound. The most prominant example of this is script/bin files, which aren't part of this install since we use --target.
@mottosso
Copy link
Contributor Author

Restored upper/lowercase of package names, so now PySide remains PySide etc.

  1. The next thing I'd like to address is the _get_dependencies() function which is a big mess.
  2. I'm also not too fond of how the list_installed_files is able to list files not actually installed; such as those in bin/ which aren't included with when pip is used with --target. But also, it seems based on what the author has put in his setup.py which can differ from what is actually installed. In the case of Qt.py, I found that a LICENCE file was included there, but not actually on disk. We could list strictly installed files ourselves, since they're right there on disk. That would be safest perhaps, but may include more than one asked for. Having said that, if more files are included in a wheel than what is actually documented, it's not our problem and can be resolved upstream.

@mottosso
Copy link
Contributor Author

mottosso commented May 30, 2019

Spotted two edgecases.

1. Python version mismatch

rez env python-3 pip-19 -- rez wheel --install PyQt5==5.7.1
Using python-3.6 
Using pip-19.1.1
Reading package lists... ok - 29.67s   
Discovering existing packages... ok - 0.02s
The following NEW packages will be installed:
  PyQt5    5.7.1   platform-windows/os-windows-10.0/python-3.4
  sip      4.19.8  platform-windows/os-windows-10.0/python-3.6
Packages will be installed to C:\Users\manima\packages
After this operation, 182.53 mb will be used.   

Here, PyQt5 is classified as being supported for 3.4, whereas the Python I'm using is 3.6. This would result in a package that, as far as Rez is concerned, isn't compatible with the Python it's being installed for.

This should ideally create a package compatible with >=3.4,<=3.6 but we can't create a variant like this. We also can't make the variant python-3, because the next version of PyQt5 may well come classified as supporting 3.7, and we wouldn't want 3.3 to pick this up either.

So instead I think it'll need to discard the version coming from the WHEEL, and instead use the version of Python used to install it with.

2. Hacked package

rez env python-3 pip-19 -- rez wheel --install PyQt5
Using python-3.6
Using pip-19.1.1
Reading package lists... ok - 30.20s
Discovering existing packages... ok - 0.02s
The following NEW packages will be installed
  PyQt5        5.12.2   platform-windows/os-windows-10.0/python-3.5
  PyQt5_sip    4.19.17  platform-windows/os-windows-10.0/python-3.6
Packages will be installed to C:\Users\manima\packages
After this operation, 125.47 mb will be used
Do you want to continue? [Y/n]

PyQt5 has started distributing sip not as a separate Python module or package, but as something that installs itself into an existing PyQt5 install. This doesn't work well with Rez and requires some manual tweaking (countering the manual tweaking done to create this distribution in the first place).

Once installation is complete, PyQt5 expects to find PyQt5.sip, but Rez has instead gone ahead and made:

~/packages/PyQt5/5.12.2
~/packages/PyQt5_sip/4.9.17

This could potentially be resolved by addressing point (2) in the previous post; of gathering files straight from disk rather than reading from the distribution. The distribution is lying in this case, as files will have been moved around on disk post-install to accommodate for this esoteric package behaviour.

Some packages, like PyQt5==5.7, specifies support for a version of Python that may differ from the Python actually being used, e.g. 3.4 running under 3.6.
This was referenced Jun 1, 2019
@JeanChristopheMorinPerso
Copy link
Member

After thinking about this, I think rez-wheel is not a good name. The wheel format should be an implementation detail, not a first class citizen. Pip is the first class citizen that we need to expose, not wheel.

Having said that, if more files are included in a wheel than what is actually documented, it's not our problem and can be resolved upstream.

This is the way to go. We should never try to be smarter than the package maintainer. If you are unhappy with which files appears in wheel, then you need to contribute to the project and propose a change for removing those said files from the wheel.

stdout=subprocess.PIPE,
stderr=subprocess.STDOUT,
universal_newlines=True,
shell=True)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason for using shell=True here?

stderr=subprocess.STDOUT,
universal_newlines=True,
bufsize=10 ** 4, # Enough to capture the version
shell=True,

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here, any reason to use shell=True?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, in both cases, shell=True enables rez wheel to be prefixed with rez env -- rez wheel. Without it, it wouldn't be able to see the Python exposed by rez env.

"""Return major.minor version of Python, prefer current context"""

import subprocess
from rez.status import status

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

subprocess is already imported at the top of the file. Also, could rez.status be moved at the top of the file too?

@nerdvegas nerdvegas added the rez-pip ingesting py pkgs into rez (pip, wheels, etc) label Jun 2, 2019
@mottosso
Copy link
Contributor Author

mottosso commented Jun 3, 2019

Should add python into the requires of each pip package, such that you can ask for just e.g. six and get Python too.

six/package.py

name = "six"
requires = ["python"]

Question about that, how does this work with variants? I.e. some packages have Python as a variant, do we still add python to requires? For example:

  • PyQt5 is installed, with python-3 as a variant, then python-3 doesn't also have to be a requirement?
  • PyQt5 is installed again, this time for Python 2. If we made python-3 a requirement, do we then change the requirement?

Is it safe to simply add python to every pip package, and trust variants to safeguard the version?

@bpabel
Copy link
Contributor

bpabel commented Jun 3, 2019

Should add python into the requires of each pip package

While it's probably safe to do this, it's not always needed if you're using the package in a DCC with embedded python like Maya or nuke.

@mottosso
Copy link
Contributor Author

mottosso commented Jun 3, 2019

Ah, yes that's true.. Did not think of that. If it's added, it could even prevent use in e.g. Nuke which provides its own binary called python.exe that may in turn conflict with the implicitly added python package.

@mottosso
Copy link
Contributor Author

mottosso commented Jun 5, 2019

After thinking about this, I think rez-wheel is not a good name.

Having tried this in practice for a few days, it doesn't sit right with me either.

Let's have a look at our options.

Option Why Why not

rez pip --install

Simple, expected Breaks backwards compatibility

rez pip --some-flag --install

Backwards compatible, predictable

Tedious, lends itself to forgetting or not know about --some-flag

rez pip2 --install

Simple

Conflicts with pip2 and pip3 which are typically reserved for installing using pip for Python 2 and 3.

rez wheel --install

Simple

Unexpected and isn't clear what the relationship is with rez pip

Normally, I'm on board with the idea of maintaining backwards compatibility, but considering we'd be maintaining compatibility with an unusable version, I would personally make an exception and let the old pip die.

@mottosso
Copy link
Contributor Author

mottosso commented Jun 6, 2019

One of the issues at the moment with installing using --target is that we don't get any of the associated scripts. I noticed these are actually preserved when using --prefix instead.

$ pip install black --prefix .
$ tree
Lib/
  site-packages/
    black/
Scripts/
  black.exe

And /bin/black on Linux

We could probably leverage this for the corresponding Rez package, adding those to a black/1.2.3/bin subdirectory, and env.PATH.prepend("{root}/bin") alongside env.PYTHONPATH.prepend("{root}/python")

The only crux is that the DistributionPath and InstalledDistribution classes don't seem to be aware of their corresponding scripts. :( Therefore, if you install two or more packages that both provide executables, there would be no way of distinguishing which script came from which package.

We could probably work around it by pip installing each package separately, into it's own --prefix directory. But I'm hoping there's something I'm missing, any ideas? On doing that however, we do leverage the fact that Python creates this folder-structure to (presumably) account for things other than site-packages being installed, but I haven't encountered anything like that as of yet. If we keep it, we could simply env.PYTHONPATH.prepend("{root}/Lib/site-packages") and not have to worry about copying files individually from each installed package like we are now.

@instinct-vfx
Copy link
Contributor

I am not up to date with a bunch of the more recent changes in pip and setuptools. But in the past you could redirect individual components of a package using install-settings options. BUT the big show-stopper here was that this always enforced using the source package which is a no-go on windows.

@bpabel
Copy link
Contributor

bpabel commented Jun 6, 2019

Yeah, I had my own pip installer tool that had to do two install steps, once to get a list of all the downloaded dependencies, and then it would create an install package for each one separately using --no-deps

@mottosso
Copy link
Contributor Author

We could probably leverage this for the corresponding Rez package

I gave this a try, but quickly realised this won't work.

No matter how the binaries are made, they'll be hard-linked to whichever Python executable was used to generate them, which isn't OK. It could be a python-3 package, or a python-2 package, or any other arbitrary version, even system-python. Whichever binary gets linked, that's the one the generated executable would use once called, regardless of which Python was requested after having been generated.

$ rez env python-3.7 rez wheel --install black
$ rez env python-2 black
> $ black
# Screw you, I'll use 3.7

The "binaries" we make must instead be e.g. shell scripts, that reference just python rather than a full path.

Yeah, I had my own pip installer tool that had to do two install steps, once to get a list of all the downloaded dependencies, and then it would create an install package for each one separately using --no-deps

Did you experience the above problem? :O

@instinct-vfx
Copy link
Contributor

In order to party get around this problem we have a patched python distribution that includes custom versions of the executable wrappers that don't include the full path to python (so it takes the first python on PATH essentially). That comes with its own can of worms of course.

I wonder if it is possible to rely on python launcher these days? Disclaimer: Have not looked into it at all. And if not: is it possible to override the executable generation? Could we just generate .bat files on windows instead?

@mottosso
Copy link
Contributor Author

Moved to here: https://github.com/mottosso/rez-pipz

@mottosso mottosso closed this Jun 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
rez-pip ingesting py pkgs into rez (pip, wheels, etc)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants