[core][state] Task events backend - gcs update path and interface [2/n] #30979

rickyyx · 2022-12-09T00:02:56Z

Why are these changes needed?

Previous PRs:

[core][state] Task events backend: config and interface definitions [0/n] #30829: Interface and protobuf definitions.
[core][state] Task events backend - split drop count on worker #30953: Splitting of drop count for various events type on worker.
[core][state] Task events backend - worker task event buffer implementation [1/n] #30867: TaskEventBuffer implementation

In this PR:

Added GcsTaskManager that stores the task events on the GCS side.
The GcsTsakManager has its own io service and io thread that's separated from the main rpc thread/io_context. Handling of rpcs will be posted to its own internal io_service.
Implementation for the update path.
Interface for the read path.

Next PRs:

Implementation for the update path of GcsTaskManager
Porting of profiling events
Porting of state api task.

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

…-backend-gcs-update

Signed-off-by: rickyyx <[email protected]>

rkooo567

Let me have additional look later.

src/ray/gcs/gcs_server/gcs_task_manager.h

rkooo567 · 2022-12-09T16:27:48Z

src/ray/gcs/gcs_server/gcs_task_manager.h

+  /// Returns the io_service.
+  ///
+  /// \return Reference to its io_service.
+  instrumented_io_context &GetIoContext() { return io_service_; }


const instrumented_io_context &GetIoContext() const { return io_service_; }

hmm - this needs to be an non-const reference since the caller takes a non-const reference.

src/ray/gcs/gcs_server/gcs_task_manager.h

src/ray/gcs/gcs_server/gcs_task_manager.cc

rkooo567 · 2022-12-09T16:39:03Z

src/ray/gcs/gcs_server/gcs_task_manager.cc

+  }
+}
+
+absl::optional<rpc::TaskEvents>


Consider returning bool? I think the dropped counter can be tracked within storage?

I feel the drop count could actually be tracked outside of the storage, such that the storage could be better decoupled w/o knowing the details of the events (be it status updates or profile events). But open to move the tracking in.

src/ray/gcs/gcs_server/gcs_task_manager.cc

ericl

Nice test! Consider adding (1) DebugString() to expose in debug state, and also (2) prometheus gauges for the count/bytes added / currently stored in the task stats storage.

src/ray/gcs/gcs_server/gcs_task_manager.h

Signed-off-by: rickyyx <[email protected]>

rickyyx · 2022-12-13T23:53:08Z

Addressed most of the comments:

Moved debug states printing to DebugString
Added metric reporting for various counters
Added e2e integration test for metric export
Removed unnecessary post on io_context
Removed Gcs read rpc definitions for next PRs.

PTAL @rkooo567 @ericl

ericl

Looks good, but consider splitting into separate metrics for reported, stored, dropped.

python/ray/tests/test_task_events.py

Signed-off-by: rickyyx <[email protected]>

…-backend-gcs-update

…n] (ray-project#30979) Previous PRs: [core][state] Task events backend: config and interface definitions [0/n] ray-project#30829: Interface and protobuf definitions. [core][state] Task events backend - split drop count on worker ray-project#30953: Splitting of drop count for various events type on worker. [core][state] Task events backend - worker task event buffer implementation [1/n] ray-project#30867: TaskEventBuffer implementation In this PR: Added GcsTaskManager that stores the task events on the GCS side. The GcsTsakManager has its own io service and io thread that's separated from the main rpc thread/io_context. Handling of rpcs will be posted to its own internal io_service. Implementation for the update path. Interface for the read path. Next PRs: Implementation for the update path of GcsTaskManager Porting of profiling events Porting of state api task. Signed-off-by: Weichen Xu <[email protected]>

**Previous PRs:** - #30829: - #30953: - #30867: - #30979: - #30934 - #31207 **In This PR:** - Remove old code for timeline/profiling backend.

**Previous PRs:** - #30829: - #30953: - #30867: - #30979: - #30934 - #31207 **This PR:** With the change, - `list_tasks` now will return tasks with attempt number as an additional column. - `get_task` might return multiple task attempt entries if there are retries. There is also some plumbing in the test and in core (esp in the test logic) given the changes. Major changes in the PR are: - Add limit support to `GcsTaskManager` - Change the state aggregator to get tasks from GCS.

…n] (#30979) Previous PRs: [core][state] Task events backend: config and interface definitions [0/n] #30829: Interface and protobuf definitions. [core][state] Task events backend - split drop count on worker #30953: Splitting of drop count for various events type on worker. [core][state] Task events backend - worker task event buffer implementation [1/n] #30867: TaskEventBuffer implementation In this PR: Added GcsTaskManager that stores the task events on the GCS side. The GcsTsakManager has its own io service and io thread that's separated from the main rpc thread/io_context. Handling of rpcs will be posted to its own internal io_service. Implementation for the update path. Interface for the read path. Next PRs: Implementation for the update path of GcsTaskManager Porting of profiling events Porting of state api task.

**Previous PRs:** - #30829: - #30953: - #30867: - #30979: - #30934 - #31207 **In This PR:** - Remove old code for timeline/profiling backend.

**Previous PRs:** - #30829: - #30953: - #30867: - #30979: - #30934 - #31207 **This PR:** With the change, - `list_tasks` now will return tasks with attempt number as an additional column. - `get_task` might return multiple task attempt entries if there are retries. There is also some plumbing in the test and in core (esp in the test logic) given the changes. Major changes in the PR are: - Add limit support to `GcsTaskManager` - Change the state aggregator to get tasks from GCS.

…n] (ray-project#30979) Previous PRs: [core][state] Task events backend: config and interface definitions [0/n] ray-project#30829: Interface and protobuf definitions. [core][state] Task events backend - split drop count on worker ray-project#30953: Splitting of drop count for various events type on worker. [core][state] Task events backend - worker task event buffer implementation [1/n] ray-project#30867: TaskEventBuffer implementation In this PR: Added GcsTaskManager that stores the task events on the GCS side. The GcsTsakManager has its own io service and io thread that's separated from the main rpc thread/io_context. Handling of rpcs will be posted to its own internal io_service. Implementation for the update path. Interface for the read path. Next PRs: Implementation for the update path of GcsTaskManager Porting of profiling events Porting of state api task. Signed-off-by: tmynn <[email protected]>

…ect#31247) **Previous PRs:** - ray-project#30829: - ray-project#30953: - ray-project#30867: - ray-project#30979: - ray-project#30934 - ray-project#31207 **In This PR:** - Remove old code for timeline/profiling backend. Signed-off-by: tmynn <[email protected]>

**Previous PRs:** - ray-project#30829: - ray-project#30953: - ray-project#30867: - ray-project#30979: - ray-project#30934 - ray-project#31207 **This PR:** With the change, - `list_tasks` now will return tasks with attempt number as an additional column. - `get_task` might return multiple task attempt entries if there are retries. There is also some plumbing in the test and in core (esp in the test logic) given the changes. Major changes in the PR are: - Add limit support to `GcsTaskManager` - Change the state aggregator to get tasks from GCS. Signed-off-by: tmynn <[email protected]>

rickyyx added 3 commits December 8, 2022 23:48

initial

26ff7aa

Signed-off-by: rickyyx <[email protected]>

Merge branch 'master' of https://github.com/ray-project/ray into task…

779bac1

…-backend-gcs-update

test

ca35869

Signed-off-by: rickyyx <[email protected]>

rickyyx assigned ericl Dec 9, 2022

rickyyx requested review from wuisawesome, ericl, AmeerHajAli and robertnishihara as code owners December 9, 2022 00:02

rickyyx assigned scv119 Dec 9, 2022

rickyyx requested review from pcmoritz, raulchen, fishbone and scv119 as code owners December 9, 2022 00:02

rickyyx assigned rkooo567 and fishbone Dec 9, 2022

lint

447aaf1

Signed-off-by: rickyyx <[email protected]>

rkooo567 reviewed Dec 9, 2022

View reviewed changes

ericl reviewed Dec 9, 2022

View reviewed changes

src/ray/gcs/gcs_server/gcs_task_manager.h 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 Dec 9, 2022

rickyyx added 4 commits December 13, 2022 23:39

test e2e

26f8edd

Signed-off-by: rickyyx <[email protected]>

lint

b3d89d7

Signed-off-by: rickyyx <[email protected]>

new line

0988760

Signed-off-by: rickyyx <[email protected]>

name

c92987e

Signed-off-by: rickyyx <[email protected]>

rickyyx removed the @author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer. label Dec 13, 2022

ericl approved these changes Dec 14, 2022

View reviewed changes

python/ray/tests/test_task_events.py 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 Dec 14, 2022

rickyyx added 2 commits December 14, 2022 02:48

split metrics

805b339

Signed-off-by: rickyyx <[email protected]>

Merge branch 'master' of https://github.com/ray-project/ray into task…

aca018f

…-backend-gcs-update

rickyyx removed the @author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer. label Dec 14, 2022

rickyyx added tests-ok The tagger certifies test failures are unrelated and assumes personal liability. and removed @author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer. labels Dec 14, 2022

rkooo567 approved these changes Dec 14, 2022

View reviewed changes

rkooo567 merged commit 74ced31 into ray-project:master Dec 14, 2022

rickyyx mentioned this pull request Dec 15, 2022

[core][state] Task events backend - gcs read path [3/n] #30934

Merged

7 tasks

rickyyx assigned rickyyx and unassigned ericl, scv119, rkooo567 and fishbone Dec 16, 2022

This was referenced Dec 20, 2022

[core][state] Task events backend - port profile events and turn on task backend [4/n] #31207

Merged

[core][state] Task backend - Clean old profiling code [5/n] #31247

Merged

[core][state] Task events backend - Port state API [5/n] #31278

Merged

ericl pushed a commit that referenced this pull request Dec 21, 2022

[core][state] Task backend - Clean old profiling code [5/n] (#31247)

ad81538

**Previous PRs:** - #30829: - #30953: - #30867: - #30979: - #30934 - #31207 **In This PR:** - Remove old code for timeline/profiling backend.

rickyyx mentioned this pull request Dec 23, 2022

[core][state] Task event backend - Fix retry task attempt state recording [6/n] #31316

Merged

7 tasks

AmeerHajAli pushed a commit that referenced this pull request Jan 12, 2023

[core][state] Task backend - Clean old profiling code [5/n] (#31247)

eab7c61

**Previous PRs:** - #30829: - #30953: - #30867: - #30979: - #30934 - #31207 **In This PR:** - Remove old code for timeline/profiling backend.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[core][state] Task events backend - gcs update path and interface [2/n] #30979

[core][state] Task events backend - gcs update path and interface [2/n] #30979

rickyyx commented Dec 9, 2022

rkooo567 left a comment

rkooo567 Dec 9, 2022

rkooo567 Dec 9, 2022

rickyyx Dec 13, 2022

rkooo567 Dec 9, 2022

rickyyx Dec 9, 2022 •

edited

Loading

ericl left a comment

rickyyx commented Dec 13, 2022

ericl left a comment

[core][state] Task events backend - gcs update path and interface [2/n] #30979

[core][state] Task events backend - gcs update path and interface [2/n] #30979

Conversation

rickyyx commented Dec 9, 2022

Why are these changes needed?

Related issue number

Checks

rkooo567 left a comment

Choose a reason for hiding this comment

rkooo567 Dec 9, 2022

Choose a reason for hiding this comment

rkooo567 Dec 9, 2022

Choose a reason for hiding this comment

rickyyx Dec 13, 2022

Choose a reason for hiding this comment

rkooo567 Dec 9, 2022

Choose a reason for hiding this comment

rickyyx Dec 9, 2022 • edited Loading

Choose a reason for hiding this comment

ericl left a comment

Choose a reason for hiding this comment

rickyyx commented Dec 13, 2022

ericl left a comment

Choose a reason for hiding this comment

rickyyx Dec 9, 2022 •

edited

Loading