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

Conversation

vfdev-5
Copy link
Contributor

@vfdev-5 vfdev-5 commented Aug 19, 2024

Description:

  • Added free_threading argument to point to correct paths of the headers and the library.

Context:

cc @vam-google

@vfdev-5 vfdev-5 force-pushed the support-for-free-threading branch 2 times, most recently from da16bc3 to fee7a72 Compare August 19, 2024 20:40
@vfdev-5 vfdev-5 marked this pull request as ready for review August 22, 2024 11:51
@@ -507,6 +509,11 @@ For more information see the official bazel docs
"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.

We also have flag_values in the python/versions.bzl used for each platform. Does this mean that we should be able to add 2 toolchains for linux, where one would be without GIL? Is no-GIL supposed to be available for all OSes?

I personally think that it would be nice to figure out how the toolchain side of things is configured as well what this PR contains.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@aignas sorry for late reply. Maybe, this part of docs can answer your questions: https://docs.python.org/3.13/whatsnew/3.13.html#free-threaded-cpython

There are two python interpreters: default with GIL and free-threading (or no-gil) for linux, windows, macosx.

I'm not very familiar with flag_values in the python/versions.bzl, so if you think we should use it instead of something from this PR, I'm happy to implement that.

Concerning the toolchain side, I compiled jax with python 3.13t specifying manually the toolchain as following (WORKSPACE):

...

load("@xla//third_party/py:python_repo.bzl", "custom_python_interpreter")
custom_python_interpreter(
    name = "python_dev",
    urls = ["https://www.python.org/ftp/python/{version}/Python-{version}{version_variant}.tgz"],
    strip_prefix = "Python-{version}{version_variant}",
    version = "3.13.0",
    version_variant = "rc1",
    configure_params = ["--enable-optimizations", "--disable-gil"],
)

load("@xla//third_party/py:python_init_toolchains.bzl", "python_init_toolchains")
python_init_toolchains()

load("@rules_python//python:repositories.bzl", "python_register_toolchains")
python_register_toolchains(
    name = "python",
    # By default assume the interpreter is on the local file system, replace
    # with proper URL if it is not the case.
    base_url = "file://",
    ignore_root_user_error = True,
    python_version = "3.13.0",
    tool_versions = {
        "3.13.0": {
            # Path to .tar.gz with Python binary. By default it points to .tgz
            # file in cache where it was built originally; replace with proper
            # file location, if you moved it somewhere else.
            "url": "/root/.cache/bazel/_bazel_root/bee4ad1fd43279be7a03b33426e824d5/external/python_dev/python_dev-3.13.0.tgz",
            "sha256": {
                # By default we assume Linux x86_64 architecture, eplace with
                # proper architecture if you were building on a different platform.
                "x86_64-unknown-linux-gnu": "87a8ea943b6968bf07f0f483273c67fb285fa5a6d47e94228611c4dd23e65310",
            },
            "strip_prefix": "python_dev-3.13.0",
        },
    },
    free_threading = True,
)
...

Copy link
Collaborator

Choose a reason for hiding this comment

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

I see that there is a new ABI tag for wheels built for the free-threaded interpreter that is called cp313t. This means that we may want to distinguish the said wheels in the PyPI integration code. However, that probably can be done in a separate PR.

I do think that the current implementation is good looking from the outside, but I am concerned that we cannot have 3.13.0 with gil and without registered at the same time. That suggests that we don't have a good way to model ABI flavours within our toolchain code.

The flag_values would have to land here:
https://github.com/bazelbuild/rules_python/blob/main/python/private/py_toolchain_suite.bzl#L35

However, the only contributing member is PLATFORMS:
https://github.com/bazelbuild/rules_python/blob/main/python/private/toolchains_repo.bzl#L74

We should probably add a config setting which would be something like //python/config_settings:gil and have 2 values - yes, no and default to yes. For now we would have to modify each value like https://github.com/bazelbuild/rules_python/blob/main/python/versions.bzl#L517
to have a yes as a flag_value in the platform and then have a way to manipulate the PLATFORMS so that a user can specify extra platforms as an input to python_register_toolchains.

We should also add it to the bzlmod python extension, but that has to wait for #2151.

So whilst this could be a good PR as a stop gap solution, the quality bar needs to be raised for long term inclusion to rules_python.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@aignas Thanks for detailed reply!

and then have a way to manipulate the PLATFORMS so that a user can specify extra platforms as an input to python_register_toolchains.

What would be the best API for that, can you please provide an example?

python_register_toolchains(
    name = "python_3_13",
    python_version = "3.13.0",
)

python_register_toolchains(
    name = "python_3_13_ft",
    python_version = "3.13.0",
    tool_versions = {
        "3.13.0": {
            flag_values: {Label("//python/config_settings:gil"): "no",}
         }
   }
)

?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry it's been a while. We have landed a few PRs and I think we could now extend the API in

python.single_version_override(
to be something like:

python.single_version_override(
    python_version = "3.13.0",
    sha256 = {
        "aarch64-apple-darwin": "deadbeef",
        "aarch64-unknown-linux-gnu": "deadb00f",
    },
    flag_values = {
        "@rules_python//python/config_settings:gil": "no",
    },  # Added when registering the toolchains
    suffix = "_nogil", # Added as the suffix for python versions
    urls = ["https://example.org/cpython-th-{python_version}+20220227-{platform}-{build}.tar.gz"],
)

The WORKSPACE implementation would modify the TOOL_VERSIONS structure to also allow specifying the flag_values as part of a single version definition. That means that we could in theory set a value of python_repository to tell it to append t to the places which you have identified.

Previously I thought we would have to use PLATFORMS in some way, but thinking about it for the second time, I am not sure that was correct.

Copy link
Contributor Author

@vfdev-5 vfdev-5 Oct 2, 2024

Choose a reason for hiding this comment

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

@aignas I'm trying to add free-threading support to build xla and jax, they are using WORKSPACE and python_register_toolchains, right now there is no way to download free-threading version of cpython for 3.13, there is no entry in TOOL_VERSIONS corresponding to what we want (maybe the next release of indygreg will include it: astral-sh/python-build-standalone#326). So, I have to build python interpreter using custom_python_interpreter from xla. Finally, I use python_register_toolchains as following:

load("@rules_python//python:repositories.bzl", "python_register_toolchains")
python_register_toolchains(
    name = "python",
    # By default assume the interpreter is on the local file system, replace
    # with proper URL if it is not the case.
    base_url = "file://",
    ignore_root_user_error = True,
    python_version = "3.13.0",
    tool_versions = {
        "3.13.0": {
            # Path to .tar.gz with Python binary. By default it points to .tgz
            # file in cache where it was built originally; replace with proper
            # file location, if you moved it somewhere else.
            "url": "/root/.cache/bazel/_bazel_root/83d0ab0b1a11dcb5adba61b76e88291b/external/python_dev/python_dev-3.13.0.tgz",
            "sha256": {
                # By default we assume Linux x86_64 architecture, eplace with
                # proper architecture if you were building on a different platform.
                "x86_64-unknown-linux-gnu": "01d224baa3ff5ddb3fc1489d8e07cbbaf17cc705652dfe0369663a658db26d49",
            },
            "strip_prefix": "python_dev-3.13.0",
        },
    },
)

to specify wanted python.
How and where can we add the flag values in this case? Appologies for silly question, I'm not fully aware of the design and the codebase of rules_python. Thanks!

EDIT:
Few more questions:

  • What is the purpose of suffix argument (used in your example as suffix = "_nogil") ?
  • I assume that "@rules_python//python/config_settings:gil": "no", is responsible of adding t postfix to
    • distutils_path = "lib/python{}t/distutils/distutils.cfg"
    • define_hermetic_runtime_toolchain_impl

@vfdev-5 vfdev-5 force-pushed the support-for-free-threading branch 2 times, most recently from 90d0776 to 91b0580 Compare October 3, 2024 10:03
@vfdev-5
Copy link
Contributor Author

vfdev-5 commented Oct 3, 2024

@aignas I tried to implement flag_values in toolchaines.
Here is the how it is invoked from XLA now:

python_register_toolchains(
    name = "python",
    # By default assume the interpreter is on the local file system, replace
    # with proper URL if it is not the case.
    base_url = "file://",
    ignore_root_user_error = True,
    python_version = "3.13.0",
    tool_versions = {
        "3.13.0": {
            # Path to .tar.gz with Python binary. By default it points to .tgz
            # file in cache where it was built originally; replace with proper
            # file location, if you moved it somewhere else.
            "url": "/root/.cache/bazel/_bazel_root/83d0ab0b1a11dcb5adba61b76e88291b/external/python_dev/python_dev-3.13.0.tgz",
            "sha256": {
                # By default we assume Linux x86_64 architecture, eplace with
                # proper architecture if you were building on a different platform.
                "x86_64-unknown-linux-gnu": "01d224baa3ff5ddb3fc1489d8e07cbbaf17cc705652dfe0369663a658db26d49",
            },
            "strip_prefix": "python_dev-3.13.0",
            # Added when registering the toolchains
            "flag_values": {
                "@rules_python//python/config_settings:free_threading": "yes",
            },
            # Added as the suffix for python versions
            "suffix": "_ft",
        },
    },
)

Can you please review and leave a feedback whether it corresponds to your idea?
I have also few questions here: #2129 (comment)
if you can answer it would be helpful!
Thanks

@vfdev-5 vfdev-5 force-pushed the support-for-free-threading branch from 91b0580 to 2163939 Compare October 3, 2024 10:07
Copy link
Collaborator

@aignas aignas left a comment

Choose a reason for hiding this comment

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

This is going in the right direction. Now, please consider adding the plumbing to the bzlmod extension for this to be mergeable - we tend to either add new features to bzlmod or both, but not to WORKSPACE.

The suffix part in my suggestion was to ensure that the free-threaded versions do not override the regular and so that the two can coexist happily together.

The python bzlmod extension is calling python_register_toolchains internally, so I think the only thing remaining is the wiring in //python/private:python.bzl file as most of the work that you've done should be applicable.

Comment on lines 132 to 136
flag_values = tool_versions[python_version].get("flag_values", None)
free_threading_label = str(Label("//python/config_settings:free_threading"))
free_threading = False
if flag_values:
free_threading = flag_values.get(free_threading_label, False) == "yes"
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 here we can just ask the user to pass free_threading to the python_register_toolchains class because the **kwargs will be forwarded to python_repository.

Copy link
Contributor Author

@vfdev-5 vfdev-5 Oct 7, 2024

Choose a reason for hiding this comment

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

@aignas sorry I'm not sure to follow your suggestion here: you suggest to pass free_threading as a kwarg to python_register_toolchains instead of a flag_value of tool_versions?

I was passing free_threading directly to python_register_toolchains in the very begining of this PR : #2129 (comment) and you were suggesting to add it into flag_values:

load("@rules_python//python:repositories.bzl", "python_register_toolchains")
python_register_toolchains(
    name = "python",
    # By default assume the interpreter is on the local file system, replace
    # with proper URL if it is not the case.
    base_url = "file://",
    ignore_root_user_error = True,
    python_version = "3.13.0",
    tool_versions = {
        "3.13.0": {
            # Path to .tar.gz with Python binary. By default it points to .tgz
            # file in cache where it was built originally; replace with proper
            # file location, if you moved it somewhere else.
            "url": "/root/.cache/bazel/_bazel_root/bee4ad1fd43279be7a03b33426e824d5/external/python_dev/python_dev-3.13.0.tgz",
            "sha256": {
                # By default we assume Linux x86_64 architecture, eplace with
                # proper architecture if you were building on a different platform.
                "x86_64-unknown-linux-gnu": "87a8ea943b6968bf07f0f483273c67fb285fa5a6d47e94228611c4dd23e65310",
            },
            "strip_prefix": "python_dev-3.13.0",
        },
    },
    free_threading = True,
)

Can you please detail more your suggestion?

@vfdev-5
Copy link
Contributor Author

vfdev-5 commented Oct 7, 2024

@aignas I managed to pass flag_values and suffix from python.single_version_override into python_register_toolchains:

python = use_extension("@rules_python//python/extensions:python.bzl", "python")
python.toolchain(
    configure_coverage_tool = True,
    # Only set when you have multiple toolchain versions.
    is_default = True,
    python_version = "3.13",
    ignore_root_user_error=True,
)
python.single_version_override(
    python_version = "3.13.0",
    sha256 = {
        "x86_64-unknown-linux-gnu": "a4b4e0a79b53938db050daf7f78dd09faf49d1e29dbba6cac87cf857c0a51fb4",
    },
    urls = ["20241002/cpython-{python_version}rc3+20241002-{platform}-{build}.tar.gz"],
    # Added when registering the toolchains
    flag_values = {
        "@rules_python//python/config_settings:free_threading": "yes",
    },
    # Added as the suffix for python versions
    suffix = "_ft",
)

However, there is an issue with the label processing in this code:

        free_threading_label = str(Label("//python/config_settings:free_threading"))
        free_threading = False
        if flag_values:
            free_threading = flag_values.get(free_threading_label, False) == "yes"

Using WORKSPACE, the value of free_threading_label is "@rules_python//python/config_settings:free_threading" but using bzlmod and python.single_version_override, the value of free_threading_label is "@@rules_python~override//python/config_settings:free_threading". Do you have a suggestion on how to handle properly all cases?

@aignas
Copy link
Collaborator

aignas commented Oct 7, 2024 via email

@vfdev-5 vfdev-5 force-pushed the support-for-free-threading branch 2 times, most recently from e005b94 to 117d37c Compare October 7, 2024 13:42
@vfdev-5
Copy link
Contributor Author

vfdev-5 commented Oct 7, 2024

  1. Normalize the dictionary so that you are sure that the keys are strings.

I tried to apply that here: https://github.com/bazelbuild/rules_python/pull/2129/files#diff-588f665d3e43008635e703e7601e5c5e295979b1a2f46566c6d8b12cee45adb3R362

  1. Set the default of free_threading value to "no" so that in bzlmod when the user selects free threading to yes, only the special versions are matched.

I'm not sure how to do that exactly. I was thinking to add something like this

# python_register_toolchains.bzl

def python_register_toolchains(
        name,
        python_version,
        register_toolchains = True,
        register_coverage_tool = False,
        set_python_version_constraint = False,
        tool_versions = None,
        minor_mapping = None,
        **kwargs):

    ...
    if flag_values:
         free_threading = flag_values.get(free_threading_label, False) == "yes"

    if free_threading and python_version < "3.13.0":
        fail("We can't specify free_threading: yes with python < 3.13.0")

but I do not know how to properly compare python versions in this language.

Can you also suggest where exactly to hook suffix in python_register_toolchains ?

python/config_settings/BUILD.bazel Outdated Show resolved Hide resolved
flag_values = {str(k): v for k, v in tag.flag_values.items()}
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?

@@ -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.

@@ -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(

"""
_ = 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.

@@ -130,7 +131,8 @@ def _python_repository_impl(rctx):
if "windows" in platform:
distutils_path = "Lib/distutils/distutils.cfg"
else:
distutils_path = "lib/python{}/distutils/distutils.cfg".format(python_short_version)
ft_postfix = "t" if free_threading else ""
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.

@@ -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.

@@ -129,6 +129,12 @@ def python_register_toolchains(
)],
)

flag_values = tool_versions[python_version].get("flag_values", None)
free_threading_label = "@rules_python//python/config_settings:free_threading"
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 a constant at the top of the file at the very least and we should be using str(Label("//python/config_settings:free_threading")) to ensure that it is normalized.

free_threading = False
if flag_values:
free_threading = flag_values.get(free_threading_label, False) == "yes"
suffix = tool_versions[python_version].get("suffix", "")
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should also do:

flag_values.setdefault(free_threading_label, "no")

To ensure that we have the correct toolchain selection behaviour (please add an analysis test for this in //tests/toolchains (if I remember the directory correctly).

@@ -129,6 +129,12 @@ def python_register_toolchains(
)],
)

flag_values = tool_versions[python_version].get("flag_values", None)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Flag values should be passed to toolchain_aliases or toolchain_repos or both - so that the target_settings config settings use them to correctly wire things up.

Copy link
Collaborator

@aignas aignas left a comment

Choose a reason for hiding this comment

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

Extra thoughts about how everything here might need to fit.

  1. In TOOL_VERSIONS we have each toolchain keyed by version. Either we have to add a tuple key (version, free_threading) or we need to have PLATFORMS = [p.with_flag_value(free_threading = v) for v in ["yes", "no"] for p in LIST_OF_CURRENT_PLATFORMS].
  2. This means that whilst my proposed API works for the case of "let user override the python toolchain for 3.13 with the free threading one, it does not work well in the WORKSPACE unless we use something like I mentioned in 1.
  3. If we used PLATFORMS for all of the flag_value passing around, the code changes for python_register_toolchains would be minimal. The coverage may need extra wiring, but that is about it.

So my thinking is - if we only need to support bzlmod, I think we could re-key the TOOL_VERSIONS dictionary internally here and that way support the extra TOOL_VERSIONS, but I presume WORKSPACE support is important, hence we may want to support something different.

Feel free to find an alternative to what I suggested to get everything wired as well.

@vfdev-5
Copy link
Contributor Author

vfdev-5 commented Oct 9, 2024

Thanks for the suggestions @aignas !
Concerning the point 1, free-threading option is an option of CPython build, like we have build set as install_only (for free-threading releases I think we can use build version freethreaded+pgo-full). We also will have other sha256 values.
I think adding free-threading option to PLATFORMS may not be the best way...

Concerning the point 3

If we used PLATFORMS for all of the flag_value passing around, the code changes for python_register_toolchains would be minimal. The coverage may need extra wiring, but that is about it.

We would like to have platform name reflecting flag value ? Let's say we have x86_64-unknown-linux-gnu platform name and flag values for free-threading yes/no: x86_64-unknown-linux-gnu and x86_64-unknown-linux-gnu-freethreading ?

Adding and handling keys like ("3.13.0", True) or maybe a "3.13.0-free-threading" to TOOL_VERSIONS seems to be a bit more appropriate. What do you think if we would just have a special handling for python versions like "3.XY.Z-free-threading" ?

Thinking out loud, it seems like our problem could be solved if we could specify build option even for other versions of python e.g. they have options like pgo-full, pgo+lto-full etc

@aignas
Copy link
Collaborator

aignas commented Oct 9, 2024 via email

@vfdev-5
Copy link
Contributor Author

vfdev-5 commented Oct 10, 2024

Thanks for the details, @aignas !

I think there are few points to discuss:

  1. how to extend python_register_toolchains to be able to have tool_versions overridden such that we could specify free-threading build option.
  2. how user could select python with free-threading without overriding tool_versions.

Concerning the point 1:
I would like to explore whether we can add build_option arg into tool_versions variable:

python_register_toolchains(
    name = "python",
    # By default assume the interpreter is on the local file system, replace
    # with proper URL if it is not the case.
    ignore_root_user_error = True,
    python_version = "3.13",
    tool_versions = {
        "3.13.0": {
            # NOTE THAT HERE WE HAVE .zst ARCHIVE AND {build} VALUE SHOULD BE freethreaded+pgo-full FOR EXAMPLE TO GET no-gil RELEASE
            "url": "20241008/cpython-{python_version}+20241008-{platform}-{build}.tar.zst",
            "sha256": {
                "x86_64-unknown-linux-gnu": "f36f79adfcbdbe13261ba364da4f06c6f3a081702fd4f155b63fe924ff1ee9a2",
            },
            "build_option": "freethreaded+pgo-full"    # this should be a flag ideally
        },
    },
)

this way. we can correctly fill {build} value in the url when rendering it:

# python_register_toolchains
        build_option = tool_versions[python_version].get("build_option", None)
        ...
        (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 ""
        ...
        python_repository(
            name = "{name}_{platform}{build_opt}".format(
                name = name,
                platform = platform,
                build_opt = build_opt_str
            ),
        ...

and

def get_release_info(
    platform, python_version, base_url = DEFAULT_RELEASE_BASE_URL, tool_versions = TOOL_VERSIONS, build_option = None
):  
    ...
    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:
        # As build_option should be a flag we would check whether it is free_threading differently
        free_threading = True if build_option.startswith("freethreaded") else False
   ...
    for u in url:
        release_filename = u.format(
            platform = platform,
            python_version = python_version,
            build = build_option,
        )
        ...
    ...
    return (release_filename, rendered_urls, strip_prefix, patches, patch_strip, free_threading)

However, there is something hard-coded inside _pip_repository_impl implementation searching for the repository exactly as @python_x86_64-unknown-linux-gnu...

Here is a draft implementation: be242fa , the issue with pip searching @python_x86_64-unknown-linux-gnu is fixed now. What do you think ?

Concerning the point 2 "how user could select python with free-threading without overriding tool_versions.".
We can maybe do something like this:

MINOR_MAPPING = {
    ...
    "3.13": "3.13.0",
    "3.13t": ("3.13.0", "freethreaded+pgo-full"),   (version, build_option)
}

TOOL_VERSIONS = {
    ...
    "3.13.0": {
        "url": "20241008/cpython-{python_version}+20241008-{platform}-{build}.tar.gz",
        "sha256": {
            "aarch64-apple-darwin": "5d3cb8d7ca4cfbbe7ae1f118f26be112ee417d982fab8c6d85cfd8ccccf70718",
            "aarch64-unknown-linux-gnu": "c1142af8f2c85923d2ba8201a35b913bb903a5d15f052c38bbecf2f49e2342dc",
            "ppc64le-unknown-linux-gnu": "1be64a330499fed4e1f864b97eef5445b0e4abc0559ae45df3108981800cf998",
            "s390x-unknown-linux-gnu": "c0b1cc51426feadaa932fdd9afd9a9af789916e128e48ac8909f9a269bbbd749",
            "x86_64-apple-darwin": "b58ca12d9ae14bbd79f9e5cf4b748211ff1953e59abeac63b0f4e8e49845669f",
            "x86_64-pc-windows-msvc": "c7651a7a575104f47c808902b020168057f3ad80f277e54cecfaf79a9ff50e22",
            "x86_64-unknown-linux-gnu": "455200e1a202e9d9ef4b630c04af701c0a91dcaa6462022efc76893fc762ec95",
        },
        "strip_prefix": "python",
   },
   ("3.13.0", "freethreaded+pgo-full"): {
       "url": "20241008/cpython-{python_version}+20241008-{platform}-{build}.tar.zst",
       "sha256": {
                "x86_64-unknown-linux-gnu": "f36f79adfcbdbe13261ba364da4f06c6f3a081702fd4f155b63fe924ff1ee9a2",
                ...
        },
        "strip_prefix": "python",
   }

Maybe, we can skip this feature for now and just enable point 1.

The url in the TOOL_VERSIONS can be a dict by platform name. So that would fit nicely in the existing workspace model. Do you see any other difficulties in using platforms for everything?

Adding cpython build option to the platform name seems a bit like a hack to me which may work. I do not know whether we'll have a similar issue with _pip_repository_impl which would look for exactly @python_x86_64-unknown-linux-gnu.
I saw some code in python/private/pypi doing certain platform speculations, so appending build option to platform may break certain assumption there ?

@vfdev-5 vfdev-5 force-pushed the support-for-free-threading branch from 117d37c to a9df135 Compare October 10, 2024 10:33
@vfdev-5 vfdev-5 requested a review from groodt as a code owner October 10, 2024 10:33
@vfdev-5 vfdev-5 force-pushed the support-for-free-threading branch from a9df135 to be242fa Compare October 10, 2024 10:33
@vfdev-5 vfdev-5 marked this pull request as draft October 10, 2024 10:50
@aignas
Copy link
Collaborator

aignas commented Oct 14, 2024

FYI, I did a few experiments in https://github.com/aignas/rules_python/tree/exp/freethreading and I think that going with defining extra PLATFORM values will make it the easiest. the things that are remaining:

  • PyPI integration needs to handle cp313t wheels.
  • We should have tests that the config settings work as intended.

I don't have time to finish it, so if you are interested feel free to cherry pick code from my branch as you wish.

@vfdev-5
Copy link
Contributor Author

vfdev-5 commented Oct 14, 2024

FYI, I did a few experiments in https://github.com/aignas/rules_python/tree/exp/freethreading and I think that going with defining extra PLATFORM values will make it the easiest. the things that are remaining:

* PyPI integration needs to handle cp313t wheels.

* We should have tests that the config settings work as intended.

I don't have time to finish it, so if you are interested feel free to cherry pick code from my branch as you wish.

I'm trying to build xla with python 3.13 freethreading (it is my local test) using your branch and specifying:

python_register_toolchains(
    name = "python",
    ignore_root_user_error = True,
    python_version = "3.13",
    tool_versions = {
        "3.13.0": {
            "url": "20241008/cpython-{python_version}+20241008-x86_64-unknown-linux-gnu-freethreaded+pgo+lto-full.tar.zst",
            "sha256": {
                "x86_64-unknown-linux-gnu-freethreaded": "00a159a64640ce614bdac064b270a9854d86d40d1d8387a822daf1fe0881e64b"
            },
        },
    },
)

and it complains about missing definition:

ERROR: Failed to load Starlark extension '@pypi//:requirements.bzl'.
Cycle in the workspace file detected. This indicates that a repository is used prior to being defined.
The following chain of repository dependencies lead to the missing definition.
 - @pypi
 - @python_x86_64-unknown-linux-gnu
This could either mean you have to add the '@python_x86_64-unknown-linux-gnu' repository with a statement like `http_archive` in your WORKSPACE file (note that transitive dependencies are not added automatically), or move an existing definition earlier in your WORKSPACE file.
INFO: Repository pypi instantiated at:
  /tmp/jax/xla/WORKSPACE:61:16: in <toplevel>
  /tmp/jax/xla/third_party/py/python_init_pip.bzl:29:14: in python_init_pip
Repository rule pip_repository defined at:
  /root/.cache/bazel/_bazel_root/83d0ab0b1a11dcb5adba61b76e88291b/external/rules_python/python/private/pypi/pip_repository.bzl:222:33: in <toplevel>
ERROR: Error computing the main repository mapping: cycles detected during computation of main repo mapping

I had that previously when integrated build_option parameter.
I can try to figure out if we can make it work but I feel that there are places where platform value is got using some heuristics and which will lack the suffix we are adding...

On the other hand, can you check this commit: be242fa with build_option. Using this code, I can build xla for cpython 3.13 freethreading

@vfdev-5
Copy link
Contributor Author

vfdev-5 commented Oct 22, 2024

@aignas can you check above message please ?

@aignas
Copy link
Collaborator

aignas commented Nov 5, 2024

I finally had some time to look into this and finished my previous POC, could you please test #2372?

github-merge-queue bot pushed a commit that referenced this pull request Nov 11, 2024
Before this PR freethreaded toolchains were not possible to be used,
this adds the minimum plumbing to get the things working. Coverage
support is also added.

Whilst at it:
- Add plumbing to print checksums only for a particular python version.
- Bump the remaining toolchain versions that used to use the 20241008
release
- Pass around the loaded platform list so that we are only defining
toolchains for the platforms that we have loaded the hermetic toolchain
for.

Tested:
```
$ bazel run --//python/config_settings:python_version=3.13.0 --//python/config_settings:py_freethreaded="yes" //python/private:current_interpreter_executable
...
Python 3.13.0 experimental free-threading build (main, Oct 16 2024, 03:26:14) [Clang 18.1.8 ] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>>
```

Closes #2129.
Work towards #2386.
@aignas aignas closed this in #2372 Nov 11, 2024
@vfdev-5 vfdev-5 deleted the support-for-free-threading branch November 21, 2024 18:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants