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

feat(bzlmod): python.override for toolchain customization #2151

Closed
wants to merge 79 commits into from

Conversation

aignas
Copy link
Collaborator

@aignas aignas commented Aug 23, 2024

Before the PR users would not be able to override various things configured by
rules_python when it comes to downloading toolchains. In the WORKSPACE world,
they would be able to do this and there could be users of rules_python who
might not be able to migrate to bzlmod due to the lack of such capability.

This PR refactors the internal register_all_versions tag class and adds a
generic way to override how python toolchains are registered and how they are
downloaded. In this way users can add extra toolchains, remove existing ones
or do anything they can do in WORKSPACE builds.

Fixes #1172
Work towards #2081

python/private/python.bzl Outdated Show resolved Hide resolved
python/private/python.bzl Outdated Show resolved Hide resolved
python/private/python.bzl Outdated Show resolved Hide resolved
python/private/python.bzl Show resolved Hide resolved
python/private/python.bzl Outdated Show resolved Hide resolved
python/private/python.bzl Outdated Show resolved Hide resolved
@rickeylev
Copy link
Contributor

Overall, I think its pretty promising.

All the override APIs need is_root checks, though -- letting intermediate modules override would cause quite a headache.

Did you look at some of the other use cases in #2081 ?

Some cases that seem relevant to this api are:

  • Using a local toolchain
  • Using the free-thread interpreters
  • Using one of the alternative LTO/debug builds
  • Using a musl interpreter

Though, I'm not sure if it makes sense to override entirely vs register an additional toolchain with an additional constraint. I guess overriding provides an escape hatch. Part of me thinks that it might make more sense to push those cases into "register your own toolchain, here's some helpers to make it easier for you" -- it feels like, if you're overriding things such that they no longer resemble the python-build-standalone tars that get extracted and processed, then you're probably better off not trying to override the machinery that expects such a tar file.

@rickeylev
Copy link
Contributor

Forgot one comment: in theory, can we use these APIs ourselves to register everything? i.e. replace versions.bzl with a series of calls in MODULE.bazel? I'm not suggesting we do that, but if we can, then that's pretty cool, and I think points to the API being in the right direction.

@aignas
Copy link
Collaborator Author

aignas commented Aug 28, 2024

All the override APIs need is_root checks, though -- letting intermediate modules override would cause quite a headache.

Right now everything is under mod.is_root if statement, but I think the code needs a little bit of a refactor and test coverage, so that will be improved.

Did you look at some of the other use cases in #2081 ?

Some cases that seem relevant to this api are:

  • Using a local toolchain

I haven't thought about the local toolchain yet.

  • Using the free-thread interpreters
  • Using one of the alternative LTO/debug builds
  • Using a musl interpreter

Three of the above could be modelled as target platform with different config settings:

  • free-threaded interpreters won't be available for all OS/Arches/versions initially, so asking the user to use a specific set of flag_values may be a good idea. The PLATFORMS right now is static, so actually doing that requires them to change rules_python, but I do think we could make it more ergonomic.
  • LTO/debug builds probably should have //python/config_settings:lto=[auto|enabled|disabled], debug builds - similarly? If we allow users to override the available platforms, that could be more doable I think.
  • MUSL interpreter should be handled by flag_values, this is already the case for glibc toolchain builds:
    Label("//python/config_settings:py_linux_libc"): "glibc",

Though, I'm not sure if it makes sense to override entirely vs register an additional toolchain with an additional constraint. I guess overriding provides an escape hatch. Part of me thinks that it might make more sense to push those cases into "register your own toolchain, here's some helpers to make it easier for you" -- it feels like, if you're overriding things such that they no longer resemble the python-build-standalone tars that get extracted and processed, then you're probably better off not trying to override the machinery that expects such a tar file.

Exactly my thoughts.

Forgot one comment: in theory, can we use these APIs ourselves to register everything? i.e. replace versions.bzl with a series of calls in MODULE.bazel? I'm not suggesting we do that, but if we can, then that's pretty cool, and I think points to the API being in the right direction.

Yeah, I think we should do that. The refactor I mentioned above is to allow for that. This idea came to me when I was looking how I was constructing and overriding the tool_versions and I think we should be able to dog food the API.

@aignas aignas requested a review from groodt as a code owner September 3, 2024 11:37
@aignas aignas modified the milestones: v1.0.0, Bzlmod GA Sep 3, 2024
@aignas aignas changed the title feat(bzlmod): allow overriding TOOL_VERSIONS and base_url feat(bzlmod): python.override for toolchain customization Sep 4, 2024
@aignas aignas marked this pull request as draft September 5, 2024 13:47
@aignas
Copy link
Collaborator Author

aignas commented Sep 5, 2024

Created #2189 and #2188 so that it is easier to review this change later.

@aignas aignas marked this pull request as ready for review September 6, 2024 04:14
@aignas
Copy link
Collaborator Author

aignas commented Sep 7, 2024

This is a bit too big to easily review, will split it up.

@aignas aignas closed this Sep 7, 2024
github-merge-queue bot pushed a commit that referenced this pull request Sep 15, 2024
This `semver` function may turn out to be useful in validating
the input for the `python.*override` tag classes to be added in
a followup PR. Because this is a refactor of an existing code and
adding tests, I decided to split it out.

For a POC see #2151, work towards #2081.
github-merge-queue bot pushed a commit that referenced this pull request Sep 22, 2024
Before this PR the users could not override how the hermetic toolchain
is downloaded when in `bzlmod`. However, the same APIs would be
available to users using `WORKSPACE`. With this we also allow root
modules to restrict which toolchain versions the non-root modules, which
may be helpful when optimizing the CI runtimes so that we don't waste
time downloading multiple `micro` versions of the same `3.X` python
version, which most of the times have identical behavior.

Whilst at it, tweak the `semver` implementation to allow for testing of
absence of values in the original input.

Work towards #2081 and this should be one of the last items that are
blocking #1361 from the API point of view.

Replaces #2151.

---------

Co-authored-by: Richard Levasseur <richardlev@gmail.com>
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

Successfully merging this pull request may close these issues.

Unable to specify base_url in python.toolchain() when using bzlmod
2 participants