test: refactor test_observability_helpers
#53875
Merged
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
5e62e4f to
df818f2
Compare
|
Ready for review @abrarsheikh @zcin ! |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
df818f2 to
5621c5c
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
5621c5c to
abd626e
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR updates the observability tests for batching by parameterizing the helper test and introducing new utility functions for sending requests and waiting for a desired number of waiters. Additionally, the type hint for the condition predictor in async_wait_for_condition has been updated.
- Updated test_observability_helpers with parameterized inputs and a signal actor.
- Added send_k_requests and wait_for_n_waiters helper functions.
- Adjusted async_wait_for_condition’s condition_predictor type hint.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| python/ray/serve/tests/test_batching.py | Updated and parameterized batching observability tests and utilities. |
| python/ray/_common/test_utils.py | Modified the condition_predictor type hint in async_wait_for_condition. |
Comments suppressed due to low confidence (2)
python/ray/serve/tests/test_batching.py:283
- The parameter 'min_num_batches' is typed as float, but it is computed using math.ceil which returns an integer. Consider changing its type annotation to int for clarity.
async def send_k_requests(
python/ray/_common/test_utils.py:114
- Changing the type hint to Callable[..., Awaitable[bool]] forces condition_predictor to be asynchronous. If synchronous functions should also be allowed, consider using a union type of bool and Awaitable[bool].
condition_predictor: Callable[..., Awaitable[bool]],
…be more extensive and less flaky Signed-off-by: Arthur <atte.book@gmail.com>
Signed-off-by: Arthur <atte.book@gmail.com>
8000
ArthurBook
force-pushed
the
arthurbook/refactor-batching-tests
branch
2 times, most recently
from
June 17, 2025 19:55
abd626e to
d987341
Compare
Signed-off-by: Arthur <atte.book@gmail.com>
abrarsheikh
approved these changes
Jun 18, 2025
test_backpressure wait_for_conditiontest_observability_helpers
aslonnie
approved these changes
Jun 23, 2025
| @@ -110,7 +111,7 @@ def wait_for_condition( | |||
|
|
|||
|
|
|||
| async def async_wait_for_condition( | |||
| condition_predictor: Callable[..., bool], | |||
| condition_predictor: Callable[..., Awaitable[bool]], | |||
There was a problem hiding this comment.
is this function only used in serve/tests/test_batching.py
There was a problem hiding this comment.
if yes, maybe move this function into that file?
zcin
approved these changes
Jun 23, 2025
minerharry
pushed a commit
to minerharry/ray
that referenced
this pull request
Jun 27, 2025
## Why are these changes needed? Fix `test_batching` for windows. --------- Signed-off-by: Arthur <atte.book@gmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Why are these changes needed?
Related issue number
Checks
git commit -s) in this PR.scripts/format.shto lint the changes in this PR.method in Tune, I've added it in
doc/source/tune/api/under thecorresponding
.rstfile.