[grpc] Add grpc server to worker #5054

zhijunfu · 2019-06-28T02:39:52Z

What do these changes do?

Add a gRPC server to the worker that registers with the raylet (but no communication through here yet). Also convert RayletClient references to unique_ptr.

For more details, refer to #5039

Related issue number

#5029

Linter

[Y] I've run scripts/format.sh to lint the changes in this PR.

src/ray/core_worker/core_worker.h

src/ray/rpc/worker_client.h

src/ray/rpc/worker_server.h

Co-Authored-By: Hao Chen <chenh1024@gmail.com>

raulchen · 2019-06-28T03:46:57Z

Could you merge master and resolve conflicts?

…ch-alliance/ray into add_grpc_server_to_worker

zhijunfu · 2019-06-28T03:53:23Z

Could you merge master and resolve conflicts?

Done, thanks.

BUILD.bazel

AmplabJenkins · 2019-06-28T05:35:18Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/14943/
Test PASSed.

AmplabJenkins · 2019-06-28T06:40:15Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/14944/
Test PASSed.

AmplabJenkins · 2019-06-28T06:57:05Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/14945/
Test PASSed.

AmplabJenkins · 2019-06-28T07:10:07Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/14947/
Test FAILed.

src/ray/core_worker/core_worker.cc

src/ray/core_worker/task_execution.cc

src/ray/core_worker/core_worker.cc

src/ray/rpc/worker/worker_client.h

src/ray/rpc/worker/worker_server.h

src/ray/rpc/client_call.h

Co-Authored-By: Stephanie Wang <swang@cs.berkeley.edu>

AmplabJenkins · 2019-06-29T13:14:55Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/14974/
Test PASSed.

AmplabJenkins · 2019-06-29T19:05:44Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-Perf-Integration-PRB/1403/
Test FAILed.

stephanie-wang

The changes to the core worker look okay, but I think we should remove any unused code from this PR before merging. Generally, we should avoid merging unused code completely.

src/ray/core_worker/core_worker.cc

…ch-alliance/ray into add_grpc_server_to_worker

zhijunfu · 2019-07-01T07:03:24Z

Comments are addressed. This also merges the content of #5062 to avoid future code conflicts.

AmplabJenkins · 2019-07-01T09:59:48Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/14996/
Test PASSed.

AmplabJenkins · 2019-07-01T17:54:32Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-Perf-Integration-PRB/1418/
Test PASSed.

stephanie-wang

I reviewed the unused code for the AssignTask request in this round, but let's please try to avoid merging any unused code in the future.

Also, thanks for merging with #5062. However, because that PR is still under review, I cannot approve this PR until #5062 is merged. There are 2 options: you can either wait for #5062 (and you may have to merge again if it changes) or you can roll back the merge.

src/ray/core_worker/transport/raylet_transport.cc

stephanie-wang · 2019-07-01T21:12:11Z

src/ray/core_worker/transport/raylet_transport.cc

+}
+
+Status CoreWorkerRayletTaskReceiver::SetTaskHandler(const TaskHandler &callback) {
+  task_handler_ = callback;


Unless there is some reason we'd overwrite this, I think this should be set as part of the constructor to avoid calling an uninitialized task_handler_.

I removed this function from this PR. Next pr would initialize the task_handler_ in constructor.

src/ray/core_worker/transport/raylet_transport.cc

stephanie-wang · 2019-07-01T21:13:25Z

src/ray/core_worker/transport/raylet_transport.h

+  ///
+  /// \param[in] request The request message.
+  /// \param[out] reply The reply message.
+  /// \param[in] done_callback The callback to be called when the request is done.


It seems brittle to require that done_callback be called inside HandleAssignTask. Can we instead pass in a lambda that calls HandleAssignTask, then the done_callback?

I'm thinking about refactor ServerCall to call done_callback there instead, but as it would require changing node manager as well as object manager, I'd prefer to do it via a later PR.

That doesn't seem necessary. I think we can just wrap HandleAssignTask in a lambda.

sorry I'm not quite sure what the suggested change would be. May you add an example code? Thanks.

Sure, I was suggesting that instead of passing in &WorkerTaskHandler::HandleAssignTask into ServerCallFactoryImpl, we could pass in something like:

[HandleAssignTask]( ... , done_callback) { HandleAssignTask(...); done_callback(); }

But actually, I think if I'm understanding the code correctly, what you're suggesting about doing it in ServerCall is the same thing. So agreed, let's just do it in a future PR.

src/ray/protobuf/worker.proto

src/ray/raylet/worker.cc

src/ray/rpc/worker/worker_server.h

Co-Authored-By: Stephanie Wang <swang@cs.berkeley.edu>

AmplabJenkins · 2019-07-02T13:16:37Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/15025/
Test PASSed.

AmplabJenkins · 2019-07-02T13:22:23Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/15026/
Test PASSed.

stephanie-wang · 2019-07-02T18:55:40Z

Thanks, @zhijunfu. However, can you please go ahead and roll back the changes that you took from #5062? Otherwise, we can't merge this as there is a lot of other code in the PR from 5062. Thanks!

…ch-alliance/ray into add_grpc_server_to_worker

zhijunfu · 2019-07-03T02:50:22Z

Thanks, @zhijunfu. However, can you please go ahead and roll back the changes that you took from #5062? Otherwise, we can't merge this as there is a lot of other code in the PR from 5062. Thanks!

I saw that one has already been merged to master, so I just sync'ed with latest master. thanks.

AmplabJenkins · 2019-07-03T06:55:12Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/15056/
Test FAILed.

AmplabJenkins · 2019-07-03T17:41:14Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-Perf-Integration-PRB/1443/
Test FAILed.

stephanie-wang

Thanks!

stephanie-wang · 2019-07-03T17:59:48Z

Btw, looks like there is a build error on Linux and a lint error.

AmplabJenkins · 2019-07-03T20:27:26Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-Perf-Integration-PRB/1444/
Test FAILed.

AmplabJenkins · 2019-07-04T05:08:12Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/15090/
Test FAILed.

zhijunfu · 2019-07-04T06:26:20Z

Btw, looks like there is a build error on Linux and a lint error.

Thanks. Fixed. The remaining errors look unrelated.

zhijunfu added 12 commits June 24, 2019 16:02

refactor grpc server

7f635b4

format

2205c87

change GetTask() to PushTask()

0703b75

merge code, and fix test

6d5228a

merge

4c3eb6e

change PushTask to AssignTask

d1b20e5

format

1a663bc

update

fb1f55f

fix test

3c68b9d

format

5842653

merge

eb413e5

merge with latest code

f4ad236

zhijunfu mentioned this pull request Jun 28, 2019

[grpc] change from "worker get task" to "raylet assign task to worker" #5039

Closed

raulchen reviewed Jun 28, 2019

View reviewed changes

src/ray/core_worker/core_worker.h Outdated Show resolved Hide resolved

src/ray/rpc/worker_client.h Outdated Show resolved Hide resolved

src/ray/rpc/worker_server.h Outdated Show resolved Hide resolved

Update src/ray/rpc/worker_client.h

e6b3e45

Co-Authored-By: Hao Chen <chenh1024@gmail.com>

zhijunfu added 2 commits June 28, 2019 11:52

merge latest code, and resolve comments

cd8ebf1

Merge branch 'add_grpc_server_to_worker' of https://github.com/ant-te…

4f98af9

…ch-alliance/ray into add_grpc_server_to_worker

raulchen reviewed Jun 28, 2019

View reviewed changes

BUILD.bazel Outdated Show resolved Hide resolved

Update BUILD.bazel

6e659c9

stephanie-wang requested changes Jun 28, 2019

View reviewed changes

Update src/ray/core_worker/task_execution.cc

170d0f1

Co-Authored-By: Stephanie Wang <swang@cs.berkeley.edu>

stephanie-wang requested changes Jun 30, 2019

View reviewed changes

src/ray/core_worker/core_worker.cc Outdated Show resolved Hide resolved

zhijunfu added 4 commits July 1, 2019 14:38

update

6f1dbab

merge

981c275

format

cbc1df7

Merge branch 'add_grpc_server_to_worker' of https://github.com/ant-te…

0c5ba42

…ch-alliance/ray into add_grpc_server_to_worker

stephanie-wang requested changes Jul 1, 2019

View reviewed changes

zhijunfu and others added 4 commits July 2, 2019 18:07

address comments

2bf13d6

format

69ef9c5

Update src/ray/rpc/worker/worker_server.h

f0a696b

Co-Authored-By: Stephanie Wang <swang@cs.berkeley.edu>

Update src/ray/protobuf/worker.proto

1469985

Co-Authored-By: Stephanie Wang <swang@cs.berkeley.edu>

zhijunfu added 3 commits July 3, 2019 10:47

merge

3ae0526

format

6e8cdf4

Merge branch 'add_grpc_server_to_worker' of https://github.com/ant-te…

8286b3b

…ch-alliance/ray into add_grpc_server_to_worker

stephanie-wang approved these changes Jul 3, 2019

View reviewed changes

zhijunfu added 2 commits July 4, 2019 09:54

fix

704817e

format

42ed4f1

jovany-wang approved these changes Jul 4, 2019

View reviewed changes

jovany-wang merged commit 54d5969 into ray-project:master Jul 4, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[grpc] Add grpc server to worker #5054

[grpc] Add grpc server to worker #5054

zhijunfu commented Jun 28, 2019 •

edited

raulchen commented Jun 28, 2019

zhijunfu commented Jun 28, 2019

AmplabJenkins commented Jun 28, 2019

AmplabJenkins commented Jun 28, 2019

AmplabJenkins commented Jun 28, 2019

AmplabJenkins commented Jun 28, 2019

AmplabJenkins commented Jun 29, 2019

AmplabJenkins commented Jun 29, 2019

stephanie-wang left a comment

zhijunfu commented Jul 1, 2019

AmplabJenkins commented Jul 1, 2019

AmplabJenkins commented Jul 1, 2019

stephanie-wang left a comment

stephanie-wang Jul 1, 2019

zhijunfu Jul 2, 2019

stephanie-wang Jul 1, 2019

zhijunfu Jul 2, 2019

stephanie-wang Jul 2, 2019

zhijunfu Jul 3, 2019

stephanie-wang Jul 3, 2019

AmplabJenkins commented Jul 2, 2019

AmplabJenkins commented Jul 2, 2019

stephanie-wang commented Jul 2, 2019

zhijunfu commented Jul 3, 2019

AmplabJenkins commented Jul 3, 2019

AmplabJenkins commented Jul 3, 2019

stephanie-wang left a comment

stephanie-wang commented Jul 3, 2019

AmplabJenkins commented Jul 3, 2019

AmplabJenkins commented Jul 4, 2019

zhijunfu commented Jul 4, 2019

[grpc] Add grpc server to worker #5054

[grpc] Add grpc server to worker #5054

Conversation

zhijunfu commented Jun 28, 2019 • edited

What do these changes do?

Related issue number

Linter

raulchen commented Jun 28, 2019

zhijunfu commented Jun 28, 2019

AmplabJenkins commented Jun 28, 2019

AmplabJenkins commented Jun 28, 2019

AmplabJenkins commented Jun 28, 2019

AmplabJenkins commented Jun 28, 2019

AmplabJenkins commented Jun 29, 2019

AmplabJenkins commented Jun 29, 2019

stephanie-wang left a comment

Choose a reason for hiding this comment

zhijunfu commented Jul 1, 2019

AmplabJenkins commented Jul 1, 2019

AmplabJenkins commented Jul 1, 2019

stephanie-wang left a comment

Choose a reason for hiding this comment

stephanie-wang Jul 1, 2019

Choose a reason for hiding this comment

zhijunfu Jul 2, 2019

Choose a reason for hiding this comment

stephanie-wang Jul 1, 2019

Choose a reason for hiding this comment

zhijunfu Jul 2, 2019

Choose a reason for hiding this comment

stephanie-wang Jul 2, 2019

Choose a reason for hiding this comment

zhijunfu Jul 3, 2019

Choose a reason for hiding this comment

stephanie-wang Jul 3, 2019

Choose a reason for hiding this comment

AmplabJenkins commented Jul 2, 2019

AmplabJenkins commented Jul 2, 2019

stephanie-wang commented Jul 2, 2019

zhijunfu commented Jul 3, 2019

AmplabJenkins commented Jul 3, 2019

AmplabJenkins commented Jul 3, 2019

stephanie-wang left a comment

Choose a reason for hiding this comment

stephanie-wang commented Jul 3, 2019

AmplabJenkins commented Jul 3, 2019

AmplabJenkins commented Jul 4, 2019

zhijunfu commented Jul 4, 2019

zhijunfu commented Jun 28, 2019 •

edited