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

feat: Add s390x release #1352

Merged
merged 1 commit into from
Aug 8, 2023
Merged

Conversation

namrata-ibm
Copy link
Contributor

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
Copy link
Contributor Author

namrata-ibm commented Aug 1, 2023

@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
Copy link
Collaborator

aignas commented Aug 1, 2023

@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
Copy link
Contributor Author

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
Copy link
Collaborator

aignas commented Aug 1, 2023 via email

@chrislovecnm
Copy link
Collaborator

@namrata-ibm
Copy link
Contributor Author

@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
Copy link
Collaborator

aignas commented Aug 4, 2023

@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
Copy link
Contributor Author

@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
Copy link
Collaborator

aignas commented Aug 5, 2023

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

aignas added a commit to aignas/rules_python that referenced this pull request Aug 5, 2023
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 bazelbuild#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 bazelbuild#1339
aignas added a commit to aignas/rules_python that referenced this pull request Aug 5, 2023
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 bazelbuild#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 bazelbuild#1339
@chrislovecnm
Copy link
Collaborator

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

@aignas did we hit a bug in buildifier?

@namrata-ibm
Copy link
Contributor Author

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

@chrislovecnm
Copy link
Collaborator

@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

Copy link
Collaborator

@chrislovecnm chrislovecnm left a comment

Choose a reason for hiding this comment

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

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

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

"x86_64-unknown-linux-gnu": "e26247302bc8e9083a43ce9e8dd94905b40d464745b1603041f7bc9a93c65d05",
},
"strip_prefix": "python",
},
}

# buildifier: disable=unsorted-dict-items
MINOR_MAPPING = {
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

@@ -153,6 +153,19 @@ TOOL_VERSIONS = {
},
"strip_prefix": "python",
},
"3.9.17": {
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Needed to build TensorFlow on s390x.
@namrata-ibm
Copy link
Contributor Author

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

Copy link
Collaborator

@chrislovecnm chrislovecnm left a comment

Choose a reason for hiding this comment

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

LGTM

@chrislovecnm chrislovecnm added this pull request to the merge queue Aug 8, 2023
Merged via the queue into bazelbuild:main with commit e549810 Aug 8, 2023
2 checks passed
github-merge-queue bot pushed a commit that referenced this pull request Aug 10, 2023
This PR bumps mappings to latest version. 

Adding changes in a separate PR as discussed
[here](#1352 (review)).

cc @chrislovecnm
aignas added a commit to aignas/rules_python that referenced this pull request Aug 11, 2023
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 bazelbuild#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 bazelbuild#1339
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.

None yet

3 participants