[State API] Add worker startup & initialization time to state API + use it for many_tasks #31916

rkooo567 · 2023-01-25T02:06:23Z

Why are these changes needed?

This PR adds information to retrieve

worker startup time
worker initialization time

from ray list workers --detail.

We also add summarization & print the result to many_tasks which will help debugging worker-related regressions.

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: SangBin Cho <rkooo567@gmail.com>

rkooo567 · 2023-01-25T02:06:53Z

src/ray/stats/metric_defs.h

@@ -202,12 +202,6 @@ static Gauge ObjectDirectoryRemovedLocations(
    "have been removed from this node.",
    "removals");

-/// Worker Pool
-static Histogram ProcessStartupTimeMs("process_startup_time_ms",


I removed it cuz I thought we don't really use it anyway. I am open to just keep it as well

rkooo567 · 2023-01-25T02:07:01Z

src/ray/protobuf/gcs.proto

+  // The field exists only when the worker is launched
+  // by a raylet. (I.e., driver worker won't have this value).
+  optional int64 worker_launch_time_ms = 25;
+  optional int64 worker_launched_time_ms = 26;


I will update the docstring

rickyyx

LGTM!
Should we make a perf metric out of it if this is being reliable and consistent?

python/ray/_private/workers/default_worker.py

scv119 · 2023-01-26T18:30:18Z

looks we did a lot of plumbing.. is it really necessary?

rkooo567 · 2023-01-26T22:30:34Z

Are you asking a feature is necessary or the plumbing is necessary?

Unfortunately plumbing is necessary to implement a feature

Signed-off-by: SangBin Cho <rkooo567@gmail.com>

scv119

comments

scv119 · 2023-01-26T18:29:44Z

src/ray/raylet/worker_pool.cc

@@ -318,6 +318,8 @@ WorkerPool::BuildProcessCommandArgs(const Language &language,
  if (language == Language::PYTHON) {
    worker_command_args.push_back("--startup-token=" +
                                  std::to_string(worker_startup_token_counter_));
+    worker_command_args.push_back("--worker-launch-time-ms=" +


hmm can we log into C++ metrics directly?

hmm we anyway need plumbing to calculate the initialization time although we use metrics
also metrics has cardinatliy issue + not practical at this point (until we have a default core dashboard, which we don't plan to have it for a while).

Having these values to worker state also makes sense given that's the direction we are going (add event information to state API)

scv119

talked offline. while i don't think this is necessary but ok to move forward as it is for now.

stale · 2023-03-11T12:48:52Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 14 days if no further activity occurs. Thank you for your contributions.

If you'd like to keep this open, just leave any comment, and the stale label will be removed.

Signed-off-by: SangBin Cho <rkooo567@gmail.com>

rickyyx · 2023-03-20T23:43:00Z

src/ray/protobuf/gcs.proto

-  // The time when this worker is launched.
+  // The time when this worker process is requested from raylet.
+  // The field exists only when the worker is launched
+  // by a raylet. (I.e., driver worker won't have this value).


why did we un-optional the fields?

I added // If the value doesn't present, it is -1.!

Signed-off-by: SangBin Cho <rkooo567@gmail.com>

rickyyx · 2023-03-23T16:55:30Z

python/ray/experimental/state/common.py

    #: The time when the worker is started and initialized.
+    #: 0 if the value doesn't exist.


should we also -1 default it?

rickyyx · 2023-03-23T16:57:03Z

should we have a release test dry run to see how this looks like?

rkooo567 · 2023-03-23T22:56:29Z

release test here https://buildkite.com/ray-project/release-tests-pr/builds/32132#01870e86-78ba-4ac8-b459-432d6105a00d

Signed-off-by: SangBin Cho <rkooo567@gmail.com>

rickyyx · 2023-03-24T17:47:31Z

Somehow not seeing th eoutputs there in the release test?

…se it for many_tasks (ray-project#31916) This PR adds information to retrieve 1. worker startup time 2. worker initialization time from `ray list workers --detail`. We also add summarization & print the result to many_tasks which will help debugging worker-related regressions. Signed-off-by: elliottower <elliot@elliottower.com>

…se it for many_tasks (ray-project#31916) This PR adds information to retrieve 1. worker startup time 2. worker initialization time from `ray list workers --detail`. We also add summarization & print the result to many_tasks which will help debugging worker-related regressions. Signed-off-by: Jack He <jackhe2345@gmail.com>

rkooo567 added 3 commits January 24, 2023 18:01

Add initialization & launch time statistics

b45ef61

Signed-off-by: SangBin Cho <rkooo567@gmail.com>

done

42082d9

Signed-off-by: SangBin Cho <rkooo567@gmail.com>

Handle failure case.

26535c9

Signed-off-by: SangBin Cho <rkooo567@gmail.com>

rkooo567 requested review from ericl, scv119, wuisawesome, AmeerHajAli, robertnishihara, pcmoritz, raulchen and fishbone as code owners January 25, 2023 02:06

rkooo567 assigned scv119 and rickyyx Jan 25, 2023

rkooo567 commented Jan 25, 2023

View reviewed changes

rickyyx approved these changes Jan 25, 2023

View reviewed changes

python/ray/_private/workers/default_worker.py Outdated Show resolved Hide resolved

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

rkooo567 added 2 commits January 27, 2023 03:15

Merge branch 'master' into worker-info-improvement

8009a7c

Fix tests.

e55c88c

Signed-off-by: SangBin Cho <rkooo567@gmail.com>

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

scv119 reviewed Jan 28, 2023

View reviewed changes

Merge branch 'master' into worker-info-improvement

3ef8a88

scv119 approved these changes Jan 31, 2023

View reviewed changes

Merge branch 'master' into worker-info-improvement

3492f46

rkooo567 added @author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer. do-not-merge Do not merge this PR! labels Feb 1, 2023

stale bot added the stale The issue is stale. It will be closed within 7 days unless there are further conversation label Mar 11, 2023

rkooo567 removed the stale The issue is stale. It will be closed within 7 days unless there are further conversation label Mar 15, 2023

rkooo567 added 2 commits March 20, 2023 16:34

Merge branch 'master' into worker-info-improvement

8e880ea

Signed-off-by: SangBin Cho <rkooo567@gmail.com>

summarize worker startup time

2e27b14

Signed-off-by: SangBin Cho <rkooo567@gmail.com>

rkooo567 requested a review from a team as a code owner March 20, 2023 23:35

rickyyx approved these changes Mar 20, 2023

View reviewed changes

rkooo567 added 3 commits March 22, 2023 00:55

Merge branch 'master' into worker-info-improvement

bde48e5

done

4615aab

Signed-off-by: SangBin Cho <rkooo567@gmail.com>

Merge branch 'master' into worker-info-improvement

20a2eb6

rickyyx reviewed Mar 23, 2023

View reviewed changes

rickyyx approved these changes Mar 23, 2023

View reviewed changes

rkooo567 added 2 commits March 24, 2023 00:44

Merge branch 'master' into worker-info-improvement

2654eb4

Fix a bug.

b334eac

Signed-off-by: SangBin Cho <rkooo567@gmail.com>

rkooo567 merged commit cd70485 into ray-project:master Mar 24, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[State API] Add worker startup & initialization time to state API + use it for many_tasks #31916

[State API] Add worker startup & initialization time to state API + use it for many_tasks #31916

rkooo567 commented Jan 25, 2023

rkooo567 Jan 25, 2023

rkooo567 Jan 25, 2023

rickyyx left a comment

scv119 commented Jan 26, 2023

rkooo567 commented Jan 26, 2023

scv119 left a comment

scv119 Jan 26, 2023

rkooo567 Jan 28, 2023

scv119 left a comment

stale bot commented Mar 11, 2023

rickyyx Mar 20, 2023

rkooo567 Mar 22, 2023

rickyyx Mar 23, 2023

rkooo567 Mar 23, 2023

rickyyx commented Mar 23, 2023 •

edited

Loading

rkooo567 commented Mar 23, 2023

rickyyx commented Mar 24, 2023

		#: The time when the worker is started and initialized.
		#: 0 if the value doesn't exist.

[State API] Add worker startup & initialization time to state API + use it for many_tasks #31916

[State API] Add worker startup & initialization time to state API + use it for many_tasks #31916

Conversation

rkooo567 commented Jan 25, 2023

Why are these changes needed?

Related issue number

Checks

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rickyyx left a comment

Choose a reason for hiding this comment

scv119 commented Jan 26, 2023

rkooo567 commented Jan 26, 2023

scv119 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scv119 left a comment

Choose a reason for hiding this comment

stale bot commented Mar 11, 2023

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

rickyyx commented Mar 23, 2023 • edited Loading

rkooo567 commented Mar 23, 2023

rickyyx commented Mar 24, 2023

rickyyx commented Mar 23, 2023 •

edited

Loading