-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Allow users to supply extra_http_client_args in MCPServerHTTP
#1925
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
Allow users to supply extra_http_client_args in MCPServerHTTP
#1925
Conversation
|
We've been using a hacky solution for this, this would be absolutely huge for my team. |
Yeah, it's feasible to monkey patch the underlying MCP |
|
@mpfaffenberger Thank you! What do you think of letting the user directly pass their own |
I'm happy to make that change if you want!! Let me cook it up really quick. |
…tp-args' into feature/mcp-http-server-extra-http-args
dmontagu
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.
Looks good to me but I defer to @Kludex
|
@DouweM Is this what you were hoping for? |
| if self.http_client and self.headers: | ||
| raise ValueError(f'In {self.__name__}, only one of `headers` or `http_client` can be provided.') | ||
|
|
||
| sse_client_partial = functools.partial( | ||
| sse_client, | ||
| url=self.url, | ||
| headers=self.headers, | ||
| timeout=self.timeout, | ||
| sse_read_timeout=self.sse_read_timeout, | ||
| ) as (read_stream, write_stream): | ||
| yield read_stream, write_stream | ||
| ) | ||
|
|
||
| if self.http_client is not None: | ||
|
|
||
| def httpx_client_factory( | ||
| headers: dict[str, str] | None = None, | ||
| timeout: httpx.Timeout | None = None, | ||
| auth: httpx.Auth | None = None, | ||
| ) -> httpx.AsyncClient: | ||
| return self.http_client # pyright: ignore | ||
|
|
||
| async with sse_client_partial(httpx_client_factory=httpx_client_factory) as (read_stream, write_stream): | ||
| yield read_stream, write_stream | ||
| else: | ||
| async with sse_client_partial(headers=self.headers) as (read_stream, write_stream): | ||
| yield read_stream, write_stream |
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.
I don't think this is what we want. We either want:
- Only allow
http_clientorheaders/timeout/auth. It would be nice to add antyping.overload. - Allow all parameters, and replace them on the factory we are creating.
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.
So for 2) let's say I run this code as a dev:
custom_client = httpx.AsyncClient(headers={"X-Api-Key": "12345"}, timeout=300)
headers = {
"X-Api-Key": "54321",
}
server = MCPServerHTTP(
http_client=custom_client,
timeout=5,
headers=headers
)How do we decide which parameters ultimately get passed to MCP's sse_client?
I think it's fine to pick one or the other as long as users get a warnings.warn or something.
I had discussed this briefly on Slack with @dmontagu
More than happy to continue iterating on this P/R btw. :)
edit: I realize this example is silly + contrived, but I expect users to do silly things :D
Kludex
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.
Let's start without the overload. It's fine.
55a4736 to
a993102
Compare
Hey everyone! Thanks for considering this P/R. The
extra_http_client_argswill allow us to send in extra config for the underlyinghttpx.AsyncClientsthat get spun up in the MCP connection pool. I personally need this to use Pydantic AI's MCP client tools in my corporate environment where I am required to use a self signed cert.