rules_python icon indicating copy to clipboard operation
rules_python copied to clipboard

py_wheel#requires only accepts a hard-coded dependency list

Open alexeagle opened this issue 3 years ago • 10 comments

When building a Wheel, you want to populate the Requires-Dist lines in the mylib.dist-info/METADATA file so that users of the wheel have the dependencies installed.

We support this here: https://github.com/bazelbuild/rules_python/blob/1487a0c416be8bb5d1033b3815fb662527c99197/python/packaging.bzl#L191

This is the attribute documentation: https://github.com/bazelbuild/rules_python/blob/main/docs/packaging.md#py_wheel-requires

The problem is we force you to encode the dependency list as a string_list attr in your BUILD file. This duplicates information Bazel already knew about the transitive dependencies of the py_libraries, and makes a maintenance burden for users to keep the lists in-sync.

A couple of proposals to improve this:

  • requires could accept a label which is a list of requirements produced by some other action
  • py_wheel could run an aspect up the deps tree to collect the correct resolved requirements itself

One problem I see is that Bazel will have an exact resolved version, say numpy==1.21.6 but libraries should never ship exact constraints as it makes it impossible for consuming applications to solve their constraints. We'd want to propagate the constraint the library author wrote instead, like numpy >= 1.21.

alexeagle avatar Jan 18 '23 18:01 alexeagle

This is hard to observe in rules_python because our example is contrived: https://github.com/bazelbuild/rules_python/blob/main/examples/wheel/BUILD.bazel#L132 has the pytest dependency, but we don't have any requirements file saying this library has such a dependency, nor is it in the deps of any py_* rule in that example.

alexeagle avatar Jan 18 '23 18:01 alexeagle

This issue has been automatically marked as stale because it has not had any activity for 180 days. It will be closed if no further activity occurs in 30 days. Collaborators can add an assignee to keep this open indefinitely. Thanks for your contributions to rules_python!

github-actions[bot] avatar Jul 17 '23 22:07 github-actions[bot]

In rules_zapp I "solved" this problem by authoring a zipapp "compiler" whose primary purpose is to have special knowledge of the structure of a .runfiles tree so that assets from given external workspaces (installed wheels) can be bundled together. Maybe something to consider here.

arrdem avatar Jul 17 '23 22:07 arrdem

Has anyone put any more thought into this? At least from my perspective this significantly hampers the usefulness of py_wheel and the compiled requirements provided by these rules. Because these requirements are also only "testable" at install-time, I'm not sure that there's a realistic way to validate that users have supplied the correct requirements relative to their dependencies.

jkaye2012 avatar Sep 22 '23 19:09 jkaye2012

This issue has been automatically marked as stale because it has not had any activity for 180 days. It will be closed if no further activity occurs in 30 days. Collaborators can add an assignee to keep this open indefinitely. Thanks for your contributions to rules_python!

github-actions[bot] avatar Mar 20 '24 22:03 github-actions[bot]

Does the feature in #1710 (and further fixed up in #1719) solve this? The requirements can be taken from a file.

The same goes for extras requirements.

aignas avatar Mar 20 '24 23:03 aignas

I don't think that satisfies this FR, no.

It's interesting if every service in a monorepo maintains a requirements.in file. But

  1. many orgs have chosen "single dependency closure at the root"
  2. that requirements.in file still requires manual updates, vs. gazelle that can update the deps of a py_library automatically
  3. the requirements.in still might include more libraries than the application actually depends on, because it's easy to overspecify it, e.g. including test-only libraries

That said, such an ability is perhaps useful for us to layer on top. A Bazel user could have their own macro that invokes a rule with an aspect to collect the actual runtime deps of the py_library they distribute in a wheel. The macro outputs a requirements.in format file, then it can be passed to that attribute of py_wheel so at least it doesn't require deep hacking in a patch on rules_python anymore.

alexeagle avatar Jun 17 '24 21:06 alexeagle

In our project I added a test target in our py_wheel macro that uses an aspect to extract the transitive deps, and validates that against the passed requires list. Not as nice as generating the file all together, but it at least serves as a guard rail. I think you could generate the file with the aspect similarly, but the thing I'm not sure about is how we would include the version constraints that you want if we were to do that.

keith avatar Sep 18 '24 21:09 keith

@keith, what does the aspect rely on when inspecting the transitive deps? Is there anything in rules_python that we can do to make it easier to work with and more maintainable in the long term?

aignas avatar Sep 19 '24 01:09 aignas

Right now it's pretty fragile and is just using the hardcoded hub name pattern we're using. It would be helpful if I could get the requirement name directly from the deps, (the undocumented aspect_hints feature might be helpful for this).

I'm not sure what to think about the version constraint, because depending on what you do with the wheel you might not want that to match your in-repo constraints either, so I think re-writing that is probably the best option.

keith avatar Sep 19 '24 01:09 keith