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 - split drop count on worker #30953

Merged
merged 1 commit into from
Dec 8, 2022
Merged

[core][state] Task events backend - split drop count on worker #30953

merged 1 commit into from
Dec 8, 2022

Conversation

rickyyx
Copy link
Contributor

@rickyyx rickyyx commented Dec 7, 2022

Signed-off-by: rickyyx [email protected]

Why are these changes needed?

  • We were reporting dropped task event count as a single value. When queried for a specific kind of events, e.g. profile events, the number of dropped events for status updates should not interfere the profile events results.
  • Fix testing: task events in exported rpc::TaskEventData' has non-deterministic order due to an intermediate merge with flat_hash_map.

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

@rickyyx
events drop count
84aed27 
Signed-off-by: rickyyx <[email protected]>
rkooo567
rkooo567 approved these changes Dec 7, 2022
View reviewed changes
Copy link
Contributor

@rkooo567 rkooo567 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we don't have profile events yet right?

@rickyyx
Copy link
Contributor Author

rickyyx commented Dec 7, 2022

we don't have profile events yet right?

Right - it will be part of porting the profile events pr later. Basically changing how the current Profiler works.

ericl
ericl reviewed Dec 8, 2022
View reviewed changes
Copy link
Contributor

@ericl ericl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code looks fine, but what is the motivation for splitting it out? Seems like we'd probably be dropping both if dropping any, not sure if there's a meaningful distinction to draw.

@rkooo567
Copy link
Contributor

rkooo567 commented Dec 8, 2022

also curious about ^. Could be useful if we have different buffers for profiling events and state change events, but iiuc that's not our plan?

@rickyyx
Copy link
Contributor Author

rickyyx commented Dec 8, 2022

The code looks fine, but what is the motivation for splitting it out? Seems like we'd probably be dropping both if dropping any, not sure if there's a meaningful distinction to draw.

I guess the motivation is mainly that assuming we will have independent queries for profile events, and for task status. So we might want to let users know the number of task status changes dropped rather than the total number (including profile events) when they query for task status changes. If this makes sense?

Could be useful if we have different buffers for profiling events and state change events, but iiuc that's not our plan?

Yeah, this could be possible if we want to enforce limit on each type of events in the future.

ericl
ericl approved these changes Dec 8, 2022
View reviewed changes
Copy link
Contributor

@ericl ericl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cpp test failures

@ericl 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 8, 2022
@rickyyx rickyyx added the tests-ok The tagger certifies test failures are unrelated and assumes personal liability. label Dec 8, 2022
@rickyyx
Copy link
Contributor Author

rickyyx commented Dec 8, 2022

Looks like test failure is due to this #30573 (comment)

@rickyyx 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 8, 2022
@ericl ericl merged commit 259b5cf into ray-project:master Dec 8, 2022
@rickyyx rickyyx mentioned this pull request Dec 9, 2022
Merged
7 tasks
rkooo567 pushed a commit that referenced this pull request Dec 14, 2022
@rickyyx
[core][state] Task events backend - gcs update path and interface [2/…
74ced31 
…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.
@rickyyx rickyyx mentioned this pull request Dec 15, 2022
Merged
7 tasks
WeichenXu123 pushed a commit to WeichenXu123/ray that referenced this pull request Dec 19, 2022
@rickyyx @WeichenXu123
Events drop count (ray-project#30953)
75c0c22 
Signed-off-by: Weichen Xu <[email protected]>
WeichenXu123 pushed a commit to WeichenXu123/ray that referenced this pull request Dec 19, 2022
@rickyyx @WeichenXu123
[core][state] Task events backend - gcs update path and interface [2/…
17ae5ff 
…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]>
This was referenced Dec 20, 2022
Merged
Merged
@rickyyx rickyyx mentioned this pull request Dec 21, 2022
Merged
7 tasks
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.
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 - gcs update path and interface [2/…
d3d54f1 
…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.
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
Events drop count (ray-project#30953)
e00eb0d 
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 events backend - gcs update path and interface [2/…
1b28ab3 
…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]>
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 approved these changes

@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

@ericl ericl

@rkooo567 rkooo567

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 @rkooo567 @ericl