-
Notifications
You must be signed in to change notification settings - Fork 7k
[Data] Decrease scale of map_groups release test
#58711
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.
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
Conversation
Signed-off-by: Balaji Veeramani <bveeramani@berkeley.edu>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request correctly reverts the scale factor for the map_groups release test from SF 100 to SF 10, which is a necessary step to resolve the consistent test failures. My review includes a suggestion to add a TODO comment to track this temporary change, ensuring it's revisited once the underlying scalability issue is fixed. This will improve the long-term maintainability of the test configuration.
| timeout: 3600 | ||
| script: > | ||
| python groupby_benchmark.py --sf 100 --map-groups --group-by {{columns}} | ||
| python groupby_benchmark.py --sf 10 --map-groups --group-by {{columns}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change is a good temporary fix to unblock the release tests. To improve long-term maintainability, it would be beneficial to add a TODO comment in the code explaining why the scale factor was reduced and referencing the tracking issue (#58312).
Since this script block uses the folded block scalar (>), adding an inline comment will comment out the rest of the script. You could change the block scalar to literal style (|) on line 195 to add the comment on a separate line.
For example:
run:
timeout: 3600
script: |
# TODO(#58312): Revert to --sf 100 once the scalability issue is fixed.
python groupby_benchmark.py --sf 10 --map-groups --group-by {{columns}}
--shuffle-strategy {{shuffle_strategy}}This will ensure the reason for this temporary change is not lost over time.
## Description ray-project#58234 increased the scale of the `map_groups` release test from SF 10 to SF 100. Since then, the release test has been consistently failing (see ray-project#58312). To avoid a perpetually broken release test, this PR reverts the scale to SF 10 while we investigate and fix the scalability issue. ## Related issues ray-project#58312 Signed-off-by: Balaji Veeramani <bveeramani@berkeley.edu> Signed-off-by: Aydin Abiar <aydin@anyscale.com>
#58710) > 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. #58711 decreased the scale of the `map_groups` tests from scale-factor 100 to scale-factor 10 because some of the `map_groups` release tests were failing. However, after more investigation, I realized that the only variant that doesn't work with scale-factor 100 is the hash shuffle with autoscaling variant (see #58734). This PR re-increases the scale and only disables the cases that fail. ## Related issues > Link related issues: "Fixes #1234", "Closes #1234", or "Related to #1234". ## Additional information > Optional: Add implementation details, API changes, usage examples, screenshots, etc. --------- Signed-off-by: Balaji Veeramani <bveeramani@berkeley.edu>
## Description ray-project#58234 increased the scale of the `map_groups` release test from SF 10 to SF 100. Since then, the release test has been consistently failing (see ray-project#58312). To avoid a perpetually broken release test, this PR reverts the scale to SF 10 while we investigate and fix the scalability issue. ## Related issues ray-project#58312 Signed-off-by: Balaji Veeramani <bveeramani@berkeley.edu> Signed-off-by: YK <1811651+ykdojo@users.noreply.github.com>
## Description ray-project#58234 increased the scale of the `map_groups` release test from SF 10 to SF 100. Since then, the release test has been consistently failing (see ray-project#58312). To avoid a perpetually broken release test, this PR reverts the scale to SF 10 while we investigate and fix the scalability issue. ## Related issues ray-project#58312 Signed-off-by: Balaji Veeramani <bveeramani@berkeley.edu>
ray-project#58710) > 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. ray-project#58711 decreased the scale of the `map_groups` tests from scale-factor 100 to scale-factor 10 because some of the `map_groups` release tests were failing. However, after more investigation, I realized that the only variant that doesn't work with scale-factor 100 is the hash shuffle with autoscaling variant (see ray-project#58734). This PR re-increases the scale and only disables the cases that fail. ## 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: Balaji Veeramani <bveeramani@berkeley.edu>
Description
#58234 increased the scale of the
map_groupsrelease test from SF 10 to SF 100. Since then, the release test has been consistently failing (see #58312).To avoid a perpetually broken release test, this PR reverts the scale to SF 10 while we investigate and fix the scalability issue.
Related issues
#58312