Feat: VideoDB Director support for MCP Client #162
Conversation
WalkthroughThis update introduces an MCP client module to manage connections and interactions with multiple MCP servers. A new Changes
Sequence Diagram(s)sequenceDiagram
participant RE as ReasoningEngine
participant MCP as MCPClient
participant Server as MCPServer
RE->>MCP: setup_mcp_servers()
MCP->>MCP: load_servers() %% Load JSON config
MCP->>Server: connect_to_server(name, config)
Server-->>MCP: Return tool list & status
MCP-->>RE: Aggregate and return tools
sequenceDiagram
participant RE as ReasoningEngine
participant MCP as MCPClient
participant Server as MCPServer
RE->>MCP: call_mcp_tool_sync(tool_name, tool_args)
MCP->>MCP: create_session(config)
MCP->>Server: call_tool(tool_name, tool_args)
Server-->>MCP: Return AgentResponse
MCP-->>RE: Deliver response
Poem
Tip ⚡🧪 Multi-step agentic review comment chat (experimental)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
backend/mcp_servers.json (1)
13-15: Consider segregating secrets from configuration files.
Storing your GitHub personal access token in a JSON file (even as a placeholder) can be risky if it ever gets committed into version control or shared inadvertently. Leveraging a secrets manager or protected environment variables (instead of including them in the committed config) typically results in a more secure architecture.backend/director/core/reasoning.py (1)
114-115: Remove unnecessary re-initialization.
There's no need to setself.mcp_client = Noneimmediately before assigning a newMCPClientinstance. You can remove line 114 for cleaner initialization.- self.mcp_client = None self.mcp_client = MCPClient()backend/director/core/mcp_client.py (2)
5-5: Remove unused import.
Theasynciomodule is not directly used in this file. You can remove the import to keep the code lean.-import asyncio🧰 Tools
🪛 Ruff (0.8.2)
5-5:
asyncioimported but unusedRemove unused import:
asyncio(F401)
84-115: Consider maintaining persistent sessions instead of creating a new session for each tool call.
Continually establishing new sessions for the same server introduces overhead and may complicate concurrency. Keeping a persistent session open for each server may be more efficient, depending on your usage patterns.Do you want me to outline a refactor for persistent session handling or open a new issue for future consideration?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
backend/director/core/mcp_client.py(1 hunks)backend/director/core/reasoning.py(4 hunks)backend/mcp_servers.json(1 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
backend/director/core/mcp_client.py
5-5: asyncio imported but unused
Remove unused import: asyncio
(F401)
| if loop and loop.is_running(): | ||
| task = loop.create_task(self.mcp_client.call_tool(tool_name, tool_args)) | ||
| return task.result() | ||
| else: | ||
| return asyncio.run(self.mcp_client.call_tool(tool_name, tool_args)) |
There was a problem hiding this comment.
Avoid race condition when returning task.result().
Calling task.result() immediately after creating the task can raise an exception if the task has not finished execution yet. You need to ensure you either await the task or run it synchronously. For a synchronous pattern, consider refactoring this method into an async method and performing an await on the task.
Below is an example of how you might refactor it into a fully asynchronous workflow (requires updating the caller to handle async calls):
-def call_mcp_tool_sync(self, tool_name, tool_args):
+async def call_mcp_tool_sync(self, tool_name, tool_args):
try:
logger.info(f"Calling MCP tool: {tool_name} with args: {tool_args}")
try:
loop = asyncio.get_running_loop()
except RuntimeError:
loop = None
if loop and loop.is_running():
- task = loop.create_task(self.mcp_client.call_tool(tool_name, tool_args))
- return task.result()
+ return await self.mcp_client.call_tool(tool_name, tool_args)
else:
return asyncio.run(self.mcp_client.call_tool(tool_name, tool_args))
except Exception as e:
...📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if loop and loop.is_running(): | |
| task = loop.create_task(self.mcp_client.call_tool(tool_name, tool_args)) | |
| return task.result() | |
| else: | |
| return asyncio.run(self.mcp_client.call_tool(tool_name, tool_args)) | |
| async def call_mcp_tool_sync(self, tool_name, tool_args): | |
| try: | |
| logger.info(f"Calling MCP tool: {tool_name} with args: {tool_args}") | |
| try: | |
| loop = asyncio.get_running_loop() | |
| except RuntimeError: | |
| loop = None | |
| if loop and loop.is_running(): | |
| return await self.mcp_client.call_tool(tool_name, tool_args) | |
| else: | |
| return asyncio.run(self.mcp_client.call_tool(tool_name, tool_args)) | |
| except Exception as e: | |
| ... |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (11)
backend/director/core/reasoning.py (5)
113-114: Storing MCP tools in a list is straightforward.
You might consider a dictionary or mapping keyed by tool name if you need faster lookups, but a list is fine for smaller tool sets.
115-115: Automated MCP server setup on initialization is convenient.
However, be mindful that callingasyncio.run()within a constructor may raise exceptions if there is already an active event loop. If your application sometimes runs inside an existing loop, consider using an async initialization method or a different approach.
120-128: Handling MCP server initialization via synchronous method.
You useasyncio.run(self.mcp_client.initialize_all_servers())in atry/except, which is good for error handling. Same caution as before regarding potential conflicts with existing loops.
245-247: Combining MCP tools with agent tools is sensible.
This approach cleanly merges them for presentation to the LLM. Ensure that any collisions in tool names are handled gracefully.
284-301: Tool dispatch logic is clear and explicit.
The distinction between MCP tools (sync call) and standard agent calls is well-defined. The fallback tostatus=AgentStatus.ERRORfor null returns is appropriate. Just keep in mind it may be worth capturing more detailed error messages or using a unified approach for calling all tools.backend/director/core/mcp_client.py (6)
10-13: Consider removing or customizinglogging.basicConfigin a library module.
Callinglogging.basicConfigat the library level affects global logging configuration and can override the end-user’s logging setup. Typically, this is done at the application entry point rather than inside a reusable library.
21-24: Loading server configurations requires error handling.
Currently, ifmcp_servers.jsonis missing, this will raise an unhandled exception. Consider a try/except around file I/O and JSON parsing to provide a clear, user-friendly error message or fallback scenario.
25-45: Session creation logic is clear but watch for potential command injection.
You rely onshutil.which(config['command'])to locate the executable, andconfig['command']might be user-provided. To prevent malicious commands, consider validating or sanitizing. Also ensure the environment variables inconfigare safe for your use case.Do you want a dedicated check for safe executables or a fallback strategy if
shutil.whichfails to find the command?
62-69: Initialization of all servers in sequence.
For performance with many servers, consider usingasyncio.gatherto parallelize connections if beneficial.
79-82: Checking for a tool name in a list is O(n).
For large tool sets, consider storing them in a dictionary keyed by tool name. This is a minor concern, but beneficial for scaling.
83-113: Re-creating a session on each tool call might be expensive.
To reduce overhead, you could cache sessions for each server, reusing them rather than creating new ones on every call. This is a good-to-have optimization if you expect many calls to the same server.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.gitignore(1 hunks)backend/director/constants.py(1 hunks)backend/director/core/mcp_client.py(1 hunks)backend/director/core/reasoning.py(5 hunks)
✅ Files skipped from review due to trivial changes (1)
- .gitignore
🔇 Additional comments (10)
backend/director/constants.py (1)
32-33: Great addition of a dedicated MCP server config constant.
DefiningMCP_SERVER_CONFIG_PATHclearly centralizes the configuration path, providing a single source of truth for MCP servers. This enhances maintainability and prevents hard-coded file paths across the codebase.backend/director/core/reasoning.py (5)
2-2: Importing asyncio is appropriate for the new async flows.
This import is necessary for your MCP async operations. No issues here.
18-18: MCPClient import looks good.
IntegratingMCPClientis aligned with your new MCP tool functionalities.
117-119: Private setter method for MCP tools is fine.
This provides clarity and modular control over how MCP tools get updated.
253-253: Passing combined tools to the LLM is correct.
This gathers all the tools in one place, simplifying the interface with the language model.
129-136:Details
❓ Verification inconclusive
Synchronous call to an async MCP method.
This is consistent with how you handle other MCP tasks. If your environment ever runs an existing loop, consider gracefully handling it by detecting the active loop and awaiting the task instead of running a new event loop.Please confirm whether your application ensures no event loop is running at this point. If uncertain, you can run a quick check in this method or refactor to fully async to avoid runtime errors.
🏁 Script executed:
#!/bin/bash # Searching for any existing references to running event loops rg "asyncio.get_running_loop"Length of output: 98
Verify event loop handling in
call_mcp_tool_sync.
The asynchronous MCP method invocation usingasyncio.runis consistent with how similar MCP tasks are handled. However, since our codebase contains a reference toasyncio.get_running_loop()(found inbackend/director/utils/asyncio.py), it's important to confirm that this synchronous method is never invoked while an event loop is active. If there’s any possibility thatcall_mcp_tool_syncmight be called in a context where an event loop is already running, please consider either verifying the absence of an active loop at runtime or refactoring this method to use an awaitable approach.
- Action Needed: Confirm that no event loop is running when
call_mcp_tool_syncis called or update the implementation to gracefully handle an active loop.backend/director/core/mcp_client.py (4)
1-4: Import statements look correct.
No immediate issues. The modulesjson,logging,shutil, andcontextlibare appropriate for file I/O, logging, and environment setup.
14-20: Well-structured MCPClient constructor.
Storing the config path, server definitions, and exit stack in__init__is logical. Keep in mind that if the JSON file is missing or invalid,load_serverswill raise an exception.Confirm whether the code should gracefully handle missing or malformed config to avoid early crashes. For instance, you could add fallback logic or a user-friendly alert.
46-61: Server connection method is properly structured.
The one-by-one tool retrieval from each server is fine. Logging for failures will help troubleshoot.
71-78: Converting tools to an LLM-friendly format is straightforward.
Ensure that any tool parameters remain valid JSON/dicts for the LLM.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (5)
backend/director/core/mcp_client.py (5)
1-11: Import structure looks good, but consider organizing them by category.The imports cover all necessary dependencies for MCP client implementation. For better readability, consider organizing imports into standard library, third-party, and local application groups with a blank line between each group.
import json import logging import os from typing import AsyncGenerator from contextlib import AsyncExitStack, asynccontextmanager + from mcp import ClientSession, StdioServerParameters from mcp.client.stdio import stdio_client, get_default_environment from mcp.client.sse import sse_client + from director.agents.base import AgentResponse, AgentStatus from director.constants import MCP_SERVER_CONFIG_PATH
16-21: Initialization looks good but consider better error handling for config loading.The initialization is straightforward, setting up necessary attributes. However, the
load_servers()call during initialization might raise exceptions (e.g., JSON parsing errors) that aren't handled here.def __init__(self): self.config_path = MCP_SERVER_CONFIG_PATH - self.servers = self.load_servers() + try: + self.servers = self.load_servers() + except json.JSONDecodeError as e: + logger.error(f"Failed to parse MCP server config: {e}") + self.servers = {} self.mcp_tools = [] self.exit_stack = AsyncExitStack()
29-86: Well-structured async context manager with proper error handling.The
create_sessionmethod is well-implemented as an async context manager with appropriate validation and error handling for different transport types. Consider adding type hints for parameters to improve code readability and add docstring for better documentation.@asynccontextmanager async def create_session( self, - server_name, - config + server_name: str, + config: dict ) -> AsyncGenerator[ClientSession, None]: + """ + Creates and manages a session with an MCP server. + + Args: + server_name: Name of the server to connect to + config: Server configuration dictionary with transport details + + Yields: + ClientSession: Active session with the MCP server + + Raises: + ValueError: If server configuration is invalid or transport is unsupported + """ if server_name not in self.servers: raise ValueError(f"Server '{server_name}' not found in configuration.")
1-168: Add type hints to all methods for better code readability and maintainability.The code would benefit from comprehensive type hints for all method parameters and return values. This improves code readability, aids IDE auto-completion, and helps catch type-related errors early.
For example:
- def load_servers(self): + def load_servers(self) -> dict: - async def connect_to_server(self, name, config): + async def connect_to_server(self, name: str, config: dict) -> list: - def mcp_tools_to_llm_format(self): + def mcp_tools_to_llm_format(self) -> list[dict]: - def is_mcp_tool_call(self, name): + def is_mcp_tool_call(self, name: str) -> bool: - async def call_tool(self, tool_name, tool_args): + async def call_tool(self, tool_name: str, tool_args: dict) -> AgentResponse:
15-167: Consider implementing reconnection logic or connection pooling.The current implementation creates and closes connections for each operation, which could be inefficient for frequent tool calls. Consider implementing connection pooling or reconnection logic for better performance, especially in high-traffic scenarios.
This would involve maintaining a pool of active connections to frequently used servers and reusing them instead of creating new connections for each tool call. This is a more complex change that would require restructuring the class, but could significantly improve performance for applications with many tool calls.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/director/core/mcp_client.py(1 hunks)backend/director/core/reasoning.py(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/director/core/reasoning.py
🔇 Additional comments (1)
backend/director/core/mcp_client.py (1)
160-163:⚠️ Potential issueError status should be ERROR, not SUCCESS.
When returning an error response in the exception handler, the status is incorrectly set to SUCCESS.
return AgentResponse( - status=AgentStatus.SUCCESS, + status=AgentStatus.ERROR, message=f"Error calling tool '{tool_name}': {e}", data={} )Likely an incorrect or invalid review comment.
1 participant
Summary by CodeRabbit
New Features
Chores