[Data][Flaky] fix test_limit_pushdown_conservative fails due to non-deterministic task ordering #58655
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request enhances the limit pushdown tests by introducing an option to disable ordering checks, which helps in stabilizing flaky tests where the output order is not guaranteed. The main change is in the _check_valid_plan_and_result helper function, which now conditionally checks for order. When order is not checked, it compares the results as sorted lists. I've suggested a minor improvement to use collections.Counter for a more idiomatic and potentially more performant unordered comparison. The application of check_ordering=False to the relevant test cases seems correct and addresses the flakiness issue.
python/ray/data/tests/test_execution_optimizer_limit_pushdown.py
Outdated
Show resolved
Hide resolved
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
python/ray/data/tests/test_execution_optimizer_limit_pushdown.py
Outdated
Show resolved
Hide resolved
ryankert01
changed the title
ryankert01
changed the title
ray-gardener
bot
added
data
ryankert01
force-pushed
the
feature/flaky-test_limit_pushdown_conservative
branch
from
d4cf9d8 to
033adb0
Compare
Contributor
Author
|
thanks for the review @400Ping ! |
python/ray/data/tests/test_execution_optimizer_limit_pushdown.py
Outdated
Show resolved
Hide resolved
ryankert01
force-pushed
the
feature/flaky-test_limit_pushdown_conservative
branch
from
c73cd4c to
838b73f
Compare
ryankert01
force-pushed
the
feature/flaky-test_limit_pushdown_conservative
branch
from
0c388c8 to
89432eb
Compare
bveeramani
reviewed
Nov 16, 2025
Comment on lines
+26
to
+27
| if check_ordering: | ||
| assert actual_result == expected_result |
Member
There was a problem hiding this comment.
Any reason we can't always use rows_same (i.e., always use check_ordering=False)? Are there any tests in this file where the ordering is guaranteed by Ray Data's interface?
Contributor
Author
There was a problem hiding this comment.
I think there's still some need to check ordering, for example test 6 the ordering is guaranteed.
Member
There was a problem hiding this comment.
Ah, I see. I guess there are a few tests that call sort
In a follow-up PR, would you mind updating all of the other tests in this module where the ordering isn't guaranteed? Seems like most of the tests assume ordering when it's not guaranteed
Member
There was a problem hiding this comment.
Separately, I think it'd be good to break up the different test cases into separate test functions for the reasons described here: https://learn.microsoft.com/en-us/dotnet/core/testing/unit-testing-best-practices#avoid-multiple-act-tasks
Contributor
Author
There was a problem hiding this comment.
Cool, I'll open a refactor pr soon!
Added check_ordering parameter to _check_valid_plan_and_result function to allow for ordering checks to be optional. Updated multiple test cases to utilize the new parameter. Signed-off-by: Ryan Huang <[email protected]> Update test_execution_optimizer_limit_pushdown.py Signed-off-by: Ryan Huang <[email protected]> Update python/ray/data/tests/test_execution_optimizer_limit_pushdown.py Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> Signed-off-by: Ryan Huang <[email protected]> remove unnessary relaxation Signed-off-by: Ryan Huang <[email protected]> remove unnessary relaxation Updated the test to not check ordering in the execution optimizer limit pushdown. Signed-off-by: Ryan Huang <[email protected]> Refactor to_hashable function for clarity Signed-off-by: Ryan Huang <[email protected]> Fix formatting in test_execution_optimizer_limit_pushdown Signed-off-by: Ryan Huang <[email protected]> please linter Signed-off-by: ryankert01 <[email protected]>
Co-authored-by: You-Cheng Lin <[email protected]> Signed-off-by: Ryan Huang <[email protected]> Signed-off-by: ryankert01 <[email protected]>
Removed comment about comparing as multisets when ordering doesn't matter. Signed-off-by: Ryan Huang <[email protected]> Signed-off-by: ryankert01 <[email protected]>
Signed-off-by: ryankert01 <[email protected]>
ryankert01
force-pushed
the
feature/flaky-test_limit_pushdown_conservative
branch
from
89432eb to
1a6a21f
Compare
Aydin-ab
pushed a commit
to Aydin-ab/ray-aydin
that referenced
this pull request
Nov 19, 2025
…eterministic task ordering (ray-project#58655) ## Description I add a param that can determin whether we should check the order or not, release restriction for flaky tests. ## Related issues Closes ray-project#58561 ## Additional information I ran test 20 times using a simple script that rans `python -m pytest python/ray/data/tests/test_execution_optimizer_limit_pushdown.py::test_limit_pushdown_conservative`: - master: `Passed: 18, Failed: 2` - feature/flaky-test_limit_pushdown_conservative: `Passed: 20, Failed: 0` --------- Signed-off-by: ryankert01 <[email protected]> Signed-off-by: Ryan Huang <[email protected]> Co-authored-by: You-Cheng Lin <[email protected]> Signed-off-by: Aydin Abiar <[email protected]>
bveeramani
pushed a commit
that referenced
this pull request
Nov 24, 2025
…ix ordering assumption (#58746) ## Description **Split multi-case test function** - `test_limit_pushdown_conservative` → 10 separate tests (basic fusion, limit fusion reversed, multiple limit fusion, maprows, mapbatches, filter, project, sort, complex interweaved operations, and between two map operators) **Fixed ordering assumptions** - Added `check_ordering=False` to union tests (blocks may interleave) - Added `check_ordering=False` to project test with `override_num_blocks` (parallel execution) ## Related issues Related to #58655 ## Additional information --------- Signed-off-by: ryankert01 <[email protected]>
ykdojo
pushed a commit
to ykdojo/ray
that referenced
this pull request
Nov 27, 2025
…eterministic task ordering (ray-project#58655) ## Description I add a param that can determin whether we should check the order or not, release restriction for flaky tests. ## Related issues Closes ray-project#58561 ## Additional information I ran test 20 times using a simple script that rans `python -m pytest python/ray/data/tests/test_execution_optimizer_limit_pushdown.py::test_limit_pushdown_conservative`: - master: `Passed: 18, Failed: 2` - feature/flaky-test_limit_pushdown_conservative: `Passed: 20, Failed: 0` --------- Signed-off-by: ryankert01 <[email protected]> Signed-off-by: Ryan Huang <[email protected]> Co-authored-by: You-Cheng Lin <[email protected]> Signed-off-by: YK <[email protected]>
ykdojo
pushed a commit
to ykdojo/ray
that referenced
this pull request
Nov 27, 2025
…ix ordering assumption (ray-project#58746) ## Description **Split multi-case test function** - `test_limit_pushdown_conservative` → 10 separate tests (basic fusion, limit fusion reversed, multiple limit fusion, maprows, mapbatches, filter, project, sort, complex interweaved operations, and between two map operators) **Fixed ordering assumptions** - Added `check_ordering=False` to union tests (blocks may interleave) - Added `check_ordering=False` to project test with `override_num_blocks` (parallel execution) ## Related issues Related to ray-project#58655 ## Additional information --------- Signed-off-by: ryankert01 <[email protected]> Signed-off-by: YK <[email protected]>
SheldonTsen
pushed a commit
to SheldonTsen/ray
that referenced
this pull request
Dec 1, 2025
…eterministic task ordering (ray-project#58655) ## Description I add a param that can determin whether we should check the order or not, release restriction for flaky tests. ## Related issues Closes ray-project#58561 ## Additional information I ran test 20 times using a simple script that rans `python -m pytest python/ray/data/tests/test_execution_optimizer_limit_pushdown.py::test_limit_pushdown_conservative`: - master: `Passed: 18, Failed: 2` - feature/flaky-test_limit_pushdown_conservative: `Passed: 20, Failed: 0` --------- Signed-off-by: ryankert01 <[email protected]> Signed-off-by: Ryan Huang <[email protected]> Co-authored-by: You-Cheng Lin <[email protected]>
SheldonTsen
pushed a commit
to SheldonTsen/ray
that referenced
this pull request
Dec 1, 2025
…ix ordering assumption (ray-project#58746) ## Description **Split multi-case test function** - `test_limit_pushdown_conservative` → 10 separate tests (basic fusion, limit fusion reversed, multiple limit fusion, maprows, mapbatches, filter, project, sort, complex interweaved operations, and between two map operators) **Fixed ordering assumptions** - Added `check_ordering=False` to union tests (blocks may interleave) - Added `check_ordering=False` to project test with `override_num_blocks` (parallel execution) ## Related issues Related to ray-project#58655 ## Additional information --------- Signed-off-by: ryankert01 <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
I add a param that can determin whether we should check the order or not, release restriction for flaky tests.
Related issues
Closes #58561
Additional information
I ran test 20 times using a simple script that rans
python -m pytest python/ray/data/tests/test_execution_optimizer_limit_pushdown.py::test_limit_pushdown_conservative:Passed: 18, Failed: 2Passed: 20, Failed: 0