-
Notifications
You must be signed in to change notification settings - Fork 1k
Python: Fix MCPStreamableHTTPTool to use new streamable_http_client API #3088
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
base: main
Are you sure you want to change the base?
Conversation
|
@copilot python/packages/core/agent_framework/_mcp.py:1002 since this is optional, I would prefer the setup to be: 1) optional argument for the tool with type |
Done. Both changes implemented in commit 9bf17fd:
|
|
@copilot set the dependency on the |
|
@copilot and <2 to prevent auto upgrade when they release the v2 version. |
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.
Pull request overview
This PR migrates MCPStreamableHTTPTool from the deprecated streamablehttp_client() API to the new streamable_http_client() API, introducing an optional http_client parameter to allow users to provide their own httpx.AsyncClient instances. However, the implementation has critical bugs that break existing functionality.
Key Changes
- Replaced deprecated
streamablehttp_clientimport withstreamable_http_client - Added optional
http_client: httpx.AsyncClient | Noneparameter to allow user-provided clients - Simplified
get_mcp_client()to delegate client management to the MCP SDK
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| python/packages/core/agent_framework/_mcp.py | Migrated to new API, added http_client parameter, simplified get_mcp_client(), and added close() override. CRITICAL: headers, timeout, and sse_read_timeout are no longer applied. |
| python/packages/core/tests/core/test_mcp.py | Updated test to verify new API parameters and added test for optional client parameter behavior. Test verifies broken behavior instead of correct behavior. |
eavanvalkenburg
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.
eavanvalkenburg
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.
|
@copilot the cicd fails, there are extra imports in the tests that shouldn't be there, and you need to update the uv.lock file |
Fixed in commit 8478805:
Regarding uv.lock: I cannot run Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
8478805 to
a539880
Compare
Python Test Coverage Report •
Python Unit Test Overview
|
||||||||||||||||||||||||||||||
5a7edd9 to
7f91051
Compare
…proper httpx client cleanup Co-authored-by: eavanvalkenburg <[email protected]>
Co-authored-by: eavanvalkenburg <[email protected]>
…er and delegate client creation to streamable_http_client Co-authored-by: eavanvalkenburg <[email protected]>
…nt API support Co-authored-by: eavanvalkenburg <[email protected]>
…ng httpx client, add version constraint <2, and properly manage client lifecycle Co-authored-by: eavanvalkenburg <[email protected]>
…ms, remove kwargs, remove close() override per feedback Co-authored-by: eavanvalkenburg <[email protected]>
…not used) Co-authored-by: eavanvalkenburg <[email protected]>
Note: The uv.lock file needs to be updated with 'uv sync' to reflect the mcp version constraint change (>=1.24.0,<2) Co-authored-by: eavanvalkenburg <[email protected]>
f45507f to
a35b89d
Compare
Motivation and Context
In long-running Python processes (e.g., FastAPI servers), sequential
MCPStreamableHTTPToolinstances reused the same httpx client connection pool, causing 409 errors from Microsoft Foundry. The deprecatedstreamablehttp_client()API created clients that were never properly closed.Description
Core Changes:
Migrated from deprecated
streamablehttp_client()to newstreamable_http_client()API with a clean, simplified design:streamablehttp_clientfrom imports, keeping only the newstreamable_http_clientAPIhttp_client: httpx.AsyncClient | None = Noneparameter to__init__(), allowing users to optionally provide their own httpx clienthttp_clientparameter (which may be None) tostreamable_http_client():streamable_http_clientAPI creates and manages a default client internallyheaders,timeout, andsse_read_timeoutparameters - users who need custom HTTP configuration should create and pass their ownhttpx.AsyncClientinstance**kwargsparameter to accept (but not use) deprecated parameters, preventing errors when users haven't updated their codeclose()method since it doesn't create or manage clients itself>=1.24.0,<2to ensurestreamable_http_clientAPI is available and prevent auto-upgrade to v2uv.lockfile will need to be regenerated withuv syncto reflect the mcp dependency version changeBefore:
After:
Usage Examples:
Design Benefits:
Contribution Checklist
Original prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.