fix: handle Ray Serve streaming response generator correctly (#190)#191
Merged
Conversation
WalkthroughThe PR refines async streaming normalization across the adapter and driver layers: both now correctly distinguish async-iterable responses from awaitables, awaiting only non-iterable awaitables and iterating async iterables directly. Tests validate that async-iterable streams are not inadvertently awaited. ChangesAsync-iterable Streaming Normalization
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (4)
tests/adapter/test_local_adapter.py (2)
268-278: ⚡ Quick winAdd return type hint to the test function.
Per coding guidelines, "Include type hints on functions and methods in Python code."
♻️ Proposed fix
- async def test_call_func_stream_does_not_await_async_iterable_response(self): + async def test_call_func_stream_does_not_await_async_iterable_response(self) -> None:🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/adapter/test_local_adapter.py` around lines 268 - 278, The test function test_call_func_stream_does_not_await_async_iterable_response is missing a return type hint; update its signature to include a return annotation (-> None) per the project's type-hinting guidelines so the function reads with an explicit return type, keeping the rest of the body unchanged and ensuring any imports or typing requirements are already satisfied.Source: Coding guidelines
12-27: ⚡ Quick winAdd type hints to the test helper class.
Per coding guidelines, "Include type hints on functions and methods in Python code." The
AwaitableAsyncStreamhelper methods should include type hints for parameters and return values.♻️ Proposed fix to add type hints
+from typing import Iterator, NoReturn + class AwaitableAsyncStream: - def __init__(self, chunks): + def __init__(self, chunks: list[str]) -> None: self._chunks = iter(chunks) - def __await__(self): + def __await__(self) -> NoReturn: raise RuntimeError("stream response should not be awaited") yield # pragma: no cover - def __aiter__(self): + def __aiter__(self) -> "AwaitableAsyncStream": return self - async def __anext__(self): + async def __anext__(self) -> str: try: return next(self._chunks) except StopIteration as exc:🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/adapter/test_local_adapter.py` around lines 12 - 27, The AwaitableAsyncStream test helper lacks type hints; add annotations to the class and its methods: annotate __init__(self, chunks: Iterable[Any]) -> None, __await__(self) -> Generator[None, None, Any] (or -> Any with appropriate Generator type), __aiter__(self) -> "AwaitableAsyncStream" (or -> AsyncIterator[Any]), and async def __anext__(self) -> Any (or -> Any) and import any needed typing names (Iterable, Any, AsyncIterator, Generator) to satisfy the coding guidelines and type checkers while keeping behavior unchanged.Source: Coding guidelines
tests/driver/test_ingress.py (2)
209-214: ⚡ Quick winAdd return type hint to the test function.
Per coding guidelines, "Include type hints on functions and methods in Python code."
♻️ Proposed fix
-async def test_register_route_stream_does_not_await_async_iterable_response(ingress, mock_app): +async def test_register_route_stream_does_not_await_async_iterable_response(ingress, mock_app) -> None:🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/driver/test_ingress.py` around lines 209 - 214, Add a return type hint to the test function test_register_route_stream_does_not_await_async_iterable_response by annotating its signature with "-> None" so it complies with the project's typing guidelines; update the def for test_register_route_stream_does_not_await_async_iterable_response (the one that constructs AwaitableAsyncStream, calls register_stream_endpoint and asserts collect_stream_response) to include the return type hint.Source: Coding guidelines
191-207: ⚡ Quick winAdd type hints and consider extracting to shared test utility.
Per coding guidelines, "Include type hints on functions and methods in Python code." The
AwaitableAsyncStreamhelper should include type hints.Additionally, this helper is duplicated from
tests/adapter/test_local_adapter.py(lines 12-27). Consider extracting it to a shared test utility module to reduce duplication and improve maintainability.♻️ Proposed fix for type hints
+from typing import Iterator, NoReturn + class AwaitableAsyncStream: - def __init__(self, chunks): + def __init__(self, chunks: list[str]) -> None: self._chunks = iter(chunks) - def __await__(self): + def __await__(self) -> NoReturn: raise RuntimeError("stream response should not be awaited") yield # pragma: no cover - def __aiter__(self): + def __aiter__(self) -> "AwaitableAsyncStream": return self - async def __anext__(self): + async def __anext__(self) -> str: try: return next(self._chunks) except StopIteration as exc:For the duplication concern, consider creating
tests/helpers.pyortests/conftest.pyand moving the helper there.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/driver/test_ingress.py` around lines 191 - 207, Add type hints to the AwaitableAsyncStream helper and move it to a shared test utility to avoid duplication: update the class AwaitableAsyncStream and its methods (__init__, __await__, __aiter__, __anext__) to include precise typing (e.g., accept Iterable[bytes] or Iterable[str] for chunks, annotate return types Optional[Iterator], AsyncIterator, etc.), and then extract the class into a common test utility module and import it from both tests so the duplicate definition in tests/driver/test_ingress.py and tests/adapter/test_local_adapter.py is removed.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/adapter/test_local_adapter.py`:
- Around line 268-278: The test function
test_call_func_stream_does_not_await_async_iterable_response is missing a return
type hint; update its signature to include a return annotation (-> None) per the
project's type-hinting guidelines so the function reads with an explicit return
type, keeping the rest of the body unchanged and ensuring any imports or typing
requirements are already satisfied.
- Around line 12-27: The AwaitableAsyncStream test helper lacks type hints; add
annotations to the class and its methods: annotate __init__(self, chunks:
Iterable[Any]) -> None, __await__(self) -> Generator[None, None, Any] (or -> Any
with appropriate Generator type), __aiter__(self) -> "AwaitableAsyncStream" (or
-> AsyncIterator[Any]), and async def __anext__(self) -> Any (or -> Any) and
import any needed typing names (Iterable, Any, AsyncIterator, Generator) to
satisfy the coding guidelines and type checkers while keeping behavior
unchanged.
In `@tests/driver/test_ingress.py`:
- Around line 209-214: Add a return type hint to the test function
test_register_route_stream_does_not_await_async_iterable_response by annotating
its signature with "-> None" so it complies with the project's typing
guidelines; update the def for
test_register_route_stream_does_not_await_async_iterable_response (the one that
constructs AwaitableAsyncStream, calls register_stream_endpoint and asserts
collect_stream_response) to include the return type hint.
- Around line 191-207: Add type hints to the AwaitableAsyncStream helper and
move it to a shared test utility to avoid duplication: update the class
AwaitableAsyncStream and its methods (__init__, __await__, __aiter__, __anext__)
to include precise typing (e.g., accept Iterable[bytes] or Iterable[str] for
chunks, annotate return types Optional[Iterator], AsyncIterator, etc.), and then
extract the class into a common test utility module and import it from both
tests so the duplicate definition in tests/driver/test_ingress.py and
tests/adapter/test_local_adapter.py is removed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6a82584b-1285-4ebb-a0e6-a1f58e653113
📒 Files selected for processing (4)
src/framex/adapter/base.pysrc/framex/driver/ingress.pytests/adapter/test_local_adapter.pytests/driver/test_ingress.py
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Summary by CodeRabbit