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.

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

Added arg for free threading support #2129

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
8 changes: 8 additions & 0 deletions python/config_settings/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ load(
load(
"//python/private/pypi:flags.bzl",
"UniversalWhlFlag",
"UseFreeThreadingFlag",
"UseWhlFlag",
"WhlLibcFlag",
"define_pypi_internal_flags",
Expand Down Expand Up @@ -170,3 +171,10 @@ string_flag(
define_pypi_internal_flags(
name = "define_pypi_internal_flags",
)

string_flag(
name = "free_threading",
build_setting_default = UseFreeThreadingFlag.NO,
values = sorted(UseFreeThreadingFlag.__members__.values()),
visibility = ["//visibility:public"],
)
27 changes: 16 additions & 11 deletions python/private/hermetic_runtime_repo_setup.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,8 @@ def define_hermetic_runtime_toolchain_impl(
extra_files_glob_exclude,
python_version,
python_bin,
coverage_tool):
coverage_tool,
free_threading = False):
"""Define a toolchain implementation for a python-build-standalone repo.
It expected this macro is called in the top-level package of an extracted
Expand All @@ -44,13 +45,17 @@ def define_hermetic_runtime_toolchain_impl(
python_version: {type}`str` The Python version, in `major.minor.micro`
format.
python_bin: {type}`str` The path to the Python binary within the
repositoroy.
repository.
coverage_tool: {type}`str` optional target to the coverage tool to
use.
free_threading: {type}`bool` optional free-threading support.
Default, False.
"""
_ = name # @unused
version_info = semver(python_version)
version_dict = version_info.to_dict()
version_dict["ft_postfix"] = "t" if free_threading else ""
Copy link
Collaborator

Choose a reason for hiding this comment

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

Thinking out loud, I am wondering if it makes sense to define a config setting using the free threading arg here and just include the files conditionally. That way we don't need to pass an extra free_threading bool arg to this macro, which could make things easier to maintain.


native.filegroup(
name = "files",
srcs = native.glob(
Expand All @@ -67,19 +72,19 @@ def define_hermetic_runtime_toolchain_impl(
"**/* *", # Bazel does not support spaces in file names.
# Unused shared libraries. `python` executable and the `:libpython` target
# depend on `libpython{python_version}.so.1.0`.
"lib/libpython{major}.{minor}.so".format(**version_dict),
"lib/libpython{major}.{minor}{ft_postfix}.so".format(**version_dict),
# static libraries
"lib/**/*.a",
# tests for the standard libraries.
"lib/python{major}.{minor}/**/test/**".format(**version_dict),
"lib/python{major}.{minor}/**/tests/**".format(**version_dict),
"lib/python{major}.{minor}{ft_postfix}/**/test/**".format(**version_dict),
"lib/python{major}.{minor}{ft_postfix}/**/tests/**".format(**version_dict),
"**/__pycache__/*.pyc.*", # During pyc creation, temp files named *.pyc.NNN are created
] + extra_files_glob_exclude,
),
)
cc_import(
name = "interface",
interface_library = "libs/python{major}{minor}.lib".format(**version_dict),
interface_library = "libs/python{major}{minor}{ft_postfix}.lib".format(**version_dict),
system_provided = True,
)

Expand All @@ -96,7 +101,7 @@ def define_hermetic_runtime_toolchain_impl(
hdrs = [":includes"],
includes = [
"include",
"include/python{major}.{minor}".format(**version_dict),
"include/python{major}.{minor}{ft_postfix}".format(**version_dict),
"include/python{major}.{minor}m".format(**version_dict),
],
)
Expand All @@ -105,11 +110,11 @@ def define_hermetic_runtime_toolchain_impl(
hdrs = [":includes"],
srcs = select({
"@platforms//os:linux": [
"lib/libpython{major}.{minor}.so".format(**version_dict),
"lib/libpython{major}.{minor}.so.1.0".format(**version_dict),
"lib/libpython{major}.{minor}{ft_postfix}.so".format(**version_dict),
"lib/libpython{major}.{minor}{ft_postfix}.so.1.0".format(**version_dict),
],
"@platforms//os:macos": ["lib/libpython{major}.{minor}.dylib".format(**version_dict)],
"@platforms//os:windows": ["python3.dll", "libs/python{major}{minor}.lib".format(**version_dict)],
"@platforms//os:macos": ["lib/libpython{major}.{minor}{ft_postfix}.dylib".format(**version_dict)],
"@platforms//os:windows": ["python3.dll", "libs/python{major}{minor}{ft_postfix}.lib".format(**version_dict)],
}),
)

Expand Down
2 changes: 1 addition & 1 deletion python/private/py_exec_tools_toolchain.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ py_exec_tools_toolchain = rule(
Provides a toolchain for build time tools.

This provides `ToolchainInfo` with the following attributes:
* `exec_tools`: {type}`PyExecToolsInfo`
* `exec_tools`: {type}`PyExecToolsInfo`
* `toolchain_label`: {type}`Label` _only present when `--visibile_for_testing=True`
for internal testing_. The rule's label; this allows identifying what toolchain
implmentation was selected for testing purposes.
Expand Down
8 changes: 8 additions & 0 deletions python/private/pypi/flags.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,14 @@ INTERNAL_FLAGS = [
"whl_pycp3x_abicp",
]

# Determines if we use CPython with free-threading (disabled GIL)
#
# buildifier: disable=name-conventions
UseFreeThreadingFlag = enum(
YES = "yes",
NO = "no",
)

def define_pypi_internal_flags(name):
for flag in INTERNAL_FLAGS:
string_flag(
Expand Down
16 changes: 16 additions & 0 deletions python/private/python.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -357,6 +357,13 @@ def _process_single_version_overrides(*, tag, _fail = fail, default):
for platform in sha256
}

if tag.flag_values:
# Normalize flag keys to strings
flag_values = {str(k): v for k, v in tag.flag_values.items()}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the normalization should be done inside python_register_toolchains call, because that macro should work equally well with label keyed dicts and with string dicts.

override["flag_values"] = flag_values
if tag.suffix:
override["suffix"] = tag.suffix
Copy link
Collaborator

Choose a reason for hiding this comment

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

The tag.suffix should be used to construct toolchain_info.name so that a 3.13.0 version with and without free threading can coexist together.

I am under an assumption that these will be two separate tar.gz files that get downloaded - one with and one without free threading support, am I right?


available_versions[tag.python_version] = {k: v for k, v in override.items() if v}

if tag.distutils_content:
Expand Down Expand Up @@ -789,6 +796,15 @@ class.
mandatory = False,
doc = "The URL template to fetch releases for this Python version. See {attr}`python.single_version_platform_override.urls` for documentation.",
),
"flag_values": attr.string_dict(
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should be the following type.

Suggested change
"flag_values": attr.string_dict(
"flag_values": attr.label_keyed_string_dict(

mandatory = False,
doc = "TODO",
),
"suffix": attr.string(
mandatory = False,
doc = "TODO",
default = "",
)
},
)

Expand Down
23 changes: 16 additions & 7 deletions python/private/python_register_toolchains.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -109,29 +109,37 @@ def python_register_toolchains(
if not sha256:
continue

build_option = tool_versions[python_version].get("build_option", None)
loaded_platforms.append(platform)
(release_filename, urls, strip_prefix, patches, patch_strip) = get_release_info(platform, python_version, base_url, tool_versions)
(release_filename, urls, strip_prefix, patches, patch_strip, free_threading) = get_release_info(
platform, python_version, base_url, tool_versions, build_option=build_option
)

build_opt_str = ("_" + build_option.replace("+", "-")) if build_option else ""
name_with_build_opt = "{name}{build_opt}".format(
name = name,
build_opt = build_opt_str
)
# allow passing in a tool version
coverage_tool = None
coverage_tool = tool_versions[python_version].get("coverage_tool", {}).get(platform, None)
if register_coverage_tool and coverage_tool == None:
coverage_tool = coverage_dep(
name = "{name}_{platform}_coverage".format(
name = name,
name = name_with_build_opt,
platform = platform,
),
python_version = python_version,
platform = platform,
visibility = ["@{name}_{platform}//:__subpackages__".format(
name = name,
name = name_with_build_opt,
platform = platform,
)],
)

python_repository(
name = "{name}_{platform}".format(
name = name,
name = name_with_build_opt,
platform = platform,
),
sha256 = sha256,
Expand All @@ -143,6 +151,7 @@ def python_register_toolchains(
urls = urls,
strip_prefix = strip_prefix,
coverage_tool = coverage_tool,
free_threading = free_threading,
**kwargs
)
if register_toolchains:
Expand All @@ -159,12 +168,12 @@ def python_register_toolchains(
platform = platform,
))

host_toolchain(name = name + "_host")
host_toolchain(name = name_with_build_opt + "_host")

toolchain_aliases(
name = name,
python_version = python_version,
user_repository_name = name,
user_repository_name = name_with_build_opt,
platforms = loaded_platforms,
)

Expand All @@ -176,5 +185,5 @@ def python_register_toolchains(
name = toolchain_repo_name,
python_version = python_version,
set_python_version_constraint = set_python_version_constraint,
user_repository_name = name,
user_repository_name = name_with_build_opt,
)
12 changes: 10 additions & 2 deletions python/private/python_repository.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@ def _python_repository_impl(rctx):
release_filename = rctx.attr.release_filename
urls = rctx.attr.urls or [rctx.attr.url]
auth = get_auth(rctx, urls)
free_threading = rctx.attr.free_threading

if release_filename.endswith(".zst"):
rctx.download(
Expand Down Expand Up @@ -126,11 +127,12 @@ def _python_repository_impl(rctx):
for patch in patches:
rctx.patch(patch, strip = rctx.attr.patch_strip)

ft_postfix = "t" if free_threading else ""
# Write distutils.cfg to the Python installation.
if "windows" in platform:
distutils_path = "Lib/distutils/distutils.cfg"
else:
distutils_path = "lib/python{}/distutils/distutils.cfg".format(python_short_version)
distutils_path = "lib/python{}{}/distutils/distutils.cfg".format(python_short_version, ft_postfix)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the libpython{}.dylib might need to be changed as well. Maybe we should just define python_short_version = python_short_version if not free_threading else "{}t".format(python_short_version)? It seems that the modelling of the version having t at the end could yield fewer mistakes.

if rctx.attr.distutils:
rctx.file(distutils_path, rctx.read(rctx.attr.distutils))
elif rctx.attr.distutils_content:
Expand All @@ -143,7 +145,7 @@ def _python_repository_impl(rctx):
# dyld lookup errors. To fix, set the full path to the dylib as
# it appears in the Bazel workspace as its LC_ID_DYLIB using
# the `install_name_tool` bundled with macOS.
dylib = "libpython{}.dylib".format(python_short_version)
dylib = "libpython{}{}.dylib".format(python_short_version, ft_postfix)
repo_utils.execute_checked(
rctx,
op = "python_repository.FixUpDyldIdPath",
Expand Down Expand Up @@ -255,13 +257,15 @@ define_hermetic_runtime_toolchain_impl(
python_version = {python_version},
python_bin = {python_bin},
coverage_tool = {coverage_tool},
free_threading = {free_threading},
)
""".format(
extra_files_glob_exclude = render.list(glob_exclude),
extra_files_glob_include = render.list(glob_include),
python_bin = render.str(python_bin),
python_version = render.str(rctx.attr.python_version),
coverage_tool = render.str(coverage_tool),
free_threading = free_threading,
)
rctx.delete("python")
rctx.symlink(python_bin, "python")
Expand Down Expand Up @@ -321,6 +325,10 @@ For more information see {attr}`py_runtime.coverage_tool`.
"Either distutils or distutils_content can be specified, but not both.",
mandatory = False,
),
"free_threading": attr.bool(
Copy link
Collaborator

Choose a reason for hiding this comment

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

If we are adding this arg, it should also be added to attrs at the end of the impl.

doc = "Whether we use CPython interpreter in free-threading mode (disabled GIL).",
default = False,
),
"ignore_root_user_error": attr.bool(
default = False,
doc = "Whether the check for root should be ignored or not. This causes cache misses with .pyc files.",
Expand Down
16 changes: 12 additions & 4 deletions python/versions.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -703,20 +703,28 @@ PLATFORMS = {
),
}

def get_release_info(platform, python_version, base_url = DEFAULT_RELEASE_BASE_URL, tool_versions = TOOL_VERSIONS):
def get_release_info(
platform, python_version, base_url = DEFAULT_RELEASE_BASE_URL, tool_versions = TOOL_VERSIONS, build_option = None
):
"""Resolve the release URL for the requested interpreter version

Args:
platform: The platform string for the interpreter
python_version: The version of the interpreter to get
base_url: The URL to prepend to the 'url' attr in the tool_versions dict
tool_versions: A dict listing the interpreter versions, their SHAs and URL
build_option: Python build option, default: "install_only"

Returns:
A tuple of (filename, url, archive strip prefix, patches, patch_strip)
A tuple of (filename, url, archive strip prefix, patches, patch_strip, free_threading)
"""

url = tool_versions[python_version]["url"]
if not build_option:
build_option = "shared-install_only" if (WINDOWS_NAME in platform) else "install_only"
free_threading = False
else:
free_threading = True if build_option.startswith("freethreaded") else False

if type(url) == type({}):
url = url[platform]
Expand All @@ -734,7 +742,7 @@ def get_release_info(platform, python_version, base_url = DEFAULT_RELEASE_BASE_U
release_filename = u.format(
platform = platform,
python_version = python_version,
build = "shared-install_only" if (WINDOWS_NAME in platform) else "install_only",
build = build_option,
)
if "://" in release_filename: # is absolute url?
rendered_urls.append(release_filename)
Expand All @@ -757,7 +765,7 @@ def get_release_info(platform, python_version, base_url = DEFAULT_RELEASE_BASE_U
else:
patch_strip = None

return (release_filename, rendered_urls, strip_prefix, patches, patch_strip)
return (release_filename, rendered_urls, strip_prefix, patches, patch_strip, free_threading)

def print_toolchains_checksums(name):
native.genrule(
Expand Down