feat: update compile_pip_requirements to support multiple input files#1067
feat: update compile_pip_requirements to support multiple input files#1067rickeylev merged 16 commits intobazel-contrib:mainfrom
compile_pip_requirements to support multiple input files#1067Conversation
|
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. |
compile_pip_requirements to support multiple input files
|
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. |
|
FYI, right now this usecase might be supported by having three files: And then use the 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. |
200def6 to
363a13f
Compare
|
@aignas I've actually tried doing exactly that! Unfortunately, I couldn't quite get it working right. We're using generated Regardless, I feel that accepting multiple input files is simpler than generating an additional "root" |
|
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 |
|
@f0rmiga using 767b050 (most recent commit on Notice the repeated "python" in the last line of output. The requirements file is being generated such that the line It seems that My best guess is that this is b/c |
|
Just an update, getting the CLA signed is taking (far) longer than originally expected. I'm still working on it! |
python/pip_install/tools/dependency_resolver/dependency_resolver.py
Outdated
Show resolved
Hide resolved
363a13f to
c7bff55
Compare
919e0df to
42f2e50
Compare
8877632 to
7ef2d10
Compare
c2c5fd5 to
2ee79b0
Compare
Using click makes it easier to parse arguments. Many args are now named arguments (options), and the need for using positional args with stub `"None"` values isn't necessary anymore. There is already a dependency on click via piptools, so this doesn't introduce a new dependency. Relates to #1067 Co-authored-by: Logan Pulley <[email protected]>
2ee79b0 to
d480be1
Compare
|
I believe the test failure is an unrelated (flaky) failure. |
7301ea1 to
4bec947
Compare
4bec947 to
9a5653c
Compare
|
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: 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. |
python/private/pypi/pip_compile.bzl
Outdated
| # cheap way to detect the bazel version | ||
| _bazel_version_4_or_greater = "propeller_optimize" in dir(native) | ||
|
|
||
| # setuptools (the default python build tool) attempts to find user configuration in the user's home direcotory. This seems to work fine on linux and macOS, but fails on Windows. We provide a fake USERPROFILE env variable to allow setuptools to proceed without finding user-provided configuration. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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!
In certain environments (notably, the [bazel sandbox](https://bazel.build/docs/sandboxing) on windows), it is possible for `pathlib.Path('~').expanduser()` to fail to find the user home directory and [raise a `RuntimeError`](https://docs.python.org/3/library/pathlib.html#pathlib.Path.expanduser). This causes `distutils` (and ultimately, `setuptools`) to fail. With this patch, we catch and handle the exception by logging a warning and continuing without a user's config file. motivated by bazel-contrib/rules_python#1067
|
FYI: I've opened pypa/distutils#278. I figure it's best to handle this upstream of |
I am wondering if we should instead open a ticket in I think I'll just raise a separate issue in rules_python to track this issue, let's continue conversation there (#2121). |
Co-authored-by: Cal Jacobson <[email protected]>
compile_pip_requirements to support multiple input filescompile_pip_requirements to support multiple input files
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. |
pip-compilecan compile multiple input files into a single output file, butrules_python'scompile_pip_requirementsdoesn't currently support this.With this change, the
requirements_inargument tocompile_pip_requirementscan 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.pyto use theclickCLI library. We felt this was acceptable sincepip-compilealready requiresclickto 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 thepip-compileCLI. While subjective, I feel this improves readability, since it's not immediately obvious what's insys.argv, but it's clear thatargvbegins empty, and is added to over the course of the program's execution.