[Serve] Unify test synchronization patterns using shared actor primitives#62191
Conversation
![gemini-code-assist[bot]](https://waps.l3s.uni-hannover.de/live/im_/https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request centralizes test utility actors—Barrier, Accumulator, and SharedFlag—into ray/serve/_private/test_utils.py and refactors several test suites to use these shared components, replacing local actor definitions and file-based synchronization. The review feedback identifies a missing asyncio import in the utility file that would cause a runtime error and suggests adding comprehensive type hints to the new classes to improve code consistency and clarity.
| def __init__(self, n: int): | ||
| self._n = n | ||
| self._count = 0 | ||
| self._event = asyncio.Event() |
There was a problem hiding this comment.
The Barrier class uses asyncio.Event, but asyncio does not appear to be imported in this file. This will cause a NameError at runtime. Please add import asyncio at the top of the file.
| self._count = 0 | ||
| self._event = asyncio.Event() | ||
|
|
||
| async def wait(self): |
There was a problem hiding this comment.
For consistency with other methods, please add a -> None return type hint to this async method.
| async def wait(self): | |
| async def wait(self) -> None: |
| def __init__(self): | ||
| self._items = [] | ||
|
|
||
| def add(self, item): |
There was a problem hiding this comment.
To improve type safety and for consistency with other type-hinted methods, please add a type hint for the item parameter.
| def add(self, item): | |
| def add(self, item: Any): |
| def add(self, item): | ||
| self._items.append(item) | ||
|
|
||
| def get(self) -> list: |
There was a problem hiding this comment.
For better type specificity, consider changing the return type hint from list to list[Any]. This is more informative and aligns with modern Python typing practices.
| def get(self) -> list: | |
| def get(self) -> list[Any]: |
| def get(self) -> list: | ||
| return list(self._items) | ||
|
|
||
| def set_once(self, item) -> bool: |
There was a problem hiding this comment.
For consistency, please add a type hint for the item parameter.
| def set_once(self, item) -> bool: | |
| def set_once(self, item: Any) -> bool: |
| return True | ||
| return False | ||
|
|
||
| def get_first(self): |
There was a problem hiding this comment.
Please add a return type hint to this method for consistency. Based on the docstring, Optional[Any] would be appropriate. Note that this and other type hint suggestions for this class will require importing Any and Optional from typing.
| def get_first(self): | |
| def get_first(self) -> Optional[Any]: |
| def __init__(self): | ||
| self._value = False | ||
|
|
||
| def set(self): |
There was a problem hiding this comment.
For consistency, please add a -> None return type hint to this method.
| def set(self): | |
| def set(self) -> None: |
| def set(self): | ||
| self._value = True | ||
|
|
||
| def clear(self): |
There was a problem hiding this comment.
For consistency, please add a -> None return type hint to this method.
| def clear(self): | |
| def clear(self) -> None: |
win5923
force-pushed
the
unify-serve-test-synchronization
branch
3 times, most recently
from
d203441 to
b329778
Compare
…ives Signed-off-by: win5923 <ken89@kimo.com>
win5923
force-pushed
the
unify-serve-test-synchronization
branch
from
b329778 to
9ed8b17
Compare
ray-gardener
bot
added
serve
abrarsheikh
left a comment
abrarsheikh
left a comment
There was a problem hiding this comment.
this is a solid refactor, tysm
2 participants
Description
Add reusable sync primitives to serve test_utils and migrate inline/file-based sync patterns to use them.
Related issues
Closes #61387
Additional information