Add `streamable_http_client` which accepts `httpx.AsyncClient` instead of `httpx_client_factory`

[Kludex]

The current name is not very intuitive considering the class name and the module.

[Kludex]

I blame copilot. 😄

[Kludex]

(it's probably a good time to replace httpx_client_factory with an object)

I think the timeouts, the auth and the httpx_client_factory CAN be replaced. The timeouts I think can be subjective, but the auth and the httpx_client_factory I think need to be replaced.

[ihrpr]

(it's probably a good time to replace httpx_client_factory with an object)

I think the timeouts, the auth and the httpx_client_factory CAN be replaced. The timeouts I think can be subjective, but the auth and the httpx_client_factory I think need to be replaced.

should we just replace it now, since we are deprecating one method, would be good to have a nice alternative and not to deprecate it twice?

[Kludex]

(it's probably a good time to replace httpx_client_factory with an object)

I think the timeouts, the auth and the httpx_client_factory CAN be replaced. The timeouts I think can be subjective, but the auth and the httpx_client_factory I think need to be replaced.

should we just replace it now, since we are deprecating one method, would be good to have a nice alternative and not to deprecate it twice?

I didn't get what you said. I agree that it's a good moment to change the parameters of the new streamable_http_client.

[dsp-ant]

Should we do this and deprecate the old module?

[felixweinberger]

FYI for viewers: on my todo list to update this

[felixweinberger]

@Kludex @ihrpr @dsp-ant gave this a shot, trying to replace the factory with a client object.

Results in some ugly header mangling that I don't see how to avoid given the existing structure. Keen for thoughts.

[fryguy04]

FWIW I pulled this PR into my venv and was able to connect to my self-signed MCP server - thx, looking forward to the merge into main

[felixweinberger]

@Kludex ready for another look

[felixweinberger]

@Kludex, @maxisbey finally ready for another look after we talked about removing the client argument from prepare_headers last week.

There's theoretically a potential for breakage because of the now deprecated arguments I commented on above, if anyone was using RequestContext or StreamableHTTPTransport directly for something. Like we discussed IIRC, none of our examples or documentation suggest using these classes directly. The only places they are used at the moment is in internal implementation details of streamable_http_client and streamablehttp_client.

streamablehttp_client should continue to work as before, as we've kept it around as @deprecated and convert the args.

[Kludex]

I didn't think this was going to be merged already. I've been thinking about this, and I'm unsure this is the cleanest approach.

I think this API is not going to last, I think the right approach is to create the MCPClient/Client that it will manage the lifecycle of the HTTP client there. Right now, most of the LLM providers are doing this thing of accepting the HTTP client, but I think what everybody should be doing is allowing to pass all the parameters (a bit contradictory to what was the motivation to this PR, since I'm saying the completely opposite).

I think the TestClient from Starlette is more or less how I see that happening (doesn't need to be with inheritance, it can be with composition).

[maxisbey]

I didn't think this was going to be merged already. I've been thinking about this, and I'm unsure this is the cleanest approach.

I think this API is not going to last, I think the right approach is to create the MCPClient/Client that it will manage the lifecycle of the HTTP client there. Right now, most of the LLM providers are doing this thing of accepting the HTTP client, but I think what everybody should be doing is allowing to pass all the parameters (a bit contradictory to what was the motivation to this PR, since I'm saying the completely opposite).

I think the TestClient from Starlette is more or less how I see that happening (doesn't need to be with inheritance, it can be with composition).

Hmm, could you make an issue for this for v2 with some thoughts? would be good to capture it when going through the refactoring

[felixweinberger]

I didn't think this was going to be merged already. I've been thinking about this, and I'm unsure this is the cleanest approach. I think this API is not going to last, I think the right approach is to create the MCPClient/Client that it will manage the lifecycle of the HTTP client there. Right now, most of the LLM providers are doing this thing of accepting the HTTP client, but I think what everybody should be doing is allowing to pass all the parameters (a bit contradictory to what was the motivation to this PR, since I'm saying the completely opposite). I think the TestClient from Starlette is more or less how I see that happening (doesn't need to be with inheritance, it can be with composition).

Hmm, could you make an issue for this for v2 with some thoughts? would be good to capture it when going through the refactoring

@maxisbey @Kludex made one here for discussion: https://github.com/modelcontextprotocol/python-sdk/issues/1773