feat(toolchain): drop 3.8 and print info level messages about it #3387
Conversation
Summary of ChangesHello @aignas, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request experimentally removes support for Python 3.8 by updating the version definitions. The primary goal is to assess what components or tests might break as a result of dropping this specific minor version, in preparation for potential future deprecation. This change impacts the available Python versions for builds and ensures that the system no longer references Python 3.8. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request begins the process of dropping Python 3.8 support by removing its definitions from python/versions.bzl. The changes correctly remove the 3.8.20 version from TOOL_VERSIONS and the 3.8 mapping from MINOR_MAPPING. These changes are correct and align with the PR's goal.
As this is an experiment to see what breaks, other areas to investigate will be CI configurations for Python 3.8 and any remaining 3.8-specific files. I also noticed an example in a comment in python/versions.bzl (lines 40-51) that still uses Python 3.8.10; updating this to a supported version would be a good follow-up to avoid confusion.
|
hmmm, it looks like the situation is the same, I guess we need to start bumping the |
|
Coffee thoughts:
|
aignas
force-pushed
the
chore/drop-3.8
branch
from
3e8010a to
9b4a71a
Compare
aignas
changed the title
aignas
left a comment
aignas
left a comment
There was a problem hiding this comment.
TODO:
- Unit test
- changelog
python/private/config_settings.bzl
Outdated
|
|
||
| # TODO @aignas 2025-11-04: use env-marker-setting with the smallest minor_mapping version | ||
| native.alias( | ||
| name = "is_python_{}".format(minor), |
There was a problem hiding this comment.
We need targets that are for other Python versions, but maybe instead of this hack to make the versions incompatible, we colud do it differently:
for minor in range("3.0", max(minor_mapping)):
native.config_setting()
That way we'll get the right behaviour. Another option would be to leave minor_mapping[3.8] = None. That way we might get the right behaviour as well.
Is this concern still valid? I see CI is green. My naive thought is just remove it from versions.bzl. If people need 3.8, they can manually register it back in. I'm overall +1 on removing an old runtime. For the is_python3.8 flags: We consider them deprecated, in favor of people writing their own config setting based on the python_version and python_version_major_minor flags. We could start the process of removing those. They're pretty low overhead though; all they are is e.g. One concern I have is: will bzlmod modules break immediately? The particular case I have in mind is e.g. |
|
So This is because our |
|
I think this PR shows that we should help |
Yeah. Or at least, so that it being in the bzlmod phase doesn't break everything if a version isn't available. I think the main problem here is the pip based integration that has to do actions at bzlmod and repo phase to answer the question of which urls to get. Which...well, since requirements.txt simply doesn't have that information, there's really no way to answer those questions without activity in those phases :. It really requires something like pylock.toml to do that. (this is also ignoring the fact that protobuf shouldn't be putting those requirements in there, since it causes multi-version confusing issues. Fixing that would be another way to solve this.) |
|
What we will do:
|
|
PTAL |
aignas
changed the title
| self._logger.info(lambda: ( | ||
| "Ignoring pip python version '{version}' for hub " + | ||
| "'{hub}' in module '{module}' because there is no registered " + | ||
| "toolchain for it." | ||
| ).format( |
There was a problem hiding this comment.
Probably more readable as a msg variable:
msg = (
"Ignoring pipi python ..."
"'{hub}' in module ..."
...
)
self._logger.info(msg.format(...))
There was a problem hiding this comment.
I'll leave this as is so that it is not outside the lambda.
| # NOTE @aignas 2025-11-18: If the python version is not present in our | ||
| # minor_mapping, then we will not register any packages and then the | ||
| # select in the hub repository will fail, which will prompt the user to | ||
| # configure the toolchain correctly and move forward. | ||
| self._logger.info(lambda: ( |
There was a problem hiding this comment.
Why log with a lambda?
There was a problem hiding this comment.
Me and @rickeylev decided to have lambda in the logger so that format calls do not cost any more than needed based on the logging level.
There was a problem hiding this comment.
Interesting, does Bazel not have the "deferred formatting" that core python logging has?
logger.info("value is: %s", value)The string formatting will not happen unless the log level is INFO or lower.
There was a problem hiding this comment.
Hmmm, I can't remember if I had something like that prototyped earlier or not, maybe we should look into this. should not be too difficult and would definitely remove the need for doing those inline lambdas.
That said, the current way allows us to use the render.dict helper to print a nice dictionary, because we are passing a lambda and there will be no penalty for that.
| # NOTE @aignas 2025-11-18: If the python version is not present in our | ||
| # minor_mapping, then we will not register any packages and then the | ||
| # select in the hub repository will fail, which will prompt the user to | ||
| # configure the toolchain correctly and move forward. | ||
| self._logger.info(lambda: ( |
There was a problem hiding this comment.
Interesting, does Bazel not have the "deferred formatting" that core python logging has?
logger.info("value is: %s", value)The string formatting will not happen unless the log level is INFO or lower.
3 participants
Before this PR we had to have at least one 3.8 toolchain to not break things.
With this we should be good to drop it.
Any python_version 3.8 registrations will be dropped if there are no actual
URLs configured, which means that 3.8 will not be selected. The same with
pip.parse, we will just ignore it and won't add it to the hub.
In order to ensure that
is_python_3.xflags continue working, wejust alias them to
@platforms//:incompatible. No deprecation message isprinted.
Work towards #2704
Next step for anyone interested and who has more time than me these days:
that one can do that.