Enhancement: update MCP example #75
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the MCP example into modular components, updates the pipeline to use a SQL-based chat history manager, and adds support for persisting messages.
- Introduce
McpStateandMcpStateKeysfor consistent state handling. - Extract response generation logic into
McpResponseSynthesizer. - Add
McpChatHistoryManagercomponent and wire up an SQL datastore.
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| examples/custom-pipeline/mcp_pipeline/state.py | Define McpState TypedDict and McpStateKeys enum |
| examples/custom-pipeline/mcp_pipeline/preset_config.py | Add mcp_server_url field to the preset config |
| examples/custom-pipeline/mcp_pipeline/pipeline.py | Refactor pipeline builder to use new components and datastore |
| examples/custom-pipeline/mcp_pipeline/components/response_synthesizer.py | Move response synthesizer logic into its own module |
| examples/custom-pipeline/mcp_pipeline/components/chat_history_manager.py | Implement chat history manager with write operation |
Comments suppressed due to low confidence (2)
examples/custom-pipeline/mcp_pipeline/preset_config.py:16
- [nitpick] Add a brief docstring description for
mcp_server_urlso users know what format or validation is expected.
mcp_server_url: str
examples/custom-pipeline/mcp_pipeline/components/chat_history_manager.py:18
- [nitpick] No tests cover the new chat history manager. Consider adding unit tests for both
readandwritepaths to ensure message persistence works as intended.
class McpChatHistoryManager(Component):
| role="user", | ||
| content=kwargs.get("query"), | ||
| parent_id=kwargs.get("parent_id") or kwargs.get("conversation_id"), | ||
| created_time=datetime.datetime.now(datetime.UTC), |
There was a problem hiding this comment.
Using datetime.UTC will raise an AttributeError; consider using datetime.timezone.utc or a valid timezone object.
| created_time=datetime.datetime.now(datetime.UTC), | |
| created_time=datetime.datetime.now(datetime.timezone.utc), |
| return SimpleState( | ||
| return McpState( | ||
| query=request.get("message"), | ||
| response=None, |
There was a problem hiding this comment.
McpState.response is declared as str but initialized to None. Either provide a default empty string or update the TypedDict to allow None.
changes: