feat: update compile_pip_requirements to support multiple input files

[cj81499]

pip-compile can compile multiple input files into a single output file, but rules_python's compile_pip_requirements doesn't currently support this.

With this change, the requirements_in argument to compile_pip_requirements can now accept a list of strings (in addition to the previously accepted argument types).

In order to support a variable number of input files, my coworker (@lpulley) and I updated dependency_resolver.py to use the click CLI library. We felt this was acceptable since pip-compile already requires click to run, so we're not adding a new dependency.

We also made changes to the script to avoid mutating sys.argv, instead opting to build a new list (argv) from scratch that'll be passed to the pip-compile CLI. While subjective, I feel this improves readability, since it's not immediately obvious what's in sys.argv, but it's clear that argv begins empty, and is added to over the course of the program's execution.

[google-cla]

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

[f0rmiga]

Thanks for this. How about we split this into 3 PRs? The first would be the changes to pre-commit, the second using click, and the third adding the new functionality.

[aignas]

FYI, right now this usecase might be supported by having three files:

# requirements.in
-r requirements_1.in
-r requirements_2.in

# requirements_1.in
numpy == 1.20.0

# requirements_2.in
boto3

And then use the data attribute to add dependencies to requirements.in compilation:

compile_pip_requirements(
    name = "pip_dependencies",
    data = [
        "requirements_1.in",
        "requirements_2.in",
    ],
    extra_args = [
        "--allow-unsafe",
        "--resolver=backtracking",
    ],
    requirements_in = "requirements.in",
    requirements_txt = "requirements_lock.txt",
)

FYI: This got implemented in #909.

[cj81499]

I split this PR into 3 as requested by @f0rmiga (#1070, #1071, and this one (which is blocked by #1071)). I expect it'll be easier to review now, so I really appreciate the suggestion!

[cj81499]

@aignas I've actually tried doing exactly that! Unfortunately, I couldn't quite get it working right. We're using generated requirements.in files and I couldn't figure out the right path to put in the "root" requirements.in file (the one that is given directly to compile_pip_requirements as requirements_in). I even tried generating the "root" requirements.in file using $(location //xxx:requirements.in) (and the other path related make variable substitution options), but I couldn't get it to work correctly.

Regardless, I feel that accepting multiple input files is simpler than generating an additional "root" requirements.in file.

[f0rmiga]

I believe my last fix on #1053 will cover your use case with what @aignas described. I merged it yesterday. I'm still open to reviewing your PR, though.

[cj81499]

I still think this would be a nice feature to have (since I'm currently using cj81499@bc45694 and it's working for my project), but I'll try out the latest commit from main to see if it's fixed later today.

[cj81499]

@f0rmiga using 767b050 (most recent commit on main at time of writing) and the -r approach, //python:requirements_test fails with:

Checking python/requirements.txt
Traceback (most recent call last):
  File "/home/<username>/.cache/bazel/_bazel_<username>/<hash>/sandbox/linux-sandbox/40/execroot/bazel-playground/bazel-out/k8-fastbuild/bin/python/requirements_test.runfiles/pypi__pip/pip/_internal/req/req_file.py", line 540, in get_file_content
    with open(url, "rb") as f:
         ^^^^^^^^^^^^^^^
FileNotFoundError: [Errno 2] No such file or directory: 'python/python/<name_of_project>/requirements.in'

Notice the repeated "python" in the last line of output.

The requirements file is being generated such that the line -r $(rootpath //python/<name_of_project>:requirements.in) will be included in the file (after make variable substitution of course). I've tried using location, rlocationpath, etc too, but none work (rootpath is closest to the expected path of python/<name_of_project>/requirements.in).

It seems that compile_pip_requirements is adding a prefix to the path for some reason?

My best guess is that this is b/c compile_pip_requirements is used from python/BUILD.bazel, rather than at WORKSPACE root (which seems to be the typical case based on the examples provided in this repo).

[cj81499]

Just an update, getting the CLA signed is taking (far) longer than originally expected. I'm still working on it!

[cj81499]

I believe the test failure is an unrelated (flaky) failure.

2023/11/02 23:20:29 could not download Bazel: could not copy from https://releases.bazel.build/6.4.0/release/bazel-6.4.0-linux-x86_64 to /var/lib/buildkite-agent/.cache/bazelisk/downloads/bazelbuild/bazel-6.4.0-linux-x86_64/bin/download461043671: stream error: stream ID 1; INTERNAL_ERROR

[cj81499]

Massive shout-out to my coworker @lpulley for pairing with me to root-cause and fix the failures on Windows!

We managed to implement a bit of workaround/hack:

https://github.com/cj81499/rules_python/blob/9a5653ca7f32fbf698dfb1ba1b7c425dc521c7d5/python/private/pypi/pip_compile.bzl#L160-L164

I think the idea that "Maybe rules_python should have it's own build-backend that does almost nothing?" might be less hacky, but I suspect a lot of work to implement and maintain (and certainly not something I have bandwidth to contribute). Hopefully the workaround/hack is sufficient.

[aignas]

Thank you very much for finding this, I think it is super good to have this fix, I definitely stumbled upon this a few times. This should be mentioned in the CHANGELOG.

nit: probably should split across multiple lines like the rest of the codebase.

[cj81499]

Sure thing.

fwiw, I think this workaround is sufficient for merge, but I also think it's worth a stab at contributing a change to setuptools itself (to tolerate failures to locate the user's home directory) so we don't have to do this at all!

[cj81499]

FYI: I've opened pypa/distutils#278. I figure it's best to handle this upstream of rules_python if possible. I don't think the workaround is too nasty to merge in the meantime, but hopefully it can be removed if/when that PR is merged to distutils (and vendored by setuptools, and released, and rules_python upgrades to a newer version of setuptools that include the fix).

[aignas]

FYI: I've opened pypa/distutils#278. I figure it's best to handle this upstream of rules_python if possible. I don't think the workaround is too nasty to merge in the meantime, but hopefully it can be removed if/when that PR is merged to distutils (and vendored by setuptools, and released, and rules_python upgrades to a newer version of setuptools that include the fix).

I am wondering if we should instead open a ticket in setuptools because distutils is deprecated (PEP632) and you may have a hard time getting response from them.

I think I'll just raise a separate issue in rules_python to track this issue, let's continue conversation there (#2121).

[aignas]

LGTM

[cj81499]

I am wondering if we should instead open a ticket in setuptools because distutils is deprecated (PEP632) and you may have a hard time getting response from them.

Maybe? https://github.com/pypa/distutils has been updated recently (within the last month or so), so I someone must still be looking at it. You're more than welcome to open an issue on https://github.com/pypa/setuptools to try to draw attention to the issue/my PR pypa/distutils#278 if you think that's worthwhile.