-
-
Notifications
You must be signed in to change notification settings - Fork 11.7k
[Feature][Responses API]Support MCP tools with streaming mode + background mode #23927
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Feature][Responses API]Support MCP tools with streaming mode + background mode #23927
Conversation
Signed-off-by: wuhang <[email protected]>
Signed-off-by: wuhang <[email protected]>
Signed-off-by: wuhang <[email protected]>
Signed-off-by: wuhang <[email protected]>
Signed-off-by: wuhang <[email protected]>
Signed-off-by: wuhang <[email protected]>
Signed-off-by: wuhang <[email protected]>
Signed-off-by: wuhang <[email protected]>
Signed-off-by: wuhang <[email protected]>
|
cc @heheda12345 |
Signed-off-by: wuhang <[email protected]>
heheda12345
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your contribution. I left some comments. And can you add a test in https://github.com/vllm-project/vllm/blob/main/tests/entrypoints/openai/test_response_api_with_harmony.py?
|
/gemini review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request adds support for MCP tools with streaming and background modes in the Responses API. The changes involve updating the retrieve_responses API endpoint to handle streaming and introducing new logic to manage background streaming requests.
My review focuses on two main points:
- A potential deadlock in the new background stream generator due to a race condition. I've provided a suggestion to fix this.
- A memory leak in the new
event_store, which could be problematic in production.
Overall, the changes look good and align with the feature goal, but these two issues should be addressed.
Signed-off-by: wuhang <[email protected]>
Signed-off-by: wuhang <[email protected]>
Signed-off-by: wuhang <[email protected]>
Signed-off-by: wuhang <[email protected]>
heheda12345
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thanks for the great job!
…round mode (vllm-project#23927) Signed-off-by: wuhang <[email protected]>
…round mode (vllm-project#23927) Signed-off-by: wuhang <[email protected]>
Purpose
Fix #23295 especially for streaming response
Test Plan
Test Result
streaming response
Essential Elements of an Effective PR Description Checklist
supported_models.mdandexamplesfor a new model.