[Data] Tune Concurrency Cap Backpressure objectstore budget ratio #58813
Conversation
Signed-off-by: Srinath Krishnamachari <srinath.krishnamachari@anyscale.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the concurrency cap backpressure policy to use a more robust available / total ratio for object store usage, instead of available / used. This is a good improvement. The corresponding tests have been updated and expanded. I've found a potential logic bug in the surrounding conditional check for this new ratio that could lead to incorrect backpressure behavior. Additionally, there's a minor inaccuracy in a test comment that could be clarified. Overall, the direction is great, but the identified issue should be addressed.
python/ray/data/_internal/execution/backpressure_policy/concurrency_cap_backpressure_policy.py
Outdated
Show resolved
Hide resolved
srinathk10
changed the title
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the concurrency cap backpressure policy to use a more intuitive object store budget ratio (available / total instead of available / used). The changes include renaming the relevant constant and environment variable, updating the calculation logic, and adjusting the tests accordingly. The refactoring of EWMA_ALPHA_UP to a class constant is a nice improvement for code clarity. The tests have also been improved with more assertions. However, I've found a potential logic bug in how zero object store usage is handled, which could lead to incorrect backpressure behavior. Please see my detailed comment.
python/ray/data/_internal/execution/backpressure_policy/concurrency_cap_backpressure_policy.py
Outdated
Show resolved
Hide resolved
python/ray/data/_internal/execution/backpressure_policy/concurrency_cap_backpressure_policy.py
Show resolved
Hide resolved
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the concurrency cap backpressure policy by introducing a more intuitive formula for the object store budget ratio. The changes also include moving a magic number to a class constant and significantly improving the test suite with more robust checks. Overall, these are great improvements. I've identified one potential issue with an edge case where memory usage is zero, which could lead to incorrect backpressure behavior, and have provided a suggestion to address it.
python/ray/data/_internal/execution/backpressure_policy/concurrency_cap_backpressure_policy.py
Outdated
Show resolved
Hide resolved
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the concurrency cap backpressure logic to use a more intuitive object store budget ratio (available / total instead of available / used). The changes are well-implemented, with corresponding updates to constants, comments, and logging. The tests have also been updated to match the new logic and enhanced with additional assertions to verify behavior, which is a great improvement. I've added a couple of minor suggestions to make the test mocks more robust by ensuring memory values remain non-negative.
…y-project#58813) > Thank you for contributing to Ray! 🚀 > Please review the [Ray Contribution Guide](https://docs.ray.io/en/master/ray-contribute/getting-involved.html) before opening a pull request. >⚠️ Remove these instructions before submitting your PR. > 💡 Tip: Mark as draft if you want early feedback, or ready for review when it's complete. ## Description > Briefly describe what this PR accomplishes and why it's needed. ### [Data] Tune Concurrency Cap Backpressure objectstore budget ratio Fix threshold when backpressure should kickin as more intuitive `OBJECT_STORE_BUDGET_RATIO = available budget / (available budget + usage) ` ## Related issues > Link related issues: "Fixes ray-project#1234", "Closes ray-project#1234", or "Related to ray-project#1234". ## Additional information > Optional: Add implementation details, API changes, usage examples, screenshots, etc. --------- Signed-off-by: Srinath Krishnamachari <srinath.krishnamachari@anyscale.com>
…y-project#58813) > Thank you for contributing to Ray! 🚀 > Please review the [Ray Contribution Guide](https://docs.ray.io/en/master/ray-contribute/getting-involved.html) before opening a pull request. >⚠️ Remove these instructions before submitting your PR. > 💡 Tip: Mark as draft if you want early feedback, or ready for review when it's complete. ## Description > Briefly describe what this PR accomplishes and why it's needed. ### [Data] Tune Concurrency Cap Backpressure objectstore budget ratio Fix threshold when backpressure should kickin as more intuitive `OBJECT_STORE_BUDGET_RATIO = available budget / (available budget + usage) ` ## Related issues > Link related issues: "Fixes ray-project#1234", "Closes ray-project#1234", or "Related to ray-project#1234". ## Additional information > Optional: Add implementation details, API changes, usage examples, screenshots, etc. --------- Signed-off-by: Srinath Krishnamachari <srinath.krishnamachari@anyscale.com> Signed-off-by: YK <1811651+ykdojo@users.noreply.github.com>
…y-project#58813) > Thank you for contributing to Ray! 🚀 > Please review the [Ray Contribution Guide](https://docs.ray.io/en/master/ray-contribute/getting-involved.html) before opening a pull request. >⚠️ Remove these instructions before submitting your PR. > 💡 Tip: Mark as draft if you want early feedback, or ready for review when it's complete. ## Description > Briefly describe what this PR accomplishes and why it's needed. ### [Data] Tune Concurrency Cap Backpressure objectstore budget ratio Fix threshold when backpressure should kickin as more intuitive `OBJECT_STORE_BUDGET_RATIO = available budget / (available budget + usage) ` ## Related issues > Link related issues: "Fixes ray-project#1234", "Closes ray-project#1234", or "Related to ray-project#1234". ## Additional information > Optional: Add implementation details, API changes, usage examples, screenshots, etc. --------- Signed-off-by: Srinath Krishnamachari <srinath.krishnamachari@anyscale.com>
Description
[Data] Tune Concurrency Cap Backpressure objectstore budget ratio
Fix threshold when backpressure should kickin as more intuitive
OBJECT_STORE_BUDGET_RATIO = available budget / (available budget + usage)Related issues
Additional information