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

Introduce targets for all_requirements, all_whl_requirements, all_data_requirements #2559

Open
aignas opened this issue Jan 14, 2025 · 6 comments

Comments

@aignas
Copy link
Collaborator

aignas commented Jan 14, 2025

Right now there are constants named all_requirements, all_whl_requirements and all_data_requirements that expose:

  • all of the py_library targets.
  • all of the wheels from the hub repo (this are just filegroups).
  • all of the data targets (these are just filegroups).

Proposal:
Introduce

  • :whl and :data targets for easy inclusion of all of the whls/packages. The name is up for improvement - should it be @pypi//:all_whl, @pypi//:all_data or something similar? Maybe plural would be better?
  • :all_pkg py_library that includes all of the other py_library targets via deps could be also a good thing to have.
  • Get rid of the is_exposed concept within the pip.parse - all of the requirements will be present in the all_*requirements constants no matter what target platforms they are available on. For gazelle one would have to use the new filegroup.
  • Introduce a constant with just the names of the distributions that the pypi repository has so that programatic traversal over all of the targets is still possible. Maybe it should be called distributions something similar?
  • Once we have the new targets and constants, we could deprecate the old constants and remove them in v2.

Pros:

  • The inclusion of wheels that are only available on certain platforms becomes easier and there is less logic in our repository rules.
  • Maintaining targets is much easier than string constants in the long run.
    Cons:
  • Users may need to introduce aspects or move their logic to build phase instead of using the constant for other use-cases than the gazelle manifest generation.

What are there other use-cases that we should be mindful of?

  • @keith, I remember you had some extra static analysis built on top of rules_python, is it only using the py_library targets? Are you using any of the all_*requirements constants?
  • @mark-thm, do you have anything using it?
  • @alexeagle, do your clients have any custom logic here that is using the symbols?

Related: #260 stabilization requires the whole ticket or at least some of it.

@aignas
Copy link
Collaborator Author

aignas commented Jan 14, 2025

Created #2560 to illustrate what API I am thinking of.

@mark-thm
Copy link
Contributor

Over time we’ve managed to remove most usage — all_requirements acts as a guard in https://github.com/theoremlp/rules_pydeps/tree/main?tab=readme-ov-file#usage so that we can determine whether a call to requirement will fail here.

@markelliot
Copy link

Sorry, requirement calls will always succeed, but the corresponding labels don't necessarily exist. I could refactor my ruleset to take in requirements.txt and and derive my own constants -- but rules_python was already doing that, so seemed better to use the constants.

@keith
Copy link
Member

keith commented Jan 14, 2025

no all_requirements usage on our side

@alexeagle
Copy link
Collaborator

Yes I have some clients who needed to patch around rules_python. Also Aspect's rules_py uses all_whl_requirements to be able to create its virtualenv https://github.com/search?q=repo%3Aaspect-build%2Frules_py%20all_whl_requirements&type=code
Could you avoid a breaking change?

@aignas
Copy link
Collaborator Author

aignas commented Jan 14, 2025

  • @mark-thm, rules_python can still provide the list of all available packages. If it provided a single py_library that includes everything, then you could traverse the transitive dependencies and get what you need.
  • @alexeagle, I see that gazelle usecase would still work with my proposal, but the resolution overrides need further looking. Just to be clear, if the users have the same wheels on all target platforms then it will keep working, but if the lockfile has a windows only dependency, then it would start breaking on linux platforms.

Thanks for the feedback. I'll make some further prototypes and see how stuff fits together. It is a tricky space to navigate there. I think adding unit tests for all of the promissed and current behaviours is probably the best.

Maybe as part of this change we need to do more to ensure that users can get the right metadata in the build context easily.

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