feat: enable pyproject.toml as single source of truth for Python version#3514
feat: enable pyproject.toml as single source of truth for Python version#3514janwinkler1 wants to merge 5 commits intobazel-contrib:mainfrom
Conversation
janwinkler1
requested review from
aignas,
groodt and
rickeylev
as code owners
Summary of ChangesHello @janwinkler1, 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 significantly enhances the Python ecosystem's integration by introducing Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. 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 introduces a significant and valuable feature by enabling pyproject.toml as a single source of truth for Python versions in both python.defaults() and pip.default(). This enhancement greatly improves consistency and maintainability across projects. The implementation includes a new repository rule (pyproject_version_repo) and utility functions to extract and validate the Python version from pyproject.toml. Additionally, config_settings in pip.default() has been made optional, which is a good usability improvement. The changes are well-tested with dedicated integration and priority tests, ensuring the new functionality works as expected and respects the defined priority order for Python version resolution. The code is clean, well-documented, and adheres to best practices.
janwinkler1
force-pushed
the
pyproject-toml-handling
branch
2 times, most recently
from
958106b to
1a1fef2
Compare
janwinkler1
force-pushed
the
pyproject-toml-handling
branch
from
1a1fef2 to
bd73882
Compare
|
Hey, thank you for the contribution. In general I am +1 on having the python version read from
What is the benefit of this? If we are setting the default version through the
+1 on this change. If you'd like to merge it earlier than we decide on the various
LGTM, but I am wondering if this is the simplest possible interface. We could instead reuse, continue using the machinery that is used by
Do we need to restrict the user to
Is the idea to set the default value for |
|
I, too, am supportive of this.
I'm not worried about that. We should just pick a python runtime to use for repo-phase logic that rules_python uses. It simplifies us being able to assume what python is being used and its capabilities. There's also the
I'd say lets just go with what the author is willing to contribute. If someone later wants |
|
hi @aignas and @rickeylev, thank you for your comments! i am currently taking a vacation, will address your comments nexy week! cheerio |
rickeylev
left a comment
rickeylev
left a comment
There was a problem hiding this comment.
Core logic is OK, but some of the impl details and tests need to change
| continue | ||
|
|
||
| for tag in mod.tags.default: | ||
| pyproject_toml = getattr(tag, "pyproject_toml", None) |
There was a problem hiding this comment.
The pyproject tag will always be present, so this can use normal . for accessing.
| ), | ||
| "config_settings": attr.label_list( | ||
| mandatory = True, | ||
| mandatory = False, |
There was a problem hiding this comment.
I think a recent PR fixed this?
python/private/pypi/extension.bzl
Outdated
| ), | ||
| "pyproject_toml": attr.label( | ||
| mandatory = False, | ||
| allow_single_file = True, |
There was a problem hiding this comment.
allow_single_file can be omitted. It has no meaning for repo/bzlmod rules
python/private/pypi/extension.bzl
Outdated
| The version must be specified as `==X.Y.Z` (exact version with full semver). | ||
| This is designed to work with dependency management tools like Renovate. | ||
|
|
||
| :::{versionadded} 1.8.0 |
There was a problem hiding this comment.
Change to VERSION_NEXT_FEATURE instead of 1.8.0
| ), | ||
| "python_version": attr.string( | ||
| mandatory = True, | ||
| mandatory = False, |
There was a problem hiding this comment.
update python_version attr doc to indicate the other option
add e.g.
:::{seealso}
The {obj}`pyproject_toml` attribute for getting the version from a project file.
:::
There was a problem hiding this comment.
Ah, this is a good opportunity to add a toml2json tool, there's another pending PR that needs it, too.
Copy the toml2json tool from https://github.com/bazel-contrib/rules_python/pull/3557/changes and use it in this PR for parsing the pyproject.toml file.
- Copy tools/toml2json; move it to tools/private/toml2json
- Copy its tests.
- Use convert_uv_lock_to_json in python/private/pypi/uv_lock.bzl as inspiration for creating a pyproject_to_json bzl function. Notably, invoke the helper toml2json script using the utility functions it uses. These ensure the script is executed correctly/safely.
| ) | ||
| if default_python_version_file: | ||
|
|
||
| # Priority order: pyproject_toml > python_version_file > python_version_env > python_version |
There was a problem hiding this comment.
This logic needs to be partially reverted, as its changing behavior.
The ENV attribute has highest precedence (this allows overriding on the command line), followed by version file. I'm not sure if .python-version or pyproject.toml should have higher precedence; I'm think pyproject.toml should be higher, though.
| # Check if pyproject_toml was specified in defaults | ||
| # If so, register a toolchain for it |
There was a problem hiding this comment.
The diff is hiding a lot of the content -- is this new logic specific to pyproject? It would make sense that this occurs for whatever default is used, not just if pyproject is.
| When specified, reads the `requires-python` field from pyproject.toml. | ||
| The version must be specified as `==X.Y.Z` (exact version with full semver). | ||
|
|
||
| :::{versionadded} 1.8.0 |
There was a problem hiding this comment.
Specify VERSION_NEXT_FEATURE
There was a problem hiding this comment.
These tests are OK, but they're integration tests. We only add integration tests when absolutely necessary because we have a limited number of jobs we can run (and we're near that limit already).
I think there are some more unittest-style things with pieces mocked out.
Otherwise, we'll just have to leave the code paths as untested 🤷
3 participants
Enables using pyproject.toml as single source of truth for Python version via
python.defaults(pyproject_toml=...)andpip.default(pyproject_toml=...).Creates
python_version_from_pyprojectrepository for BUILD file usage:config_settingsoptional inpip.default(). Previously, users hadto pass an empty list (
config_settings = []) even when not using platform-specificconfigurations. Now it can be omitted entirely when not needed.
Changes:
pyproject_tomlattribute topython.defaults()andpip.default()requires-pythonfield (must be==X.Y.Zformat)config_settingsoptional inpip.default()(was mandatory, even for empty lists)python_versionoptional inpip.parse()when usingpip.default(pyproject_toml=...)Requirements:
requires-python = "==X.Y.Z"format in pyproject.toml