[Core][Enable gcs scheduler 7/n] Prefer actor owner's node #30789

Chong-Li · 2022-11-30T14:24:41Z

Signed-off-by: Chong-Li lc300133@antgroup.com

Why are these changes needed?

This PR tries to finalize gcs actor scheduler, with the following changes:

Similar to the legacy scheduler, gcs now prefers the actor owner's node. It's usually required by RL cases for better colocation.
The normal task workers report zero CPU resources (instead of the allocated one) when they are blocked.
Similar to the legacy scheduler, gcs now schedules empty-resource actors randomly.
A new release test is added: multiple masters/drivers are creating (slave) actors concurrently. The case is able to expose the difference between centralized (gcs-based, less scheduling conflicts) and distributed schedulers.

The feature flag is temporarily turned on when going through the CI pipline. We still need another dedicated PR to turn it on by default.

Related issue number

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 <lc300133@antgroup.com>

Chong-Li · 2022-12-02T04:11:21Z

The RLLib release test results with raylet-based scheduler: https://buildkite.com/ray-project/release-tests-pr/builds/22539#_

With gcs-based-scheduler: https://buildkite.com/ray-project/release-tests-pr/builds/22674#_

I did not find notable performance regression. But the variations may need @rkooo567 to double check. cc. @scv119 @iycheng @wumuzi520

wumuzi520 · 2022-12-06T14:38:53Z

python/ray/_private/ray_constants.py

@@ -359,7 +359,7 @@ def env_bool(key, default):


 def gcs_actor_scheduling_enabled():
-    return os.environ.get("RAY_gcs_actor_scheduling_enabled") == "true"
+    return os.environ.get("RAY_gcs_actor_scheduling_enabled") != "false"


Does this mean that gcs actor scheduling is enabled by default?

wumuzi520 · 2022-12-06T14:39:36Z

src/ray/common/ray_config_def.h

@@ -602,7 +602,7 @@ RAY_CONFIG(uint64_t, subscriber_timeout_ms, 300 * 1000)
 RAY_CONFIG(uint64_t, gcs_actor_table_min_duration_ms, /*  5 min */ 60 * 1000 * 5)

 /// Whether to enable GCS-based actor scheduling.
-RAY_CONFIG(bool, gcs_actor_scheduling_enabled, false);
+RAY_CONFIG(bool, gcs_actor_scheduling_enabled, true);


Does this mean that gcs actor scheduling is enabled by default?

It is only turned on to go through CI pipeline. Will turn it off after code review.

wumuzi520 · 2022-12-06T14:48:09Z

src/ray/gcs/gcs_server/gcs_actor_scheduler.cc

@@ -49,7 +49,8 @@ GcsActorScheduler::GcsActorScheduler(
 void GcsActorScheduler::Schedule(std::shared_ptr<GcsActor> actor) {
  RAY_CHECK(actor->GetNodeID().IsNil() && actor->GetWorkerID().IsNil());

-  if (RayConfig::instance().gcs_actor_scheduling_enabled()) {
+  if (RayConfig::instance().gcs_actor_scheduling_enabled() &&
+      !actor->GetCreationTaskSpecification().GetRequiredResources().IsEmpty()) {


This condition is a little weird here.

This is just a quick way to make the empty-resource actors go through the legacy logic (randomly select a forward raylet).

wumuzi520

It looks good overall.
BTW, don't forget to change the switch RAY_gcs_actor_scheduling_enabled back.

Chong-Li · 2022-12-11T08:18:43Z

Is there any more suggestions to this PR? @iycheng @rkooo567

rkooo567

Request change until we revert the gcs actor scheduling enabled by default

I have mainly two questions.

Is there any way to merge "prioritize_local_node "and "preferred_node_id"? I feel like they are the same thing.
Some questions regarding the nightly tests (follow up from comments)

Also I am waiting for the review from @scv119

python/ray/_private/ray_constants.py

rkooo567 · 2022-12-13T15:21:24Z

release/nightly_tests/many_nodes_tests/multi_master_test.py

+
+
+def test_max_actors_launch(cpus_per_actor, total_actors, num_masters):
+    # By default, there are 50 groups, each group has 1 master and 99 slaves.


have you run this test? (do you need help from us running this? )

Please see this slack post (https://ray-distributed.slack.com/archives/C0334JQHG86/p1669131998615139) for the results. Generally, gcs scheduler shows ~2.5X better latency compared to raylet scheduler.

rkooo567 · 2022-12-13T15:21:45Z

release/nightly_tests/many_nodes_tests/multi_master_test.py

+    actor_ready_end = perf_counter()
+    actor_ready_time = actor_ready_end - actor_ready_start
+
+    print(f"Actor launch time: {actor_launch_time} ({args.total_actors} actors)")


can you tell me a bit more about the success / failure criteria of this test?

This test is similar to the existing one: https://github.com/ray-project/ray/blob/master/release/nightly_tests/many_nodes_tests/actor_test.py. They both focus on the scheduling latency of a large number of actors.

The only difference from the actor_test.py is that this test simulates a multi-master(driver) scenario. This means (distributed) master actors are creating slave actors concurrently. Please see this discussion (https://ray-distributed.slack.com/archives/C0334JQHG86/p1667468940596309) for details.

rkooo567 · 2022-12-13T15:22:02Z

release/release_tests.yaml

+    timeout: 7200
+    script: python many_nodes_tests/multi_master_test.py
+    wait_for_nodes:
+      num_nodes: 251


can we test this in smaller # of nodes?

This config is actually the same as the one of actor_test.py. They should have the same scale to make an apple-to-apple comparison.

this is pretty expensive. Can you comment it out for now? We can re-enable it once we enable GCS actor scheduler by default. (Let's just use it for benchmarking purpose for now.)

rkooo567 · 2022-12-13T15:22:26Z

src/ray/common/ray_config_def.h

@@ -602,7 +602,7 @@ RAY_CONFIG(uint64_t, subscriber_timeout_ms, 300 * 1000)
 RAY_CONFIG(uint64_t, gcs_actor_table_min_duration_ms, /*  5 min */ 60 * 1000 * 5)

 /// Whether to enable GCS-based actor scheduling.
-RAY_CONFIG(bool, gcs_actor_scheduling_enabled, false);
+RAY_CONFIG(bool, gcs_actor_scheduling_enabled, true);


I will request for changes until this change is reverted

Sure. I was meaning to revert it after code review. But could do it right now.

Signed-off-by: Chong-Li <lc300133@antgroup.com>

…d_7/n

Signed-off-by: Chong-Li <lc300133@antgroup.com>

…d_7/n

Chong-Li · 2023-01-03T12:24:08Z

Request change until we revert the gcs actor scheduling enabled by default

I have mainly two questions.

Is there any way to merge "prioritize_local_node "and "preferred_node_id"? I feel like they are the same thing.

Some questions regarding the nightly tests (follow up from comments)

Also I am waiting for the review from @scv119

These two questions have been resolved. Are there any more comments? @rkooo567 @scv119

rkooo567

Looks pretty good! Last couple comments.!

rkooo567 · 2023-01-12T14:26:22Z

release/release_tests.yaml

+    timeout: 7200
+    script: python many_nodes_tests/multi_master_test.py
+    wait_for_nodes:
+      num_nodes: 251


this is pretty expensive. Can you comment it out for now? We can re-enable it once we enable GCS actor scheduler by default. (Let's just use it for benchmarking purpose for now.)

src/ray/gcs/gcs_server/gcs_actor_scheduler.cc

src/ray/raylet/local_task_manager.cc

src/ray/raylet/scheduling/cluster_resource_scheduler.cc

src/ray/common/task/task.h

rkooo567 · 2023-01-12T14:30:01Z

src/ray/raylet/scheduling/cluster_task_manager.cc

@@ -191,7 +192,8 @@ void ClusterTaskManager::TryScheduleInfeasibleTask() {
    bool is_infeasible;
    cluster_resource_scheduler_->GetBestSchedulableNode(
        task.GetTaskSpecification(),
-        work->PrioritizeLocalNode(),
+        /*preferred_node_id*/ work->PrioritizeLocalNode() ? self_node_id_.Binary()
+                                                          : task.GetPreferredNodeID(),


How does it work if there's no preferred node id?

Empty preferred node_id is handled specially (if empty, fallback to local node) inside HybridSchedulingPolicy.

Is it possible to just pass nullopt instead? It makes sense, but I think it is pretty confusing from code to understand

Right now, preferred_node_id is just a std::string in ClusterResourceScheduler, RayTask, SchedulingOptions and HybridSchedulingPolicy (empty string by default). Do you mean changing it to std::optional<std::string> globally?

As suggested, more comments about the empty string have been added. @rkooo567

src/ray/raylet/scheduling/policy/hybrid_scheduling_policy.cc

src/ray/raylet/scheduling/cluster_resource_scheduler_test.cc

Signed-off-by: Chong-Li <lc300133@antgroup.com>

…d_7/n

rkooo567

There are test failures

rkooo567 · 2023-01-19T23:31:41Z

I started a mac build. Please wait a little!

Chong-Li · 2023-01-23T08:38:02Z

I started a mac build. Please wait a little!

All tests passed. @rkooo567

rkooo567 · 2023-01-23T08:39:13Z

cc @iycheng

Prefer actor owner's node

30b0199

Signed-off-by: Chong-Li <lc300133@antgroup.com>

Chong-Li marked this pull request as draft November 30, 2022 14:24

Chong-Li added 2 commits November 30, 2022 23:00

Fix

5fe48e6

Signed-off-by: Chong-Li <lc300133@antgroup.com>

Fix

1bf9eb1

Signed-off-by: Chong-Li <lc300133@antgroup.com>

Chong-Li marked this pull request as ready for review December 2, 2022 04:05

Chong-Li requested review from scv119, fishbone, wumuzi520 and rkooo567 December 2, 2022 04:05

scv119 assigned rkooo567 and fishbone Dec 2, 2022

wumuzi520 reviewed Dec 6, 2022

View reviewed changes

wumuzi520 approved these changes Dec 6, 2022

View reviewed changes

rkooo567 requested changes Dec 13, 2022

View reviewed changes

Chong-Li added 2 commits December 14, 2022 11:46

Fix

abebc82

Signed-off-by: Chong-Li <lc300133@antgroup.com>

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

cbaf785

…d_7/n

Chong-Li force-pushed the refactor_gcs_sched_7/n branch from 41b3307 to 3e4ed6c Compare January 3, 2023 07:08

Merge preferred_node_id and prioritize_local_node

c2308af

Signed-off-by: Chong-Li <lc300133@antgroup.com>

Chong-Li force-pushed the refactor_gcs_sched_7/n branch from 3e4ed6c to c2308af Compare January 3, 2023 09:47

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

354cc60

…d_7/n

Chong-Li requested a review from rkooo567 January 3, 2023 12:24

rkooo567 reviewed Jan 12, 2023

View reviewed changes

rkooo567 added the @author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer. label Jan 12, 2023

Fix

8dc68ba

Signed-off-by: Chong-Li <lc300133@antgroup.com>

Chong-Li requested a review from rkooo567 January 16, 2023 13:53

Chong-Li added 2 commits January 19, 2023 18:17

More comments about empty string

8b42002

Signed-off-by: Chong-Li <lc300133@antgroup.com>

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

a1e1070

…d_7/n

rkooo567 approved these changes Jan 19, 2023

View reviewed changes

Chong-Li requested a review from rkooo567 January 23, 2023 08:38

rkooo567 merged commit ec3243d into ray-project:master Jan 23, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Core][Enable gcs scheduler 7/n] Prefer actor owner's node #30789

[Core][Enable gcs scheduler 7/n] Prefer actor owner's node #30789

Chong-Li commented Nov 30, 2022 •

edited

Loading

Chong-Li commented Dec 2, 2022 •

edited

Loading

wumuzi520 Dec 6, 2022

wumuzi520 Dec 6, 2022

Chong-Li Dec 7, 2022

wumuzi520 Dec 6, 2022

Chong-Li Dec 11, 2022

wumuzi520 left a comment •

edited

Loading

Chong-Li commented Dec 11, 2022

rkooo567 left a comment •

edited

Loading

rkooo567 Dec 13, 2022

Chong-Li Dec 14, 2022

rkooo567 Dec 13, 2022

Chong-Li Dec 14, 2022

rkooo567 Dec 13, 2022

Chong-Li Dec 14, 2022

rkooo567 Jan 12, 2023

rkooo567 Dec 13, 2022

Chong-Li Dec 14, 2022

Chong-Li commented Jan 3, 2023

rkooo567 left a comment

rkooo567 Jan 12, 2023

rkooo567 Jan 12, 2023

Chong-Li Jan 16, 2023

rkooo567 Jan 17, 2023

Chong-Li Jan 19, 2023 •

edited

Loading

Chong-Li Jan 19, 2023

rkooo567 left a comment

rkooo567 commented Jan 19, 2023

Chong-Li commented Jan 23, 2023

rkooo567 commented Jan 23, 2023



		def test_max_actors_launch(cpus_per_actor, total_actors, num_masters):
		# By default, there are 50 groups, each group has 1 master and 99 slaves.

[Core][Enable gcs scheduler 7/n] Prefer actor owner's node #30789

[Core][Enable gcs scheduler 7/n] Prefer actor owner's node #30789

Conversation

Chong-Li commented Nov 30, 2022 • edited Loading

Why are these changes needed?

Related issue number

Checks

Chong-Li commented Dec 2, 2022 • edited 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

Choose a reason for hiding this comment

wumuzi520 left a comment • edited Loading

Choose a reason for hiding this comment

Chong-Li commented Dec 11, 2022

rkooo567 left a comment • edited 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

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

Chong-Li commented Jan 3, 2023

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

Chong-Li Jan 19, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rkooo567 left a comment

Choose a reason for hiding this comment

rkooo567 commented Jan 19, 2023

Chong-Li commented Jan 23, 2023

rkooo567 commented Jan 23, 2023

Chong-Li commented Nov 30, 2022 •

edited

Loading

Chong-Li commented Dec 2, 2022 •

edited

Loading

wumuzi520 left a comment •

edited

Loading

rkooo567 left a comment •

edited

Loading

Chong-Li Jan 19, 2023 •

edited

Loading