Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

py_wheel#requires only accepts a hard-coded dependency list #994

Open
alexeagle opened this issue Jan 18, 2023 · 10 comments
Open

py_wheel#requires only accepts a hard-coded dependency list #994

alexeagle opened this issue Jan 18, 2023 · 10 comments

Comments

@alexeagle
Copy link
Collaborator

alexeagle commented Jan 18, 2023

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:

metadata_contents.append("Requires-Dist: %s" % requirement)

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
Copy link
Collaborator Author

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.

@github-actions
Copy link

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 github-actions bot added the Can Close? Will close in 30 days if there is no new activity label Jul 17, 2023
@arrdem
Copy link
Contributor

arrdem commented Jul 17, 2023

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.

@github-actions github-actions bot removed the Can Close? Will close in 30 days if there is no new activity label Jul 18, 2023
@jkaye2012
Copy link

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.

Copy link

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 github-actions bot added the Can Close? Will close in 30 days if there is no new activity label Mar 20, 2024
@aignas
Copy link
Collaborator

aignas commented Mar 20, 2024

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.

@github-actions github-actions bot removed the Can Close? Will close in 30 days if there is no new activity label Mar 21, 2024
@alexeagle
Copy link
Collaborator Author

alexeagle commented Jun 17, 2024

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.

@keith
Copy link
Member

keith commented Sep 18, 2024

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.

@aignas
Copy link
Collaborator

aignas commented Sep 19, 2024

@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?

@keith
Copy link
Member

keith commented Sep 19, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants