[Core] Make Placement Group Wildcard and Indexed Resource Assignments Consistent #48088

MengjinYan · 2024-10-17T18:17:47Z

Why are these changes needed?

Currently when we allocate resources to tasks or actors with placement group,

if the resource request doesn't specify the placement group bundle index, we will not subtract the resources from the indexed resource and will only subtract the resource from the wildcard resource.
In addition, if the bundle index is specified, the resource allocation is not consistent between the wildcard resource and the indexed resource for unit resources (e.g. GPU).

The behavior above leads to incorrect GPU assignment when specifying bundle index. Also, task can potentially be allocated across bundles if no bundle index specified.

This PR fix the above behavior to make the allocation on the wildcard resource and the indexed resource consistent with the following changes:

Updated the TryAllocate function in resource_instance_set.cc to add logic to handle the pg resource allocation. If the resource requirement include resource in a placement group,
(1) it will always first try to allocation on index resources. For resource requirements without a bundle index, it will iterate through all the bundles and try to find one with enough space.
(2)Then it will try to allocate the wildcard resource based on the index resource allocation.
Added corresponding helper functions & auxiliary data structures
Added/Modified tests for the new behavior

Related issue number

Closes #29811

Checks

I've signed off every commit(by using the -s flag, i.e., git commit -s) in this PR.
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/master/.
- I've added any new APIs to the API Reference. For example, if I added a
  method in Tune, I've added it in doc/source/tune/api/ under the
  corresponding .rst file.
I've made sure the tests are passing. Note that there might be a few flaky tests, see the recent failures at https://flakey-tests.ray.io/
Testing Strategy
- Unit tests
- Release tests
- This PR is not tested :(

Signed-off-by: Mengjin Yan <mengjinyan3@gmail.com>

rkooo567 · 2024-10-21T03:20:25Z

can you run some pg related release tests @MengjinYan ?

rkooo567 · 2024-10-25T01:23:18Z

@MengjinYan there are some failures from core tests. can you take a look? I am in progress with reviewing

MengjinYan · 2024-10-25T16:24:31Z

@MengjinYan there are some failures from core tests. can you take a look? I am in progress with reviewing

Yes. I took a look and found the bug. I didn't consider the case where multiple PGs coexists on a host and allocated the indexed bundle regardless of the pg_id when the index is not specified. Will submit a fix for that soon.

…ces in the case where the bundle id is not specified Signed-off-by: Mengjin Yan <mengjinyan3@gmail.com>

rkooo567

Generally lgtm! Some concerns around performance (because I remember the scheduler was sensitive to performance for some large stress tests)

rkooo567 · 2024-10-24T21:55:32Z

src/ray/common/scheduling/resource_instance_set.cc

+                                       /*for_indexed_resource=*/true);
+  if (data) {
+    ResourceID original_resource_id(data->original_resource);
+    absl::flat_hash_set<ResourceID> &resource_set =


I think if you get data by bracket and if the data doesn't exist, it. creates a new one

Suggested change

absl::flat_hash_set<ResourceID> &resource_set =

pg_indexed_resources_.find(original_resource_id) != pg_indexed_resources_.end() {

it->second.erase(...)

}

Good point! Will update.

rkooo567 · 2024-10-24T22:02:17Z

src/ray/common/scheduling/resource_instance_set.cc

+                                         /*for_wildcard_resource=*/false,
+                                         /*for_indexed_resource=*/true);
+    if (data) {
+      pg_indexed_resources_[ResourceID(data->original_resource)].emplace(resource_id);


same comment as above

Actually for this particular case, I'd like to create a default vector when the particular resource id doesn't exist. I'm wondering if square brackets is okay here. As I don't have a lot of context on c++ best practices so please let me know what's the best pattern.

rkooo567 · 2024-10-24T22:02:42Z

src/ray/common/scheduling/resource_instance_set.cc

@@ -43,6 +45,21 @@ bool NodeResourceInstanceSet::Has(ResourceID resource_id) const {

 void NodeResourceInstanceSet::Remove(ResourceID resource_id) {
  resources_.erase(resource_id);
+
+  // Remove from the pg_indexed_resources_ as well
+  auto data = ParsePgFormattedResource(resource_id.Binary(),


I suspect this will add significant overhead to the scheduler (because parsing string is so much more expensive). is there any way to do this only when we know resource id is pg?

Discussed offline. And I checked the code. The Remove function is only called in the DeleteLocalResource function in LocalResourceManager when returning the pg bundle. The overhead added shouldn't impact the schedule of the task.

At the same time, the concerns on the Set and TryAllocate are valid. One potential idea I can think of is adding a boolean in the ResourceID to indicate whether it is a pg or not. Will further explore the idea.

I think the overhead will be smaller if PG resources starts with a prefix so we can check using startswith instead of regex.

Make sense!

At same time, I think the downside is: we need to make sure any custom resources won't start with the same prefix and we still need to do the parsing for all the placement group allocation.

Also, as the release didn't show noticeable performance regression, the code can be checked in as it is. I'll add the idea to the TODO for the future improvement.

rkooo567 · 2024-10-24T22:03:02Z

src/ray/common/scheduling/resource_instance_set.cc

@@ -69,6 +86,14 @@ NodeResourceInstanceSet &NodeResourceInstanceSet::Set(ResourceID resource_id,
    resources_.erase(resource_id);
  } else {
    resources_[resource_id] = std::move(instances);
+
+    // Popluate the pg_indexed_resources_map_
+    auto data = ParsePgFormattedResource(resource_id.Binary(),


same comment as above

rkooo567 · 2024-10-24T22:21:07Z

src/ray/common/scheduling/resource_instance_set.h

@@ -87,6 +87,12 @@ class NodeResourceInstanceSet {
    return resources_;
  }

+  /// Only for testing.


is it really used? I couldn't find it

Right. It is not used. I was to follow the above function for resources_.

At the same time, I think it makes sense to remove the function and add it when it is needed in the test

src/ray/common/scheduling/resource_instance_set.cc

rkooo567 · 2024-10-28T18:39:32Z

src/ray/common/scheduling/resource_instance_set.cc

-      allocations[resource_id] = std::move(*allocation);
+    auto data = ParsePgFormattedResource(resource_id.Binary(),
+                                         /*for_wildcard_resource*/ true,
+                                         /*for_indexed_resource*/ true);


isn't this supposed to be false?

The idea here is to find all pg resources including both wildcard resources and indexed resources.

And my understanding is that we need to set both "for_wildcard_resource" and "for_indexed_resource" to be true to find the resource ids for both.

So here I think we should still set it to be true.

rkooo567 · 2024-10-28T18:41:40Z

src/ray/common/scheduling/resource_instance_set.cc

+        return std::nullopt;
+      }
+    } else {
+      // The case where the bundle index is specified


is there no case where the demands only include indexed resources, but not wildcard resources? I assume no?

(feel like the logic assumes this to be correct. maybe add it to the docstring?)

IIUC, here:

ray/src/ray/common/bundle_spec.cc

Line 199 in d80b22f

// Note that all nodes that have placement group have a special

it will always include the wildcard resource.

At the same time, I'll add the assumption to the comment as well.

rkooo567 · 2024-10-28T18:47:18Z

src/ray/common/scheduling/resource_instance_set.cc

+          auto allocation = TryAllocate(pair.first, resource_demands.Get(pair.first));
+
+          if (allocation) {
+            wildcard_allocation = *allocation;


maybe do this only once to avoid repetitive copy?

Good point! Currently, the same allocation will used for both wildcard resource and indexed resource.

Instead of doing a copy here and 2 moves for both wildcard and index resource, I think I can be a copy for the indexed resource and a move for the wildcard resource,

Co-authored-by: SangBin Cho <rkooo567@gmail.com> Signed-off-by: Mengjin Yan <mengjinyan3@gmail.com>

Signed-off-by: Mengjin Yan <mengjinyan3@gmail.com>

MengjinYan · 2024-10-30T20:32:18Z

Generally lgtm! Some concerns around performance (because I remember the scheduler was sensitive to performance for some large stress tests)

Discussed offline and ran the "many tasks" benchmark test.

From the benchmark test result, we didn't see noticeable performance regression compare to results from tests running on the master branch. At the same time, the current idea that we have for the improvement will need non-trivial effort and would be worth a separate effort for that.

I'll also run the stress_test_many_tasks tests to further verify the latency. And if there still noticeable regression, the code should e able to check in without further performance improvements regarding the string parsing.

At the same time, I also added "TODO" comments at the place where we do the parsing with the improvement idea.

MengjinYan · 2024-10-30T23:09:27Z

Generally lgtm! Some concerns around performance (because I remember the scheduler was sensitive to performance for some large stress tests)

Discussed offline and ran the "many tasks" benchmark test.

From the benchmark test result, we didn't see noticeable performance regression compare to results from tests running on the master branch. At the same time, the current idea that we have for the improvement will need non-trivial effort and would be worth a separate effort for that.

I'll also run the stress_test_many_tasks tests to further verify the latency. And if there still noticeable regression, the code should e able to check in without further performance improvements regarding the string parsing.

At the same time, I also added "TODO" comments at the place where we do the parsing with the improvement idea.

Performance benchmark result from stress_test_many_tasks:

[{"perf_metric_name":"stage_0_time","perf_metric_value":8.555947065353394,"perf_metric_type":"LATENCY"},
  {"perf_metric_name":"stage_1_avg_iteration_time","perf_metric_value":27.85337915420532,"perf_metric_type":"LATENCY"},
  {"perf_metric_name":"stage_2_avg_iteration_time","perf_metric_value":68.38944630622863,"perf_metric_type":"LATENCY"},
  {"perf_metric_name":"stage_3_creation_time","perf_metric_value":1.8722784519195557,"perf_metric_type":"LATENCY"},
  {"perf_metric_name":"stage_3_time","perf_metric_value":3337.7971575260162,"perf_metric_type":"LATENCY"},
  {"perf_metric_name":"stage_4_spread","perf_metric_value":0.35827938592076564,"perf_metric_type":"LATENCY"}]

Compare to the current result and there is no noticeable regression:

jjyao · 2024-10-31T20:31:45Z

src/ray/common/bundle_spec.cc

@@ -95,15 +95,15 @@ std::string BundleSpecification::DebugString() const {
 }

 std::string FormatPlacementGroupResource(const std::string &original_resource_name,
-                                         const PlacementGroupID &group_id,
+                                         const std::string &group_id_str,


group_id_hex?

jjyao · 2024-10-31T20:33:06Z

src/ray/common/bundle_spec.h

+///                     [original_resource_name]_group_[bundle_index]_[group_id_str]
+/// \return The corresponding formatted placement group resource string.
+std::string FormatPlacementGroupResource(const std::string &original_resource_name,
+                                         const std::string &group_id_str,


group_id_hex?

src/ray/common/scheduling/resource_instance_set.h

jjyao · 2024-10-31T20:50:00Z

src/ray/common/scheduling/resource_instance_set.cc

@@ -43,6 +45,21 @@ bool NodeResourceInstanceSet::Has(ResourceID resource_id) const {

 void NodeResourceInstanceSet::Remove(ResourceID resource_id) {
  resources_.erase(resource_id);
+
+  // Remove from the pg_indexed_resources_ as well
+  auto data = ParsePgFormattedResource(resource_id.Binary(),


I think the overhead will be smaller if PG resources starts with a prefix so we can check using startswith instead of regex.

Co-authored-by: Jiajun Yao <jeromeyjj@gmail.com> Signed-off-by: Mengjin Yan <mengjinyan3@gmail.com>

Signed-off-by: Mengjin Yan <mengjinyan3@gmail.com>

rkooo567 · 2024-11-07T14:07:11Z

premerge failures!

jjyao · 2024-11-07T17:03:08Z

Test failures are unrelated rllib tests

… Consistent (ray-project#48088) Signed-off-by: Mengjin Yan <mengjinyan3@gmail.com>

… Consistent (ray-project#48088) Signed-off-by: Mengjin Yan <mengjinyan3@gmail.com> Signed-off-by: mohitjain2504 <mohit.jain@dream11.com>

MengjinYan added 2 commits October 16, 2024 23:21

Fix placement group resource assignment

e62696c

Signed-off-by: Mengjin Yan <mengjinyan3@gmail.com>

Fix CI test build issue

b266809

Signed-off-by: Mengjin Yan <mengjinyan3@gmail.com>

MengjinYan assigned MengjinYan and rkooo567 and unassigned MengjinYan Oct 17, 2024

MengjinYan requested a review from rkooo567 October 18, 2024 16:18

MengjinYan added 4 commits October 21, 2024 09:08

Merge branch 'master' into issue-29811

02d7e68

Merge branch 'master' into issue-29811

20fc4e9

Merge branch 'master' into issue-29811

1bc549a

Merge branch 'master' into issue-29811

6b319ae

rkooo567 added the go add ONLY when ready to merge, run all tests label Oct 24, 2024

MengjinYan added 2 commits October 25, 2024 09:50

Fix the bug where the pg_id is ignored when allocating indexed resour…

e9ced9f

…ces in the case where the bundle id is not specified Signed-off-by: Mengjin Yan <mengjinyan3@gmail.com>

Merge branch 'master' into issue-29811

7b7837d

rkooo567 reviewed Oct 28, 2024

View reviewed changes

MengjinYan and others added 6 commits October 28, 2024 13:17

Update src/ray/common/scheduling/resource_instance_set.cc

795ea6c

Co-authored-by: SangBin Cho <rkooo567@gmail.com> Signed-off-by: Mengjin Yan <mengjinyan3@gmail.com>

Update src/ray/common/scheduling/resource_instance_set.cc

8bd2969

Co-authored-by: SangBin Cho <rkooo567@gmail.com> Signed-off-by: Mengjin Yan <mengjinyan3@gmail.com>

Merge branch 'master' into issue-29811

bc16850

Merge branch 'master' into issue-29811

92092d1

Merge branch 'master' into issue-29811

a7813ab

Fix comments

e0c0b15

Signed-off-by: Mengjin Yan <mengjinyan3@gmail.com>

jjyao reviewed Oct 31, 2024

View reviewed changes

MengjinYan and others added 4 commits October 31, 2024 17:23

Update src/ray/common/scheduling/resource_instance_set.h

4377f8b

Co-authored-by: Jiajun Yao <jeromeyjj@gmail.com> Signed-off-by: Mengjin Yan <mengjinyan3@gmail.com>

Fix comment

af50264

Signed-off-by: Mengjin Yan <mengjinyan3@gmail.com>

Fix test flakiness

444423a

Signed-off-by: Mengjin Yan <mengjinyan3@gmail.com>

Merge branch 'master' into issue-29811

b6571cc

rkooo567 approved these changes Nov 7, 2024

View reviewed changes

jjyao merged commit 8deca45 into master Nov 7, 2024
4 of 5 checks passed

jjyao deleted the issue-29811 branch November 7, 2024 17:10

rynewang mentioned this pull request Nov 12, 2024

[<Ray component: util>] PlacementGroup bundle index not effect when num_gpus<1 #48685

Closed

JP-sDEV pushed a commit to JP-sDEV/ray that referenced this pull request Nov 14, 2024

[Core] Make Placement Group Wildcard and Indexed Resource Assignments…

b998317

… Consistent (ray-project#48088) Signed-off-by: Mengjin Yan <mengjinyan3@gmail.com>

eelxpeng mentioned this pull request Nov 28, 2024

Why create_colocated_worker_cls and spawn volcengine/verl#29

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Core] Make Placement Group Wildcard and Indexed Resource Assignments Consistent #48088

[Core] Make Placement Group Wildcard and Indexed Resource Assignments Consistent #48088

MengjinYan commented Oct 17, 2024 •

edited

Loading

rkooo567 commented Oct 21, 2024

rkooo567 commented Oct 25, 2024

MengjinYan commented Oct 25, 2024

rkooo567 left a comment

rkooo567 Oct 24, 2024

MengjinYan Oct 29, 2024

rkooo567 Oct 24, 2024

MengjinYan Oct 29, 2024

rkooo567 Oct 24, 2024

MengjinYan Oct 28, 2024

jjyao Oct 31, 2024

MengjinYan Nov 1, 2024

rkooo567 Oct 24, 2024

rkooo567 Oct 24, 2024

MengjinYan Oct 29, 2024

rkooo567 Oct 28, 2024

MengjinYan Oct 28, 2024

rkooo567 Oct 28, 2024

rkooo567 Oct 28, 2024

MengjinYan Oct 29, 2024

rkooo567 Oct 28, 2024

MengjinYan Oct 29, 2024

MengjinYan commented Oct 30, 2024

MengjinYan commented Oct 30, 2024

jjyao Oct 31, 2024

jjyao Oct 31, 2024

jjyao Oct 31, 2024

rkooo567 commented Nov 7, 2024

jjyao commented Nov 7, 2024

-    absl::flat_hash_set<ResourceID> &resource_set =
+    pg_indexed_resources_.find(original_resource_id) != pg_indexed_resources_.end() {
+        it->second.erase(...)
+    }

[Core] Make Placement Group Wildcard and Indexed Resource Assignments Consistent #48088

[Core] Make Placement Group Wildcard and Indexed Resource Assignments Consistent #48088

Conversation

MengjinYan commented Oct 17, 2024 • edited Loading

Why are these changes needed?

Related issue number

Checks

rkooo567 commented Oct 21, 2024

rkooo567 commented Oct 25, 2024

MengjinYan commented Oct 25, 2024

rkooo567 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MengjinYan commented Oct 30, 2024

MengjinYan commented Oct 30, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rkooo567 commented Nov 7, 2024

jjyao commented Nov 7, 2024

MengjinYan commented Oct 17, 2024 •

edited

Loading