Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged
merged 58 commits into from
Dec 20, 2022
Merged

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

merged 58 commits into from
Dec 20, 2022

Conversation

rickyyx
Copy link
Contributor

@rickyyx rickyyx commented Dec 19, 2022 •
edited
Loading

Why are these changes needed?

Previous PRs:

In this PR:

  • New task backend on by default.
  • Changed the profiling event's generation in profiling.cc
    • Removed Profiler to use TaskEventsBuffer directly when emitting ProfileEvent
  • Changed front-end (ray.timeline) to get profiling events from TaskEvents in GCS.

In Future PRs:

  1. Remove legacy code for profiling events
  2. State API porting.

Related issue number

Checks

RAY_PROFILING=1 pytest python/ray/tests/test_advanced.py::test_profiling_api -vs

image

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

@rickyyx
task event buffer impl
f40dcad 
Signed-off-by: rickyyx <[email protected]>
@rickyyx
bazel
932f123 
Signed-off-by: rickyyx <[email protected]>
@rickyyx
fixed
ad47be1 
Signed-off-by: rickyyx <[email protected]>
@rickyyx
todo
61a759e 
Signed-off-by: rickyyx <[email protected]>
@rickyyx
default refactor
73b8231 
Signed-off-by: rickyyx <[email protected]>
@rickyyx
test
7c5815c 
Signed-off-by: rickyyx <[email protected]>
@rickyyx
setstatus
6d82645 
Signed-off-by: rickyyx <[email protected]>
@rickyyx
lock
c2f2a47 
Signed-off-by: rickyyx <[email protected]>
@rickyyx
comments
c1da76d 
Signed-off-by: rickyyx <[email protected]>
@rickyyx
gcs initial
64c7626 
Signed-off-by: rickyyx <[email protected]>
@rickyyx
enabled
88e81e5 
Signed-off-by: rickyyx <[email protected]>
@rickyyx
test
b35bdef 
Signed-off-by: rickyyx <[email protected]>
@rickyyx
test
186b34d 
Signed-off-by: rickyyx <[email protected]>
@rickyyx
lock
8b4d305 
Signed-off-by: rickyyx <[email protected]>
@rickyyx
disabled
f38002b 
Signed-off-by: rickyyx <[email protected]>
@rickyyx
fixed override
248cd5c 
Signed-off-by: rickyyx <[email protected]>
@rickyyx
Merge branch 'task-backend-impl' into task-backend-gcs
57d4937
@rickyyx
initial
72c76ec 
Signed-off-by: rickyyx <[email protected]>
@rickyyx
events drop count
84aed27 
Signed-off-by: rickyyx <[email protected]>
@rickyyx
merged
147c757 
Signed-off-by: rickyyx <[email protected]>
@rickyyx
tests
cb1b55b 
Signed-off-by: rickyyx <[email protected]>
@rickyyx
styled
01479bd 
Signed-off-by: rickyyx <[email protected]>
@rickyyx
Merge branch 'master' of https://github.com/ray-project/ray into task…
c9a4d2a 
…-backend-gcs
@rickyyx
initial
26ff7aa 
Signed-off-by: rickyyx <[email protected]>
@rickyyx
Merge branch 'master' of https://github.com/ray-project/ray into task…
779bac1 
…-backend-gcs-update
@rickyyx
test
ca35869 
Signed-off-by: rickyyx <[email protected]>
@rickyyx
lint
447aaf1 
Signed-off-by: rickyyx <[email protected]>
@rickyyx
test e2e
26f8edd 
Signed-off-by: rickyyx <[email protected]>
@rickyyx
lint
b3d89d7 
Signed-off-by: rickyyx <[email protected]>
@rickyyx
new line
0988760 
Signed-off-by: rickyyx <[email protected]>
@rickyyx rickyyx changed the title [core][state] Task events backend - port profile events [4/n] [core][state] Task events backend - port profile events and turn on task backend [4/n] Dec 20, 2022
@rickyyx
remove tests
8cc0354 
Signed-off-by: rickyyx <[email protected]>
@rickyyx
Copy link
Contributor Author

rickyyx commented Dec 20, 2022

Some selected benchmark results from the run: https://buildkite.com/ray-project/release-tests-pr/builds/24015#_

tldr: A few microbenchmark metrics have some regression from up to 15%, while most of the e2e tests see a negligible impact.

Metric Master PR
microbenchmark
single_client_tasks_sync 1252 1238
single_client_tasks_async 11180 10648
multi_client_tasks_async 33785 28934
1_1_actor_calls_sync 2155 1881
1_1_actor_calls_async 7061 6926
1_n_actor_calls_async 10689 10822
n_n_actor_calls_async 34513 30988
n_n_async_actor_calls_async 28600 25367
stress_many_tasks
stage_0_time 8.67 7.70
stage_1_avg_iteration_time 22.27 22.87
stage_2_avg_iteration_time 55.53 51.76
stage_3_creation_time 0.052 0.053
many_tasks
tasks_per_second 27.11 26.67
used_cpus_by_deadline 1759 1753
many_actors
actors_per_second 292.82 435.88
single_node
args_time 17.02 17.06
returns_time 6.11 6.38
get_time 25.07 25.47
queued_time 198 205
large_object_time 259 251
many_nodes
tasks_per_second 9.38 9.15

@rickyyx 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 20, 2022
@rickyyx
Copy link
Contributor Author

rickyyx commented Dec 20, 2022

Test failures unrelated (some python dependency issue + memory_monitor_test that's still flaky)

@ericl ericl merged commit 53f68cd into ray-project:master Dec 20, 2022
@ericl
Copy link
Contributor

ericl commented Dec 20, 2022

LGTM

@rickyyx rickyyx assigned rickyyx and unassigned ericl and rkooo567 Dec 20, 2022
This was referenced Dec 20, 2022
Merged
Merged
ericl pushed a commit that referenced this pull request Dec 21, 2022
@rickyyx
[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 rickyyx mentioned this pull request Dec 22, 2022
Closed
ericl pushed a commit that referenced this pull request Dec 23, 2022
@rickyyx
[core][state] Task backend - Port state API [6/n] (#31278)
eb096f7 
**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.
@rickyyx rickyyx mentioned this pull request Dec 23, 2022
Merged
7 tasks
AmeerHajAli pushed a commit that referenced this pull request Jan 12, 2023
@rickyyx @AmeerHajAli
[core][state] Task events backend - port profile events and turn on t…
147d58e 
…ask backend [4/n] (#31207)
AmeerHajAli pushed a commit that referenced this pull request Jan 12, 2023
@rickyyx @AmeerHajAli
[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.
AmeerHajAli pushed a commit that referenced this pull request Jan 12, 2023
@rickyyx @AmeerHajAli
[core][state] Task backend - Port state API [6/n] (#31278)
ffb6711 
**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.
tamohannes pushed a commit to ju2ez/ray that referenced this pull request Jan 25, 2023
@rickyyx @tamohannes
[core][state] Task events backend - port profile events and turn on t…
9821c2f 
…ask backend [4/n] (ray-project#31207)

Signed-off-by: tmynn <[email protected]>
tamohannes pushed a commit to ju2ez/ray that referenced this pull request Jan 25, 2023
@rickyyx @tamohannes
[core][state] Task backend - Clean old profiling code [5/n] (ray-proj…
7a5eec5 
…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]>
tamohannes pushed a commit to ju2ez/ray that referenced this pull request Jan 25, 2023
@rickyyx @tamohannes
[core][state] Task backend - Port state API [6/n] (ray-project#31278)
0e1eee1 
**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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rkooo567 rkooo567 rkooo567 left review comments

@ericl ericl ericl approved these changes

@wuisawesome wuisawesome Awaiting requested review from wuisawesome

@AmeerHajAli AmeerHajAli Awaiting requested review from AmeerHajAli

@robertnishihara robertnishihara Awaiting requested review from robertnishihara

@pcmoritz pcmoritz Awaiting requested review from pcmoritz pcmoritz is a code owner

@raulchen raulchen Awaiting requested review from raulchen raulchen is a code owner

@fishbone fishbone Awaiting requested review from fishbone

@scv119 scv119 Awaiting requested review from scv119

Assignees

@rickyyx rickyyx

Labels
tests-ok The tagger certifies test failures are unrelated and assumes personal liability.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@rickyyx @ericl @rkooo567