feat: Add s390x release by namrata-ibm · Pull Request #1352 · bazel-contrib/rules_python

namrata-ibm · 2023-08-01T05:39:52Z

Include s390x in release and update python-build-standalone to 3.9.17, 3.10.12, 3.11.4.
Latest python-build-standalone release has s390x support added.

These changes are needed to build TensorFlow on s390x, which is currently blocked due to missing support.

namrata-ibm · 2023-08-01T07:24:55Z

@rickeylev the error seen in the CI builds can be fixed with below patch:

diff --git a/WORKSPACE b/WORKSPACE
index a833de8..7438bb8 100644
--- a/WORKSPACE
+++ b/WORKSPACE
@@ -72,7 +72,7 @@ _py_gazelle_deps()
 # Install twine for our own runfiles wheel publishing.
 # Eventually we might want to install twine automatically for users too, see:
 # https://github.com/bazelbuild/rules_python/issues/1016.
-load("@python//3.11.1:defs.bzl", "interpreter")
+load("@python//3.11.4:defs.bzl", "interpreter")
 load("@rules_python//python:pip.bzl", "pip_parse")

 pip_parse(

However I am not sure about the impact of this change.
Could you please provide your inputs?

Also please let me know whether updating "MINOR_MAPPING" versions as done in this PR is not ideal.

aignas · 2023-08-01T08:48:34Z

@namrata-ibm, #1340 should fix what you mention in your message, then we would only have 3.11:defs.bzl instead of 3.11.1:defs.bzl. That said, it would be fixing it only for bzlmod and for traditional workspaces we would likely need to do additional fixes.

namrata-ibm · 2023-08-01T09:12:14Z

Thank you @aignas for pointing out.
Will check #1340 for further updates. If it is ongoing work and might take longer, then I can revert the MINOR_MAPPING changes from this PR.
However it would be good to have, to avoid explicit python version setting(eg in TensorFlow) during build on s390x.

aignas · 2023-08-01T13:27:37Z

I think the minor mapping changes are good and they should stay in this PR, if any other code is broken, it should be fixed.

…

On Tue, 1 Aug 2023, 18:12 Namrata Bhave, ***@***.***> wrote: Thank you @aignas <https://github.com/aignas> for pointing out. Will check #1340 <#1340> for further updates. If it is ongoing work and might take longer, then I can revert the MINOR_MAPPING changes from this PR. However it would be good to have, to avoid explicit python version setting(eg in TensorFlow) during build on s390x. — Reply to this email directly, view it on GitHub <#1352 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAB22KVIXRBISSLHB2I3H43XTDB7TANCNFSM6AAAAAA27G6IVE> . You are receiving this because you were mentioned.Message ID: ***@***.***>

chrislovecnm · 2023-08-03T19:03:52Z

@namrata-ibm please take a look at CI/CD failures: https://buildkite.com/bazel/rules-python-python/builds/5380#0189af9c-468d-4b55-ac85-9954d0197118

namrata-ibm · 2023-08-04T06:16:35Z

@chrislovecnm Thank you for checking, all the CI failures are same and addressed in above comments. It would be great to hear comments about the queries listed up.

aignas · 2023-08-04T09:14:20Z

@namrata-ibm, could you see if you can fix the WORKSPACE with the expected command please? Replacing 3.11.1 with 3.11.4 in the WORKSPACE and other places where it is failing would be good enough.

namrata-ibm · 2023-08-04T10:01:27Z

@aignas I added the WORKSPACE change and all earlier failures have passed in latest run. However I see Buildifier failed with 1 format issue. I tried running buildifier python/versions.bzl with v6.1.0 , however I do not see any error. Could you please help?

aignas · 2023-08-05T02:17:51Z

@namrata-ibm, this looks like a CI flake.

Before this PR the `bzlmod` and legacy setups would only expose the multi-version python toolchain aliases as the string that is passed to the `python_register_multi_toolchains` function. This meant that if the user decided to pass the full version (e.g. `3.11.1`) then they had to import the version aware `py_*` defs via `@foo//3.11.1:defs.bzl`. This PR changes it such that the user can still do the old way of importing but we print a deprecation warning and ask the user to use `3.11:defs.bzl` instead which better reflects how the multi-version toolchain should be used. This also means that the PRs bumping the minor versions like in bazel-contrib#1352 won't need updating Python code elsewhere. Whilst at it, we introduce a validation that disallows multiple Python toolchains of the same `X.Y` version. This is handled in the `bzlmod` by printing warnings that the toolchains are ignored, whilst the non-bzlmod users will get validation errors, which is a potentially breaking change. Fixes bazel-contrib#1339

chrislovecnm · 2023-08-05T14:31:06Z

@namrata-ibm, this looks like a CI flake.

@aignas did we hit a bug in buildifier?

namrata-ibm · 2023-08-07T09:47:51Z

@aignas @chrislovecnm @rickeylev could you please provide your inputs on how to proceed further?
Thank you.

chrislovecnm · 2023-08-07T15:15:19Z

@namrata-ibm the error from CI

:bazel: buildifier: found 1 format issue in your WORKSPACE, BUILD and *.bzl files

--
| If this repo uses a pre-commit hook, then you should install it. Otherwise, please download buildifier 6.1.0 and run the following command in your workspace:

buildifier python/versions.bzl

chrislovecnm

Thanks for the PR. Please run buildifier to get CI passing. I think we should have two PRs here.

include support for the new Python versions and support for s390x.
after that PR bump the mappings to increase the default version numbers

chrislovecnm · 2023-08-07T15:20:42Z

python/versions.bzl

 }

 # buildifier: disable=unsorted-dict-items
 MINOR_MAPPING = {


I would rather have the updates to MINOR_MAPPING in a different PR. If we have problems with updating this mapping, we can back that out, and keep the s390x support.

chrislovecnm · 2023-08-07T15:23:56Z

python/versions.bzl

        },
        "strip_prefix": "python",
    },
+    "3.9.17": {


@rickeylev @aignas any other Python versions that we should have s390x support for?

Needed to build TensorFlow on s390x.

namrata-ibm · 2023-08-08T09:29:01Z

@chrislovecnm Thank you, I have updated this PR to keep only s390x and python version updates.
Will raise PR for Minor mapping changes later.

Could you please check and help merge this?

chrislovecnm

LGTM

@chrislovecnm

This PR bumps mappings to latest version. Adding changes in a separate PR as discussed [here](#1352 (review)). cc @chrislovecnm

Before this PR the `bzlmod` and legacy setups would only expose the multi-version python toolchain aliases as the string that is passed to the `python_register_multi_toolchains` function. This meant that if the user decided to pass the full version (e.g. `3.11.1`) then they had to import the version aware `py_*` defs via `@foo//3.11.1:defs.bzl`. This PR changes it such that the user can still do the old way of importing but we print a deprecation warning and ask the user to use `3.11:defs.bzl` instead which better reflects how the multi-version toolchain should be used. This also means that the PRs bumping the minor versions like in bazel-contrib#1352 won't need updating Python code elsewhere. Whilst at it, we introduce a validation that disallows multiple Python toolchains of the same `X.Y` version. This is handled in the `bzlmod` by printing warnings that the toolchains are ignored, whilst the non-bzlmod users will get validation errors, which is a potentially breaking change. Fixes bazel-contrib#1339

namrata-ibm requested a review from rickeylev as a code owner August 1, 2023 05:39

aignas mentioned this pull request Aug 5, 2023

fix: prefer X.Y over X.Y.Z version aliases in python.toolchain #1340

Closed

chrislovecnm suggested changes Aug 7, 2023

View reviewed changes

namrata-ibm force-pushed the s390x-release branch from 4c9417a to 53213fa Compare August 8, 2023 07:22

Add support for s390x and update python version

1785d40

Needed to build TensorFlow on s390x.

namrata-ibm force-pushed the s390x-release branch from eeea7a1 to 1785d40 Compare August 8, 2023 09:18

chrislovecnm approved these changes Aug 8, 2023

View reviewed changes

chrislovecnm added this pull request to the merge queue Aug 8, 2023

Merged via the queue into bazel-contrib:main with commit e549810 Aug 8, 2023

namrata-ibm mentioned this pull request Aug 9, 2023

feat: Update MINOR_MAPPING to latest version #1370

Merged

github-merge-queue bot pushed a commit that referenced this pull request Aug 10, 2023

feat: Update MINOR_MAPPING to latest version (#1370)

504caab

This PR bumps mappings to latest version. Adding changes in a separate PR as discussed [here](#1352 (review)). cc @chrislovecnm

Uh oh!

Conversation

namrata-ibm commented Aug 1, 2023

Uh oh!

namrata-ibm commented Aug 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aignas commented Aug 1, 2023

Uh oh!

namrata-ibm commented Aug 1, 2023

Uh oh!

aignas commented Aug 1, 2023 via email

Uh oh!

chrislovecnm commented Aug 3, 2023

Uh oh!

namrata-ibm commented Aug 4, 2023

Uh oh!

aignas commented Aug 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

namrata-ibm commented Aug 4, 2023

Uh oh!

aignas commented Aug 5, 2023

Uh oh!

chrislovecnm commented Aug 5, 2023

Uh oh!

namrata-ibm commented Aug 7, 2023

Uh oh!

chrislovecnm commented Aug 7, 2023

:bazel: buildifier: found 1 format issue in your WORKSPACE, BUILD and *.bzl files

Uh oh!

chrislovecnm left a comment

Choose a reason for hiding this comment

Uh oh!

chrislovecnm Aug 7, 2023

Choose a reason for hiding this comment

Uh oh!

chrislovecnm Aug 7, 2023

Choose a reason for hiding this comment

Uh oh!

namrata-ibm commented Aug 8, 2023

Uh oh!

chrislovecnm left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

namrata-ibm commented Aug 1, 2023 •

edited

Loading

aignas commented Aug 4, 2023 •

edited

Loading