[ray_client] close ray connection upon client deactivation #13919

richardliaw · 2021-02-04T23:53:38Z

Why are these changes needed?

Disconnects from Ray when num_clients drops to 0. This kills actors/tasks when job disconnects.

However, I think this doesn't really work if you have multiple clients.

Questions:

~~I disabled a check for ray initialization upon client connection. Is this safe? (probably not?)~~ Fixed
~~Should we create a new protobuf type for "servicer alive" (as opposed to servicer alive == ray connected)?~~
~~What's the right way to write a test here?~~ Done

Related issue number

Closes #13354

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>

ericl · 2021-02-05T00:12:20Z

python/ray/util/client/worker.py

-                    break
-                # Ray is not ready yet, wait a timeout
-                time.sleep(timeout)
+                # # Now the HTTP2 channel is ready, or proxied, but the


Hmm what problem was this causing? Shouldn't is_initialized still work as before after the client connects?

Oh I see, the data connection isn't added until later. How about we add a new dummy ping operation here then instead of is_initialized()? It can be anything really, is_initialized was just used as a convenient no_op.

is_initialized() also had the advantage of checking if ray was initialized, which is kinda useful here. What you might want to do though is do the data connection earlier. you might need to wait for /it/ to return ready (meaning the send/receive loop thread pair is kicked off and sets a flag, or even better, returns its client_id) -- but in general, yeah, some op here is useful.

OK i did something, take a look! happy to hear any suggestions

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

ericl · 2021-02-05T00:15:09Z

Great start! Comments inline.

However, I think this doesn't really work if you have multiple clients.

Yeah this is probably fine for now, if we consider the common use case here a cluster to have one or zero clients. There might be extra clients attached just for debugging purposes.

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

barakmich · 2021-02-05T00:34:51Z

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

-                address=args.redis_address,
-                _redis_password=args.redis_password)
+
+    def ray_connect_handler():


Maybe make this top-level and pass in redis_address/redis_password?

barakmich · 2021-02-05T00:38:06Z

python/ray/util/client/worker.py

-                    break
-                # Ray is not ready yet, wait a timeout
-                time.sleep(timeout)
+                # # Now the HTTP2 channel is ready, or proxied, but the


is_initialized() also had the advantage of checking if ray was initialized, which is kinda useful here. What you might want to do though is do the data connection earlier. you might need to wait for /it/ to return ready (meaning the send/receive loop thread pair is kicked off and sets a flag, or even better, returns its client_id) -- but in general, yeah, some op here is useful.

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

barakmich · 2021-02-05T00:41:33Z

src/ray/protobuf/ray_client.proto

@@ -139,6 +139,7 @@ message ClusterInfoType {
    CLUSTER_RESOURCES = 2;
    AVAILABLE_RESOURCES = 3;
    RUNTIME_CONTEXT = 4;
+    IS_ALIVE = 5;


Not sure if a protocol change is strictly necessary -- we know it's alive, for instance, if the handshake from the data channel completes (which is what you'd want to test anyway)

Yeah agreed; I'm ok with this though as an explicit check since it seems straightforward.

This reverts commit 0410c37.

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

richardliaw · 2021-02-05T05:02:15Z

python/ray/tests/test_client_init.py

-        ray.disconnect()
-    finally:
-        ray_client_server.shutdown_with_server(server_handle.grpc_server)
-        time.sleep(2)


I just moved this stuff into the fixture

richardliaw · 2021-02-05T05:03:32Z

python/ray/util/client/worker.py

            raise ConnectionError("ray client connection timeout")
-
-        # Initialize the streams to finish protocol negotiation.
-        self.data_client = DataClient(self.channel, self._client_id,


@barakmich I just moved this into the try-except

python/ray/tests/test_client_init.py

ericl · 2021-02-05T08:12:02Z

python/ray/util/client/worker.py

-                # Ray is not ready yet, wait a timeout
-                time.sleep(timeout)
+                # Initialize the streams to finish protocol negotiation.
+                self.data_client = DataClient(self.channel, self._client_id,


The constructor of dataclient doesn't look like it actually does anything, so this won't suffice as a health check. How about we just add a dummy ping RPC instead and preserve the original code structure here.

Dumb question - what's the easiest way to implement a ping here? Should I modify the protobuf struct to have a dummy value that I will handle on the server side?

ericl · 2021-02-05T08:48:04Z

Probably you can follow the existing is_initialized code as an example and copy it to return "ok".

…

On Fri, Feb 5, 2021, 12:29 AM Richard Liaw ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In python/ray/util/client/worker.py <#13919 (comment)>: > @@ -94,16 +94,11 @@ def __init__(self, # RayletDriverStub, allowing for unary requests. self.server = ray_client_pb2_grpc.RayletDriverStub( self.channel) - # Now the HTTP2 channel is ready, or proxied, but the - # servicer may not be ready. Call is_initialized() and if - # it throws, the servicer is not ready. On success, the - # `ray_ready` result is checked. - ray_ready = self.is_initialized() - if ray_ready: - # Ray is ready! Break out of the retry loop - break - # Ray is not ready yet, wait a timeout - time.sleep(timeout) + # Initialize the streams to finish protocol negotiation. + self.data_client = DataClient(self.channel, self._client_id, Dumb question - what's the easiest way to implement a ping here? Should I modify the protobuf struct to have a dummy value? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#13919 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAADUSXNZYNITJHTJZWWCIDS5OT7RANCNFSM4XDYT2SQ> .

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

richardliaw · 2021-02-05T09:06:15Z

OK, ptal, i tried something

…

On Fri, Feb 5, 2021 at 12:48 AM Eric Liang ***@***.***> wrote: Probably you can follow the existing is_initialized code as an example and copy it to return "ok". On Fri, Feb 5, 2021, 12:29 AM Richard Liaw ***@***.***> wrote: > ***@***.**** commented on this pull request. > ------------------------------ > > In python/ray/util/client/worker.py > <#13919 (comment)>: > > > @@ -94,16 +94,11 @@ def __init__(self, > # RayletDriverStub, allowing for unary requests. > self.server = ray_client_pb2_grpc.RayletDriverStub( > self.channel) > - # Now the HTTP2 channel is ready, or proxied, but the > - # servicer may not be ready. Call is_initialized() and if > - # it throws, the servicer is not ready. On success, the > - # `ray_ready` result is checked. > - ray_ready = self.is_initialized() > - if ray_ready: > - # Ray is ready! Break out of the retry loop > - break > - # Ray is not ready yet, wait a timeout > - time.sleep(timeout) > + # Initialize the streams to finish protocol negotiation. > + self.data_client = DataClient(self.channel, self._client_id, > > Dumb question - what's the easiest way to implement a ping here? Should I > modify the protobuf struct to have a dummy value? > > — > You are receiving this because you were mentioned. > Reply to this email directly, view it on GitHub > <#13919 (comment)>, or > unsubscribe > < https://github.com/notifications/unsubscribe-auth/AAADUSXNZYNITJHTJZWWCIDS5OT7RANCNFSM4XDYT2SQ > > . > — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#13919 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABCRZZNEY7NUSSNOQAV7CYLS5OWFLANCNFSM4XDYT2SQ> .

python/ray/tests/test_client_init.py

python/ray/util/client/worker.py

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

ericl · 2021-02-06T08:21:33Z

Test client failing

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

…ct#13919)

…ay-project#13919)" This reverts commit 8787c70.

richardliaw added 2 commits February 4, 2021 15:50

first-pass

b679a10

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

not-sure-about-this

1373f48

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

ericl self-assigned this Feb 5, 2021

ericl reviewed Feb 5, 2021

View reviewed changes

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

ericl added the @author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer. label Feb 5, 2021

ericl assigned barakmich Feb 5, 2021

richardliaw added 2 commits February 4, 2021 16:32

ok

0410c37

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

ok

f3ad8a8

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

barakmich reviewed Feb 5, 2021

View reviewed changes

richardliaw added 5 commits February 4, 2021 16:56

Revert "ok"

e517bb4

This reverts commit 0410c37.

try-this

682ed30

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

ok

3e6ce82

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

clientdisable

321751d

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

update

dff6906

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

richardliaw commented Feb 5, 2021

View reviewed changes

python/ray/tests/test_client_init.py Show resolved Hide resolved

ericl reviewed Feb 5, 2021

View reviewed changes

richardliaw added 2 commits February 5, 2021 01:03

ping

3120440

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

ok

ef24c44

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

ericl reviewed Feb 5, 2021

View reviewed changes

python/ray/tests/test_client_init.py Outdated Show resolved Hide resolved

ericl reviewed Feb 5, 2021

View reviewed changes

python/ray/util/client/worker.py Show resolved Hide resolved

ericl reviewed Feb 5, 2021

View reviewed changes

python/ray/util/client/worker.py Show resolved Hide resolved

richardliaw commented Feb 5, 2021

View reviewed changes

python/ray/util/client/worker.py Show resolved Hide resolved

Update python/ray/util/client/worker.py

1162160

ok

f4b5e8e

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

ericl approved these changes Feb 5, 2021

View reviewed changes

richardliaw added 5 commits February 6, 2021 01:14

fix

c302816

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

client-server-disable

1e47947

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

client

a78c3c7

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

client

7061aa3

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

Merge branch 'master' into fix-connection-client

dc12804

richardliaw merged commit 3a230fa into ray-project:master Feb 7, 2021

richardliaw deleted the fix-connection-client branch February 7, 2021 21:11

fishbone pushed a commit to fishbone/ray that referenced this pull request Feb 16, 2021

[ray_client] close ray connection upon client deactivation (ray-proje…

8787c70

…ct#13919)

fishbone added a commit to fishbone/ray that referenced this pull request Feb 16, 2021

Revert "[ray_client] close ray connection upon client deactivation (r…

cb00f8d

…ay-project#13919)" This reverts commit 8787c70.

krfricke mentioned this pull request Apr 14, 2021

[release/microbenchmarks] Please take a look at the results (and possibly sign off) #15247

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ray_client] close ray connection upon client deactivation #13919

[ray_client] close ray connection upon client deactivation #13919

richardliaw commented Feb 4, 2021 •

edited

Loading

ericl Feb 5, 2021

ericl Feb 5, 2021

barakmich Feb 5, 2021

richardliaw Feb 5, 2021

ericl commented Feb 5, 2021

barakmich Feb 5, 2021

barakmich Feb 5, 2021

barakmich Feb 5, 2021

ericl Feb 5, 2021

richardliaw Feb 5, 2021

richardliaw Feb 5, 2021

ericl Feb 5, 2021

richardliaw Feb 5, 2021 •

edited

Loading

ericl commented Feb 5, 2021 via email

richardliaw commented Feb 5, 2021 via email

ericl commented Feb 6, 2021

[ray_client] close ray connection upon client deactivation #13919

[ray_client] close ray connection upon client deactivation #13919

Conversation

richardliaw commented Feb 4, 2021 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericl commented Feb 5, 2021

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

richardliaw Feb 5, 2021 • edited Loading

Choose a reason for hiding this comment

ericl commented Feb 5, 2021 via email

richardliaw commented Feb 5, 2021 via email

ericl commented Feb 6, 2021

richardliaw commented Feb 4, 2021 •

edited

Loading

richardliaw Feb 5, 2021 •

edited

Loading