Move task to common module and add checks in getter methods by raulchen · Pull Request #5147 · ray-project/ray

raulchen · 2019-07-09T08:06:20Z

What do these changes do?

Moves Task, TaskSpecification, TaskExecutionSpecification and other related code to "common/task" directory. Because these classes should be shared by both worker and raylet. Currently, core worker still depends on raylet_lib, because of raylet client. After we migrate raylet client to grpc, core_worker and raylet will independent.
Add checks in the getter methods in TaskSpecification to avoid getting wrong attributes.

Related issue number

#4850

Linter

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

raulchen · 2019-07-09T08:10:52Z

@zhijunfu @jiangzihao2009

AmplabJenkins · 2019-07-09T10:01:48Z

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

AmplabJenkins · 2019-07-09T10:46:35Z

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

AmplabJenkins · 2019-07-09T10:49:11Z

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

AmplabJenkins · 2019-07-09T11:05:06Z

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

zhijunfu

Looks good. Left some minor comments.

zhijunfu · 2019-07-09T11:21:29Z

src/ray/common/task/task_common.h

+using rpc::Language;
+
+// Alias `ray::rpc::TaskType` in `ray` namespace.
+using rpc::TaskType;


normally it's not recommended to do this in .h files. It might be fine if there's no other Language and TaskType definitions in the future.

The purpose of doing this is to hide the implementation details that these two enums are defined in protobuf. I'd like other code to use them as if they are defined in task_common.h.
So, in case we need to get rid of protobuf in the future, the refactor would be easier.

Any idea about how we can better handle this?

updated comment

zhijunfu · 2019-07-09T11:23:00Z

BUILD.bazel

+    copts = COPTS,
+    deps = [
+        ":common_cc_proto",
+        ":node_manager_fbs",


will this be removed later?

also add a comment on the core_worker section that the dependency to raylet lib will be removed later?

yes, it'll be removed.

AmplabJenkins · 2019-07-09T11:38:03Z

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

AmplabJenkins · 2019-07-10T04:32:23Z

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

AmplabJenkins · 2019-07-10T06:14: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/1569/
Test FAILed.

AmplabJenkins · 2019-07-10T07:05:53Z

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

AmplabJenkins · 2019-07-10T07:07:25Z

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

AmplabJenkins · 2019-07-10T09:08:45Z

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

AmplabJenkins · 2019-07-10T11:26:10Z

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

AmplabJenkins · 2019-07-10T11:26:27Z

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

AmplabJenkins · 2019-07-10T16:20:08Z

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

AmplabJenkins · 2019-07-10T19:46:40Z

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

stephanie-wang · 2019-07-10T22:28:47Z

Great idea!
One thing that I also discussed with @stephanie-wang last week is removing the runtime dependencies from TaskExecutionSpec and replacing them with an optional actor task dependency in TaskSpec. The list of dependencies is actually just leftover from previous design iterations and is only ever used to pass in a single such dependency.
This would also simplify the core worker code, as it would only need to pass around a single struct on the submit path (should be able to scrap the TaskInfo wrapper struct). It should be left to a separate PR, but any thoughts on that idea?

Yeah, sounds great. I found @stephanie-wang removed the code that modifies the dependencies at runtime. But I wasn't sure wether she would add that back in the future. If we can get rid of it, code would be simplified.

Yes, let's just remove it for now. Thanks!

AmplabJenkins · 2019-07-11T04:01:53Z

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

AmplabJenkins · 2019-07-11T04:28:51Z

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

AmplabJenkins · 2019-07-11T06:07:11Z

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

AmplabJenkins · 2019-07-11T06:09:55Z

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

AmplabJenkins · 2019-07-11T06:39:52Z

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

AmplabJenkins · 2019-07-11T07:00:02Z

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

AmplabJenkins · 2019-07-11T08:10:03Z

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

AmplabJenkins · 2019-07-11T08:47:24Z

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

raulchen requested review from pcmoritz and robertnishihara July 9, 2019 08:06

move task to common dir

b358cd2

raulchen requested a review from jovany-wang July 9, 2019 09:05

raulchen added 14 commits July 9, 2019 17:06

fix namespace

426292c

fix namespace

ef30941

fix namespace

e0bbf8f

move to common/task

3f1d831

fix namespace

1618bee

fix include

048d291

fix

ccaf9f1

remove cython ;

60b85bf

format

3e4af5c

change include "" to <>

085eedd

comment

8b30440

fix header guard

eaad597

format

53342a2

Add checks in task spec getter methods

840058a

raulchen force-pushed the common_task branch from d5fd065 to 840058a Compare July 9, 2019 09:11

raulchen changed the title ~~Move task definition to common module~~ Move task to common module and add checks in getter methods Jul 9, 2019

fix python

4197848

zhijunfu reviewed Jul 9, 2019

View reviewed changes

update comment

f444e9e

zhijunfu approved these changes Jul 9, 2019

View reviewed changes

fix enum declare

da382d4

fix

3cf1932

fix python

a3c94f7

edoakes mentioned this pull request Jul 11, 2019

Remove dependencies from TaskExecutionSpecification #5166

Merged

1 task

raulchen added 2 commits July 11, 2019 11:34

Merge branch 'master' into common_task

cf6bf9f

fix

d84ef69

Merge branch 'master' into common_task

7a88ef0

jovany-wang approved these changes Jul 11, 2019

View reviewed changes

fix conflicts

6dcc8e6

raulchen merged commit fd835d1 into ray-project:master Jul 11, 2019

raulchen deleted the common_task branch July 11, 2019 09:07

Conversation

raulchen commented Jul 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What do these changes do?

Related issue number

Linter

Uh oh!

raulchen commented Jul 9, 2019

Uh oh!

AmplabJenkins commented Jul 9, 2019

Uh oh!

AmplabJenkins commented Jul 9, 2019

Uh oh!

AmplabJenkins commented Jul 9, 2019

Uh oh!

AmplabJenkins commented Jul 9, 2019

Uh oh!

zhijunfu left a comment

Choose a reason for hiding this comment

Uh oh!

zhijunfu Jul 9, 2019

Choose a reason for hiding this comment

Uh oh!

raulchen Jul 9, 2019

Choose a reason for hiding this comment

Uh oh!

raulchen Jul 9, 2019

Choose a reason for hiding this comment

Uh oh!

zhijunfu Jul 9, 2019

Choose a reason for hiding this comment

Uh oh!

zhijunfu Jul 9, 2019

Choose a reason for hiding this comment

Uh oh!

raulchen Jul 9, 2019

Choose a reason for hiding this comment

Uh oh!

AmplabJenkins commented Jul 9, 2019

Uh oh!

AmplabJenkins commented Jul 10, 2019

Uh oh!

AmplabJenkins commented Jul 10, 2019

Uh oh!

AmplabJenkins commented Jul 10, 2019

Uh oh!

AmplabJenkins commented Jul 10, 2019

Uh oh!

AmplabJenkins commented Jul 10, 2019

Uh oh!

AmplabJenkins commented Jul 10, 2019

Uh oh!

AmplabJenkins commented Jul 10, 2019

Uh oh!

AmplabJenkins commented Jul 10, 2019

Uh oh!

AmplabJenkins commented Jul 10, 2019

Uh oh!

stephanie-wang commented Jul 10, 2019

Uh oh!

AmplabJenkins commented Jul 11, 2019

Uh oh!

AmplabJenkins commented Jul 11, 2019

Uh oh!

AmplabJenkins commented Jul 11, 2019

Uh oh!

AmplabJenkins commented Jul 11, 2019

Uh oh!

AmplabJenkins commented Jul 11, 2019

Uh oh!

AmplabJenkins commented Jul 11, 2019

Uh oh!

AmplabJenkins commented Jul 11, 2019

Uh oh!

AmplabJenkins commented Jul 11, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

raulchen commented Jul 9, 2019 •

edited

Loading