Skip to content
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.

Sign up for GitHub

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

feat: make variable substitution for py_wheel abi, python_tag args #1113

Merged
merged 2 commits into from
Mar 9, 2023
Merged

feat: make variable substitution for py_wheel abi, python_tag args #1113

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0d12bd9
feat: make variable substitution for py_wheel abi, python_tag args
stonier Mar 7, 2023
3287a90
Doc that the make_variable_tags_impl is just an example
rickeylev Mar 9, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 24 additions & 1 deletion examples/wheel/BUILD.bazel
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
# limitations under the License.

load("@bazel_skylib//rules:build_test.bzl", "build_test")
load("//examples/wheel/private:wheel_utils.bzl", "directory_writer")
load("//examples/wheel/private:wheel_utils.bzl", "directory_writer", "make_variable_tags")
load("//python:defs.bzl", "py_library", "py_test")
load("//python:packaging.bzl", "py_package", "py_wheel")
load("//python:versions.bzl", "gen_python_config_settings")
Expand Down Expand Up @@ -62,6 +62,29 @@ py_wheel(
],
)

# Populate a rule with "Make Variable" arguments for
# abi, python_tag and version. You might want to do this
# for the following use cases:
# - abi, python_tag: introspect a toolchain to map to appropriate cpython tags
# - version: populate given this or a dependent module's version
make_variable_tags(
name = "make_variable_tags",
)

py_wheel(
name = "minimal_with_py_library_with_make_variables",
testonly = True,
abi = "$(ABI)",
distribution = "example_minimal_library",
python_tag = "$(PYTHON_TAG)",
toolchains = ["//examples/wheel:make_variable_tags"],
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: this seems like a strange example to me, would you actually expect users to write a custom rule and understand toolchains just to set these?

https://bazel.build/reference/be/make-variables#custom_variables

Rule and toolchain writers can define...

I think that's referring to us, so if users need a thing spelled $(ABI) I'd expect something in rules_python to provide it.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I agree it'd be best if the rules just told us this directly and users didn't have to write a custom rule. That said...

We could factor out a helper rule so a user doesn't need to write something a custom rule and use TemplateVariableInfo themselves, e.g.:

py_wheel(..., toolchains=[":vars"])
py_wheel_vars(name="vars", python_tag="py3", abi="abi3", platform="any")

But I'm not sure what value that really has -- it'd be easier to just set those values in the py_wheel() call. If you want to customize how it determines e.g. abi, then you're back to writing a custom rule.

Second, the more I learn about these tags, the more complicated it is to know the right value. We can get most of the way there for most users, though, I think.

PyInfo.uses_shared_libraries is probably the biggest indicator -- if there are no shared libraries, we can probably just set python_tag=py3, abi=none, platform=any as the defaults. This logic would have to live in py_wheel (more specifically, something that consumes the libraries to get that info).

If there are shared libraries, then it gets complicated.

For python tag, we need to use the major.minor python version. We can know this by look at the multi-python flags or the --python3_version flag.

For abi, we need the toolchain to tell us the abi value somehow. Normally this comes from sysconfig. A hack would be to call it in an action; this would work as long as host==target.

For platform tag, this one is complicated from what I'm told. I think a simple mapping of e.g. (linux, x86) -> "linux_x86_64" would go a long way (we even suggest such in the docs). I don't know how we'd know to use "manylinux", though -- from what I'm told, it's a specific docker image with specific versions of things used. It really sounds like it'd be modeled as a separate platform in Bazel-land with its own cc toolchain etc.

Some refs I found:

Copy link
Contributor Author

@stonier stonier Mar 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't believe you can infer the abi/python_tag from the toolchain that works for all use cases. It depends on how and what you are packaging into the wheel.

For instance, we're using pybind11 to create bindings around c++ libraries. These bindings are major.minor locked since pybind11 uses features outside the stable C API. Build a wheel with python3.8 and you can't use it with python3.10. End result, we need to set cp38 on wheels that package our python3.8 bindings.

There is work going on to improve that state of affairs (e.g. python/cpython#93012), so at some point, we will need to use, e.g. cp38 for older versions and perhaps abi3 when built with some later version of py3 where pybind11 can guarantee c python stability. In this case you have no way of being able to infer the correct abi tag from the toolchain without knowing what is going on with pybind11.

Internally, we're using a custom rule to introspect the currently resolved toolchain to determine an abi tag that is appropriate for our pybind11 wheels. That then gets fed into py_wheel. With this pipeline, we can build python versions for multiple python toolchains without having to hardcode any toolchain information into the bazel BUILD files. You just specify your py3XY toolchain on the command line (via --extra-toolchains) and the rest just works.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NB: It'd be super useful if python toolchains exposed their major.minor.patch version somehow.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Second, the more I learn about these tags, the more complicated it is to know the right value. We can get most of the way there for most users, though, I think.

Aye, I think the non-shared library case could get automagic handling. Problems in the shared library cases.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You just specify your py3XY toolchain on the command line (via --extra-toolchains) and the rest just works.

Nice!

without knowing what is going on with pybind11.

Hm, yeah, the toolchains aren't enough -- we would need e.g. the pybind rules (or whatever extension-building rules) to also communicate their abi needs. Then something like py_wheel could look at it them to figure out what to use. Which may not always have a single answer, which is ok. Actually, I wonder if an aspect would help here...aight i'm just thinking out loud now.

version = "$(VERSION)",
deps = [
"//examples/wheel/lib:module_with_data",
"//examples/wheel/lib:simple_module",
],
)

build_test(
name = "dist_build_tests",
targets = [":minimal_with_py_library.dist"],
Expand Down
17 changes: 17 additions & 0 deletions examples/wheel/private/wheel_utils.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,3 +54,20 @@ directory_writer = rule(
),
},
)

def _make_variable_tags_impl(ctx): # buildifier: disable=unused-variable
rickeylev marked this conversation as resolved.
Show resolved Hide resolved
# This example is contrived. In a real usage, this rule would
# look at flags or dependencies to determine what values to use.
# If all you're doing is setting constant values, then you can simply
# set them in the py_wheel() call.
vars = {}
vars["ABI"] = "cp38"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you have an example of what you're using it for yourself? I'm just thinking that a less contrived example would better demonstrate why you'd want to do this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See above.

vars["PYTHON_TAG"] = "cp38"
vars["VERSION"] = "0.99.0"
return [platform_common.TemplateVariableInfo(vars)]

make_variable_tags = rule(
attrs = {},
doc = """Make variable tags to pass to a py_wheel rule.""",
implementation = _make_variable_tags_impl,
)
11 changes: 7 additions & 4 deletions python/private/py_wheel.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -207,12 +207,15 @@ def _input_file_to_arg(input_file):
return "%s;%s" % (py_package_lib.path_inside_wheel(input_file), input_file.path)

def _py_wheel_impl(ctx):
abi = _replace_make_variables(ctx.attr.abi, ctx)
python_tag = _replace_make_variables(ctx.attr.python_tag, ctx)
version = _replace_make_variables(ctx.attr.version, ctx)

outfile = ctx.actions.declare_file("-".join([
_escape_filename_segment(ctx.attr.distribution),
_escape_filename_segment(version),
_escape_filename_segment(ctx.attr.python_tag),
_escape_filename_segment(ctx.attr.abi),
_escape_filename_segment(python_tag),
_escape_filename_segment(abi),
_escape_filename_segment(ctx.attr.platform),
]) + ".whl")

Expand All @@ -237,8 +240,8 @@ def _py_wheel_impl(ctx):
args = ctx.actions.args()
args.add("--name", ctx.attr.distribution)
args.add("--version", version)
args.add("--python_tag", ctx.attr.python_tag)
args.add("--abi", ctx.attr.abi)
args.add("--python_tag", python_tag)
args.add("--abi", abi)
args.add("--platform", ctx.attr.platform)
args.add("--out", outfile)
args.add("--name_file", name_file)
Expand Down