[WIP]Placement group dev by Alisahhh · Pull Request #8914 · ray-project/ray

Alisahhh · 2020-06-12T13:32:24Z

Why are these changes needed?

To alloc resources in node before creating actors and store the resources map in gcs server.

Now I almost finish the cpp part.

TODO:

Finish cpp part and add python API and JAVA api.
Add unit test.

NOTES:

This part is WIP and I will submit this change(cpp part) in about three small pr.

Related issue number

Checks

I've run scripts/format.sh to lint the changes in this PR.
I've included any doc changes needed for https://docs.ray.io/en/latest/.
I've made sure the tests are passing. Note that there might be a few flaky tests, see the recent failure rates at https://ray-travis-tracker.herokuapp.com/.
Testing Strategy
- Unit tests
- Release tests
- This PR is not tested (please justify below)

…om raylet

AmplabJenkins · 2020-06-12T13:36:43Z

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

ericl · 2020-06-12T19:05:13Z

@Alisahhh when it's ready can you add a python example script to the PR description so others can easily know how to try it out?

Alisahhh · 2020-06-15T02:07:17Z

Sure~I will add some example when i finish python api :) Eric Liang <notifications@github.com> 于 2020年6月13日周六上午3:05写道：

…

@Alisahhh <https://github.com/Alisahhh> when it's ready can you add a python example script to the PR description so others can easily know how to try it out? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#8914 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AHIO5OYUSVB7WU64Y5YJ7IDRWJ37PANCNFSM4N4KU6XA> .

ericl · 2020-06-15T02:12:52Z

Sounds great! Eric

…

On Sun, Jun 14, 2020 at 7:07 PM Alisa ***@***.***> wrote: Sure~I will add some example when i finish python api :) Eric Liang ***@***.***> 于 2020年6月13日周六上午3:05写道： > @Alisahhh <https://github.com/Alisahhh> when it's ready can you add a > python example script to the PR description so others can easily know how > to try it out? > > — > You are receiving this because you were mentioned. > Reply to this email directly, view it on GitHub > <#8914 (comment)>, or > unsubscribe > < https://github.com/notifications/unsubscribe-auth/AHIO5OYUSVB7WU64Y5YJ7IDRWJ37PANCNFSM4N4KU6XA > > . > — You are receiving this because you were assigned. Reply to this email directly, view it on GitHub <#8914 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAADUSRGVQQXC4BVFIMZBT3RWV66DANCNFSM4N4KU6XA> .

AmplabJenkins · 2020-06-15T05:37:17Z

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

AmplabJenkins · 2020-06-15T08:39:46Z

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

AmplabJenkins · 2020-06-16T13:56:18Z

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

AmplabJenkins · 2020-06-18T03: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/27272/
Test FAILed.

AmplabJenkins · 2020-06-19T09:16:23Z

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

ericl

Hey @Alisahhh one big question I have from skimming the header files is: why not represent the bundle id as std::pair<PlacementGroupId, int>? That seems simpler and ensures you can always find the placement group given the bundle identifier.

ericl · 2020-06-19T22:04:11Z

src/ray/raylet/node_manager.cc

+  RAY_LOG(DEBUG) << "bundle lease request " << bundle_spec.BundleId();
+  auto resource_ids = ScheduleBundle(cluster_resource_map_, bundle_spec);
+  if (resource_ids.AvailableResources().size() == 0) {
+    send_reply_callback(Status::OutOfMemory("reserve resource failed"), nullptr, nullptr);


This should be returned as a boolean field in the proto message indicating failure, not a system-level OOM exception.

Change in the #9039

ericl · 2020-06-19T22:06:20Z

src/ray/protobuf/common.proto

+message Bundle {
+  bytes bundle_id = 1;
+  map<string, double> unit_resources = 2;
+  int64 unit_count = 3;


Do we need unit count in the first iteration? Consider leaving it out and implementing it later if needed.

ericl · 2020-06-19T22:07:12Z

src/ray/protobuf/gcs.proto

+    // State of a placement group.
+    enum PlacementGroupState {
+      // Placement Group is pending or scheduling
+      PENDING = 0;


Does it means it goes back to PENDING if reconstructing?

Yep, if a placement group creating failed, it will be added into the tail of the queue and the status will keep PENDING

ericl · 2020-06-19T22:08:18Z

src/ray/protobuf/gcs_service.proto

+// Service for placement group info access.
+service PlacementGroupInfoGcsService{
+  // Create placement group via gcs service
+  rpc CreatePlacementGroup(CreatePlacementGroupRequest) returns (CreatePlacementGroupReply);


How do we check the status of a placement group? Or is that for a later PR?

I will add the check in next pr. It looks like an actor creation in the core worker callback.

Alisahhh · 2020-06-22T02:55:21Z

Hey @Alisahhh one big question I have from skimming the header files is: why not represent the bundle id as std::pair<PlacementGroupId, int>? That seems simpler and ensures you can always find the placement group given the bundle identifier.

We have discussed this question last week and given multi plan. Now the bundle id will be given when the bundle creates which is earlier than the placement group creates.
BTW, I think a placement group is a virtual concept, so the bundle should not bound to the placement group?

ericl · 2020-06-22T03:22:56Z

What if someone wants to place into any bundle in the group or some range of bundles? That could be represented as a placement group id with no index restriction. I think passing around raw bundle IDs is problematic for that reason, and also because it's lower level than placement group.

…

On Sun, Jun 21, 2020, 7:55 PM Alisa ***@***.***> wrote: Hey @Alisahhh <https://github.com/Alisahhh> one big question I have from skimming the header files is: why not represent the bundle id as std::pair<PlacementGroupId, int>? That seems simpler and ensures you can always find the placement group given the bundle identifier. We have discussed this question last week and given multi plan. Now the bundle id will be given when the bundle creates which is earlier than the placement group creates. BTW, I think a placement group is a virtual concept, so the bundle should not bound to the placement group? — You are receiving this because you were assigned. Reply to this email directly, view it on GitHub <#8914 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAADUSQQIDMJQATS2OJF6BTRX3B2LANCNFSM4N4KU6XA> .

Alisahhh · 2020-06-22T06:22:03Z

What if someone wants to place into any bundle in the group or some range of bundles? That could be represented as a placement group id with no index restriction. I think passing around raw bundle IDs is problematic for that reason, and also because it's lower level than placement group.
…
On Sun, Jun 21, 2020, 7:55 PM Alisa @.***> wrote: Hey @Alisahhh https://github.com/Alisahhh one big question I have from skimming the header files is: why not represent the bundle id as std::pair<PlacementGroupId, int>? That seems simpler and ensures you can always find the placement group given the bundle identifier. We have discussed this question last week and given multi plan. Now the bundle id will be given when the bundle creates which is earlier than the placement group creates. BTW, I think a placement group is a virtual concept, so the bundle should not bound to the placement group? — You are receiving this because you were assigned. Reply to this email directly, view it on GitHub <#8914 (comment)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAADUSQQIDMJQATS2OJF6BTRX3B2LANCNFSM4N4KU6XA .

Sure,

What if someone wants to place into any bundle in the group or some range of bundles? That could be represented as a placement group id with no index restriction. I think passing around raw bundle IDs is problematic for that reason, and also because it's lower level than placement group.
…
On Sun, Jun 21, 2020, 7:55 PM Alisa @.***> wrote: Hey @Alisahhh https://github.com/Alisahhh one big question I have from skimming the header files is: why not represent the bundle id as std::pair<PlacementGroupId, int>? That seems simpler and ensures you can always find the placement group given the bundle identifier. We have discussed this question last week and given multi plan. Now the bundle id will be given when the bundle creates which is earlier than the placement group creates. BTW, I think a placement group is a virtual concept, so the bundle should not bound to the placement group? — You are receiving this because you were assigned. Reply to this email directly, view it on GitHub <#8914 (comment)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAADUSQQIDMJQATS2OJF6BTRX3B2LANCNFSM4N4KU6XA .

I want to check if you want I assign the BundleID like PlacementID+index, which is like the actor id has the information about task id and worker id.
BTW, If I want to include the placement group id information in bundle id, I need to copy the whole array in the core worker, I don't know if it a good implementation?

ericl · 2020-06-22T18:38:52Z

I want to check if you want I assign the BundleID like PlacementID+index, which is like the actor id has the information about task id and worker id.
BTW, If I want to include the placement group id information in bundle id, I need to copy the whole array in the core worker, I don't know if it a good implementation?

I don't think we need a "BundleID" at all. The way I think of it there are two cases here:

The representation of a bundle identifier (INTERNAL, this data structure should never appear in any public APIs), which can be

message BundleIdentifier {
   PlacementGroupId group_id;
   int32 bundle_index; 
}

The representation of a placement specification targeting bundle(s), which is exposed to the user. This can be something like

message PlacementGroupRequirements {
    PlacementGroupId group_id;
    int32 target_bundle [default = -1];   # -1 means any bundle is ok to place in
}

@Alisahhh let me know if you see any problems.

raulchen · 2020-06-23T10:32:51Z

@ericl are you mainly concerned about exposing BundleId to the user API?
I agree that it's not a good idea to expose BundleId. Ideally, we shouldn't expose PlacementGroupId either (due to the same reason why we change ObjectId to ObjectRef).

What about just using the placement group and bundle object in the API?

placement_group = ray.create_placement_group([{"CPU": 1}, {"CPU": 2}])  # Type of `placement_group` is `PlacementGroup`. 
bundle0 = placement_group.get_bundle(0)  # Type of `bundle` is `Bundle`.
# You can pass either a `PlacementGroup` or a `Bundle` object when creating an actor.
actor1 = MyActor.options(placement=placement_group).remote()
actor2 = MyActor.options(placement=bundle0).remote()

ericl · 2020-06-23T16:51:35Z

Hao, that's right. Two concerns: 1. The user API as you mentioned. I agree with the proposal you have. 2. The internal representation. I don't see a compelling reason why bundles should exist separately as an entity independent from placement groups. If they must be always be referred to via index, that seems cleaner to me and less error prone (you don't need inverse mappings of bundles to groups and so on).

…

On Tue, Jun 23, 2020, 3:33 AM Hao Chen ***@***.***> wrote: @ericl <https://github.com/ericl> are you mainly concerned about exposing BundleId to the user API? I agree that it's not a good idea to expose BundleId. Ideally, we shouldn't expose PlacementGroupId either (due to the same reason why we change ObjectId to ObjectRef). What about just using the placement group and bundle object in the API? placement_group = ray.create_placement_group([{"CPU": 1}, {"CPU": 2}]) # Type of `placement_group` is `PlacementGroup`. bundle0 = placement_group.get_bundle(0) # Type of `bundle` is `Bundle`.# You can pass either a `PlacementGroup` or a `Bundle` object when creating an actor.actor1 = MyActor.options(placement=placement_group).remote()actor2 = MyActor.options(placement=bundle0).remote() — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#8914 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAADUSTDWJWLNTFCXFA43G3RYCAGFANCNFSM4N4KU6XA> .

ericl · 2020-06-23T19:15:52Z

In the future, when we have referencing counting for PlacementGroup handles, having bundles be implemented as a placement group + index also simplifies the situation. The entire placement group is referenced if any bundle is.

alisa and others added 29 commits May 27, 2020 10:34

add api to implement placement group

581e568

add gRPC from node to gcs

5f651c5

add grpc from gcs to redis

4feb51a

add placement group manager and shcedule. (todo) try to lease work fr…

24a3012

…om raylet

can't run

a435db6

fix the bug in last commit and pass the compile

9b2c09f

implement the function structure for raylet to schedule the resource

2be262f

add lease resource

a7c29f7

add clock

2fc7c4f

add release resource when acquire resource failed

16dc98c

add some method and change part of actor api

5168f66

finish actor create

8ebb4b2

format

12eb206

fix some todo and add test

9a85f49

add init and storage

418179c

merge master

a54c7f5

fix merge

5dab627

fix merge

e94d293

fix test

67dcb60

fix ci

df7c944

fix id and ci

5a5ec58

fix ci

0299afa

fix ci and delete max_restart

14fc8de

fix type warning

7b32340

fix type warning

5c6241a

add part of pytho api

914398c

change some api

527370d

delete bundle in actor

646d3a9

fix test

5f8d578

rkooo567 self-requested a review June 12, 2020 15:51

ericl self-assigned this Jun 12, 2020

add globle resourece schedule in scheduling_resouce.h

917ac2f

format

323d9c6

add placement group schedule test and fix some bug

0faa45c

change some test in scheduler and add schedule resource test

31c3ebf

fix the resource api & add accessor

f5ca328

rkooo567 mentioned this pull request Jun 19, 2020

[Core] Add placement group scheduler and some api in resource scheduler #9039

Merged

4 tasks

ericl reviewed Jun 19, 2020

View reviewed changes

ericl removed their assignment Jun 29, 2020

Alisahhh closed this Jul 2, 2020

Conversation

Alisahhh commented Jun 12, 2020

Why are these changes needed?

TODO:

NOTES:

Related issue number

Checks

Uh oh!

AmplabJenkins commented Jun 12, 2020

Uh oh!

ericl commented Jun 12, 2020

Uh oh!

Alisahhh commented Jun 15, 2020 via email

Uh oh!

ericl commented Jun 15, 2020 via email

Uh oh!

AmplabJenkins commented Jun 15, 2020

Uh oh!

AmplabJenkins commented Jun 15, 2020

Uh oh!

AmplabJenkins commented Jun 16, 2020

Uh oh!

AmplabJenkins commented Jun 18, 2020

Uh oh!

AmplabJenkins commented Jun 19, 2020

Uh oh!

ericl left a comment

Choose a reason for hiding this comment

Uh oh!

ericl Jun 19, 2020

Choose a reason for hiding this comment

Uh oh!

Alisahhh Jun 22, 2020

Choose a reason for hiding this comment

Uh oh!

ericl Jun 19, 2020

Choose a reason for hiding this comment

Uh oh!

ericl Jun 19, 2020

Choose a reason for hiding this comment

Uh oh!

Alisahhh Jun 22, 2020

Choose a reason for hiding this comment

Uh oh!

ericl Jun 19, 2020

Choose a reason for hiding this comment

Uh oh!

Alisahhh Jun 22, 2020

Choose a reason for hiding this comment

Uh oh!

Alisahhh commented Jun 22, 2020

Uh oh!

ericl commented Jun 22, 2020 via email

Uh oh!

Alisahhh commented Jun 22, 2020

Uh oh!

ericl commented Jun 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

raulchen commented Jun 23, 2020

Uh oh!

ericl commented Jun 23, 2020 via email • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ericl commented Jun 23, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ericl commented Jun 22, 2020 •

edited

Loading

ericl commented Jun 23, 2020 via email •

edited

Loading