Incrementally download wheels at workspace time. #432
Conversation
Refactor pip_repository rule to invoke different scripts based on the value of the incremental attribute to the rule. Create a new macro in repositories.bzl which in the context of an incremental repo will instantiate all the child repos representing individual python packages. Refactor code which is repeated between the create_incremental_repo scripts and the extract wheels script.
hrfuller
force-pushed
the
hrfuller/incremental_deps_import
branch
from
9d804e7 to
4e1c72f
Compare
|
Thanks, probably a copy paste error on my part.
…On Sun, Mar 14, 2021 at 10:39 UebelAndre ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In README.md
<#432 (comment)>
:
> +to supply a fully resolved and pinned requirements_lock.txt file (named to distinguish it from requirments.txt
+used in `pip_install`). The `requirements` attribute is replaced with a `requirements_lock` attribute to make it
+clear that a fully pinned transitive resolve is needed.
+
+To add incremental pip dependencies to your `WORKSPACE` load
+the `pip_install_incremental` function, and call it to create a main
+repo which contains a macro called `install_deps()` which is used
+to create child repos for each package in your requirements_lock.txt.
+
+
+```python
***@***.***_python//python:pip.bzl", "pip_install_incremental")
+
+# Create a central repo that knows about the dependencies needed for
+# requirements.txt.
+pip_install(
Should this not be pip_install_incremental?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#432 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABFQH5DNACGEEFWPUPTDGMTTDTYGPANCNFSM4YYVNS6A>
.
|
README.md
Outdated
| load("@rules_python//python:pip.bzl", "pip_install_incremental") | ||
|
|
||
| # Create a central repo that knows about the dependencies needed for | ||
| # requirements.txt. |
There was a problem hiding this comment.
is there ever a use-case like in rules_go where I'd want to check in that resulting requirements.bzl file so I could hand-edit? like to patch maybe?
There was a problem hiding this comment.
That's an interesting idea. I think they'd probably be platform dependent potentially.
There was a problem hiding this comment.
Maybe, but to me that would suggest that the resolver you used didn't do a good job applying your requirements / constraints. Let's revisit that if it comes up?
| args.requirements_lock = requirements_lock.name | ||
| args.repo = "pip_incremental" | ||
| contents = generate_incremental_requirements_contents(args) | ||
| library_target = sanitised_repo_library_label("foo", create_incremental_repo_prefix(args.repo)) |
There was a problem hiding this comment.
in a test I'd prefer you hardcode expected values rather than calculate them, both to document what it looks like, and as an additional assertion that the sanitised_* helper does what we expect
- Rename helpers and executables to match this new convention. - Make readme more readable. - Add integration test for the example.
Use a requirements_lock.txt which is compatible with the cp3.5 platform.
|
Hey @alexeagle would you take another look when you get a change. I believe I addressed all concerns. |
| repo_name = "foo" | ||
| index_url = "--index_url=pypi.org/simple" | ||
| args_dict = vars(parser.parse_args( | ||
| args=["--repo", repo_name, "--extra_pip_args={index_url}".format(index_url=index_url)])) |
There was a problem hiding this comment.
Hey, thanks for the feature, we are already trying to use it from the PR directly and it's a game-changer!
Problem: we use our own PyPi index in requirements.txt (--index-url=... at the top of the requirements file) and it seems like it is being ignored (I can be wrong or misusing the tool), but if that is the case, it would be great if that is passed to extra_pip_args automatically.
Also when we try to pass the pypi url via pip_extra_args explicitly, like this:
pip_parse(
name = "my_repo",
quiet = False,
timeout = 1500,
requirements_lock = "@//.../requirements-lock.txt",
python_interpreter = "python3.7",
extra_pip_args=["--index-url=http://<pypi-host>/repository/pypi-all/simple", "--trusted-host=<pypi-host>"]
)
We get an error, like this:
ERROR: .../WORKSPACE:50:13: //external:repo_pypi__setuptools: expected value of type 'list(string)' for attribute 'extra_pip_args' in 'whl_library' rule, but got "{\"args\":[\"--index-url=http://.../repository/pypi-all/simple\"]}" (string)
There was a problem hiding this comment.
Hey thanks for the feedback, I probably missed the --index-url argument. I'll try to repro the issue with extra_pip_args as well.
There was a problem hiding this comment.
I fixed the issue with --extra_pip_args not being passed to the child repos correctly. I will leave the issue of pip flags in a requirements.txt file as a TODO, as this is an experimental rule for the moment.
Don't write serialized json into the generated requirements.bzl file because whl_library doesn't understand it.
Commits in this PR can be reviewed individually, they build on each other.
PR Checklist
Please check if your PR fulfills the following requirements:
.parfiles. See CONTRIBUTING.md for infoPR Type
What kind of change does this PR introduce?
What is the current behavior?
The current pip_install repository rule requires that all pip packages be resolved and downloaded before bazel can start to run build actions. This can lead to painful wait times when invoking pip_install with large requirement.txt files on clean repos, or whenever changes to the external repo configurations are made.
Issue Number: #395
What is the new behavior?
Add an experimental repository rule called pip_install_incremental. Which sets up an external repository for each package in the requirements lock file passed by users.
The largest difference from pip_install is that pip_install_incremental doesn't do a full resolve over the requirements_lock file passed in. As such users must supply a requirements_lock.txt file containing fully resolved and pinned transitive closure of the pypi dependencies needed by their workspace. We don't provide a way to resolve a requirements lock file in this PR, nor do we verify the integrity of the lock file, but we could provide something in the future to do this.
Does this PR introduce a breaking change?