[Core] Upgrade grpc from 1.46.6 to 1.57.0 #39210

jjyao · 2023-09-01T16:24:01Z

Why are these changes needed?

1.46.6 has a memory leak and 1.57.0 fixes it.

grpc: 1.46.6 -> 1.57.0
protobuf: v19.4 -> v23.4
absl: 2022062.1 -> 20230125.3
boringssl: b9232f9e27e5668bc0414879dcdedb2a59ea75f2 -> 342e805bc1f5dfdd650e3f031686d6c939b095d9

For a serve workload, the memory usage before:

after:

NOTE
For now, we check in the auto generated proto source files for python since we need to use an old version of protoc to support python 3.7. Once we deprecate 3.7, we can undo this part.

Related issue number

Closes #38591

Checks

I've signed off every commit(by using the -s flag, i.e., git commit -s) in this PR.
I've run scripts/format.sh to lint the changes in this PR.
I've included any doc changes needed for https://docs.ray.io/en/master/.
- I've added any new APIs to the API Reference. For example, if I added a
  method in Tune, I've added it in doc/source/tune/api/ under the
  corresponding .rst file.
I've made sure the tests are passing. Note that there might be a few flaky tests, see the recent failures at https://flakey-tests.ray.io/
Testing Strategy
- Unit tests
- Release tests
- This PR is not tested :(

Signed-off-by: Jiajun Yao <jeromeyjj@gmail.com>

jjyao · 2023-09-01T16:25:27Z

cpp/src/ray/runtime/task/local_mode_task_submitter.h

@@ -54,7 +54,8 @@ class LocalModeTaskSubmitter : public TaskSubmitter {

  absl::Mutex actor_contexts_mutex_;

-  std::unordered_map<std::string, ActorID> named_actors_ GUARDED_BY(named_actors_mutex_);
+  std::unordered_map<std::string, ActorID> named_actors_
+      ABSL_GUARDED_BY(named_actors_mutex_);


Breaking change from absl:

The legacy spellings of the thread annotation macros/functions (e.g. GUARDED_BY()) [have been removed by default](https://github.com/abseil/abseil-cpp/commit/6acb60c161f1203e6eca929b87f2041da7714bfe) in favor of the ABSL_ prefixed versions (e.g. ABSL_GUARDED_BY()) due to clashes with other libraries. The compatibility macro ABSL_LEGACY_THREAD_ANNOTATIONS can be defined on the compile command-line to temporarily restore these spellings, but this compatibility macro will be removed in the future.

Signed-off-by: Jiajun Yao <jeromeyjj@gmail.com>

jjyao · 2023-09-01T16:54:57Z

python/setup.py

@@ -349,7 +349,7 @@ def get_packages(self):
        "numpy >= 1.16; python_version < '3.9'",
        "numpy >= 1.19.3; python_version >= '3.9'",
        "packaging",
-        "protobuf >= 3.15.3, != 3.19.5",
+        "protobuf >= 3.20.0",


https://stackoverflow.com/questions/71759248/importerror-cannot-import-name-builder-from-google-protobuf-internal

Signed-off-by: Jiajun Yao <jeromeyjj@gmail.com>

edoakes · 2023-09-01T18:08:19Z

@jjyao can you post screenshots of the before and after memory usage for the serve workload?

Signed-off-by: Jiajun Yao <jeromeyjj@gmail.com>

rkooo567 · 2023-09-02T02:44:47Z

Note: all the core tests are failing





ImportError: cannot import name 'builder' from 'google.protobuf.internal' (/opt/miniconda/lib/python3.8/site-packages/google/protobuf/internal/__init__.py)
 

<br class="Apple-interchange-newline">

rkooo567 · 2023-09-02T02:45:49Z

.bazelrc

@@ -8,6 +8,11 @@ build:windows --action_env=PATH
 # For --compilation_mode=dbg, consider enabling checks in the standard library as well (below).
 build --compilation_mode=opt
 # Using C++ 17 on all platforms.
+build:linux --host_cxxopt="-std=c++17"


Are these necessary?

This is needed to compile upgraded absil.

rkooo567 · 2023-09-02T02:47:03Z

bazel/ray_deps_setup.bzl

+        strip_prefix = "protobuf-2c5fa078d8e86e5f4bd34e6f4c9ea9e8d7d4d44a",
+        urls = [
+            # https://github.com/protocolbuffers/protobuf/commits/v23.4
+            "https://github.com/protocolbuffers/protobuf/archive/2c5fa078d8e86e5f4bd34e6f4c9ea9e8d7d4d44a.tar.gz",


Does it mean we will use the lower version? (3.19 -> 2.3?)

It's not 2.3 but 23 so 19 -> 23

rkooo567 · 2023-09-02T02:48:47Z

bazel/ray_deps_setup.bzl

@@ -279,11 +289,11 @@ def ray_deps_setup():
        # https://github.com/grpc/grpc/blob/1ff1feaa83e071d87c07827b0a317ffac673794f/bazel/grpc_deps.bzl#L189
        # Ensure this rule matches the rule used by grpc's bazel/grpc_deps.bzl
        name = "boringssl",
-        sha256 = "534fa658bd845fd974b50b10f444d392dfd0d93768c4a51b61263fd37d851c40",
-        strip_prefix = "boringssl-b9232f9e27e5668bc0414879dcdedb2a59ea75f2",
+        sha256 = "0675a4f86ce5e959703425d6f9063eaadf6b61b7f3399e77a154c0e85bad46b1",


how did we choose this? is it for a specific version (add a comment for the version?)

# Ensure this rule matches the rule used by grpc's bazel/grpc_deps.bzl

rkooo567 · 2023-09-02T02:49:33Z

bazel/ray_deps_setup.bzl

+        patches = [
+            "@com_github_grpc_grpc//third_party:protobuf.patch",
+        ],
+        patch_args = ["-p1"],


can you comment why this patch is needed from code?

# This is copied from grpc's bazel/grpc_deps.bzlc

rkooo567 · 2023-09-02T02:49:59Z

java/dependencies.bzl

@@ -8,8 +8,8 @@ def gen_java_deps():
            "com.github.java-json-tools:json-schema-validator:2.2.14",
            "com.google.code.gson:gson:2.9.1",
            "com.google.guava:guava:32.0.1-jre",
-            "com.google.protobuf:protobuf-java:3.19.6",
-            "com.google.protobuf:protobuf-java-util:3.19.6",
+            "com.google.protobuf:protobuf-java:3.23.4",


maybe not necessary?

Needed since we upgraded protoc.

rkooo567 · 2023-09-02T02:50:47Z

thirdparty/patches/grpc-cython-copts.patch

@@ -30,6 +30,8 @@ diff --git bazel/cython_library.bzl bazel/cython_library.bzl
 +            srcs = [stem + ".cpp"] + cc_kwargs.pop("srcs", []),


can you tell me why we need these changes?

Existing patches, I don't know why we need it in the first place.

rkooo567 · 2023-09-02T02:52:17Z

thirdparty/patches/opencensus-cpp-shutdown-api.patch

@@ -5,7 +5,7 @@ index 3bb5962..706b9b4 100644
 @@ -25,7 +25,7 @@ namespace opencensus {
 namespace exporters {
 namespace stats {
-


why are there logic changes in this file?

No logic change, only GUARDED_BY -> ABSL_GUARDED_BY

rkooo567 · 2023-09-02T02:53:05Z

thirdparty/patches/grpc-python.patch

@@ -1,12 +0,0 @@
-diff --git third_party/py/python_configure.bzl third_party/py/python_configure.bzl


should we delete a file? Seems like it is not used?

Signed-off-by: Jiajun Yao <jeromeyjj@gmail.com>

fishbone · 2023-09-04T19:36:47Z

BUILD.bazel

+        "@io_opencensus_proto//opencensus/proto/metrics/v1:metrics.proto",
+        "@io_opencensus_proto//opencensus/proto/resource/v1:resource.proto",
+    ] + glob([
+        "src/ray/protobuf/**/*.proto",


I feel not all proto is needed actually. For example, not all proto is compiled with py.

Yea, I just keep it simple to compile everything, it should be no harm.

Signed-off-by: Jiajun Yao <jeromeyjj@gmail.com>

edoakes

codeowner stamp for lint change

rkooo567

LGTM if tests pass from my end (please get @iycheng;s approval before merging it!). It seems like it has 13K lines of changes, is this expected?

fishbone · 2023-09-06T22:04:14Z

@rkooo567 I think this is unfortunate :(

Let's clean this part with the highest priority after the summit @jjyao

Signed-off-by: Jiajun Yao <jeromeyjj@gmail.com>

aslonnie · 2023-09-06T23:10:51Z

It seems like it has 13K lines of changes, is this expected?

I think a lot of it is checking in generated pb files.

jjyao · 2023-09-07T05:06:46Z

Core nightly: https://buildkite.com/ray-project/release-tests-pr/builds/52249#_

rkooo567 · 2023-09-07T06:52:45Z

@jjyao I think the core nightly test failure is fixed by 313a923. You can double check it by downloading a log file from gcs_server.out and check if the check failure is from the autoscaler manager

Can you also post the performance difference from the master here just in case to see if there's a big change?

Signed-off-by: Jiajun Yao <jeromeyjj@gmail.com>

jjyao · 2023-09-07T16:14:40Z

Microbenchmark:



single_client_get_calls_Plasma_Store = [10093.560227255326, 79.94432751246542]
--
  | single_client_put_calls_Plasma_Store = [5471.864933158648, 77.70833287530638]
  | multi_client_put_calls_Plasma_Store = [12589.685634208958, 301.70029150589943]
  | single_client_put_gigabytes = [18.872557806854374, 4.919710267611302]
  | single_client_tasks_and_get_batch = [9.987917616953759, 0.22729131561338817]
  | multi_client_put_gigabytes = [37.13926701774409, 2.090084721396276]
  | single_client_get_object_containing_10k_refs = [14.861631575625921, 0.2784344123799417]
  | single_client_wait_1k_refs = [5.186563025540161, 0.17960575727533448]
  | single_client_tasks_sync = [1153.9544609729708, 12.884716556349781]
  | single_client_tasks_async = [10847.68019096285, 140.62998721211127]
  | multi_client_tasks_async = [33906.66399830253, 1032.6713613141028]
  | 1_1_actor_calls_sync = [2276.6743562161837, 46.55844259513228]
  | 1_1_actor_calls_async = [7999.982926145655, 177.79202152826016]
  | 1_1_actor_calls_concurrent = [4914.962538438439, 205.359840538267]
  | 1_n_actor_calls_async = [10837.931244604233, 181.07739735803023]
  | n_n_actor_calls_async = [31542.914183828845, 544.3859776512819]
  | n_n_actor_calls_with_arg_async = [2985.308087160443, 20.223620426350703]
  | 1_1_async_actor_calls_sync = [1369.8267490983353, 23.629841247319142]
  | 1_1_async_actor_calls_async = [2906.732546793207, 106.32202389863562]
  | 1_1_async_actor_calls_with_args_async = [2029.1677566872995, 93.01617892866881]
  | 1_n_async_actor_calls_async = [9490.779932946629, 70.19529690530324]
  | n_n_async_actor_calls_async = [25918.686289834965, 312.93067513177465]
  | placement_group_create/removal = [895.525259837609, 7.231674604368287]
  | client__get_calls = [1202.9198053395726, 38.9750139739174]
  | client__put_calls = [857.4076319486344, 15.278249054607969]
  | client__put_gigabytes = [0.07557427646923043, 0.0015435192016046153]
  | client__tasks_and_put_batch = [11657.759680450217, 166.3039789077301]
  | client__1_1_actor_calls_sync = [546.5581484960015, 19.95411267369889]
  | client__1_1_actor_calls_async = [1060.8005790473765, 47.92145784771617]
  | client__1_1_actor_calls_concurrent = [949.0861020977036, 19.170808568330436]
  | client__tasks_and_get_batch = [1.0392002851143174, 0.020889671274458985]

Seems ok.

Signed-off-by: Jiajun Yao <jeromeyjj@gmail.com>

rickyyx

Stamping for test_state_api_log being flaky for known problems. #39410

1.46.6 has a memory leak and 1.57.0 fixes it. grpc: 1.46.6 -> 1.57.0 protobuf: v19.4 -> v23.4 absl: 2022062.1 -> 20230125.3 boringssl: b9232f9e27e5668bc0414879dcdedb2a59ea75f2 -> 342e805bc1f5dfdd650e3f031686d6c939b095d9 Signed-off-by: Jiajun Yao <jeromeyjj@gmail.com> Co-authored-by: Yi Cheng <74173148+iycheng@users.noreply.github.com>

* [Core] Upgrade grpc from 1.46.6 to 1.57.0 (#39210) 1.46.6 has a memory leak and 1.57.0 fixes it. grpc: 1.46.6 -> 1.57.0 protobuf: v19.4 -> v23.4 absl: 2022062.1 -> 20230125.3 boringssl: b9232f9e27e5668bc0414879dcdedb2a59ea75f2 -> 342e805bc1f5dfdd650e3f031686d6c939b095d9 Signed-off-by: Jiajun Yao <jeromeyjj@gmail.com> Co-authored-by: Yi Cheng <74173148+iycheng@users.noreply.github.com> * fix Signed-off-by: Jiajun Yao <jeromeyjj@gmail.com> * up Signed-off-by: Jiajun Yao <jeromeyjj@gmail.com> * fix Signed-off-by: Jiajun Yao <jeromeyjj@gmail.com> --------- Signed-off-by: Jiajun Yao <jeromeyjj@gmail.com> Co-authored-by: Yi Cheng <74173148+iycheng@users.noreply.github.com>

1.46.6 has a memory leak and 1.57.0 fixes it. grpc: 1.46.6 -> 1.57.0 protobuf: v19.4 -> v23.4 absl: 2022062.1 -> 20230125.3 boringssl: b9232f9e27e5668bc0414879dcdedb2a59ea75f2 -> 342e805bc1f5dfdd650e3f031686d6c939b095d9 Signed-off-by: Jiajun Yao <jeromeyjj@gmail.com> Co-authored-by: Yi Cheng <74173148+iycheng@users.noreply.github.com> Signed-off-by: Jim Thompson <jimthompson5802@gmail.com>

1.46.6 has a memory leak and 1.57.0 fixes it. grpc: 1.46.6 -> 1.57.0 protobuf: v19.4 -> v23.4 absl: 2022062.1 -> 20230125.3 boringssl: b9232f9e27e5668bc0414879dcdedb2a59ea75f2 -> 342e805bc1f5dfdd650e3f031686d6c939b095d9 Signed-off-by: Jiajun Yao <jeromeyjj@gmail.com> Co-authored-by: Yi Cheng <74173148+iycheng@users.noreply.github.com> Signed-off-by: Victor <vctr.y.m@example.com>

Revert back to the old way (before #39210) of auto-generating protobuf and grpc code so we don't need to manually generate and check them in. Also decouple the protobuf version we use in c++ and the protobuf version we use to auto generate python and java code (the protobuf version to auto generate python code affects the python protobuf library version that Ray can support) so that they can be upgraded independently. Signed-off-by: Jiajun Yao <jeromeyjj@gmail.com>

Upgrade grpc from 1.46.6 to 1.57.0

ea63987

Signed-off-by: Jiajun Yao <jeromeyjj@gmail.com>

jjyao requested review from kfstorm, raulchen, ericl, fishbone, WangTaoTheTonic, SongGuyang and a team as code owners September 1, 2023 16:24

jjyao commented Sep 1, 2023

View reviewed changes

jjyao assigned fishbone Sep 1, 2023

rkooo567 self-assigned this Sep 1, 2023

up

819796f

Signed-off-by: Jiajun Yao <jeromeyjj@gmail.com>

jjyao requested review from richardliaw, edoakes and a team as code owners September 1, 2023 16:53

jjyao commented Sep 1, 2023

View reviewed changes

jjyao added 2 commits September 1, 2023 10:20

up

ce52a84

Signed-off-by: Jiajun Yao <jeromeyjj@gmail.com>

up

2ed0ebd

Signed-off-by: Jiajun Yao <jeromeyjj@gmail.com>

jjyao added 2 commits September 1, 2023 13:08

fix

d07b6cf

Signed-off-by: Jiajun Yao <jeromeyjj@gmail.com>

up

476db83

Signed-off-by: Jiajun Yao <jeromeyjj@gmail.com>

rkooo567 reviewed Sep 2, 2023

View reviewed changes

rkooo567 assigned aslonnie Sep 2, 2023

jjyao added 2 commits September 3, 2023 15:42

up

ea88c87

Signed-off-by: Jiajun Yao <jeromeyjj@gmail.com>

up

2c03909

Signed-off-by: Jiajun Yao <jeromeyjj@gmail.com>

fishbone reviewed Sep 4, 2023

View reviewed changes

jjyao added 3 commits September 4, 2023 14:31

try

df2a2e8

Signed-off-by: Jiajun Yao <jeromeyjj@gmail.com>

try

f861519

Signed-off-by: Jiajun Yao <jeromeyjj@gmail.com>

try

c1a082b

Signed-off-by: Jiajun Yao <jeromeyjj@gmail.com>

edoakes approved these changes Sep 6, 2023

View reviewed changes

rkooo567 approved these changes Sep 6, 2023

View reviewed changes

comments

447fb55

Signed-off-by: Jiajun Yao <jeromeyjj@gmail.com>

jjyao added 4 commits September 7, 2023 01:30

fix ray

6f762d2

Signed-off-by: Jiajun Yao <jeromeyjj@gmail.com>

comments

9783065

Signed-off-by: Jiajun Yao <jeromeyjj@gmail.com>

gitattributes

1fb6d80

Signed-off-by: Jiajun Yao <jeromeyjj@gmail.com>

Merge branch 'master' of github.com:ray-project/ray into jjyao/upgrade

b291345

Mark test_actor_bounded_threads as flaky

a351711

Signed-off-by: Jiajun Yao <jeromeyjj@gmail.com>

jjyao mentioned this pull request Sep 7, 2023

[Core] [Test] Mark the test test_actor_bounded_threads flaky #39390

Closed

Mark test_state_api_log as flaky

3e3f3c7

Signed-off-by: Jiajun Yao <jeromeyjj@gmail.com>

rickyyx approved these changes Sep 7, 2023

View reviewed changes

jjyao merged commit 07d6e67 into ray-project:master Sep 7, 2023
73 of 111 checks passed

jjyao deleted the jjyao/upgrade branch September 7, 2023 23:03

This was referenced Sep 7, 2023

[Core] Upgrade grpc from 1.46.6 to 1.57.0 (#39210) #39419

Merged

[Core] Update usage_pb2.py #39425

Merged

This was referenced Oct 19, 2023

Ray >=2.8 will not support Python 3.7 #39395

Closed

Use ray-project fork in CI build and Dockerfile beacon-biosignals/Ray.jl#202

Draft

rickyyx mentioned this pull request Oct 24, 2023

[core] microbenchmark regression #40606

Closed

jjyao mentioned this pull request Oct 25, 2023

[Core] Auto generate protobuf and grpc code #40663

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Core] Upgrade grpc from 1.46.6 to 1.57.0 #39210

[Core] Upgrade grpc from 1.46.6 to 1.57.0 #39210

jjyao commented Sep 1, 2023 •

edited

Loading

jjyao Sep 1, 2023

jjyao Sep 1, 2023

edoakes commented Sep 1, 2023

rkooo567 commented Sep 2, 2023

rkooo567 Sep 2, 2023

jjyao Sep 6, 2023

rkooo567 Sep 2, 2023

jjyao Sep 6, 2023

rkooo567 Sep 2, 2023

jjyao Sep 6, 2023

rkooo567 Sep 2, 2023

jjyao Sep 6, 2023

rkooo567 Sep 2, 2023

jjyao Sep 6, 2023

rkooo567 Sep 2, 2023

jjyao Sep 6, 2023

rkooo567 Sep 2, 2023

jjyao Sep 6, 2023

rkooo567 Sep 2, 2023

fishbone Sep 4, 2023

jjyao Sep 6, 2023

edoakes left a comment

rkooo567 left a comment

fishbone commented Sep 6, 2023

aslonnie commented Sep 6, 2023

jjyao commented Sep 7, 2023

rkooo567 commented Sep 7, 2023

jjyao commented Sep 7, 2023

rickyyx left a comment •

edited

Loading

		@@ -30,6 +30,8 @@ diff --git bazel/cython_library.bzl bazel/cython_library.bzl
		+ srcs = [stem + ".cpp"] + cc_kwargs.pop("srcs", []),

		@@ -1,12 +0,0 @@
		diff --git third_party/py/python_configure.bzl third_party/py/python_configure.bzl

[Core] Upgrade grpc from 1.46.6 to 1.57.0 #39210

[Core] Upgrade grpc from 1.46.6 to 1.57.0 #39210

Conversation

jjyao commented Sep 1, 2023 • edited Loading

Why are these changes needed?

Related issue number

Checks

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edoakes commented Sep 1, 2023

rkooo567 commented Sep 2, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edoakes left a comment

Choose a reason for hiding this comment

rkooo567 left a comment

Choose a reason for hiding this comment

fishbone commented Sep 6, 2023

aslonnie commented Sep 6, 2023

jjyao commented Sep 7, 2023

rkooo567 commented Sep 7, 2023

jjyao commented Sep 7, 2023

rickyyx left a comment • edited Loading

Choose a reason for hiding this comment

jjyao commented Sep 1, 2023 •

edited

Loading

rickyyx left a comment •

edited

Loading