MrLixm commented 9 months ago

[!WARNING] Draft pull request, check back when its ready !

Hello, I don't know if it was in the goal of rez_pip to be usable as a python library and not only as a CLI, but given its current "WIP" state I found myself in need of such behaviour so I could further custom rez_pip behaviour.

The goal is to have rez_pip part of a larger python script, doing the heavy work of installing pip package, but allowing to retrieve those for further manipulations.

changes

⭐ Extract the cli._run private function to a new main module, as run_full_installation function.

benefits:

indirectly allow to clearly defined the "inputs" of the function that were hidden being the args variable from argparse before

improve readability by using parameters with more complex names than ones defined in the CLI. examples: pip > pipPath, packages > pipPackageNames

remarks:

are you satisfied with the name chosen for the new module + new function ?

⭐ Split run_full_installation into an additional lower-level function run_installation_for_python

benefits:

offer a workaround for #46

slightly increase readability by making run_full_installation lighter

are you satisfied with the name chosen for the new function ?

have rez.createPackage return the rez.package_maker.PackageMaker object created

benefits:

allow to further manipulate the package created if needed, or retrieve additional information from it

have new run_full_installation function return a dict of rez package created for each python version, dependent on previous change.

benefits:

fit in the goal of allowing rez_pip to be part of a larger script that could use that ouput for further refinement.

⭐ add a creationCallback parameter to rez.createPackage and main.run_full_installation (in f7b6519ef2220c31d6561d302d5c043674436505)

benefits:

allow to apply specific override per package when using rez_pip as a python library. This could help address some pitfalls of packaging pip packages to rez.

remarks:

the 3 arguments provided to the callable were arbitrarily chosen based on their probability of use. Don't hesitate to comment if you think more or less should be provided.

extract 2 functions from rez.getPythonExecutables: getPythonExecutable and findPythonPackages

benefits:

improve readability by having smaller functions overall

make it easier to re-use specific behavior when using rez_pip as library

remarks:

do you think the change is pertinent ?

change some variable to be pathlib.Path objects instead of str (in 13b7ee41256f57eae1d1b58928757533027ab921 and a184ebf9a8d64f10d5c33f657ee9cf31cba6a558)

benefits:

improve readability by clearly stating that we are expecting filesystem paths and not just strings.

easier to use path related function on them (no need for os.path)

remarks:

do you think the change is pertinent ?

what to review

Not all those changes are necessary for the initial need. Do not hesitate to oppose to some of them if you think they are not pertinent.

Essential changes are marked with a ⭐ emoji.

notes

I intended commit history to be pretty clean but I actually did some change I choose to revert later for maintaining simplicity.

pitfalls

After a few test it seems rez_pip log/print system is not designed for a library use for now. But we could perhaps address those in another PR.

misc suggestions

Not related to this PR but were found during the development process.

remove release param from rez.createPackage, instead only use the prefix param to determine the path of the installation (that could also be renamed to be more explicit).
rename rez.getPythonExecutables to getPythonExecutableFromRange ?

example

Here is a contextual use-case (in reference to issue #88 ):

import logging
import shutil
import sys
import tempfile
from pathlib import Path

import rez.package_maker
import rez_pip.main
import rez_pip.pip

LOGGER = logging.getLogger(__name__)

def patch_cyclic_dependency(
    rez_package: rez.package_maker.PackageMaker,
    pip_distribution,
    python_version,
    is_released: bool,
):
    """
    Fix issue on some sphinx dependency that are required by ``sphinx`` but also require
    ``sphinx`` creating a cyclic dependency.

    We simply set a weak reference on all their ``requires`` to fix the issue.
    """
    if not rez_package.name.startswith("sphinx") or rez_package.name == "sphinx":
        return

    LOGGER.info(f"patching {rez_package.name}")
    rez_package.requires = ["~" + require for require in rez_package.requires]

def main():
    tmp_dir = Path(tempfile.mkdtemp(suffix="rez_pip"))
    tmp_dir = tmp_dir / "sphinx"
    tmp_dir.mkdir()

    LOGGER.info("calling rez_pip ...")
    installation = rez_pip.main.run_full_installation(
        pipPackageNames=["sphinx==7.2.6"],
        pythonVersionRange="==3.10.11",
        pipPath=Path(rez_pip.pip.getBundledPip()),
        pipWorkArea=tmp_dir,
        rezPackageCreationCallback=patch_cyclic_dependency,
    )
    LOGGER.info(
        f"installed {len([package for pyv, packages in installation.items() for package in packages])} packages"
    )

    shutil.rmtree(tmp_dir)

if __name__ == "__main__":
    # XXX rez_pip will go crazy if we override its logger
    _root_logger = logging.root
    logging.root = LOGGER
    logging.basicConfig(
        level=logging.INFO,
        format="{levelname: <7} | {asctime} [{name}] {message}",
        style="{",
        stream=sys.stdout,
    )
    logging.root = _root_logger
    main()

todo

[x] ensure existing tests succeed
[x] ensure mypy suceed
[ ] add tests for run_full_installation

codecov[bot] commented 9 months ago

Codecov Report

Attention: 35 lines in your changes are missing coverage. Please review.

Comparison is base (b593438) 81.91% compared to head (3dc8b08) 81.68%.

Files	Patch %	Lines
src/rez_pip/main.py	37.73%	31 Missing and 2 partials :warning:
src/rez_pip/rez.py	91.66%	1 Missing and 1 partial :warning:

Additional details and impacted files

```diff @@ Coverage Diff @@ ## main #89 +/- ## ========================================== - Coverage 81.91% 81.68% -0.24% ========================================== Files 8 9 +1 Lines 719 759 +40 Branches 150 153 +3 ========================================== + Hits 589 620 +31 - Misses 115 123 +8 - Partials 15 16 +1 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

JeanChristopheMorinPerso commented 9 months ago

Hi @MrLixm thanks a lot for creating this PR and for the great PR description :)

First of all, rez-pip was not designed to be used as an API. I made design choices knowing full well that the API wouldn't be useable as is. That was kind of intentional.

Could you explain in more details what use cases you are facing that rez-pip doesn't cover or what kind of use cases this would enable? I know that we need to fix a couple of bugs, for example the name casing thing or even how PySide is handled. To fix some of these, I've been thinking about implementing a lightweight plugin system that would allow to create hooks, similar to how PyInstaller solves these problems.

Basically, before opening the doors to a public API, I'd like to see if we can look at hooks or other solutions.

Would you like to collaborate on such an effort? I'm definitely searching for collaborators that are active users to make sure that the solution we come up with fits the needs of users.

Let me know what you think.

MrLixm commented 9 months ago

Hello, Nice to know. I still think for now a public API can be useful, even if minimal. And by minimal I mean instead of doing a subprocess.call() to rez_pip, just having access to the top-most-level function that the CLI is calling to avoid going through subprocess to reproduce the same behavior.

My current context is I am trying to ensure a robust deploy pipeline for pip packages. You can't have any developer calling rez_pip XXX --release affecting the rez central repository without leaving trace of why and how they did it.

So my goal is to have version controlled repository that contain a "install script" for every pip package. So if one would run all those scripts on a fresh new central repository, you would get a mirror of the current one.

This could be in theory just a bunch of powershell/shell scripts that call the CLI, but with the current limitations and issues of rez_pip I have found myself in need of doing a lot of workaround that are made much easier in python. You could argue that you should not decide to change the scope (CLI > library) of a tool because it is currently missing feature and you'd be right, so it is indeed important to have a look if its really pertinent.

Here is some of my current needs:

my rez python packages have non-standard versioning, in the end rez_pip just needs an official python version for pip, and a python executable. It would be great if I could directly provide those without rez_pip guessing them in-between.
some of the pip packages I have tried to install have their quirks that need to be worked around:
- PySide which need shiboken next to it to work
- Sphinx has circular dependency on some of its extensions, supported by pip but not by rez
- and we probably found more edge-case in the future
being able to customize further the rez packages generated with for example new attributes specific to my pipeline
being able to know exactly what rez_pip did and generate a report out of it. If I call 2 times the same rez_pip command, how many packages are actually created/edited on disk ?

I would be very happy to collaborate with you if I can be of any help.