[Core][Enable gcs scheduler 5/n] Adapt gcs scheduler with external modules #28162

Chong-Li · 2022-08-30T08:37:33Z

Why are these changes needed?

This the second split PR of #25075, which tried to enable gcs scheduler by default.

This split PR mainly includes:

In GcsResourceManager::HandleGetAllResourceUsage(), we export gcs' pending task info without adding an extra entry to the batch list. So as usual, the batch list still only contains the worker nodes (some tests depend on this).
To pass tests like test_actor_groups, rpc GetAllNodeInfo has to return gcs' pending task info additionally (do we need a dedicated rpc for that?).

Related issue number

#25075, #27084

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 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: Chong-Li <[email protected]>

rkooo567 · 2022-09-01T17:14:36Z

cc @wuisawesome for the autoscaler changes

rkooo567 · 2022-09-01T17:15:26Z

dashboard/datacenter.py

        infeasible_tasks = sum(
            (
                list(node_stats.get("infeasibleTasks", []))
                for node_stats in DataSource.node_stats.values()
            ),
            [],
        )
+        # Collect infeasible tasks in gcs.


This is only for actors right?

Yes. I'll emphasize this in that in-line comment.

src/ray/gcs/gcs_server/gcs_node_manager.h

src/ray/protobuf/gcs_service.proto

rkooo567 · 2022-09-01T17:18:24Z

src/ray/protobuf/gcs_service.proto

+}
+
+message GcsStats {
+  repeated TaskSpec infeasible_tasks = 1;


can you only report necessary fields?

If you take a look at test_actor_groups in test_actor.py, these fields are necessary when gcs scheduling is enabled.

src/ray/protobuf/gcs_service.proto

rkooo567 · 2022-09-01T17:24:50Z

src/ray/gcs/gcs_server/gcs_resource_manager.cc

@@ -93,15 +93,16 @@ void GcsResourceManager::HandleGetAllAvailableResources(
      if (using_resource_reports) {
        auto resource_iter =
            node_resource_usages_[node_id].resources_available().find(resource_name);
-        if (resource_iter != node_resource_usages_[node_id].resources_available().end()) {
+        if (resource_iter != node_resource_usages_[node_id].resources_available().end() &&
+            resource_iter->second > 0) {


If I look at the else statement, it seems like we update resources when the value > 0. Is it expected?

Every resource in node_resources.available has value greater than 0, because ResourceRequest automatically erases any resource with zero value. So while iterating over node_resources.available, if using_resource_reports == true, we have to make sure no resource with zero value inserted. I believe there is a test requiring this behavior, but can not remember which one for now.

If this behavior is important and we don't expect to report any 0 values, should we assert this instead?

This resource_iter->second > 0 seems not needed. I'll just revert this. @wuisawesome

rkooo567 · 2022-09-01T17:28:49Z

src/ray/gcs/gcs_server/gcs_resource_manager.cc

-          << "[UpdateFromResourceReport]: received resource usage from unknown node id "
-          << node_id;
+  // Only need to update worker nodes' resource usage.
+  if (node_id != local_node_id_) {


if (node_id == local_node_id_) { return; } if (RayConfig::instance().gcs_actor_scheduling_enabled()) { UpdateNodeNormalTaskResources(node_id, data); } else { if (!cluster_resource_manager_.UpdateNodeAvailableResourcesIfExist( scheduling::NodeID(node_id.Binary()), data)) { RAY_LOG(INFO) << "[UpdateFromResourceReport]: received resource usage from unknown node id " << node_id; } } UpdateNodeResourceUsage(node_id, data);

// Only need to update worker nodes' resource usage.
if (node_id != local_node_id_) {

Hmm I don't understand this part. Why do we only need to update worker nodes' resource usage?

We don't run any normal tasks or actors in gcs node (server). Even with gcs scheduler enabled, gcs server only queues and schedules tasks, without allocating any local resources to them. In terms of scheduling, gcs node actually has zero total resources, so we don't need to update it here.

src/ray/gcs/gcs_server/gcs_resource_manager.cc

Signed-off-by: Chong-Li <[email protected]>

src/ray/protobuf/gcs_service.proto

fishbone

@Chong-Li Thanks for pushing the last mile effort.

I'm not sure, but I think our high-level direction is to make sure scheduler are decoupled from Raylet and GCS, which means, we really shouldn't have things treated specialized. There should be no GCS scheduler, and only things like scheduler. I feel you are not doing things in this direction and things are treated differently for different component. If it's hard, we should justify that and maybe we can do that later.

Chong-Li · 2022-09-06T04:06:22Z

@Chong-Li Thanks for pushing the last mile effort.

I'm not sure, but I think our high-level direction is to make sure scheduler are decoupled from Raylet and GCS, which means, we really shouldn't have things treated specialized. There should be no GCS scheduler, and only things like scheduler. I feel you are not doing things in this direction and things are treated differently for different component. If it's hard, we should justify that and maybe we can do that later.

@iycheng I think the primary goal is making gcs scheduler pass all tests without breaking any existing user behavior. In terms of this PR, we're trying to make external modules and public APIs work the same way no matter which scheduler is enabled. After these functionality requirements are achieved, we could try to reduce the difference between gcs and raylet schedulers (using the lowest amount of gcs scheduling feature flag) in a more organized way (I'll definitely do that).

Of course, I totally agree that we should not add up too much scheduler difference throughout this plan. So about the implementation details of this PR, maybe we should not put many gcs-based actor scheduler only in-line comments (or if..else.. statement on gcs scheduling feature flag) there? @rkooo567 .

Signed-off-by: Chong-Li <[email protected]>

…d_5/n

Signed-off-by: Chong-Li <[email protected]>

fishbone · 2022-09-08T04:32:59Z

@Chong-Li it makes sense. I'm Ok with make it work first! It's a really useful features and can make the actor scheduling faster for some cases and we should just open it by default.

rkooo567 · 2022-09-08T17:21:58Z

yeah let's make it work first and focus on unification! It'd be great if @wuisawesome can review this PR... let me ping him again.

wuisawesome · 2022-09-08T18:33:16Z

src/ray/gcs/gcs_server/gcs_resource_manager.cc

@@ -93,15 +93,16 @@ void GcsResourceManager::HandleGetAllAvailableResources(
      if (using_resource_reports) {
        auto resource_iter =
            node_resource_usages_[node_id].resources_available().find(resource_name);
-        if (resource_iter != node_resource_usages_[node_id].resources_available().end()) {
+        if (resource_iter != node_resource_usages_[node_id].resources_available().end() &&
+            resource_iter->second > 0) {


If this behavior is important and we don't expect to report any 0 values, should we assert this instead?

This resource_iter->second > 0 seems not needed. I'll just revert this. @wuisawesome

wuisawesome · 2022-09-08T18:34:01Z

python/ray/autoscaler/_private/monitor.py

@@ -253,6 +253,12 @@ def update_load_metrics(self):

        mirror_node_types = {}
        cluster_full = False
+        if (


This should go away if you rebase right?

Because there might be pending actors in gcs server (if enabling gcs actor scheduler), we need to not only check if any worker node has detected cluster full (see line 276-281), but also check gcs server's report (this part).

wuisawesome · 2022-09-08T18:38:39Z

src/ray/gcs/gcs_server/gcs_resource_manager.cc

@@ -111,6 +112,12 @@ void GcsResourceManager::HandleGetAllAvailableResources(

 void GcsResourceManager::UpdateFromResourceReport(const rpc::ResourcesData &data) {
  NodeID node_id = NodeID::FromBinary(data.node_id());
+  // We only need to update worker nodes' resource usage. Gcs node ifself does not


You should be able to assert this right? GCS shouldn't ever send out a resource report right?

This part is actually from another feature, which I'll do in the next split PR. So I'll just revert this here.

…d_5/n

Signed-off-by: Chong-Li <[email protected]>

scv119 · 2022-09-09T16:23:21Z

src/ray/protobuf/gcs_service.proto

+  /// True if gcs finds infeasible or pending actor creation tasks
+  /// locally (when gcs actor scheduler is enabled). This field is
+  /// expected to help triggering auto-scaling.
+  bool cluster_full_of_actors_detected_by_gcs = 3;


hmm, is there any reason we can't reuse the ResourceUsageBatchData?

ResourceUsageBatchData is a gcs proto, which is also used for table storage and pubsub (although the pub-sub one might be removed later.) So adding a cluster_full_of_actors_detected_by_gcs in ResourceUsageBatchData might increase unnecessary overhead in most cases.

src/ray/protobuf/gcs_service.proto

Signed-off-by: Chong-Li <[email protected]>

…d_5/n

scv119 · 2022-09-16T21:38:16Z

Thanks for the contribution!

…dules (ray-project#28162) This the second split PR of ray-project#25075, which tried to enable gcs scheduler by default. This split PR mainly includes: In GcsResourceManager::HandleGetAllResourceUsage(), we export gcs' pending task info without adding an extra entry to the batch list. So as usual, the batch list still only contains the worker nodes (some tests depend on this). To pass tests like test_actor_groups, rpc GetAllNodeInfo has to return gcs' pending task info additionally (do we need a dedicated rpc for that?). Signed-off-by: PaulFenton <[email protected]>

Chong-Li requested review from wuisawesome, DmitriGekhtman, ericl, AmeerHajAli, robertnishihara, pcmoritz, raulchen, fishbone and scv119 as code owners August 30, 2022 08:37

Chong-Li requested a review from wumuzi520 August 30, 2022 08:38

Adapt gcs scheduler

707bd61

Signed-off-by: Chong-Li <[email protected]>

Chong-Li force-pushed the refactor_gcs_sched_5/n branch from b0af7be to 707bd61 Compare August 30, 2022 08:39

Chong-Li changed the title ~~[Core][Enable gcs scheduler 5/n] Adapt gcs scheduler with peripheral modules~~ [Core][Enable gcs scheduler 5/n] Adapt gcs scheduler with external modules Sep 1, 2022

rkooo567 assigned scv119, wuisawesome and rkooo567 Sep 1, 2022

rkooo567 assigned fishbone Sep 1, 2022

rkooo567 reviewed Sep 1, 2022

View reviewed changes

Chong-Li added 2 commits September 5, 2022 18:10

Some fix

ba9f452

Signed-off-by: Chong-Li <[email protected]>

Add in-line comments

c55a7a1

Signed-off-by: Chong-Li <[email protected]>

fishbone reviewed Sep 6, 2022

View reviewed changes

src/ray/protobuf/gcs_service.proto Outdated Show resolved Hide resolved

fishbone reviewed Sep 6, 2022

View reviewed changes

Chong-Li added 3 commits September 7, 2022 15:32

Use a seperate RPC to get gcs stats

da88560

Signed-off-by: Chong-Li <[email protected]>

Merge remote-tracking branch 'upstream/master' into refactor_gcs_sche…

523a932

…d_5/n

Remove unnecessarities.

fca95d2

Signed-off-by: Chong-Li <[email protected]>

Chong-Li requested review from rkooo567 and fishbone September 7, 2022 07:58

wuisawesome reviewed Sep 8, 2022

View reviewed changes

Chong-Li added 3 commits September 9, 2022 12:27

Merge remote-tracking branch 'upstream/master' into refactor_gcs_sche…

f605805

…d_5/n

Fix comment

db89ccb

Signed-off-by: Chong-Li <[email protected]>

Add comment

dbfe7e0

Signed-off-by: Chong-Li <[email protected]>

Chong-Li requested a review from wuisawesome September 9, 2022 05:06

scv119 reviewed Sep 9, 2022

View reviewed changes

Fix

b73a86b

Signed-off-by: Chong-Li <[email protected]>

Chong-Li requested a review from scv119 September 13, 2022 09:39

Format

e62e88c

Signed-off-by: Chong-Li <[email protected]>

wuisawesome approved these changes Sep 14, 2022

View reviewed changes

rkooo567 approved these changes Sep 14, 2022

View reviewed changes

Chong-Li added 2 commits September 15, 2022 00:35

Merge remote-tracking branch 'upstream/master' into refactor_gcs_sche…

1b72546

…d_5/n

Merge remote-tracking branch 'upstream/master' into refactor_gcs_sche…

afb8997

…d_5/n

scv119 merged commit 9b8fbe1 into ray-project:master Sep 16, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Core][Enable gcs scheduler 5/n] Adapt gcs scheduler with external modules #28162

[Core][Enable gcs scheduler 5/n] Adapt gcs scheduler with external modules #28162

Chong-Li commented Aug 30, 2022

rkooo567 commented Sep 1, 2022

rkooo567 Sep 1, 2022

Chong-Li Sep 4, 2022

rkooo567 Sep 1, 2022

Chong-Li Sep 5, 2022

rkooo567 Sep 1, 2022

Chong-Li Sep 5, 2022

wuisawesome Sep 8, 2022 •

edited by Chong-Li

Loading

rkooo567 Sep 1, 2022

rkooo567 Sep 1, 2022

Chong-Li Sep 5, 2022

fishbone left a comment

Chong-Li commented Sep 6, 2022 •

edited

Loading

fishbone commented Sep 8, 2022

rkooo567 commented Sep 8, 2022

wuisawesome Sep 8, 2022 •

edited by Chong-Li

Loading

wuisawesome Sep 8, 2022

Chong-Li Sep 9, 2022 •

edited

Loading

wuisawesome Sep 8, 2022

Chong-Li Sep 9, 2022 •

edited

Loading

scv119 Sep 9, 2022

Chong-Li Sep 13, 2022

scv119 commented Sep 16, 2022

[Core][Enable gcs scheduler 5/n] Adapt gcs scheduler with external modules #28162

[Core][Enable gcs scheduler 5/n] Adapt gcs scheduler with external modules #28162

Conversation

Chong-Li commented Aug 30, 2022

Why are these changes needed?

Related issue number

Checks

rkooo567 commented Sep 1, 2022

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

wuisawesome Sep 8, 2022 • edited by Chong-Li Loading

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

fishbone left a comment

Choose a reason for hiding this comment

Chong-Li commented Sep 6, 2022 • edited Loading

fishbone commented Sep 8, 2022

rkooo567 commented Sep 8, 2022

wuisawesome Sep 8, 2022 • edited by Chong-Li Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Chong-Li Sep 9, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Chong-Li Sep 9, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scv119 commented Sep 16, 2022

wuisawesome Sep 8, 2022 •

edited by Chong-Li

Loading

Chong-Li commented Sep 6, 2022 •

edited

Loading

wuisawesome Sep 8, 2022 •

edited by Chong-Li

Loading

Chong-Li Sep 9, 2022 •

edited

Loading

Chong-Li Sep 9, 2022 •

edited

Loading