[core] Update placement group retry implementation #18842

fishbone · 2021-09-23T06:35:45Z

Why are these changes needed?

Please check the ticket for more detail of this PR.

Previously, we always post a retry task even there is one in the queue because the waiting is implemented based on defer submission. There are several problems here:

retry accumulates in the pool
the retried one doesn't schedule the failed one in some cases
more complicated retry is not supported.

This PR fixed this issue:

Use priority queue for pending placement group
Don't use io context for retry
Exponential backoff for retry

Related issue number

Closes #18541

#18651

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/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 :(

fishbone · 2021-09-25T06:52:29Z

I'm working on unit testing.
e2e test is running.

ericl

Very nice!

src/ray/common/ray_config_def.h

src/ray/gcs/gcs_server/gcs_placement_group_manager.h

…ay-project#18706)" This reverts commit 2e99fb2.

python/ray/tune/tests/test_trial_runner_pg.py

python/ray/tune/utils/placement_groups.py

src/ray/gcs/gcs_server/gcs_placement_group_manager.cc

src/ray/gcs/gcs_server/test/gcs_placement_group_manager_test.cc

src/ray/gcs/test/gcs_test_util.h

src/ray/util/util.h

fishbone · 2021-10-01T19:18:05Z

new

success = 1
avg_pg_create_time_ms = 1.1399193753719132
avg_pg_remove_time_ms = 4.032320572071604
_runtime = 479.986270904541
_session_url = https://beta.anyscale.com/o/anyscale-internal/projects/prj_LFKjNprpiYt4AjAf1NdLDJwn/clusters/ses_yjTUA8gaZDSjF43Et5YJHfj5
_commit_url = https://ray-wheels.s3.us-west-2.amazonaws.com/test_wheels/pg-schedule/01b13b601bca5f168fbb367341f330db191c9e83/ray-2.0.0.dev0-cp37-cp37m-manylinux2014_x86_64.whl
_stable = True

fishbone · 2021-10-01T19:19:43Z

prod





success = 1
  | avg_pg_create_time_ms = 1.0101662852838733
  | avg_pg_remove_time_ms = 4.4615456231242465
  | _runtime = 461.87430810928345
  | _session_url = https://beta.anyscale.com/o/anyscale-internal/projects/prj_2xR6uT6t7jJuu1aCwWMsle/clusters/ses_K3pD2PKyVqkGDV4iD4XBdqrX
  | _commit_url = https://s3-us-west-2.amazonaws.com/ray-wheels/master/6dc1a6b72f687c8ed8bcb8e687d928afd442f5f5/ray-2.0.0.dev0-cp37-cp37m-manylinux2014_x86_64.whl
  | _stable = True
 ````

fishbone · 2021-10-02T00:35:18Z

@rkooo567 it's good for another look now

rkooo567

Last comments.

rkooo567 · 2021-10-04T04:24:30Z

BUILD.bazel

-#         "@com_google_googletest//:gtest_main",
-#     ],
-# )
+cc_test(


Is this relevant to this PR?

No. But if you ask, I can move it to another PR.

I think this is fine, but why is it here? Was it intentional? (I wondered if it has bad merge with the master or sth initially)

Yes. I think it's actually bad behavior to mix things not related to this PR here.

It's intentional... I feel shame about this... Let me remove this change and submit another PR for this.

src/ray/gcs/gcs_server/gcs_placement_group_manager.h

rkooo567 · 2021-10-04T04:25:41Z

src/ray/gcs/gcs_server/test/gcs_placement_group_manager_mock_test.cc

+  ASSERT_EQ(1, pending_queue.size());
+  ASSERT_EQ(rank, pending_queue.begin()->first);
+
+  absl::SleepFor(absl::Nanoseconds(rank - absl::GetCurrentTimeNanos()));


Can you at least use wait for ...? Based on my experience having a absolute time sleep usually was the source of flakniess

src/ray/gcs/gcs_server/test/gcs_placement_group_manager_mock_test.cc

rkooo567

LGTM. Feel free to merge if tests pass!

rkooo567 · 2021-10-04T05:42:31Z

src/ray/gcs/gcs_server/test/gcs_placement_group_manager_mock_test.cc

+  ASSERT_EQ(1, pending_queue.size());
+  ASSERT_EQ(rank, pending_queue.begin()->first);
+
+  absl::SleepFor(absl::Nanoseconds(rank - absl::GetCurrentTimeNanos()));


No need to run 1000 times. Since cpp tests are almost never flaky, it will
Be easy to detect if that happens. If you think this is fine, I can just merge and see how the result will look like

src/ray/gcs/gcs_server/test/gcs_placement_group_manager_mock_test.cc

fishbone · 2021-10-04T20:56:17Z

@rkooo567 just updated it. let me know if I missed anything

rkooo567 · 2021-10-04T22:56:30Z

Seems like this fails

fishbone · 2021-10-04T23:32:26Z

Seems like this fails
Change LT to LE because of lock resolutions is us and within 1us it finished the code.

fishbone · 2021-10-05T04:31:43Z

windows test of //python/ray/tests:test_client_reconnect is flaky

waleedkadous · 2021-10-05T19:09:04Z

src/ray/gcs/gcs_server/gcs_placement_group_manager.cc

+
+  if (!exp_backer) {
+    exp_backer = ExponentialBackOff(
+        1000000 *


What is 1000000? Why times 1000000 if it's ms?

fishbone added 4 commits September 23, 2021 04:02

exp backoff

febbc23

up

dc723c0

format

234cc45

up

0370575

rkooo567 self-assigned this Sep 23, 2021

fishbone added 8 commits September 24, 2021 03:11

up

1bcb800

up

65cb5a1

up

26e50a5

up

f82ccb1

format

88107ed

fix

c7de2e3

up

4328300

format

e6d4d52

fishbone changed the title ~~[wip] Pg schedule~~ [core] Update placement group retry implementation Sep 25, 2021

fishbone marked this pull request as ready for review September 25, 2021 06:51

fishbone requested a review from rkooo567 September 25, 2021 06:51

fishbone assigned ericl Sep 25, 2021

fishbone changed the title ~~[core] Update placement group retry implementation~~ [wip][core] Update placement group retry implementation Sep 25, 2021

ericl reviewed Sep 25, 2021

View reviewed changes

src/ray/common/ray_config_def.h Outdated Show resolved Hide resolved

src/ray/gcs/gcs_server/gcs_placement_group_manager.h Outdated Show resolved Hide resolved

ericl added the @author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer. label Sep 25, 2021

fishbone added 3 commits September 25, 2021 21:20

adjust ordering

b16bffb

up

9303b52

Revert "[tune] Cache unstaged placement groups for potential re-use (r…

93425fd

…ay-project#18706)" This reverts commit 2e99fb2.

ericl approved these changes Sep 26, 2021

View reviewed changes

up

8732091

rkooo567 reviewed Sep 27, 2021

View reviewed changes

fishbone added 3 commits September 28, 2021 01:12

update

aa32378

format

d606522

up

09d50fe

fishbone added 7 commits September 30, 2021 20:54

Merge remote-tracking branch 'upstream/master' into pg-schedule

7289fff

up

2cd8919

up

4b0521c

add test

e1262cb

format

df6346a

up

0992f23

up

01b13b6

fishbone added 4 commits October 1, 2021 20:16

Merge remote-tracking branch 'upstream/master' into pg-schedule

5b090b7

fix lint

e1856b4

format

c557a9c

fix

a1d359c

fishbone removed the @author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer. label Oct 2, 2021

rkooo567 approved these changes Oct 4, 2021

View reviewed changes

rkooo567 reviewed Oct 4, 2021

View reviewed changes

rkooo567 approved these changes Oct 4, 2021

View reviewed changes

fishbone added 2 commits October 4, 2021 20:54

Merge remote-tracking branch 'upstream/master' into pg-schedule

38abcd6

format

876d07c

fishbone added the @author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer. label Oct 4, 2021

fix

480c5da

up

40076bc

fishbone merged commit 056c3af into ray-project:master Oct 5, 2021

waleedkadous reviewed Oct 5, 2021

View reviewed changes

larrylian mentioned this pull request Nov 28, 2022

fix gcs_placement_group_test flaky #30577

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[core] Update placement group retry implementation #18842

[core] Update placement group retry implementation #18842

fishbone commented Sep 23, 2021 •

edited

Loading

fishbone commented Sep 25, 2021

ericl left a comment

fishbone commented Oct 1, 2021 •

edited

Loading

fishbone commented Oct 1, 2021

fishbone commented Oct 2, 2021

rkooo567 left a comment

rkooo567 Oct 4, 2021

fishbone Oct 4, 2021

rkooo567 Oct 4, 2021

fishbone Oct 4, 2021

rkooo567 Oct 4, 2021

rkooo567 left a comment

rkooo567 Oct 4, 2021

fishbone commented Oct 4, 2021

rkooo567 commented Oct 4, 2021

fishbone commented Oct 4, 2021 •

edited

Loading

fishbone commented Oct 5, 2021

waleedkadous Oct 5, 2021

[core] Update placement group retry implementation #18842

[core] Update placement group retry implementation #18842

Conversation

fishbone commented Sep 23, 2021 • edited Loading

Why are these changes needed?

Related issue number

Checks

fishbone commented Sep 25, 2021

ericl left a comment

Choose a reason for hiding this comment

fishbone commented Oct 1, 2021 • edited Loading

fishbone commented Oct 1, 2021

fishbone commented Oct 2, 2021

rkooo567 left a comment

Choose a reason for hiding this comment

rkooo567 Oct 4, 2021

Choose a reason for hiding this comment

fishbone Oct 4, 2021

Choose a reason for hiding this comment

rkooo567 Oct 4, 2021

Choose a reason for hiding this comment

fishbone Oct 4, 2021

Choose a reason for hiding this comment

rkooo567 Oct 4, 2021

Choose a reason for hiding this comment

rkooo567 left a comment

Choose a reason for hiding this comment

rkooo567 Oct 4, 2021

Choose a reason for hiding this comment

fishbone commented Oct 4, 2021

rkooo567 commented Oct 4, 2021

fishbone commented Oct 4, 2021 • edited Loading

fishbone commented Oct 5, 2021

waleedkadous Oct 5, 2021

Choose a reason for hiding this comment

fishbone commented Sep 23, 2021 •

edited

Loading

fishbone commented Oct 1, 2021 •

edited

Loading

fishbone commented Oct 4, 2021 •

edited

Loading