-
Notifications
You must be signed in to change notification settings - Fork 115
MCP server support #1021
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?
MCP server support #1021
Conversation
Also made cleanup changes
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 commented on the need to register MCP servers with the plugin.
Is there a way for us to support the context manager pattern as in the samples?
Regarding the implementation, it looks like one approach would be to do what the OpenAI Agents SDK does when defining servers, e.g. class MCPServerStdio(_MCPServerWithClientSession) and then supply an Would that also give you some nice things that |
Talked to Dan offline about the forked mcp client transport alternative. It's a good one, and something we should follow up on, but doesn't work for all mcp servers, which this PR is designed to support. |
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.
Nothing blocking
This approach is suitable for simple use cases where connection overhead is acceptable | ||
and you don't need to maintain state between operations. It is encouraged when possible as it provides | ||
a better set of durability guarantees that the stateful 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.
The docstrings in this PR are placing too much emphasis on connection overhead, and not quite being clear enough about when one would/would not use stateful/stateless.
It's not clear how often connection overhead matters given the round-trips to activity workers.
On the other hand, suppose a user is using an MCP server that must be used statefully, such as https://github.com/modelcontextprotocol/servers/tree/main/src/sequentialthinking. For that user we would want our docstrings to communicate that
(a) they must use the stateful version for correctness reasons, regardless of performance
(b) Temporal simply cannot make it "durable" since the MCP server stores state in volatile memory in an OS process.
To list out the cases:
-
MCP server retains state between calls in the same session and this is required for the server's functionality. In this case, using
StatelessTemporalServer
would be incorrect. -
MCP server retains state between calls, but this is not required for correct server functionality, and so user might choose not to make use of this and use
StatelessTemporalServer
. But how common is this -- do we have any examples where this makes sense? -
MCP server does not retain any state between calls. In this case they could use either of our server wrapper classes, and we recommend that they use
StatelessTemporalServer
.
Maybe some of the above is helpful for docstring content.
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 want to frame things in some of this way because some of it will only make sense with the current duality of server, but not necessarily be accurate in future. I'll go back over the docstrings, but I want the servers to state their limitations, not the scenarios in which they must be used.
What was changed
Added two mechanisms for supporting
MCPServer
. One is a stateless model which has the best durability guarantees but is not able to maintain call state if required. The other is stateful, but requires the user to handle cases where it fails, since all the calls need to happen on the same worker, which could potentially fail.Why?
Users would like to use MCP servers without hosting them accessible to openai's servers
Checklist
Closes
How was this tested:
A whole set of new tests
Any docs updates needed?
Yes, readme updates should be made once review has progressed.