Track newly created actor's parent actor by vipulharsh · Pull Request #5098 · ray-project/ray

vipulharsh · 2019-07-02T21:12:41Z

What do these changes do?

Related issue number

Linter

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

stephanie-wang

Thanks!

stephanie-wang · 2019-07-02T21:37:40Z

src/ray/raylet/node_manager.cc

+    auto parent_task_id = task.GetTaskSpecification().ParentTaskId();
+    RAY_CHECK_OK(gcs_client_->raylet_task_table().Lookup(
+        JobID::Nil(), parent_task_id,
+        //success_callback


Our convention is to use /success_callback=/ for comments that describe a kwarg.

src/ray/raylet/node_manager.cc

stephanie-wang · 2019-07-02T21:40:19Z

src/ray/raylet/node_manager.cc

+          // get the parent actor id.
+          auto message = flatbuffers::GetRoot<protocol::Task>(parent_task_data.task().data());
+          Task parent_task(*message);
+          new_actor_data.set_parent_actor_id(parent_task.GetTaskSpecification().ActorId().Binary());


It's a bit ugly, but there are actually two cases where an actor might start another actor, either in a task for the parent actor (which you have here), or during the parent actor's creation task. You can check for the latter with parent_task.GetTaskSpecification().IsActorCreationTask(), and the actor ID will be parent_task.GetTaskSpecification().ActorCreationId().

stephanie-wang · 2019-07-02T21:40:44Z

src/ray/raylet/node_manager.cc

+          if(!resumed_from_checkpoint){
+            // The actor was not resumed from a checkpoint. We extend the actor's
+            // frontier as usual since there is no frontier to restore.
+            ExtendActorFrontier(task, actor_id, actor_handle_id);


Can we move this if statement into FinishAssignedActorTaskHelper?

stephanie-wang · 2019-07-02T21:41:39Z

src/ray/raylet/node_manager.cc

  }
-
-  if (!resumed_from_checkpoint) {
+  else if (!resumed_from_checkpoint) {


This looks like it could just be an else (you would never have resumed_from_checkpoint for regular actor tasks). Also, please move to the previous line.

The if condition existed before, hence I stayed with it. Can remove if it's unnecessary.

Yeah it used to be necessary because we also wanted to call ExtendActorFrontier for actor creation tasks that didn't resume from a checkpoint, but now we're calling that in the callback for the actor table lookup. So we can just make this an else now.

stephanie-wang · 2019-07-02T21:42:15Z

src/ray/raylet/node_manager.cc

+  }
+}
+
+void NodeManager::ExtendActorFrontier(const Task &task, ActorID &actor_id, ActorHandleID &actor_handle_id){


This looks like it only needs to take in task.GetTaskSpecification().ActorDummyObject(), not the whole task. This would allow us to avoid capturing task in FinishAssignedActorTask.

That makes sense. However, we would still need to capture one of TaskSpecification and ActorTableData as CreateActorTableDataFromCreationTask would require TaskSpecification if we move the CreateActorTableDataFromCreationTask call to the helper function

Takes dummy_object now instead of the entire task.

stephanie-wang · 2019-07-02T21:43:39Z

src/ray/raylet/node_manager.cc

+    RAY_CHECK_OK(gcs_client_->raylet_task_table().Lookup(
+        JobID::Nil(), parent_task_id,
+        //success_callback
+        [this, task, actor_id, actor_handle_id, new_actor_data, resumed_from_checkpoint]


To avoid capturing new_actor_data, we can just call CreateActorTableDataFromCreationTask inside the callback, and maybe pass in the parent actor ID to that helper function.

Skipped this for now as it would require capturing TaskSpecification for CreateActorTableDataFromCreationTask.

Hmm I would prefer capturing the TaskSpecification over new_actor_data so we can avoid marking the lambda as mutable.

stephanie-wang · 2019-07-02T21:44:12Z

src/ray/raylet/node_manager.cc

+  HandleObjectLocal(dummy_object);
+}
+
+void NodeManager::FinishAssignedActorTaskHelper(const ActorID& actor_id, const ActorTableData new_actor_data, bool resumed_from_checkpoint) {


I would name this something more descriptive, like FinishAssignedActorCreationTask.

stephanie-wang · 2019-07-02T21:44:49Z

src/ray/raylet/node_manager.h

+  /// \return Void.
+  void FinishAssignedActorTaskHelper(const ActorID& actor_id,
+            const ActorTableData new_actor_data, bool resumed_from_checkpoint);
+  /// Extend actor frontier when an actor task or actor creation task arrives.


Suggested change

/// Extend actor frontier when an actor task or actor creation task arrives.

/// Extend actor frontier after an actor task or actor creation task executes.

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

AmplabJenkins · 2019-07-03T00:40:49Z

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

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

AmplabJenkins · 2019-07-03T03:17:22Z

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

AmplabJenkins · 2019-07-03T07:21:31Z

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

AmplabJenkins · 2019-07-03T07:22:24Z

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

stephanie-wang · 2019-07-03T17:39:34Z

src/ray/raylet/node_manager.cc

 void NodeManager::FinishAssignedActorTask(Worker &worker, const Task &task) {
  ActorID actor_id;
  ActorHandleID actor_handle_id;
+  TaskSpecification task_spec = task.GetTaskSpecification();


const TaskSpecification &task_spec

stephanie-wang · 2019-07-03T17:44:23Z

src/ray/raylet/node_manager.cc

  }
-
-  if (!resumed_from_checkpoint) {
+  else if (!resumed_from_checkpoint) {


Yeah it used to be necessary because we also wanted to call ExtendActorFrontier for actor creation tasks that didn't resume from a checkpoint, but now we're calling that in the callback for the actor table lookup. So we can just make this an else now.

stephanie-wang

Thanks, a couple small comments, but otherwise looks good!

stephanie-wang · 2019-07-03T17:50:42Z

src/ray/raylet/node_manager.cc

+            (ray::gcs::AsyncGcsClient *client, const TaskID &parent_task_id) mutable {
+          // The parent task was not in the GCS task table. It must therefore be in the
+          // lineage cache.
+          RAY_CHECK(lineage_cache_.ContainsTask(parent_task_id))


Unfortunately, one of the travis tests for the new C++ worker is failing on this check, I think because it does not add the driver task to the GCS. Let's make this a non-fatal check for now. If the task is not in the GCS or the lineage cache, we can just set the parent actor ID to nil (and leave a note/todo explaining this case).

AmplabJenkins · 2019-07-04T05:03:11Z

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

AmplabJenkins · 2019-07-04T05:03:11Z

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

AmplabJenkins · 2019-07-04T07:20: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/15097/
Test PASSed.

AmplabJenkins · 2019-07-05T02:30:32Z

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

AmplabJenkins · 2019-07-05T08:08:58Z

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

AmplabJenkins · 2019-07-09T00:47:04Z

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

AmplabJenkins · 2019-07-09T02:37:32Z

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

AmplabJenkins · 2019-07-09T02:59:26Z

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

AmplabJenkins · 2019-07-09T04:41:16Z

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

Change-Id: I9c1a65134dc23a2d175047e96b86ab9d9cf61971

AmplabJenkins · 2019-07-09T22:26:55Z

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

AmplabJenkins · 2019-07-10T00:20:12Z

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

Change-Id: I1def06218130b399d2527b999258aecf9abb98dd

AmplabJenkins · 2019-07-10T23:13:27Z

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

AmplabJenkins · 2019-07-10T23:49:45Z

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

raulchen · 2019-07-12T04:05:18Z

@vipulharsh @stephanie-wang CI seems broken after merging this PR. Can you take a look?
https://travis-ci.com/ray-project/ray/builds/118854481

vipulharsh added 4 commits July 2, 2019 00:30

Track parent actor of actor

31a7eb7

Merge branch 'master' of https://github.com/vipulharsh/ray

8a68f71

Merge branch 'master' of https://github.com/vipulharsh/ray

8c1ec7f

Merge branch 'master' of https://github.com/vipulharsh/ray

c66759d

stephanie-wang requested changes Jul 2, 2019

View reviewed changes

Update src/ray/raylet/node_manager.cc

0028ed5

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

vipulharsh and others added 4 commits July 2, 2019 20:31

Update src/ray/raylet/node_manager.cc

a86fa32

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

Merge branch 'master' of https://github.com/vipulharsh/ray

e03ceaa

fixing a comment

5f4dc67

Fixing typo in a comment

480aa4d

stephanie-wang reviewed Jul 3, 2019

View reviewed changes

vipulharsh added 4 commits July 3, 2019 23:11

capturing task_spec instead of actor_data

8d09527

Merge branch 'master' of https://github.com/vipulharsh/ray

270131c

adding const for some local variables

c77dc38

changing an if else to else

0cc0d7e

stephanie-wang approved these changes Jul 8, 2019

View reviewed changes

vipulharsh added 2 commits July 8, 2019 19:04

Linted version

88b1f82

merging with ray:master

ea5b10d

stephanie-wang self-assigned this Jul 9, 2019

use updated method to create task from task_data

63d6f54

Change-Id: I9c1a65134dc23a2d175047e96b86ab9d9cf61971

fixing linter issues

72dc792

Change-Id: I1def06218130b399d2527b999258aecf9abb98dd

stephanie-wang merged commit 3b42d5c into ray-project:master Jul 11, 2019

	/// Extend actor frontier when an actor task or actor creation task arrives.
	/// Extend actor frontier after an actor task or actor creation task executes.

Conversation

vipulharsh commented Jul 2, 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!

stephanie-wang left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AmplabJenkins commented Jul 3, 2019

Uh oh!

AmplabJenkins commented Jul 3, 2019

Uh oh!

AmplabJenkins commented Jul 3, 2019

Uh oh!

AmplabJenkins commented Jul 3, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stephanie-wang left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AmplabJenkins commented Jul 4, 2019

Uh oh!

AmplabJenkins commented Jul 4, 2019

Uh oh!

AmplabJenkins commented Jul 4, 2019

Uh oh!

vipulharsh commented Jul 2, 2019 •

edited

Loading