[core worker] Refactor CoreWorker member classes #5062

stephanie-wang · 2019-06-29T00:13:54Z

What do these changes do?

Refactoring CoreWorker class to encapsulate functionality a bit more. Removes references to the parent CoreWorker from the CoreWorkerObjectInterface, CoreWorkerTaskInterface, and CoreWorkerTaskExecutionInterface.

Linter

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

stephanie-wang · 2019-06-29T00:30:09Z

cc @raulchen @jovany-wang @zhijunfu, please let me know if these changes don't align with what you guys are thinking!

AmplabJenkins · 2019-06-29T03:03:44Z

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

AmplabJenkins · 2019-06-29T04:29:21Z

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/1398/
Test FAILed.

zhijunfu

Looks good to me, thanks for the effort. I left a few minor comments.

zhijunfu · 2019-06-29T09:56:24Z

src/ray/core_worker/common.h

 /// Information about a remote function.
 struct RayFunction {
  /// Language of the remote function.
-  const WorkerLanguage language;
+  const Language language;


The original intent of having a WorkerLanguage structure is that users don't need to look at the generated protobuf file for the Language definition, as that generated .h file is complex so we want to hide that complexity. But yes I agree that using Language directly makes the core worker code simpler, probably that's the right thing to do. Maybe we can consider separating Language definition to a different protobuf file say common.proto ?

I see, yes, I think having a single Language type is cleaner. Sure, should I move Language to a new protobuf file now?

never mind, I could take care of it in my next PR:)

FYI, I'm working on common.proto right now.

zhijunfu · 2019-06-29T09:57:45Z

src/ray/core_worker/store_provider/plasma_store_provider.cc

+  // TODO(zhijunfu): currently RayletClient would crash in its constructor if it cannot
+  // connect to Raylet after a number of retries, this needs to be changed
+  // so that the worker (java/python .etc) can retrieve and handle the error
+  // instead of crashing.


This comment actually refers to RayletClient , and should better be moved to CoreWorker where raylet_client_ is initialized?

Sure, thanks!

AmplabJenkins · 2019-06-30T21:16:08Z

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

AmplabJenkins · 2019-06-30T23:24:04Z

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/1412/
Test FAILed.

zhijunfu

LGTM. thanks.

pcmoritz · 2019-07-01T01:11:29Z

@stephanie-wang There is a compile error in one of the tests, do you want to fix it?

src/ray/core_worker/mock_worker.cc: In constructor 'ray::MockWorker::MockWorker(const string&, const string&)':
src/ray/core_worker/mock_worker.cc:20:37: error: 'WorkerLanguage' has not been declared
       : worker_(WorkerType::WORKER, WorkerLanguage::PYTHON, store_socket, raylet_socket,

raulchen · 2019-07-01T05:49:37Z

I'm actually in favor of having the CoreWorker and the sub interfaces friend each other.

Because the public methods in these interfaces are supposed to exposed to Java/Python workers and should be limited. And on the other hand, inside these interfaces, we sometimes want to use other interfaces' private methods. E.g., DirectActorCallTransport would need the private api in ObjectInterface that puts an object directly in local memory.

The initial reason why we split CoreWorker into sub interfaces was mainly to avoid making CoreWorker class too large, not because of encapsulation.

Does this make sense?

stephanie-wang · 2019-07-01T21:44:50Z

I'm actually in favor of having the CoreWorker and the sub interfaces friend each other.

Because the public methods in these interfaces are supposed to exposed to Java/Python workers and should be limited. And on the other hand, inside these interfaces, we sometimes want to use other interfaces' private methods. E.g., DirectActorCallTransport would need the private api in ObjectInterface that puts an object directly in local memory.

The initial reason why we split CoreWorker into sub interfaces was mainly to avoid making CoreWorker class too large, not because of encapsulation.

Does this make sense?

Sorry, I don't understand your example about the DirectActorCallTransport. Can you describe specifically what the inheritance/friend relationships would be in that case? That seems like you would want to make DirectActorCallTransport a friend of an ObjectInterface, which I think would be okay in certain cases for the reasons you mentioned, but it does not explain why ObjectInterface needs to be a friend of CoreWorker.

In general, I just want to avoid giving classes access to private fields that they are not currently using; for example, before this PR, the CoreWorkerTaskInterface had access to the plasma store_socket_, even though it never uses it.

AmplabJenkins · 2019-07-02T00:40:59Z

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

raulchen · 2019-07-02T03:25:09Z

@stephanie-wang Initially, I was thinking that all the sub interfaces would intensively use other interfaces' private methods. After thinking more, this doesn't seem true. I think what you said should be better. Only use friend in particular classes that need it. Thanks.

zhijunfu · 2019-07-02T04:16:42Z

@stephanie-wang there's still a compile error, may you please fix it? There are a few such places in core_worker_test.cc

src/ray/core_worker/core_worker_test.cc: In member function 'virtual void ray::TwoNodeTest_TestObjectInterfaceCrossNodes_Test::TestBody()':
src/ray/core_worker/core_worker_test.cc:370:42: error: 'WorkerLanguage' has not been declared
   CoreWorker worker1(WorkerType::DRIVER, WorkerLanguage::PYTHON,

stephanie-wang · 2019-07-02T04:27:47Z

@stephanie-wang there's still a compile error, may you please fix it? There are a few such places in core_worker_test.cc

src/ray/core_worker/core_worker_test.cc: In member function 'virtual void ray::TwoNodeTest_TestObjectInterfaceCrossNodes_Test::TestBody()':
src/ray/core_worker/core_worker_test.cc:370:42: error: 'WorkerLanguage' has not been declared
   CoreWorker worker1(WorkerType::DRIVER, WorkerLanguage::PYTHON,

Oops, thank you!

AmplabJenkins · 2019-07-02T07:24:07Z

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

kfstorm · 2019-07-02T13:38:56Z

FYI, I changed type of language to ray::rpc::language in #5034.

stephanie-wang · 2019-07-02T19:15:05Z

FYI, I changed type of language to ray::rpc::language in #5034.

Thanks, I changed it to just use Language everywhere. Hope this is okay...

AmplabJenkins · 2019-07-02T19:28:48Z

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/1429/
Test FAILed.

AmplabJenkins · 2019-07-02T22:21:21Z

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

AmplabJenkins · 2019-07-03T01:17:01Z

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/1434/
Test FAILed.

AmplabJenkins · 2019-07-04T07:30:59Z

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/1449/
Test FAILed.

stephanie-wang added 7 commits June 28, 2019 16:23

Move store client mutex inside CoreWorkerPlasmaStoreProvider

2af44ca

Move PlasmaClient inside CoreWorkerStoreProvider

2d3cfd1

Remove CoreWorkerObjectInterface's ref to CoreWorker

f4f80a9

Remove WorkerLanguage

d89c006

Remove CoreWorkerTaskInterface's ref to CoreWorker

e4fae6e

Remove CoreWorkerTaskExecutionInterface's ref to CoreWorker

b58b99f

lint

401eecb

stephanie-wang requested review from raulchen and jovany-wang June 29, 2019 00:13

zhijunfu reviewed Jun 29, 2019

View reviewed changes

move comment

0e5a843

zhijunfu approved these changes Jul 1, 2019

View reviewed changes

zhijunfu mentioned this pull request Jul 1, 2019

[grpc] Add grpc server to worker #5054

Merged

Fix build

e98abfb

raulchen approved these changes Jul 2, 2019

View reviewed changes

Fix build

d49e5c0

stephanie-wang added 2 commits July 2, 2019 12:03

Merge branch 'master' into core-worker-refactor

942e839

Merge branch 'master' into core-worker-refactor

2d22626

stephanie-wang merged commit 71d4637 into ray-project:master Jul 2, 2019

stephanie-wang deleted the core-worker-refactor branch July 2, 2019 22:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[core worker] Refactor CoreWorker member classes #5062

[core worker] Refactor CoreWorker member classes #5062

stephanie-wang commented Jun 29, 2019 •

edited

stephanie-wang commented Jun 29, 2019

AmplabJenkins commented Jun 29, 2019

AmplabJenkins commented Jun 29, 2019

zhijunfu left a comment

zhijunfu Jun 29, 2019

stephanie-wang Jun 30, 2019

zhijunfu Jul 1, 2019

raulchen Jul 1, 2019

zhijunfu Jun 29, 2019

stephanie-wang Jun 30, 2019

AmplabJenkins commented Jun 30, 2019

AmplabJenkins commented Jun 30, 2019

zhijunfu left a comment

pcmoritz commented Jul 1, 2019

raulchen commented Jul 1, 2019

stephanie-wang commented Jul 1, 2019

AmplabJenkins commented Jul 2, 2019

raulchen commented Jul 2, 2019

zhijunfu commented Jul 2, 2019

stephanie-wang commented Jul 2, 2019

AmplabJenkins commented Jul 2, 2019

kfstorm commented Jul 2, 2019

stephanie-wang commented Jul 2, 2019

AmplabJenkins commented Jul 2, 2019

AmplabJenkins commented Jul 2, 2019

AmplabJenkins commented Jul 3, 2019

AmplabJenkins commented Jul 4, 2019

Navigation Menu

[core worker] Refactor CoreWorker member classes #5062

[core worker] Refactor CoreWorker member classes #5062

Conversation

stephanie-wang commented Jun 29, 2019 • edited

What do these changes do?

Linter

stephanie-wang commented Jun 29, 2019

AmplabJenkins commented Jun 29, 2019

AmplabJenkins commented Jun 29, 2019

zhijunfu left a comment

Choose a reason for hiding this comment

zhijunfu Jun 29, 2019

Choose a reason for hiding this comment

stephanie-wang Jun 30, 2019

Choose a reason for hiding this comment

zhijunfu Jul 1, 2019

Choose a reason for hiding this comment

raulchen Jul 1, 2019

Choose a reason for hiding this comment

zhijunfu Jun 29, 2019

Choose a reason for hiding this comment

stephanie-wang Jun 30, 2019

Choose a reason for hiding this comment

AmplabJenkins commented Jun 30, 2019

AmplabJenkins commented Jun 30, 2019

zhijunfu left a comment

Choose a reason for hiding this comment

pcmoritz commented Jul 1, 2019

raulchen commented Jul 1, 2019

stephanie-wang commented Jul 1, 2019

AmplabJenkins commented Jul 2, 2019

raulchen commented Jul 2, 2019

zhijunfu commented Jul 2, 2019

stephanie-wang commented Jul 2, 2019

AmplabJenkins commented Jul 2, 2019

kfstorm commented Jul 2, 2019

stephanie-wang commented Jul 2, 2019

AmplabJenkins commented Jul 2, 2019

AmplabJenkins commented Jul 2, 2019

AmplabJenkins commented Jul 3, 2019

AmplabJenkins commented Jul 4, 2019

stephanie-wang commented Jun 29, 2019 •

edited