Fix release test -- client remote put #15325

richardliaw · 2021-04-15T04:05:12Z

Why are these changes needed?

Enables a system config for the ray client tests:

New output for remote put:

2021-04-14 20:52:48,892	INFO logservicer.py:102 -- New logs connection established. Total clients: 1
client: remote put calls per second 68286.7 +- 3223.44

Prior to this PR:

client: remote put calls per second 7104.87 +- 154.42

Full run:

(raybuild) ➜  scripts git:(fix-benchmark) ✗ ray microbenchmark
Tip: set TESTS_TO_RUN='pattern' to run a subset of benchmarks
2021-04-14 20:47:54,835	INFO services.py:1264 -- View the Ray dashboard at http://127.0.0.1:8265
single client get calls per second 51038.52 +- 1901.46
single client put calls per second 31553.03 +- 1743.21
multi client put calls per second 97801.13 +- 2437.81
2021-04-14 20:48:26,564	INFO services.py:1264 -- View the Ray dashboard at http://127.0.0.1:8265
single client get calls (Plasma Store) per second 9160.23 +- 619.74
single client put calls (Plasma Store) per second 4707.35 +- 100.23
multi client put calls (Plasma Store) per second 6917.51 +- 417.71
single client put gigabytes per second 2.62 +- 2.41
multi client put gigabytes per second 3.46 +- 0.53
single client tasks sync per second 1835.03 +- 78.99
single client tasks async per second 12123.7 +- 296.05
multi client tasks async per second 19161.18 +- 715.67
1:1 actor calls sync per second 2703.54 +- 83.52
1:1 actor calls async per second 7024.55 +- 188.21
1:1 actor calls concurrent per second 7773.28 +- 519.77
1:n actor calls async per second 14367.64 +- 334.6
n:n actor calls async per second 24377.59 +- 828.75
n:n actor calls with arg async per second 8593.7 +- 213.81
1:1 async-actor calls sync per second 1759.53 +- 52.62
1:1 async-actor calls async per second 3961.22 +- 77.88
1:1 async-actor calls with args async per second 3017.21 +- 27.51
1:n async-actor calls async per second 9922.37 +- 259.87
n:n async-actor calls async per second 14935.92 +- 664.38
2021-04-14 20:52:22,179	INFO logservicer.py:102 -- New logs connection established. Total clients: 1
client: get calls per second 2660.24 +- 90.76
2021-04-14 20:52:35,455	INFO logservicer.py:102 -- New logs connection established. Total clients: 1
client: put calls per second 1451.36 +- 7.64
2021-04-14 20:52:48,892	INFO logservicer.py:102 -- New logs connection established. Total clients: 1
client: remote put calls per second 68286.7 +- 3223.44
2021-04-14 20:53:02,733	INFO logservicer.py:102 -- New logs connection established. Total clients: 1
2021-04-14 20:53:03,820	INFO services.py:1264 -- View the Ray dashboard at http://127.0.0.1:8265
client: 1:1 actor calls sync per second 814.32 +- 14.79
client: 1:1 actor calls async per second 908.16 +- 13.03
client: 1:1 actor calls concurrent per second 910.06 +- 9.59

Related issue number

Addresses part of #15247

Checks

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 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: Richard Liaw <rliaw@berkeley.edu>

fishbone · 2021-04-15T04:15:24Z

python/ray/util/client/server/dataservicer.py

+                if self.num_clients == 0:
+                    with disable_client_hook():


Why do we change swap this?

fishbone · 2021-04-15T04:17:26Z

python/ray/_private/ray_client_microbenchmark.py

+    }
+
+    def ray_connect_handler(job_config=None):
+        import ray as real_ray


should import ray just work? I feel disable_client_hook will take care of everything.

Well, if you don't have the import in here and only the disable_client_hook, the behavior is actually unexpected (and fails).

So it's important to have some form of import in here, but I felt that real_ray was more readable and would prevent scoping problems

Ah, you misunderstood me.
I mean we don't necessarily rename it to real_ray since it doesn't quite make sense here.

If you don't call with disable_client_hook, it's actually not real_ray, right?

OK i tried a small fix here!

fishbone · 2021-04-15T04:18:44Z

@richardliaw could you give more details about this PR? Or could you give any link to the issues related to this PR? I don't quite understand what's the issue fixed here.

python/ray/util/client/server/dataservicer.py

richardliaw · 2021-04-15T04:53:45Z

Thanks @iycheng for the fast review! I've added some contextual information in the PR description. Please take a look again?

fishbone

Overall, I feel good about this PR. I'll give an approval. The comment is NIT. It's up to you to change this.

python/ray/_private/ray_client_microbenchmark.py

krfricke

Looks good to me - as ray_start_client_server seems to be a testing utility I'm fine with changing this just for this benchmarking test.

I'm wondering though if we should move the new ray connect handler to ray_start_cluster_client_server_pair(), but I'm lacking client code context to really judge that.

Happy to go forward with this solution for now as it won't affect other tests.

ijrsvt

This looks good to me. It may make sense to move other tests to use this fixture in the future, but just scoping it to perf tests for now is okay with me.

wuisawesome · 2021-04-15T20:30:33Z

4 Approvals oughta be enough.

test_object_spilling_2 failure looks unrelated

merging

* fix-test Signed-off-by: Richard Liaw <rliaw@berkeley.edu> * fix Signed-off-by: Richard Liaw <rliaw@berkeley.edu> * Update python/ray/util/client/server/dataservicer.py * Update python/ray/util/client/server/dataservicer.py * Update python/ray/_private/ray_client_microbenchmark.py

richardliaw added 2 commits April 14, 2021 19:40

fix-test

0448fb6

Signed-off-by: Richard Liaw <rliaw@berkeley.edu>

fix

779a8f8

Signed-off-by: Richard Liaw <rliaw@berkeley.edu>

richardliaw requested review from amogkam, krfricke and rkooo567 April 15, 2021 04:06

richardliaw assigned amogkam, krfricke, rkooo567, fishbone, ericl, wuisawesome and ijrsvt Apr 15, 2021

richardliaw added the release-blocker P0 Issue that blocks the release label Apr 15, 2021

fishbone reviewed Apr 15, 2021

View reviewed changes

richardliaw commented Apr 15, 2021

View reviewed changes

python/ray/util/client/server/dataservicer.py Outdated Show resolved Hide resolved

Update python/ray/util/client/server/dataservicer.py

223f2e3

richardliaw commented Apr 15, 2021

View reviewed changes

python/ray/util/client/server/dataservicer.py Outdated Show resolved Hide resolved

Update python/ray/util/client/server/dataservicer.py

a739d9c

fishbone approved these changes Apr 15, 2021

View reviewed changes

richardliaw commented Apr 15, 2021

View reviewed changes

python/ray/_private/ray_client_microbenchmark.py Outdated Show resolved Hide resolved

Update python/ray/_private/ray_client_microbenchmark.py

0a2b878

krfricke approved these changes Apr 15, 2021

View reviewed changes

ijrsvt approved these changes Apr 15, 2021

View reviewed changes

wuisawesome approved these changes Apr 15, 2021

View reviewed changes

wuisawesome merged commit eaa3ce3 into ray-project:master Apr 15, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix release test -- client remote put #15325

Fix release test -- client remote put #15325

richardliaw commented Apr 15, 2021 •

edited

Loading

fishbone Apr 15, 2021

fishbone Apr 15, 2021

richardliaw Apr 15, 2021

fishbone Apr 15, 2021

richardliaw Apr 15, 2021

fishbone commented Apr 15, 2021

richardliaw commented Apr 15, 2021

fishbone left a comment

krfricke left a comment

ijrsvt left a comment

wuisawesome commented Apr 15, 2021

Fix release test -- client remote put #15325

Fix release test -- client remote put #15325

Conversation

richardliaw commented Apr 15, 2021 • edited Loading

Why are these changes needed?

Related issue number

Checks

fishbone Apr 15, 2021

Choose a reason for hiding this comment

fishbone Apr 15, 2021

Choose a reason for hiding this comment

richardliaw Apr 15, 2021

Choose a reason for hiding this comment

fishbone Apr 15, 2021

Choose a reason for hiding this comment

richardliaw Apr 15, 2021

Choose a reason for hiding this comment

fishbone commented Apr 15, 2021

richardliaw commented Apr 15, 2021

fishbone left a comment

Choose a reason for hiding this comment

krfricke left a comment

Choose a reason for hiding this comment

ijrsvt left a comment

Choose a reason for hiding this comment

wuisawesome commented Apr 15, 2021

richardliaw commented Apr 15, 2021 •

edited

Loading