feat(python.toolchain): support file-based default Python version #2588
Conversation
This change adds a new `default_version_file` attribute to `python.toolchain`. If set, the toolchain compares the file's contents to its `python_version`, and if they match, treats that toolchain as default (ignoring `is_default`). This allows Bazel to synchronize the default Python version with external tools (e.g., pyenv) that use a `.python-version` file or environment variables. Fixes bazel-contrib#2587.
There was a problem hiding this comment.
The implementation in general looks to me and I think this is probably a fine way to support such usecase. My only ask would be to see if there is a way to add a unit test in //tests/python/ .bzl file.
EDIT: sorry it took a while to dedicate attention to this and I really appreciate the write up and the explanation behind the usecase. Thank you once more for your contributions.
There was a problem hiding this comment.
I actually wanted to write an example here and realized that the current API is not very clear to the user. What one would have to do is do something like below:
python.toolchain(
version = "3.9",
default_version_file = ".python-version"
)
python.toolchain(
version = "3.10",
)which is very similar and the user may be confused as to where and how to use the API. Is the user supposed to have:
python.toolchain(version = "3.10", default_version_file = ".python-version")
python.toolchain(version = "3.11", default_version_file = ".python-version")
python.toolchain(version = "3.12", default_version_file = ".python-version")?
Should we have a separate tag_class that sets the defaults? I think I want to finish #2578 PR to understand if we can learn something from the builder API implemented there. The potential is to have a more DRY version:
python.defaults(version_file = ".python-version)
python.toolchain(version = "3.9")
python.toolchain(version = "3.10")
python.toolchain(version = "3.11")
|
Exactly, it would be python.toolchain(version = "3.10", default_version_file = ".python-version")
python.toolchain(version = "3.11", default_version_file = ".python-version")
python.toolchain(version = "3.12", default_version_file = ".python-version")yes. I don't think that's ideal, though. I like the idea of a separate python.defaults(python_version_file = ".python-version")
python.toolchain(version = "3.9")
python.toolchain(version = "3.10")
python.toolchain(version = "3.11")and something like python.defaults(python_version = "3.11")
python.toolchain(version = "3.9")
python.toolchain(version = "3.10")
python.toolchain(version = "3.11")and perhaps even implement a default of looking in the root module |
|
#2578 implements the design for defaults that got approved, so I think we could almost copy paste that here. |
As an alternative to python.toolchain.is_default, introduce a python.defaults tag class with attributes python_version, python_version_env and python_version_file. This allows to read the default python version from your projects .python-version files, similar to other tools. It also allows using an environment variable, with a fallback if the environment variable is not set.
|
I originally had the idea that supporting reading the Python version from a file would be sufficient to also solve the use case where you want to read it from an environment variable, since you could generate a file with whatever logic you need. However, I read in the documentation of |
There was a problem hiding this comment.
Thanks a lot for the new design. I like how clean the interfaces look and I really like that rules_python reads .python-version by default, which makes it integrate with existing python tooling better.
I am torn if we should include this in 1.3.0 or if the ship has sailed. Since we are releasing right now and there may be a few more comments here and there, I would vote to just rebase and put this under unreleased CHANGELOG section.
python/private/python.bzl
Outdated
| getenv = module_ctx.os.environ.get | ||
| default_python_version = getenv(default_python_version_env, default_python_version) | ||
| if not default_python_version: | ||
| fallback_python_version_file = module_ctx.path("@@//:.python-version") |
There was a problem hiding this comment.
How does this actually work? Will @@ not expand to rules_python in all cases? Is it possible to add a test for this? Ah, because we are not using Label("@@//:.python-version") we are going to use the root module's file. This may be good to document as an inline comment.
There was a problem hiding this comment.
It didn't actually work. 🙈 It does work when I switch to using Label("@@//:.python-version"), which resolves to the root module's file. There is a potential issue if there is a .python-version file in the root module, but no BUILD or BUILD.bazel file in the root module. I don't know any way to detect it and ignore it:
fallback_python_version_file = module_ctx.path(Label("@@//:.python-version"))
Error in path: Unable to load package for //:.python-version: BUILD file not found in any of the following directories. Add a BUILD file to a directory to mark it as a package.
For testing, there's the mocked tests in //tests/python, which doesn't really catch what .python-version we resolve to because of the mocking, and I've been playing around in examples/multi_python_versions, but I can't have the example test both the python.defaults tag class and the .python-version fallback, unless I copy the entire example. Not sure how to proceed.
There was a problem hiding this comment.
We can get the path to module file of the root module in the same way and then try reading .python-version in the same dir.
There was a problem hiding this comment.
Just realized that my suggestion does not help.
The likelihood of people not having a BUILD.bazel file at the root module is low.
However it is a little annoying that module_ctx is making it hard to do this. Maybe we could let the users specify //python:none as the .python-version in those cases? However given that we are following semantic versioning, we will only be able to default to that in 2.0.
There was a problem hiding this comment.
The easiest approach is to just have the users say python_version_file="//:.python-version" if they want to opt in. That's not necessarily a bad thing, as being explicit allows anyone to grep the repo for the label and find out what cares about that file (5bdfbe2).
| :::{versionchanged} VERSION_NEXT_FEATURE | ||
| This setting is ignored if the default version is set using the `defaults` | ||
| tag class. | ||
| ::: |
There was a problem hiding this comment.
Should we deprecate this in favour of the defaults tag_class?
cc: @rickeylev what do you think?
I think you could read the env in a repo rule and write a file (but everything has to be in |
Label("@@//:.python-version") resolves to the .python-version file of
the root module, if such a file exists.
Implement python.defaults.python_version_env in terms of module_ctx.getenv, which was introduced in Bazel 7.1. This means that Bazel 7.0 users who wish to use the new python_version_env will have to upgrade to Bazel 7.1 or later.
If we read a python_version_file (specified with the defaults tag class or the default @@//:.python-version file), explicitly start watching that file. There are some circumstances where such a watch wouldn't be allowed, but since it's new functionality it doesn't break anything for anyone to insist on the watch. If a specific use case requires the relaxation of the requirement, that can always be considered later.
The idea to read .python-version in the root module was problematic for two reasons. For one thing, if there is a .python-version file in the root module, but no BUILD or BUILD.bazel file on the top level, we produce an error that we can't catch. This would be a breaking change, requiring users to add a BUILD.bazel file. Also, if the user has a .python-version file, and also sets is_default on a toolchain, we would fail the build (or, if we didn't, we'd change the default Python version against the users explicit instructions). That would also be a breaking change. Removing the @@//:.python-version logic means the user needs to opt in by saying python_version_file="@@//.python-version". That's not necessarily a bad thing, as being explicit allows anyone to grep the repo for the label and find out what cares about that file (as pointed out by @fmeum on Slack; good point).
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
This change adds a new
default_version_fileattribute topython.toolchain. If set, the toolchain compares the file's contents to itspython_version, and if they match, treats that toolchain as default (ignoringis_default). This allows Bazel to synchronize the default Python version with external tools (e.g., pyenv) that use a.python-versionfile or environment variables.Fixes #2587.