Introduce concept of resources required for placing a task. by robertnishihara · Pull Request #2837 · ray-project/ray

robertnishihara · 2018-09-07T06:46:32Z

This PR introduces the concept of "resources that are required for scheduling/placing a task", which are distinct from (and a superset of) "resources acquired during task execution". Typically, these two things are the same, however, in the case of actor creation tasks, we require additional resources to schedule the task on a node because it must take into account the future resource requirements of the actor methods. This fixes #2803.

robertnishihara · 2018-09-07T06:46:48Z

@atumanov @ericl please take a look.

AmplabJenkins · 2018-09-07T08:17: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/8045/
Test PASSed.

AmplabJenkins · 2018-09-07T08:28: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/8046/
Test FAILed.

atumanov · 2018-09-07T16:04:25Z

can we make it just a little less verbose than Java? How about GetRequiredPlacementResources

atumanov

partial set of comments for now.

atumanov · 2018-09-07T16:05:17Z

do we need to wrap this line? Hard to tell, but looks like < 90chars to me if combined. Ignore if not the case.

atumanov · 2018-09-07T16:10:54Z

are you trying to generate a json-compatible string here? I wonder if we should just use the json library or add a ToJson() method that explicitly does that.

The goal was not to make a JSON compatible string, but rather to make the printing nicer. This changes it from {GPU,1.000000}, {CPU,1.000000}, (note the extra comma and space at the end) to {GPU,1.000000}, {CPU,1.000000}.

(can we reduce the number of decimals?)

also there's no implode/join built-in for this?

AmplabJenkins · 2018-09-07T18:41: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/8075/
Test FAILed.

atumanov · 2018-09-07T19:03:14Z

const ref : const auto &spec to avoid a copy.

atumanov · 2018-09-07T19:04:35Z

const ref : const auto &spec (same as above)

AmplabJenkins · 2018-09-07T20:04:58Z

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

robertnishihara · 2018-09-07T20:08:33Z

jenkins, retest this please

AmplabJenkins · 2018-09-07T20:54:26Z

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

ericl · 2018-09-08T05:57:50Z

Is it always 1 more? What about the num_cpus=0 or num_cpus=N case?

Oh good catch, in that case it should be 0. I'll fix this.

That was a bug!

richardliaw · 2018-09-13T18:49:29Z

IMO this error message would be better if it reported the current state (i.e., resources currently used out of total resources in cluster)

richardliaw · 2018-09-13T18:55:27Z

same here (more info about "no nodes have the necessary resources")

richardliaw · 2018-09-13T19:00:24Z

(can we reduce the number of decimals?)

richardliaw · 2018-09-13T20:05:25Z

also there's no implode/join built-in for this?

robertnishihara · 2018-10-02T23:18:02Z

@raulchen @jovan-wong What is the correct procedure for updating java/runtime/src/main/java/org/ray/runtime/generated/TaskInfo.java after updating the flatbuffer definition?

I basically had to copy it over from ray/build/generated/java and then modify a handful of things by hand (e.g., change the imports at the top and the code at the bottom to minimize the diff).

What's the reason for committing the autogenerated files?

AmplabJenkins · 2018-10-02T23:22:34Z

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

AmplabJenkins · 2018-10-02T23:34:35Z

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

AmplabJenkins · 2018-10-03T00:18:50Z

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

AmplabJenkins · 2018-10-03T00:42: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/8516/
Test PASSed.

jovany-wang · 2018-10-03T04:41:14Z

@robertnishihara Thanks for reminding me about this.

I took a look at TaskInfo.java.
The reason for this is that there are some manual code here. And it can be moved out of TaskInfo i think. It's time to auto-gen all of these code right now.

But there is a problem that is not well resolved:
We should add the package declaration in TaskInfo.java, like package org.ray.runtime.generated; . This requires us to declare the namespace in fbs file, like namespace org.ray.runtime.generated, right?
But I think this namespace declaration in fbs will not suitable in python or cpp.

How do you guys think of it?

robertnishihara · 2018-10-03T19:23:32Z

cc @romilbhardwaj

atumanov

Just a few minor comments. Made a full pass on C++.

atumanov · 2018-10-04T04:31:55Z

 void NodeManager::SubmitTask(const Task &task, const Lineage &uncommitted_lineage,
                             bool forwarded) {
-  const TaskID task_id = task.GetTaskSpecification().TaskId();
+  const TaskID &task_id = task.GetTaskSpecification().TaskId();


atumanov · 2018-10-04T04:34:36Z

        int client_key_index = distribution(gen_);
        const ClientID &dst_client_id = client_keys[client_key_index];
-        decision[t.GetTaskSpecification().TaskId()] = dst_client_id;
+        decision[spec.TaskId()] = dst_client_id;


I think we can just use task_id here

atumanov · 2018-10-04T04:35:34Z

-        RAY_LOG(INFO) << "This task requires "
-                      << t.GetTaskSpecification().GetRequiredResources().ToString()
-                      << ", but no nodes have the necessary resources.";
+        RAY_LOG(INFO) << "The task with ID " << spec.TaskId() << " requires "


same, spec.TaskId() --> task_id

atumanov · 2018-10-04T04:42:00Z


 bool ResourceSet::operator==(const ResourceSet &rhs) const {
-  return (this->IsSubset(rhs) && rhs.IsSubset(*this));
+  return (IsSubset(rhs) && rhs.IsSubset(*this));


actually, in the case where we overload binary operators, it's useful to keep this-> for code readability. You get "this" is a subset of the right hand side. Removing it makes it a little less clear what rhs is supposed to be a subset of (because this-> is implicit).

atumanov · 2018-10-04T04:42:50Z

 /// Test whether this ResourceSet is precisely equal to the other ResourceSet.
 bool ResourceSet::IsEqual(const ResourceSet &rhs) const {
-  return (this->IsSubset(rhs) && rhs.IsSubset(*this));
+  return (IsSubset(rhs) && rhs.IsSubset(*this));


same, I would prefer to keep this-> for readability when overloading binary operators of the form A.BinaryOperator(B).

atumanov · 2018-10-04T04:43:21Z

    return false;
  }
-  if (this->resource_capacity_.count(resource_name) == 0) {
+  if (resource_capacity_.count(resource_name) == 0) {


and here, it makes sense to drop this-> as you did.

atumanov · 2018-10-04T04:45:48Z

+  it++;
+
+  // Add the remaining elements to the string (along with a comma).
+  while (it != resource_capacity_.end()) {


why not:
for ( ; it != resource_capacity_.end() ; ++it )
But I don't have a strong opinion on this.

AmplabJenkins · 2018-10-04T05:27: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/8541/
Test FAILed.

AmplabJenkins · 2018-10-04T07:41:44Z

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

AmplabJenkins · 2018-10-04T08:25:36Z

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

atumanov

LGTM

robertnishihara changed the title ~~Introduce concept of resources required for placement.~~ Introduce concept of resources required for placing a task. Sep 7, 2018

atumanov reviewed Sep 7, 2018

View reviewed changes

Comment thread src/ray/raylet/scheduling_policy.cc Outdated

Copy link
Copy Markdown

Contributor

atumanov Sep 7, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

const ref : const auto &spec to avoid a copy.

atumanov reviewed Sep 7, 2018

View reviewed changes

Comment thread src/ray/raylet/scheduling_policy.cc Outdated

Copy link
Copy Markdown

Contributor

atumanov Sep 7, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

const ref : const auto &spec (same as above)

ericl reviewed Sep 8, 2018

View reviewed changes

richardliaw reviewed Sep 13, 2018

View reviewed changes

robertnishihara force-pushed the placementresources branch from c75e887 to e8c92bf Compare October 2, 2018 22:01

robertnishihara added 8 commits October 3, 2018 17:59

Introduce concept of resources required for placement.

a9d7aaf

Fix linting.

782707d

Small changes.

01d9269

Make task specs const ref.

fd6606e

Add placement resources to task spec

5d10f76

improve error message

d3b8f00

Fix linting

b32776b

Update java worker

7c7c865

robertnishihara added 2 commits October 3, 2018 17:59

Update taskinfo.java

ebf8289

Small cleanups

2cf31d5

robertnishihara force-pushed the placementresources branch from d991d43 to 2cf31d5 Compare October 4, 2018 04:01

atumanov reviewed Oct 4, 2018

View reviewed changes

robertnishihara added 2 commits October 3, 2018 23:08

Minor

6a08e66

Linting

c9b98f8

atumanov approved these changes Oct 4, 2018

View reviewed changes

atumanov merged commit faa31ae into ray-project:master Oct 4, 2018

robertnishihara deleted the placementresources branch November 17, 2018 02:19

kfstorm mentioned this pull request Mar 11, 2021

Actor placement resources should be deprecated #14619

Closed

Conversation

robertnishihara commented Sep 7, 2018

Uh oh!

robertnishihara commented Sep 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AmplabJenkins commented Sep 7, 2018

Uh oh!

AmplabJenkins commented Sep 7, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

atumanov 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!

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 Sep 7, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AmplabJenkins commented Sep 7, 2018

Uh oh!

robertnishihara commented Sep 7, 2018

Uh oh!

AmplabJenkins commented Sep 7, 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!

robertnishihara commented Oct 2, 2018

Uh oh!

AmplabJenkins commented Oct 2, 2018

Uh oh!

AmplabJenkins commented Oct 2, 2018

Uh oh!

AmplabJenkins commented Oct 3, 2018

Uh oh!

AmplabJenkins commented Oct 3, 2018

Uh oh!

jovany-wang commented Oct 3, 2018

Uh oh!

robertnishihara commented Oct 3, 2018

Uh oh!

atumanov 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

robertnishihara commented Sep 7, 2018 •

edited

Loading