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 installs dist-info dir to root, causes probs with some pkgs #892

Closed
nerdvegas opened this issue May 22, 2020 · 5 comments · Fixed by #1128
Closed

rez-pip installs dist-info dir to root, causes probs with some pkgs #892

nerdvegas opened this issue May 22, 2020 · 5 comments · Fixed by #1128
Labels
enhancement rez-pip ingesting py pkgs into rez (pip, wheels, etc)

Comments

@nerdvegas
Copy link
Contributor

Some pip packages (eg importlib_metadata) expect the dist-info dir to be present in the root dir of the python package, which would be under python/ in a rez-pip'd package. Rez currently installs dist-info dir one level above this, at the variant root.

Since there is no real need to store dist-info at the variant root, we should move it to python/ in order to fix these cases.

@nerdvegas nerdvegas added enhancement rez-pip ingesting py pkgs into rez (pip, wheels, etc) labels May 22, 2020
@zachlewis
Copy link
Contributor

I discovered the same thing, and came here suggest doing exactly this. You beat me to it by two hours.

On a related note, I've discovered that tricking pip into doing a --user install to the rez build install path seems to automatically solve many (most? all?) of the package deployment problems I've encountered with rez-pip in one fell swoop, like this dist-info placement issue, and others that I'm controlling for via #866.

I've been using this pattern a lot lately when I need to manually rezzify certain python packages :

...

def pre_build_commands():
    env.PYTHONUSERBASE = '{build.install_path}'

build_command = "python -m install {root} --no-deps --user --no-compile --global-option='build_ext'"

def commands():
    env.PATH.append('{root}/bin')
    env.PYTHONPATH.append('{root}/lib/python{resolve.python.version.major}.'
                           '{resolve.python.version.minor}/site-packages')

As you can see, the tradeoff is a kind of gnarly relative location for the python packages themselves; although I suppose it would suffice to symlink the site-packages path to the pkg_root/python at build time and continue appending '{root}/python' to PYTHONPATH, and nobody would be the wiser...

Maybe you guys have already considered this, but the $PYTHONUSERBASE thing was news to me.

Cheers!

@instinct-vfx
Copy link
Contributor

We are using PYTHONUSERBASE in our python package together with generating a .pth file. That makes python start way quicker with a lot of Packages. I still owe a write up on that. The path could be turned into the standard path in rez-pip easily enough though. Anyone else looked into that already?

@spsalefeve
Copy link
Contributor

Hey, there. Can you tell me if someone is working on this issue?

@ColinKennedy
Copy link

Hello all, I'm having the same issue with rez-pip'ed packages due to this issue and would like a fix for this. @instinct-vfx as far as I know, .pth files are possibly being deprecated so I'd like to move forward with @nerdvegas initial suggestion of moving the dist-info folder into python folder, unless there's a better approach.

Though WRT to ".pth deprecation", I'm a bit skeptical of that thread. Because
A. The work was scheduled for Python 3.8 and Python 3.8 is already out
B. The thread seems to go back and forth about the need for deprecation. This post sums it up well, IMO

Regardless, the chat has an overall lean towards eventual deprecation. So I think we should move the dist-info folder under python since it's the simplest way to solve the problem while keeping future Python versions in mind.

@j0yu
Copy link
Contributor

j0yu commented Mar 30, 2021

Hey, there. Can you tell me if someone is working on this issue?

@spsalefeve I managed to get it working by patching our own copy of src/rez/pip.py and moving all the remapping logic into rezconfig.py

rezconfig.py:pip_install_remaps
pip_install_remaps = [
    # Fix for importlib_metadata https://github.com/nerdvegas/rez/issues/892
    # Path in record              | pip installed to    | copy to rez destination
    # ----------------------------|---------------------|--------------------------
    # importlib_metadata*.dist/*  | *                   | python/*
    {
        "record_path": r"^(importlib_metadata-\S+\.dist-info{s}.*)",
        "pip_install": r"\1",
        "rez_install": r"python{s}\1",
    },
    # Default 1st behaviour: keep .dist-info folder in root of rez package
    # Path in record          | pip installed to    | copy to rez destination
    # ------------------------|---------------------|--------------------------
    # *.dist-info             | *                   | *
    {
        "record_path": r"^([^{s}]+\.dist-info{s}.*)",
        "pip_install": r"\1",
        "rez_install": r"\1",
    },
    # Default 2nd behaviour: Copy bin folder to root of rez package
    # Path in record          | pip installed to    | copy to rez destination
    # ------------------------|---------------------|--------------------------
    # ../../bin/*             | bin/*               | bin/*
    {
        "record_path": r"^{p}{s}{p}{s}(bin{s}.*)",
        "pip_install": r"\1",
        "rez_install": r"\1",
    },
    # Fix for https://github.com/nerdvegas/rez/issues/821
    # Path in record          | pip installed to    | copy to rez destination
    # ------------------------|---------------------|--------------------------
    # ../../lib/python/*      | *                   | python/*
    {
        "record_path": r"^{p}{s}{p}{s}lib{s}python{s}(.*)",
        "pip_install": r"\1",
        "rez_install": r"python{s}\1",
    },
    # Default final behaviour: copy non-relative paths into
    #                          python folder in rez package
    # Path in record          | pip installed to    | copy to rez destination
    # ------------------------|---------------------|--------------------------
    # *                       | *                   | python/*
    {
        "record_path": r"^(?!{p}{s})(.*)",  # (?!...) doesn't count as a group
        "pip_install": r"\1",
        "rez_install": r"python{s}\1",
    },
]

The patched pip.py might not be friendly for everyone's use cases and the current implementation is not compatible yet for merging as it moves some fairly fundamental ordering of pathing logic that's baked in. It will typically break for other's rez-pip unless they update the pip_install_remaps explicitly

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

Successfully merging a pull request may close this issue.

6 participants