[gRPC] Enable Python 3 for Bazel to Run Tests by lidizheng · Pull Request #17644 · grpc/grpc

lidizheng · 2019-01-05T02:49:36Z

Related: #17622

With this PR, gRPC developers should be able to use --config=python3 to switch the Bazel Python interpreter to Python 3.

bazel test --config=python3 --test_output=errors //src/python/grpcio_tests/tests/...

lidizheng · 2019-01-08T00:20:46Z

@gnossen @ericgribkoff PTAL. The behavior is similar to our internal tool, except the Python 3 version is no longer pinned, but depend on which version your machine's python3 is pointing to.

gnossen

Awesome stuff, @lidizheng !

gnossen · 2019-01-08T17:10:34Z

src/python/grpcio_tests/tests/reflection/_reflection_servicer_test.py

+    # always fail.
+    def _assert_sequence_of_proto_equal(self, x, y):
+        self.assertSequenceEqual(
+            list(map(lambda x: x.SerializeToString(), x)),


Do you think we should file a bug against the python protobuf compiler for this?

I attempt several times to make a minimum reproducible example without gRPC package involved, but no luck yet.

This test appears to pass for me locally, without this change. Is bazel+python3 run as part of our CI with this PR?

It disappears after I do bazel clean --expunge. I have reverted this hack.

It maybe introduced by mixing the artifacts of Python 2 and Python 3 on my local machine... Further more, since I added force_python=PY3, the generated artifacts in latest version of this PR will not be mixed but separated in different folders. I presume it might not happen again...

Also, I enabled Python 3 Bazel test for future PRs.

From discussion offline this override here is still necessary. Please file an issue and change the NOTE(lidiz) to TODO(issue link). If I remember correctly we can't reproduce without gRPC involved, so the issue should just go on our repo for now.

third_party/py/python_configure.bzl

* Reverted hack in _reflection_servicer_test.py * To see if Kokoro is happy about it

lidizheng · 2019-01-12T00:55:28Z

Blocking Bazel Issues:

They are all pointing to the Python namespace package handling of Bazel Python rules. There are several hacks, but haven't found one that can fix ours.

ericgribkoff

@lidizheng If I understand correctly, there are two blockers here: (aside: one of the four blocking bazel issues you linked to is marked closed. Are all of those actually blocking, or just related?)

The equality failure in _reflection_servier_test.py. I believe when we discussed this offline that you thought the error may be to due to bazel making a change in the namespace of one of the proto libraries, so that the equality check in the test failed - but I don't see this detail here, and AFAIK there's not already a tracking bug for this. This shouldn't block merging of this PR, so if there's not a known issue that maps 1:1 with the failure here, please create a tracking issue on our repo (since we can't repro without gRPC yet, so the fault may be ours), link to it with a TODO on _assert_sequence_of_proto_equal, and self-assign it before merging this PR.
The import ordering errors still showing on Kokoro, which is tracked by the issue you filed earlier, bazelbuild/bazel#6844. If that is the problem here, please ping the issue on the bazel repo and say this is blocking our use of Bazel with Python 3 on Kokoro.... however, that issue wasn't specific to Python 3, so is that really the same thing that's happening here?

I'm approving the PR (as everything looks good) but let's hold off on merging this until there's a solution for (2) that gets the tests passing on Python 3. Disabling the failing tests for bazel + Python 3 (with an appropriate issue on our repo linked to in a TODO) qualifies as an acceptable solution for now as well.

lidizheng · 2019-01-14T18:16:14Z

@ericgribkoff Thanks for reviewing, I agree we should solve the blockers before merging. I hope the bug can be solved, and it can be a competent replacement for setuptools.

The "closed" issue Bazel breaks python import with init.py doesn't introduce a solid solution to the override of __init__.py behavior of Bazel. The google. namespace package is still encountering troubles while importing... I put it there because its has a thorough investigation of the problem itself and an effective workaround.

About the failure in _reflection_servier_test.py, I will open a issue in this week with a minimal reproduce example, and will do the TODO issue and comment.

About the import ordering, I presume the bazelbuild/bazel#6844 and this issue share the same root cause. The __init__.py override of Bazel was hackable in Python 2 by manipulating the import order, but in Python 3 even if we change the import order, the namespace package still doesn't work.

jtattermusch

LGTM after tests pass.

This reverts commit 206a76b.

lidizheng · 2019-01-16T20:16:16Z

Known failure #17729

lidizheng · 2019-01-16T21:27:28Z

@ericgribkoff @gnossen PTAL.

Here is the brief of the root cause:

The Bazel Python rules will create symbolic links of packages and add them to sys.path for each test. It works on most cases, except namespace packages which relies on correct parsing of the .pth file (see more).

# Content of "protobuf-3.6.1-py2.7-nspkg.pth"
import sys, types, os;has_mfs = sys.version_info > (3, 5);p = os.path.join(sys._getframe(1).f_locals['sitedir'], *('google',));importlib = has_mfs and __import__('importlib.util');has_mfs and __import__('importlib.machinery');m = has_mfs and sys.modules.setdefault('google', importlib.util.module_from_spec(importlib.machinery.PathFinder.find_spec('google', [os.path.dirname(p)])));m = m or sys.modules.setdefault('google', types.ModuleType('google'));mp = (m or []) and m.__dict__.setdefault('__path__',[]);(p not in mp) and mp.append(p)

The .pth file is just another Python file that register the module to a specific path. And the only case that the Python will parse .pth file is when the package is inside one of the site-directory.

So, my workaround for this is explicitly call a bazel_patch function that makes sure all items in sys.path are treated as site-directory, hence force Python process to load .pth file if there is any.

def bazel_patch():
    """Add valid sys.path item to site directory to parse the .pth files."""
    for item in sys.path:
        if os.path.exists(item):
            site.addsitedir(item)

ericgribkoff · 2019-01-23T19:49:15Z

third_party/py/python_configure.bzl

-    if python_bin != None:
-        return python_bin
-    python_bin_path = repository_ctx.which("python")
+    python_bin = repository_ctx.os.environ.get(_PYTHON_BIN_PATH, 'python')


Why is this changing the semantics of the PYTHON_BIN_PATH environment variable?

This library is forked from TensorFlow's repo. The main purpose of this package is locating the CPython header files to help Cython compile. In its original logic, to use Python 3, we need to set PYTHON_BIN_PATH to a absolute path like /usr/bin/python3 or /usr/local/bin/python3. It's not very adaptive across environments. So, in the new logic, even if the PYTHON_BIN_PATH is not given an absolute path, it can try to find the correct path using which.

Also, it doesn't break backward compatibility, developers who used this flag should observe no behavior change, because which command will return the exact same path as input if the input is an absolute path.

which does not allow slashes, so providing an absolute (or relative, for that matter) path causes this script to fail. At master, the following command works:

PYTHON_BIN_PATH='/usr/bin/python' bazel test --test_output=errors //src/python/grpcio_tests/tests/...

On this PR, it fails with an error.

File ".../grpc/third_party/py/python_configure.bzl", line 142, in _get_python_bin repository_ctx.which(python_bin) Program argument of which() may not contains a / or a \ ('/usr/bin/python' given) and referenced by '//src/python/grpcio/grpc/_cython:cygrpc.so'

Can you clarify why you are even changing this code? I don't see the burden of requiring an absolute path here (which does indeed seem to be the existing behavior, although it's not documented as such anywhere I could find).

I see that this is used by the build:python3 --action_env=PYTHON_BIN_PATH=python3 addition in tools/bazel.rc. If the error reported above is indeed valid, I would suggest instead adding a new environment variable (PYTHON_BIN_NAME, or something similar, which defaults to python) that can be set in bazel.rc to python3, and pass this value to the existing which query.

ericgribkoff · 2019-01-28T18:02:25Z

src/python/grpcio_tests/tests/BUILD.bazel

+    srcs = ["bazel_patch.py"],
+    visibility = ["//visibility:public"],
+    data=[
+        "//src/python/grpcio_tests/tests/unit/credentials",


This doesn't seem needed

ericgribkoff · 2019-01-28T18:13:30Z

third_party/py/python_configure.bzl

-    if python_bin != None:
-        return python_bin
-    python_bin_path = repository_ctx.which("python")
+    python_bin = repository_ctx.os.environ.get(_PYTHON_BIN_PATH, 'python')


I see that this is used by the build:python3 --action_env=PYTHON_BIN_PATH=python3 addition in tools/bazel.rc. If the error reported above is indeed valid, I would suggest instead adding a new environment variable (PYTHON_BIN_NAME, or something similar, which defaults to python) that can be set in bazel.rc to python3, and pass this value to the existing which query.

ericgribkoff · 2019-01-28T18:14:43Z

src/python/grpcio_tests/tests/BUILD.bazel

@@ -0,0 +1,8 @@
+py_library(
+    name = "bazel_patch",


This needs a scarier-sounding name, something like bazel_namespace_package_hack

ericgribkoff · 2019-01-28T18:15:07Z

src/python/grpcio_tests/tests/bazel_patch.py

+# Python process to parse the .pth file in the sys.path to resolve namespace
+# package in the right place.
+# Analysis in depth: https://github.com/bazelbuild/rules_python/issues/55
+def bazel_patch():


This should also sound scarier than patch, e.g., sys_path_to_site_dir_hack or similar

ericgribkoff · 2019-01-28T18:16:28Z

src/python/grpcio_tests/tests/BUILD.bazel

+py_library(
+    name = "bazel_patch",
+    srcs = ["bazel_patch.py"],
+    visibility = ["//visibility:public"],


Please keep this visibility restricted to a whitelist of specific tests that need it.

ericgribkoff · 2019-01-28T18:18:05Z

src/python/grpcio_tests/tests/reflection/_reflection_servicer_test.py

+    # always fail.
+    def _assert_sequence_of_proto_equal(self, x, y):
+        self.assertSequenceEqual(
+            list(map(lambda x: x.SerializeToString(), x)),


From discussion offline this override here is still necessary. Please file an issue and change the NOTE(lidiz) to TODO(issue link). If I remember correctly we can't reproduce without gRPC involved, so the issue should just go on our repo for now.

lidizheng · 2019-01-28T22:58:20Z

@ericgribkoff PTAL. All of your comments are resolved, and the new issue is filed at #17844.

ericgribkoff

LGTM

jtattermusch · 2019-01-29T08:26:12Z

Be more careful when checking test results on a PR next time. This PR introduces a breakage and it could have been prevented if test results were checked carefully (#17850 filed).

//src/python/grpcio_tests/tests/qps:basic_benchmark_test                 FAILED in 25.1s
  /root/.cache/bazel/_bazel_root/d2dc70c3d9da3fab488ba0dcbbd35051/execroot/com_github_grpc_grpc/bazel-out/k8-py3-fastbuild/testlogs/src/python/grpcio_tests/tests/qps/basic_benchmark_test/test.log

This reverts commit a25828a, reversing changes made to 5176fd8.

Revert "Merge pull request #17644 from lidizheng/bzl-py3"

This reverts commit 7da0aac.

lidizheng added 2 commits January 4, 2019 18:45

Enable Python 3

814c858

Make Python 3 pass all unit tests

0d931d9

lidizheng added lang/Python infra/Bazel release notes: yes Indicates if PR needs to be in release notes labels Jan 7, 2019

lidizheng changed the title ~~[WIP] Enable Python 3 for Bazel~~ Enable Python 3 for Bazel Jan 8, 2019

lidizheng requested review from ericgribkoff and gnossen January 8, 2019 00:20

lidizheng assigned gnossen and ericgribkoff Jan 8, 2019

gnossen approved these changes Jan 8, 2019

View reviewed changes

lidizheng added 3 commits January 8, 2019 10:08

Pump force Python version & simplify code

7105626

Enable Bazel Python 3 for every PR

7df3475

* Reverted hack in _reflection_servicer_test.py * To see if Kokoro is happy about it

Bazel clean between builds of Python 2/3

15be9de

Revert the compare of protobuf message to comparing encoded result

222db62

lidizheng force-pushed the bzl-py3 branch from b54aced to 222db62 Compare January 12, 2019 01:04

ericgribkoff approved these changes Jan 14, 2019

View reviewed changes

Upgrade Bazel to 21.0

206a76b

lidizheng requested review from apolcyn, jtattermusch and nicolasnoble as code owners January 15, 2019 18:04

lidizheng changed the title ~~Enable Python 3 for Bazel~~ [gRPC] Enable Python 3 for Bazel to Run Tests Jan 16, 2019

jtattermusch approved these changes Jan 16, 2019

View reviewed changes

lidizheng added 2 commits January 16, 2019 10:32

Revert "Upgrade Bazel to 21.0"

109a6a9

This reverts commit 206a76b.

Use monkey patch function to solve namespace package issue

17fa4b0

lidizheng force-pushed the bzl-py3 branch from fc61abb to 17fa4b0 Compare January 16, 2019 19:23

ericgribkoff reviewed Jan 23, 2019

View reviewed changes

ericgribkoff reviewed Jan 28, 2019

View reviewed changes

lidizheng added 2 commits January 28, 2019 10:47

Avoid 'which' failure

ee23fc3

Adopt reviewer's advices

7e90dad

lidizheng mentioned this pull request Jan 28, 2019

Redundant gRPC Python Protobuf Message Descriptor with Bazel + Python 3 #17844

Closed

Point the hack of proto message comparison to new issue

fcbb126

ericgribkoff approved these changes Jan 28, 2019

View reviewed changes

lidizheng merged commit a25828a into grpc:master Jan 29, 2019

yashykt mentioned this pull request Jan 29, 2019

Bazel Basic Tests for Python (Local) failure //src/python/grpcio_tests/tests/qps:basic_benchmark_test #17850

Closed

ericgribkoff added a commit to ericgribkoff/grpc that referenced this pull request Jan 29, 2019

Revert "Merge pull request grpc#17644 from lidizheng/bzl-py3"

7da0aac

This reverts commit a25828a, reversing changes made to 5176fd8.

ericgribkoff mentioned this pull request Jan 29, 2019

Revert "Merge pull request #17644 from lidizheng/bzl-py3" #17865

Merged

ericgribkoff added a commit that referenced this pull request Jan 29, 2019

Merge pull request #17865 from ericgribkoff/revert_bazel_py3

82dd9df

Revert "Merge pull request #17644 from lidizheng/bzl-py3"

lidizheng mentioned this pull request Jan 30, 2019

[Roll Forward] Enable Python 3 for Bazel to Run Tests #17881

Merged

lidizheng added a commit to lidizheng/grpc that referenced this pull request Feb 20, 2019

Revert "Revert "Merge pull request grpc#17644 from lidizheng/bzl-py3""

d209deb

This reverts commit 7da0aac.

lock bot locked as resolved and limited conversation to collaborators Apr 29, 2019

lidizheng added the area/infra label Feb 19, 2020

Conversation

lidizheng commented Jan 5, 2019

Uh oh!

lidizheng commented Jan 8, 2019

Uh oh!

gnossen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

lidizheng commented Jan 12, 2019

Uh oh!

ericgribkoff left a comment

Choose a reason for hiding this comment

Uh oh!

lidizheng commented Jan 14, 2019

Uh oh!

jtattermusch left a comment

Choose a reason for hiding this comment

Uh oh!

lidizheng commented Jan 16, 2019

Uh oh!

lidizheng commented Jan 16, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lidizheng commented Jan 28, 2019

Uh oh!

ericgribkoff left a comment

Choose a reason for hiding this comment

Uh oh!

jtattermusch commented Jan 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jtattermusch commented Jan 29, 2019 •

edited

Loading