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

rez-pip does not account for python versions having differing requires #895

Open
jopollack opened this issue Jun 3, 2020 · 7 comments
Open
Labels
bug rez-pip ingesting py pkgs into rez (pip, wheels, etc)

Comments

@jopollack
Copy link

As an example, take cmd2-0.8.9, since this happened to be the one I installed when I discovered this. If you read the Metadata, you'll see this:

Requires-Python: >=2.7
Requires-Dist: pyparsing (>=2.0.1)
Requires-Dist: pyperclip
Requires-Dist: six
Requires-Dist: subprocess32; python_version<'3.0'
Requires-Dist: enum34; python_version<'3.4'
Requires-Dist: contextlib2; python_version<'3.5'

I was installing versions for python-2.7 and python-3.7, and my package.py came out looking like this:

requires = [
    'contextlib2',
    'enum34',
    'subprocess32',
    'six',
    'pyperclip',
    'wcwidth',
    'pyparsing-2.0.1+'
]

variants = [
    ['python-2.7'],
    ['python-3.7']
]

My python-3.7 cmd2 rez-env now fails to resolve, because I don't have contextlib2, enum34 or subprocess32 packages for python-3.7.

@nerdvegas
Copy link
Contributor

nerdvegas commented Jun 3, 2020 via email

@jopollack
Copy link
Author

I'll correct this a bit: It does seem to work if the python package doesn't exist at all. I just ran into this with commonmark, which requires futures-0.14.0+ for python-2 and correctly puts that in the variant, since futures doesn't exist for python-3.

I repeated the rez-pip install for cmd2-0.8.9 using just python-3 in an isolated workflow, and it just installed python-3 versions of the unnecessary packages. So it does resolve, but its pulling in unnecessary packages.

@herronelou
Copy link
Contributor

I've run into this problem quite a few times this week, I have noticed many of the re-pip installed packages are ignoring the python requirements from the pip package, often making a single 2.7 variant if I install from python 2.7, or a 3.7 variant if I install from 3.7, even when the metadata says it requires python-2.7+<3|3.5+
If I run the pip-rez install twice from 2 different python versions, I get 2 variants which are exact copies of each other, using twice the amount of space, and still it would fail if I tried to resolve in a 3.8 or 3.9 context, which should normally work.

If nobody is working on this, I would be willing to look at putting a merge request.

@nerdvegas
Copy link
Contributor

The problem is that we're suffering a kind of confirmation bias here. You've pointed out that there are pip packages where the variants are identical, and so (in theory) the major.minor variants are unnecessary. However, there are lots of packages and edge cases where this is not true, and the problem lies in our ability to detect these cases. Building the major.minor variant is basically the conservative approach - we're erring on the side of caution.

I'll leave it up to others with more python packaging experience than me to delve into the details (hi @maxnbk @JeanChristopheMorinPerso !). Fixing this involves a bigger discussion, I don't think it's a simple thing to solve.

@herronelou
Copy link
Contributor

The above is of course assuming pip and/or setuptools provides that information needed, which I assumed it did due to the metadata.
For now, I have written a utility that lets me merge pip variants as a new package so that I can manually clean things up a bit before releasing them, it's a bit manual but I was having issues modifying the variants by hand due to the hashing of pip packages.

@nerdvegas
Copy link
Contributor

Please note: We have two different things going on in this ticket. I want to clarify the original issue here.

According to the initial report it sounds as though python-specific-deps are being added to requires when in fact they should be added into the relevant variant instead.

We're now also talking about how to handle python variants in general, which does overlap with the original issue (and in fact, the original issue is one example of why we're build major.minor variants in the first place!), but isn't quite the same thing.

@herronelou
Copy link
Contributor

Oops, that is correct. Maybe it could be summarised as rez-pip dependencies/variants are not lining up properly with pip's own version of dependencies/variants. Whether that is the per-python dependencies, or the python version itself. I have encountered both issues last week doing a rez install from scratch.

@JeanChristopheMorinPerso JeanChristopheMorinPerso added bug rez-pip ingesting py pkgs into rez (pip, wheels, etc) labels Mar 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug rez-pip ingesting py pkgs into rez (pip, wheels, etc)
Projects
None yet
Development

No branches or pull requests

4 participants