[xray] Basic actor support #1835

stephanie-wang · 2018-04-05T22:57:49Z

What do these changes do?

This adds basic support in the Raylet backend for actors, specifically single-caller actors with no ray.put, reconstruction, or checkpointing.

Backend changes include:

An actor table to publish notifications about new actor creations.
A new TaskSpecification constructor for actor tasks.
An ActorRegistration class to store information about an actor's execution state.
NodeManager support for handling actor creation as well as queuing, forwarding, and assigning actor tasks.

AmplabJenkins · 2018-04-06T00:00:28Z

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

robertnishihara · 2018-04-05T23:41:43Z

python/ray/actor.py

                                            actor_method_cpus,
                                            ray.worker.global_worker)
+            # Increment the actor counter to account for the creation task.
+            actor_counter += 1


Later on I think it would make sense to have state like actor_counter in a C++ worker class instead of in Python. Certainly not in this PR, this is just a comment.

robertnishihara · 2018-04-05T23:49:20Z

src/ray/raylet/node_manager.cc


+void NodeManager::HandleActorCreation(const ActorID &actor_id,
+                                      const std::vector<ActorTableDataT> &data) {
+  RAY_LOG(DEBUG) << "Actor creation notification received: " << actor_id.hex();


just actor_id, no hex()

robertnishihara · 2018-04-05T23:49:36Z

src/ray/raylet/node_manager.cc

+      // executing a task. Clean up the assigned task's resources, return an
+      // error to the driver.
+      // RAY_CHECK(worker->GetAssignedTaskId().is_nil())
+      //    << "Worker died while executing task: " << worker->GetAssignedTaskId().hex();


remove .hex()

robertnishihara · 2018-04-05T23:49:50Z

src/ray/raylet/node_manager.cc


-void NodeManager::FinishTask(const TaskID &task_id) {
+void NodeManager::FinishAssignedTask(std::shared_ptr<Worker> worker) {
+  TaskID task_id = worker->GetAssignedTaskId();


not part of this PR really but can you remove the .hex() in the line below?

robertnishihara · 2018-04-05T23:50:02Z

src/ray/raylet/node_manager.cc

+    actor_notification->driver_id = JobID::nil().binary();
+    actor_notification->node_manager_id =
+        gcs_client_->client_table().GetLocalClientId().binary();
+    RAY_LOG(DEBUG) << "Publishing actor creation: " << actor_id.hex();


remove the .hex()

robertnishihara · 2018-04-05T23:51:27Z

src/ray/raylet/task_spec.h

                    int64_t num_returns,
                    const std::unordered_map<std::string, double> &required_resources);

+  TaskSpecification(UniqueID driver_id, TaskID parent_task_id, int64_t parent_counter,


all IDs should be const ref

robertnishihara · 2018-04-06T00:16:45Z

Still looking over things, overall the code looks way cleaner!

AmplabJenkins · 2018-04-06T01:02:33Z

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

robertnishihara · 2018-04-06T03:47:16Z

src/local_scheduler/local_scheduler_algorithm.cc

+  auto &task_counters =
+      algorithm_state->local_actor_infos[actor_id].task_counters;
+  RAY_CHECK(task_counters[ActorHandleID::nil()] == 0);
+  task_counters[ActorHandleID::nil()]++;


Can you explain this change? I wouldn't have expected any changes to the old code path.

It's because I now count the actor creation task as part of the actor counter on the Python side. Without this change, the old codepath will never run any tasks after the actor creation task since the counters don't match.

robertnishihara · 2018-04-06T04:11:38Z

src/ray/raylet/node_manager.cc

+      // executing a task. Clean up the assigned task's resources, return an
+      // error to the driver.
+      // RAY_CHECK(worker->GetAssignedTaskId().is_nil())
+      //    << "Worker died while executing task: " << worker->GetAssignedTaskId();


What changed here?

This never happened before, but now it happens if the actor goes out of scope and executes the __ray_terminate__ task, which exits mid-task. I thought it might be a good idea to try and handle this case better in the new version, so I held off on it for now.

robertnishihara · 2018-04-06T04:14:03Z

src/ray/raylet/node_manager.cc

+      // We do not have a registered location for the object, so either the
+      // actor has not yet been created or we missed the notification for the
+      // actor creation. Look up the actor's registered location in case we
+      // missed the creation notification.


When would we miss the creation notification? E.g., if a node joins late or something? It seems like it will be very easy for this code path to not get tested. What do you think?

Or will this actually be pretty likely to get called on the first actor method?

Yes, I think it should only happen in the case where a node joins after the actor has already been published. It won't get tested in a single-node setting, unfortunately. I think we should keep it, though, since it's necessary for things like autoscaling. I can add a note about testing it in a cluster setting.

robertnishihara · 2018-04-06T04:31:09Z

src/ray/raylet/node_manager.cc

+
+  // Try to get an idle worker that can execute this task.
+  std::shared_ptr<Worker> worker = worker_pool_.PopWorker(spec.ActorId());
+  if (worker == nullptr) {


Is this a thing that could happen a lot? E.g., would AssignTask be called if there are no workers available?

It can happen pretty often, either at the beginning when workers might not have started/connected yet, or during execution when existing workers are blocked in a ray.get.

robertnishihara · 2018-04-06T04:33:01Z

src/ray/raylet/node_manager.cc

 }

-void NodeManager::AssignTask(const Task &task) {
+void NodeManager::AssignTask(Task &task) {


Is it an assumption going into AssignTask that task is currently unqueued?

Yeah, I can add that to the documentation.

AmplabJenkins · 2018-04-06T04:37:33Z

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

AmplabJenkins · 2018-04-06T04:43:19Z

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

robertnishihara · 2018-04-06T04:45:54Z

src/ray/raylet/node_manager.h


 private:
+  // Handler for the addition of a new GCS client.
+  void ClientAdded(gcs::AsyncGcsClient *client, const UniqueID &id,


I think we should document the arguments for these methods. In particular, I'm not sure what id is (part of the reason is that it is a UniqueID as opposed to something more specific).

Good point, thanks. In this particular case, I don't think UniqueID should even be an argument here...I'll remove it.

robertnishihara · 2018-04-06T04:46:29Z

src/ray/raylet/scheduling_queue.cc

  std::vector<Task> removed_tasks;

  // Try to find the tasks to remove from the waiting tasks.
+  removeTasksFromQueue(uncreated_actor_methods_, task_ids, removed_tasks);


I'm a bit worried about RemoveTasks being expensive, but that's an issue for later.

Yeah, I think eventually the plan is to make the implementation of RemoveTasks store a hashmap from TaskID to iterator so that we can remove it from the queue quickly.

AmplabJenkins · 2018-04-06T05:30:42Z

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

AmplabJenkins · 2018-04-06T05:57: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/4688/
Test PASSed.

AmplabJenkins · 2018-04-06T06:14: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/4690/
Test PASSed.

stephanie-wang added 11 commits April 3, 2018 11:26

Define a TaskSpecification constructor for actor tasks

4fb8fd5

Add ActorID to the workers

bb6d91b

Add an ActorTable to the GCS

16a4afd

Node manager converts worker to actors and publishes creation

4865eab

Move NodeManager GCS callbacks into a separate Register function

ecb9734

Register new actors based on notifications from the actor table

68f32f5

Queue actor methods until actor location is received

4b97e45

Schedule actor tasks based on execution dependencies

a126ddc

Mutate actor execution dependencies to reflect execution order

c4d3f5c

Filter out duplicate actor tasks during assignment

4bfaf29

Merge branch 'master' into xray-actors

e535903

stephanie-wang requested review from atumanov, pcmoritz and robertnishihara April 5, 2018 22:57

robertnishihara reviewed Apr 6, 2018

View reviewed changes

Doc

0b22779

stephanie-wang and others added 2 commits April 5, 2018 20:34

Fixes

c3c6aac

Fix compilation on MacOS.

8a0ac94

robertnishihara reviewed Apr 6, 2018

View reviewed changes

Fix linting.

e9d9cdd

robertnishihara reviewed Apr 6, 2018

View reviewed changes

Fix typo.

9a6ae1e

robertnishihara reviewed Apr 6, 2018

View reviewed changes

robertnishihara approved these changes Apr 6, 2018

View reviewed changes

Cleanups

8d115be

robertnishihara merged commit bf194db into ray-project:master Apr 6, 2018

robertnishihara deleted the xray-actors branch April 6, 2018 07:17

[xray] Basic actor support #1835

[xray] Basic actor support #1835

Uh oh!

Conversation

stephanie-wang commented Apr 5, 2018

What do these changes do?

Uh oh!

AmplabJenkins commented Apr 6, 2018

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!

robertnishihara commented Apr 6, 2018

Uh oh!

AmplabJenkins commented Apr 6, 2018

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 Apr 6, 2018

Uh oh!

AmplabJenkins commented Apr 6, 2018

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 Apr 6, 2018

Uh oh!

AmplabJenkins commented Apr 6, 2018

Uh oh!

AmplabJenkins commented Apr 6, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants