-
Notifications
You must be signed in to change notification settings - Fork 7k
[bazel] add python 3.10 runtime pair #58455
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request correctly adds the Bazel configuration for a Python 3.10 toolchain by mirroring the existing setup for Python 3.9. The changes are straightforward and look good. I have included one suggestion to enhance maintainability by refactoring the duplicated toolchain definitions into a macro, which could be addressed in a follow-up. As a reminder, for this new toolchain to be active, it will need to be registered in the WORKSPACE file.
| py_runtime( | ||
| name = "py310_runtime", | ||
| interpreter = python310, | ||
| python_version = "PY3", | ||
| visibility = ["//visibility:private"], | ||
| ) | ||
|
|
||
| py_runtime_pair( | ||
| name = "py310_runtime_pair", | ||
| py2_runtime = None, | ||
| py3_runtime = ":py310_runtime", | ||
| visibility = ["//visibility:private"], | ||
| ) | ||
|
|
||
| toolchain( | ||
| name = "py310_toolchain", | ||
| exec_compatible_with = ["//:hermetic_python"], | ||
| toolchain = ":py310_runtime_pair", | ||
| toolchain_type = "@bazel_tools//tools/python:toolchain_type", | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This block of code for Python 3.10 is a copy of the existing definitions for Python 3.9. While this is correct, it introduces code duplication. To improve maintainability and make it easier to add future Python versions, consider refactoring this into a macro.
For example, a macro could be defined in a .bzl file (e.g., bazel/python.bzl):
# In a .bzl file, e.g., bazel/python.bzl
def create_python_toolchain(version, interpreter):
native.py_runtime(
name = f"py{version}_runtime",
interpreter = interpreter,
python_version = "PY3",
visibility = ["//visibility:private"],
)
native.py_runtime_pair(
name = f"py{version}_runtime_pair",
py2_runtime = None,
py3_runtime = f":py{version}_runtime",
visibility = ["//visibility:private"],
)
native.toolchain(
name = f"py{version}_toolchain",
exec_compatible_with = ["//:hermetic_python"],
toolchain = f":py{version}_runtime_pair",
toolchain_type = "@bazel_tools//tools/python:toolchain_type",
)Then in BUILD.bazel, you could replace the duplicated blocks for both Python 3.9 and 3.10 with calls to this macro. This could be a good follow-up improvement.
Signed-off-by: Lonnie Liu <lonnie@anyscale.com>
16b4972 to
c5b5f28
Compare
getting ready to run things on python 3.10 Signed-off-by: Lonnie Liu <lonnie@anyscale.com>
getting ready to run things on python 3.10 Signed-off-by: Lonnie Liu <lonnie@anyscale.com>
getting ready to run things on python 3.10 Signed-off-by: Lonnie Liu <lonnie@anyscale.com> Signed-off-by: Aydin Abiar <aydin@anyscale.com>
getting ready to run things on python 3.10 Signed-off-by: Lonnie Liu <lonnie@anyscale.com> Signed-off-by: YK <1811651+ykdojo@users.noreply.github.com>
getting ready to run things on python 3.10 Signed-off-by: Lonnie Liu <lonnie@anyscale.com>
getting ready to run things on python 3.10