Skip to content

[ray_client]: Implement object retain/release and Data Streaming API#12818

Merged
ericl merged 21 commits intoray-project:masterfrom
barakmich:client_datastream
Dec 18, 2020
Merged

[ray_client]: Implement object retain/release and Data Streaming API#12818
ericl merged 21 commits intoray-project:masterfrom
barakmich:client_datastream

Conversation

@barakmich
Copy link
Copy Markdown
Contributor

@barakmich barakmich commented Dec 12, 2020 •
edited
Loading

Implements data over a gRPC stream. While this data connection is open, we can now maintain a client's references on the server. When the data stream is closed, all references are released regardless.

Meanwhile, if a reference is garbage collected on the client, an asynchronous message is sent on the stream to release that reference.

This doesn't entirely happen, as the server, when serializing an ObjectRef for a handle, causes that reference to be pinned. That issue is tracked at #12863

Happy to review it now with that follow-up tracking caveat; it's at least as good as the previous master branch, if not better, so as to handle comments sooner rather than later

Why are these changes needed?

Related issue number

Closes #11977

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 :(

@barakmich barakmich marked this pull request as ready for review December 15, 2020 01:35
@barakmich barakmich added the tests-ok The tagger certifies test failures are unrelated and assumes personal liability. label Dec 15, 2020
@barakmich barakmich requested a review from ericl December 15, 2020 01:36
ericl
ericl reviewed Dec 15, 2020
View reviewed changes
@ericl ericl added @author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer. and removed tests-ok The tagger certifies test failures are unrelated and assumes personal liability. labels Dec 15, 2020
@ericl
Copy link
Copy Markdown
Contributor

ericl commented Dec 15, 2020 •
edited
Loading

@barakmich one test case to verify the multiple client refs thing would be something like this:

import ray

ray.init()

@ray.remote
class A:
    def __init__(self):
        self.x = ray.put("hi")

    def get(self):
        return [self.x]


a = A.remote()
ref1 = ray.get(a.get.remote())[0]
ref2 = ray.get(a.get.remote())[0]
del a
print(ray.get(ref1))
del ref1
print(ray.get(ref2))
del ref2

Simpler version might even be:

@ray.remote
def f():
   x = ray.put("hi")
   return [x, x]

@barakmich
Copy link
Copy Markdown
Contributor Author

barakmich commented Dec 17, 2020

Added the test you suggested. Getting there from here was a doozy.

But the outcome was great. Now we have multiple references, references in references, reference counting, and a simpler mental model.

To wit: ClientRefs/ClientStubs should exist on the client only. There is a specific client<->server pickling pair that creates or decodes them. As much as possible, this step is where the server can inject the "real" version of the same entity, for all future goings-on with real ray.

@barakmich barakmich added tests-ok The tagger certifies test failures are unrelated and assumes personal liability. and removed @author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer. labels Dec 17, 2020
@barakmich
Copy link
Copy Markdown
Contributor Author

barakmich commented Dec 17, 2020 •
edited
Loading

(Hah, the PR went from +400 -42 to +940 −288 since the last review. I've been busy)

AmeerHajAli
AmeerHajAli reviewed Dec 17, 2020
View reviewed changes
AmeerHajAli
AmeerHajAli reviewed Dec 17, 2020
View reviewed changes
AmeerHajAli
AmeerHajAli reviewed Dec 17, 2020
View reviewed changes
AmeerHajAli
AmeerHajAli reviewed Dec 17, 2020
View reviewed changes
AmeerHajAli
AmeerHajAli reviewed Dec 17, 2020
View reviewed changes
AmeerHajAli
AmeerHajAli reviewed Dec 17, 2020
View reviewed changes
AmeerHajAli
AmeerHajAli reviewed Dec 17, 2020
View reviewed changes
AmeerHajAli
AmeerHajAli reviewed Dec 17, 2020
View reviewed changes
AmeerHajAli
AmeerHajAli reviewed Dec 17, 2020
View reviewed changes
AmeerHajAli
AmeerHajAli reviewed Dec 17, 2020
View reviewed changes
AmeerHajAli
AmeerHajAli reviewed Dec 17, 2020
View reviewed changes
AmeerHajAli
AmeerHajAli reviewed Dec 17, 2020
View reviewed changes
AmeerHajAli
AmeerHajAli reviewed Dec 17, 2020
View reviewed changes
AmeerHajAli
AmeerHajAli reviewed Dec 17, 2020
View reviewed changes
AmeerHajAli
AmeerHajAli reviewed Dec 17, 2020
View reviewed changes
AmeerHajAli
AmeerHajAli reviewed Dec 17, 2020
View reviewed changes
@barakmich
formatting
36ce34b 
Change-Id: I8078fe24e1ee0a2561412f0ab3bea9d340e5c58a
@barakmich
simple fixes
44f9e2c 
Change-Id: I8967c2811d9bcc53daa9520cd75281eb5d123e59
@barakmich
support actor del
500fc09 
Change-Id: Ic10a3966c54a4a1670f2260f00b69b8a03e975be
@barakmich
unify reference counting on worker, and unify type signature for call…
a1afb0e 
…_remote

Change-Id: I60497f8b1c4879f4b74c9aae62566664811f51b5
@barakmich
client<->server pickling
98fce92 
Change-Id: Iac9148a1f7877065cd979e1f69a84de3b0dfe377
@barakmich
And associated changes
ada6d5d 
Change-Id: Ic27777bb3efc7a9e58b0789220fd01668855d67d
@barakmich
now with pickling of a number of stubs
48c4519 
Change-Id: I46b096242802994ff7fc13e11fa2c1ab7e50bf80
@barakmich
move get errors in-band and it's finally working
178cb5e 
Change-Id: I0cf2fa6fdd531734082782c24d65a67b0ef07d45
@barakmich
some output cleanup
a7b99f7 
Change-Id: Ifa39b6404fc5dde7cdec390f6d4cb677dc802eee
@barakmich
more cleanup
3b5f9c6 
Change-Id: Ia620795591f9071fdfc501ea7a9111447aed059b
@barakmich
formatting
1fcbfa9 
Change-Id: I4e48e02cfa416d0f23dc066e7a89872300225319
@barakmich
support multiple references
0671f2f 
Change-Id: Iae03190e1cd4056a5b02b8af4f1f5b25e9e449a4
@barakmich
misc type signatures and fix pickle signatures for 3.6
4d94f35 
Change-Id: If759e2a323ed1d8e819645c002ed0666b7e8df51
@barakmich
simplify and clean up ray_client.proto
946aa84 
Change-Id: I8659fc8ebe1f0a631451a04ebdcd975a5fd380c3
@barakmich
add comments and formatting
a82d35a 
Change-Id: I641ee8a5e9411fa34b6f9b587f8308eb31326171
@ericl
Copy link
Copy Markdown
Contributor

ericl commented Dec 18, 2020

LGTM; but LINT is failing

@ericl ericl added the @author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer. label Dec 18, 2020
@barakmich barakmich mentioned this pull request Dec 18, 2020
Merged
6 tasks
@barakmich
fix lint on the proto file
0fa64ec 
Change-Id: Ia3d08c14b2a64ccdabe63b75c5710c5f0ef32807
@barakmich
Remove testing file
3026a2f 
Change-Id: I7dbce85a80bef4d9d2ccc4fa1fe1254d305b03bd
@ericl ericl merged commit 5cfa193 into ray-project:master Dec 18, 2020
@barakmich barakmich mentioned this pull request Dec 19, 2020
Closed
This was referenced Dec 21, 2020
Closed
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ericl ericl ericl approved these changes

@AmeerHajAli AmeerHajAli AmeerHajAli left review comments

Assignees

@ericl ericl

@AmeerHajAli AmeerHajAli

Labels

@author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Support basic reference counting [15]

3 participants

@barakmich @ericl @AmeerHajAli