[serve] add result(...) to DeploymentResponseGenerator to fix static typing#58522
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request correctly addresses a static typing issue by adding a .result() method to DeploymentResponseGenerator. The implementation, which raises a TypeError at runtime, is a good approach to guide users to the correct API usage for streaming responses. The change is logical and well-implemented. My main feedback is the lack of tests for this new behavior. I've added a comment with a suggestion for a test case to ensure this behavior is maintained and prevent future regressions.
| def result( | ||
| self, | ||
| *, | ||
| timeout_s: Optional[float] = None, | ||
| _skip_asyncio_check: bool = False, | ||
| ) -> Any: | ||
| """This method is not available on DeploymentResponseGenerator. | ||
|
|
||
| DeploymentResponseGenerator is returned when using streaming handles. | ||
| To get results, iterate over the generator using `for` (outside deployments) | ||
| or `async for` (inside deployments) instead of calling `.result()`. | ||
|
|
||
| Raises: | ||
| TypeError: Always raises, as this method is not supported for generators. | ||
| """ | ||
| raise TypeError( | ||
| "`DeploymentResponseGenerator` doesn't support `.result()`. " | ||
| "Use iteration instead: `for item in response` (outside deployments) " | ||
| "or `async for item in response` (inside deployments)." | ||
| ) |
There was a problem hiding this comment.
This is a good fix for the static typing issue. However, the change should be accompanied by a test case to ensure that calling .result() on a DeploymentResponseGenerator raises a TypeError with the expected message. This will prevent future regressions.
You could add a test in python/ray/serve/tests/test_handle_streaming.py within TestAppHandleStreaming, for example:
def test_result_on_generator_raises_error(self, serve_instance, deployment: Deployment):
h = serve.run(deployment.bind()).options(stream=True)
gen = h.remote(5)
with pytest.raises(TypeError, match="doesn't support `.result()`"):
gen.result()
|
This pull request has been automatically marked as stale because it has not had You can always ask for help on our discussion forum or Ray's public slack channel. If you'd like to keep this open, just leave any comment, and the stale label will be removed. |
github-actions
bot
added
the
stale
|
please fix the premerge |
SolitaryThinker
force-pushed
the
will/serve_typing_generator
branch
from
cf28c60 to
ce0a877
Compare
|
looking into docs error |
github-actions
bot
added
unstale
SolitaryThinker
force-pushed
the
will/serve_typing_generator
branch
from
ce0a877 to
9eb64cb
Compare
…typing (ray-project#58522) ## Description This PR adds a dummy `.result()` API to DeploymentResponseGenerator`. `DeploymentResponseGenerator` currently doesn't support `.result()`, however when calling `.remote()` on a DeploymentHandle, the return type is a Union of `DeploymentResponseGenerator` and `DeploymentResponse`. Meaning if `.result()` is on the object, a typing error is raised. Example error without this PR: <img width="1838" height="1286" alt="image" src="https://github.com/user-attachments/assets/8f6a0b48-2926-4fa0-ad2f-1bd411ff7362" /> ## Related issues fixes ray-project#52493 ## Additional information > Optional: Add implementation details, API changes, usage examples, screenshots, etc. --------- Signed-off-by: will.lin <will.lin@anyscale.com> Signed-off-by: peterxcli <peterxcli@gmail.com>
2 participants
Description
This PR adds a dummy
.result()API to DeploymentResponseGenerator`.DeploymentResponseGeneratorcurrently doesn't support.result(), however when calling.remote()on a DeploymentHandle, the return type is a Union ofDeploymentResponseGeneratorandDeploymentResponse. Meaning if.result()is on the object, a typing error is raised.Example error without this PR:
Related issues
fixes #52493
Additional information