-
-
Notifications
You must be signed in to change notification settings - Fork 591
Allow root module to customize toolchains #2081
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
Comments
Wanted to create a separate issue for this but realized that this is already there. We could just create a Currently the Initial solution could be a
|
This value has been hardcoded for a long time, so let's add the `patch_strip` attribute to the `WORKSPACE` setups now so that we add less tech debt in the `bzlmod` world later and need to migrate `bzlmod` users from the hard coded value to something that can be configured. This should be backwards compatible because of the default `int` value of `1` for the `patch_strip` attribute. Summary: - feat: add `patch_strip` to `python_repository`. - feat: add the default value of patch_strip to get_release_info. - refactor: handle patch_strip key in `TOOL_VERSIONS`. Work towards #2081. --------- Co-authored-by: Richard Levasseur <[email protected]>
With this PR we get rudimentary unit tests for the `python` bzlmod extension which allows us to unit test the override behaviour that will become more complex soon. Summary: - refactor: inline the python_register_toolchains - refactor: use toolchain_info to call python_register_toolchains - refactor: move the registration out of the main loop - refactor: split the parsing of the modules to a separate function - test(bzlmod): add python.toolchain module parsing tests Work towards #2081 --------- Co-authored-by: Richard Levasseur <[email protected]>
… in full_version (#2219) This PR just makes the `MINOR_MAPPING` overridable and explicit in many macros/rules that we own. Even though technically new API is exposed, I am not sure if it is possible to use it and I am not sure if we should advertise it. Explicit minor_mapping results in easier wiring of `python.override` `bzlmod` extension tag class planned for #2081.
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. Work towards bazel-contrib#2081 and this should be one of the last items that are blocking bazel-contrib#1361 from the API point of view.
This is a different take on #2205. Summary: - Remove version constraints from the `//python/config_settings:python_version`. - Remove `is_python_config_setting` macro and use a different implementation to retain the same functionality. This in general will improve the situation on how the toolchains are registered and hopefully is a step towards resolving issues for #2081.
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 bazel-contrib#2081 and this should be one of the last items that are blocking bazel-contrib#1361 from the API point of view.
…separate files (#2232) This makes the dependency management in WORKSPACE much easier to do. Summary: - refactor: split out the py_repositories call to a separate file - refactor: split out the python_repository rule to a separate file - refactor: split out the standalone interpreter utility function - refactor: split out the python_register_toolchains function - refactor: rename the remaining file Work towards #2081.
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 <[email protected]>
With this change we set the default value of `--python_version` when the `python.toolchain` is used in `bzlmod` and we generate the appropriate config settings based on the registered toolchains and given overrides by the root module. This means that we expect the `--python_version` to be always set to a non-empty value under `bzlmod` and we can cleanup code which was handling `//conditions:default` case. This also means that we can in theory drop the requirement for `python_version` in `pip.parse` and just setup dependencies for all packages that we find in the `requirements.txt` file and move on. This is left as future work by myself or anyone willing to contribute. We can also start reusing the same `whl_library` instance for multi-platform packages because the python version flag is always set - this will simplify the layout and makes the feature non-experimental anymore under bzlmod. Summary: * Add `@pythons_hub` to the `WORKSPACE` with dummy data to make pythons_hub work. * Add `MINOR_MAPPING` and `PYTHON_VERSIONS` to pythons_hub to generate the config settings. * Remove handling of the default version in `pypi` code under bzlmod. Work towards #2081, #260, #1708 --------- Co-authored-by: Richard Levasseur <[email protected]>
I'm taking a stab at customizing the target_compatible_with and target_settings parts of a toolchain. API I'm looking at is to add args to python.single_version_platform_override, so one can do e.g.
The net effect being: you can pick any of the python-build-standalone URLs and make it available with whatever custom flags you desire. Similarly, you can use any URL for an archive that extracts out to something structurally compatible with with python-build-standalone does. |
This makes the python bzlmod extension handle generating the platform-specific toolchain entries ("python_3_10_{platform}"). This is to eventually allow adding additional toolchains that aren't part of the PLATFORMS mapping in versions.bzl and have their own custom constraints. The main things this refactor does are: 1. The bzlmod phase passes the full list of implementation toolchains to create (previously, it relied on `hub_repo` to generate the implementation names). 2. The name of a toolchain (the toolchain.name arg) and the repo that implements it (the py_toolchain_suite.user_repository_repo arg) are separate. This allows future work to mixin toolchains that point to arbitrary repos. 3. The platform meta data uses a list of target settings instead of dict of flag values. This allows more arbitrary target settings. For now, flag values on the platform metadata is still looked for because it's known that users patch python/versions.bzl. Along the way: * Factor out a platform_info helper in versions.bzl * Factor out a NOT_ACTUALLY_PUBLIC constants to better denote things that are public visibility, but actually internal. * Add some docs to some internals so we don't have to chase down their definitions. Work towards #2081
Ideally it would be great to start using the Though I am not sure where this belongs. The more I think about it, the more I realize that the target python platforms that we want to define have to be somewhat similar/related between the |
Yes. I'll take it a step further: I like the idea of fetching a remote config of all available toolchains (i.e. all ~500 python-build-standalone runtimes). But, by default, we only register one toolchain -- what works on the host. If people want more than that, they have to opt-in somehow (e.g. MODULE.bazel changes). Similarly, ideally, waving the magic wand 🧙 some more, the set of toolchains built into rules_python source code is only the most recent supported python versions for populate platforms. Anything beyond that requires the remote manifest or user opt-in But, yeah, quite a bit of refactoring and work needed to get to that point. In the meantime, the upcoming PR to let single_version_platform_override add mostly-arbitrary PBS runtimes will go a long ways.
Yeah, this is pretty close to my followup PR from the recent refactor PR to python.bzl. The design of "there's a dict of platform->settings; a toolchain has a platform key, which is looked up in that dict" is baked pretty deeply into various places. My upcoming PR adds a second platforms dict that single_version_platform_override adds into, the two are merged, and that merged result gets passed down to functions that need the platform dict (instead of using the PLATFORMS constant). Ideally, I don't think we should be defining these platform-description-strings. The values we have today mostly just derive from the python-build-standalone file names and the need to have a unique, valid, repo name for the downloaded runtime.
How so? Do you mean there is some code in the pip-integration that looks at the PLATFORMS global to do something-or-other? Ah, maybe the part where pip.parse tries to automatically lookup the correct interpreter? The need for that should go away with pipstar, right? It'll only be needed for sdist building -- and if we move that to build phase, then that need will go away (and we're only left with the problem of setup_requires et al for sdist building) |
Yeah this is a big one. I have the following concerns about the feasibility:
Yeah, this riffs to what you are saying above. Maybe we could split the hermetic python toolchain into a separate module, where people can easily register stuff? If
May thinking was that the platform description (the thing that you said you would like to leave out) is right now done in three places This is why I was thinking that the target platform concept would be nice, because that would allow us to include/create only the actually required |
We've had several requests where users want to modify something about the toolchain definitions that are generated for the hermetic runtimes. Typically these are small tweaks. Allowing other modules to tweak toolchains is a no-go, but allowing the root module to change things is reasonable.
The "simple" option for customizing toolchains is to add args to
python_register_toolchains
andpython.toolchain
.The two main issues I see with adding args is:
--bootstrap_impl=script
.Adding args to
python.toolchain
is, I think, a bad idea. Having something else, e.g.python.root_config
might make a bit more sense. That said, args onpython.toolchain()
make it easy to associate something to a particular toolchain, whilepython.root_config
would need some arg to specify which toolchain it wanted to restrict the setting to.This issue is to collect the use cases and try and figure out some options.
Things people want to change:
Overriding bootstrap template
python_repository
#2032python_register_toolchains()
.Customizing stub_shebang: https://bazelbuild.slack.com/archives/CA306CEV6/p1721464127008299
-S PYTHONNOUSERSITE=1
for all their Python invocationsinterpreter_args
and makeenv
populate values into the bootstrap.The register_coverage_tool arg
Control whether local or hermetic runtimes are used
local=True|False
arg when defining a toolchain. It decides whether the hermetic runtime repo rule or local runtime repo rule is used under the hood.Control whether runtimes are in-build or platform runtimes
inbuild=True|False
arg when defining a toolchain. It controls whether the underlyingpy_runtime()
targets generated useinterpreter=
orinterpreter_path=
Control constraints of toolchains
Control what toolchain versions are available
The text was updated successfully, but these errors were encountered: