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

Sjoshi/push manager patch #48475

Open
wants to merge 15 commits into
base: releases/2.10.0
Choose a base branch
from
Open

Sjoshi/push manager patch #48475

wants to merge 15 commits into from
+413 −27

Conversation

sjoshi6
Copy link
Contributor

@sjoshi6 sjoshi6 commented Oct 31, 2024

Why are these changes needed?

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 added any new APIs to the API Reference. For example, if I added a
      method in Tune, I've added it in doc/source/tune/api/ under the
      corresponding .rst file.
  • 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 :(

votrou and others added 15 commits October 31, 2024 11:55
@votrou @iamyangchen
[object_store] add zstd compression to object_store push payload (#5) (…
dd6fb94 
…#9)

We use ray.data on remote workers to process training data before
feeding it into the trainer. Our training data sees good compression
ratio when stored on disk; yet with ray.data pipeline we can only
transfer raw data between workers.

We would like to introduce zstd data compression to the object_manager's
Push payloads. In our production environments, we see significant (35% -
40%) training throughput (examples/sec) improvement for one of our
model.

Test plan:

https://docs.google.com/document/d/13XSiAdHbH6qbkSkFHqOKOcdCJG92XldOYgmBbijhtNY/edit?usp=sharing

<!-- Thank you for your contribution! Please review
https://github.com/ray-project/ray/blob/master/CONTRIBUTING.rst before
opening a pull request. -->

<!-- Please add a reviewer to the assignee section when you create a PR.
If you don't have the access to it, we will shortly find a reviewer and
assign them to your PR. -->

## Why are these changes needed?

<!-- Please give a short summary of the change and the problem this
solves. -->

## Related issue number

<!-- For example: "Closes ray-project#1234" -->

## 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 added any new APIs to the API Reference. For example, if I
added a
method in Tune, I've added it in `doc/source/tune/api/` under the
           corresponding `.rst` file.
- [ ] 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: cyang <[email protected]>
Co-authored-by: Chen Yang <[email protected]>
Signed-off-by: Saurabh Vishwas Joshi <[email protected]>
@votrou
add more iteration and file reading metrics (#10)
6ac26f9 
<!-- Thank you for your contribution! Please review
https://github.com/ray-project/ray/blob/master/CONTRIBUTING.rst before
opening a pull request. -->

<!-- Please add a reviewer to the assignee section when you create a PR.
If you don't have the access to it, we will shortly find a reviewer and
assign them to your PR. -->

## Why are these changes needed?

<!-- Please give a short summary of the change and the problem this
solves. -->

## Related issue number

<!-- For example: "Closes ray-project#1234" -->

## 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 added any new APIs to the API Reference. For example, if I
added a
method in Tune, I've added it in `doc/source/tune/api/` under the
           corresponding `.rst` file.
- [ ] 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: Saurabh Vishwas Joshi <[email protected]>
@votrou @can-anyscale
[ci] upgrade manylinux image (ray-project#46361) (#11)
26160d3 
This PR is necessary to get the Ray pipeline to build images for us

Upgrade manylinux image to see if it fixes the mirrolist.centoos
deprecated issue

Test:
- CI

<!-- Thank you for your contribution! Please review
https://github.com/ray-project/ray/blob/master/CONTRIBUTING.rst before
opening a pull request. -->

<!-- Please add a reviewer to the assignee section when you create a PR.
If you don't have the access to it, we will shortly find a reviewer and
assign them to your PR. -->

## Why are these changes needed?

<!-- Please give a short summary of the change and the problem this
solves. -->

## Related issue number

<!-- For example: "Closes ray-project#1234" -->

## 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 added any new APIs to the API Reference. For example, if I
added a
method in Tune, I've added it in `doc/source/tune/api/` under the
           corresponding `.rst` file.
- [ ] 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: can <[email protected]>
Co-authored-by: Cuong Nguyen <[email protected]>
Signed-off-by: Saurabh Vishwas Joshi <[email protected]>
@votrou @cchen777
[Backport][Ray Dashboard] make memory profile href link relative path (…
5c94f1e 
…ray-project#46564) (#15)

When we host ray dashboard behind a prefix URL Path e.g.
https://<some>.<ray>.<domain_name>/ray_dashboard/ it's important to use
relative href link instead of having a forward slash / to prevent reset
the URL Path , this change is to ensure consistent behavior with other
similar profiling URL such as cpu profile and traceback

ideally we want
https://<some>.<ray>.<domain_name>/ray_dashboard/memory_profile...
instead of https://<some>.<ray>.<domain_name>/memory_profile...

<!-- Thank you for your contribution! Please review
https://github.com/ray-project/ray/blob/master/CONTRIBUTING.rst before
opening a pull request. -->

<!-- Please add a reviewer to the assignee section when you create a PR.
If you don't have the access to it, we will shortly find a reviewer and
assign them to your PR. -->

## Why are these changes needed?

<!-- Please give a short summary of the change and the problem this
solves. -->

## Related issue number

<!-- For example: "Closes ray-project#1234" -->

## 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 added any new APIs to the API Reference. For example, if I
added a
method in Tune, I've added it in `doc/source/tune/api/` under the
           corresponding `.rst` file.
- [ ] 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 :(

Co-authored-by: Chia-Wei <[email protected]>
Signed-off-by: Saurabh Vishwas Joshi <[email protected]>
@votrou @brycehuang30
[Ray backport 2.11] [dashboard] add memray to requirements.txt again (r…
9ffe71b 
…ay-project#44401) (#13)

ray-project#44315 is reverted since windows tests are failing. The reason is that
memray does not support windows so the pip requirements is failed to
run.

This PR fixed the requirements issue by conditional install memray when
current system platform is not win32

Note: In last PR, we added memray as a required dependency for dashboard
(default), and it's not set required in this PR.

---------

<!-- Thank you for your contribution! Please review
https://github.com/ray-project/ray/blob/master/CONTRIBUTING.rst before
opening a pull request. -->

<!-- Please add a reviewer to the assignee section when you create a PR.
If you don't have the access to it, we will shortly find a reviewer and
assign them to your PR. -->

## Why are these changes needed?

<!-- Please give a short summary of the change and the problem this
solves. -->

## Related issue number

<!-- For example: "Closes ray-project#1234" -->

## 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 added any new APIs to the API Reference. For example, if I
added a
method in Tune, I've added it in `doc/source/tune/api/` under the
           corresponding `.rst` file.
- [ ] 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: bhuang <[email protected]>
Co-authored-by: Bryce Huang <[email protected]>
Signed-off-by: Saurabh Vishwas Joshi <[email protected]>
@votrou
Add more observability to Ray Data operator metrics (#14)
d512e84 
Adds a few new metrics:
- In-Task Backpressure: Cumulative amount of time in tasks spent idle
waiting to "yield" a block
- CPU Time: Cumulative time for a task spent with CPU
- Also fixes the memory related ray data metrics

<img width="1645" alt="Screenshot 2024-07-25 at 1 40 39 PM"
src="https://github.com/user-attachments/assets/40e8283f-70b2-4d17-be74-35090f15297f">
<img width="1655" alt="Screenshot 2024-07-25 at 1 40 59 PM"
src="https://github.com/user-attachments/assets/b66fdd14-7207-42f8-92b9-ffcf8d366089">

Signed-off-by: Saurabh Vishwas Joshi <[email protected]>
@lee1258561
[Ray Core] Fix potential race condition when modifying os.environ (#20)
96ba808 
<!-- Thank you for your contribution! Please review
https://github.com/ray-project/ray/blob/master/CONTRIBUTING.rst before
opening a pull request. -->

<!-- Please add a reviewer to the assignee section when you create a PR.
If you don't have the access to it, we will shortly find a reviewer and
assign them to your PR. -->

## Why are these changes needed?

<!-- Please give a short summary of the change and the problem this
solves. -->

## Related issue number

<!-- For example: "Closes ray-project#1234" -->

## 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 added any new APIs to the API Reference. For example, if I
added a
method in Tune, I've added it in `doc/source/tune/api/` under the
           corresponding `.rst` file.
- [ ] 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: Saurabh Vishwas Joshi <[email protected]>
@lee1258561
Revert "[Backport][Ray Dashboard] make memory profile href link relat… (
791c793 
#23)

…ive path (ray-project#46564) (#15)"

This reverts commit 1531ab3.

<!-- Thank you for your contribution! Please review
https://github.com/ray-project/ray/blob/master/CONTRIBUTING.rst before
opening a pull request. -->

<!-- Please add a reviewer to the assignee section when you create a PR.
If you don't have the access to it, we will shortly find a reviewer and
assign them to your PR. -->

## Why are these changes needed?

<!-- Please give a short summary of the change and the problem this
solves. -->

## Related issue number

<!-- For example: "Closes ray-project#1234" -->

## 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 added any new APIs to the API Reference. For example, if I
added a
method in Tune, I've added it in `doc/source/tune/api/` under the
           corresponding `.rst` file.
- [ ] 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: Saurabh Vishwas Joshi <[email protected]>
@lee1258561
Revert "[Ray backport 2.11] [dashboard] add memray to requirements.tx… (
fe06ac7 
#21)

…t again (ray-project#44401) (#13)"

This reverts commit f32c3e5.

<!-- Thank you for your contribution! Please review
https://github.com/ray-project/ray/blob/master/CONTRIBUTING.rst before
opening a pull request. -->

<!-- Please add a reviewer to the assignee section when you create a PR.
If you don't have the access to it, we will shortly find a reviewer and
assign them to your PR. -->

## Why are these changes needed?

<!-- Please give a short summary of the change and the problem this
solves. -->

## Related issue number

<!-- For example: "Closes ray-project#1234" -->

## 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 added any new APIs to the API Reference. For example, if I
added a
method in Tune, I've added it in `doc/source/tune/api/` under the
           corresponding `.rst` file.
- [ ] 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: Saurabh Vishwas Joshi <[email protected]>
@lee1258561
Revert "Add more observability to Ray Data operator metrics (#14)" (#22)
21d1bf5 
This reverts commit 3387e67.

<!-- Thank you for your contribution! Please review
https://github.com/ray-project/ray/blob/master/CONTRIBUTING.rst before
opening a pull request. -->

<!-- Please add a reviewer to the assignee section when you create a PR.
If you don't have the access to it, we will shortly find a reviewer and
assign them to your PR. -->

## Why are these changes needed?

<!-- Please give a short summary of the change and the problem this
solves. -->

## Related issue number

<!-- For example: "Closes ray-project#1234" -->

## 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 added any new APIs to the API Reference. For example, if I
added a
method in Tune, I've added it in `doc/source/tune/api/` under the
           corresponding `.rst` file.
- [ ] 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: Saurabh Vishwas Joshi <[email protected]>
@votrou
Revert "Revert "[Backport][Ray Dashboard] make memory profile href li…
47389f1 
…nk relat…" (#25)

Reverts #23

Re-adds logic of the base PR

Signed-off-by: Saurabh Vishwas Joshi <[email protected]>
@votrou
Revert "Revert "Add more observability to Ray Data operator metrics (#14
911ac7b 
)"" (#26)

Reverts #22

Signed-off-by: Saurabh Vishwas Joshi <[email protected]>
@votrou
Revert "Revert "[Ray backport 2.11] [dashboard] add memray to require…
2050da7 
…ments.tx…" (#27)

Reverts #21

Signed-off-by: Saurabh Vishwas Joshi <[email protected]>
@votrou @matthewdeng
[backport 2.11] prevent long recursive stack traces (#24)
06bb945 
This backports the following PR:
https://github.com/ray-project/ray/pull/43952/files
It prevents stack traces from recursing over and over until they hit a
limit, and show unrelated errors to the user. This way the error
stacktraces will be much clearer.

<!-- Thank you for your contribution! Please review
https://github.com/ray-project/ray/blob/master/CONTRIBUTING.rst before
opening a pull request. -->

<!-- Please add a reviewer to the assignee section when you create a PR.
If you don't have the access to it, we will shortly find a reviewer and
assign them to your PR. -->

## Why are these changes needed?

<!-- Please give a short summary of the change and the problem this
solves. -->

## Related issue number

<!-- For example: "Closes ray-project#1234" -->

## 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 added any new APIs to the API Reference. For example, if I
added a
method in Tune, I've added it in `doc/source/tune/api/` under the
           corresponding `.rst` file.
- [ ] 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: Matthew Deng <[email protected]>
Co-authored-by: matthewdeng <[email protected]>
Signed-off-by: Saurabh Vishwas Joshi <[email protected]>
[push_manager] change fulfillment of push request from round robin to…
aa7c042 
… FIFO

Signed-off-by: Saurabh Vishwas Joshi <[email protected]>
@sjoshi6 sjoshi6 force-pushed the sjoshi/push_manager_patch branch from 47adfbc to aa7c042 Compare October 31, 2024 18:55
aslonnie
aslonnie reviewed Oct 31, 2024
View reviewed changes
Copy link
Collaborator

@aslonnie aslonnie left a comment

Choose a reason for hiding this comment

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

(skipping review)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aslonnie aslonnie aslonnie left review comments

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

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

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

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

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

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

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

@stephanie-wang stephanie-wang Awaiting requested review from stephanie-wang stephanie-wang is a code owner

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

@can-anyscale can-anyscale Awaiting requested review from can-anyscale can-anyscale is a code owner

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

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

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@sjoshi6 @aslonnie @votrou @lee1258561