Sandbox 0119 #712
Sandbox 0119 #712
Conversation
Summary of ChangesHello @kevinfeng030, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a significant optimization to how MCP (Multi-Component Platform) server connections are managed within the sandbox environment. By adding a 'reuse' mode, the system can now cache and reuse server instances for subsequent tool calls, which is expected to reduce connection overhead and improve the efficiency of tool interactions. This change involves new utility functions for tool description transformation and invocation, updates to the sandbox's configuration to enable this feature, and conditional logic in the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a reuse mode for MCP server connections to improve performance by caching and reusing server instances. The changes are implemented across the sandbox and MCP client utilities. While the feature is a good addition, the current implementation has some significant issues. My review highlights a critical performance bug due to a blocking I/O call in an async function, several instances of code duplication that impact maintainability, and violations of best practices such as using global variables and overly broad exception handling. Addressing these points will improve the code's robustness, performance, and long-term maintainability.
| except Exception as e: | ||
| logger.warning(f"server_name:{server_name} translate failed: {e}") | ||
| elif "api" == server_config.get("type", ""): | ||
| api_result = requests.get(server_config["url"] + "/list_tools") |
There was a problem hiding this comment.
The use of requests.get() here is a synchronous, blocking network call within an async function. This will block the event loop and severely degrade the performance of your asynchronous application. You should use an asynchronous HTTP client, such as httpx, for all network requests in async code.
You will need to add import httpx at the top of the file.
| api_result = requests.get(server_config["url"] + "/list_tools") | |
| api_result = await httpx.get(server_config["url"] + "/list_tools") |
| async def mcp_tool_desc_transform_v2_reuse( | ||
| tools: List[str] = None, mcp_config: Dict[str, Any] = None, context: Context = None, | ||
| server_instances: Dict[str, Any] = None, | ||
| black_tool_actions: Dict[str, List[str]] = None, | ||
| sandbox_id: Optional[str] = None, | ||
| tool_actions: Optional[List[str]] = None | ||
| ) -> List[Dict[str, Any]]: |
There was a problem hiding this comment.
This new function mcp_tool_desc_transform_v2_reuse is almost a complete duplicate of mcp_tool_desc_transform_v2. This significant code duplication makes maintenance difficult, as any changes or bug fixes will need to be applied in both places.
Please consider refactoring to reduce this duplication. A good approach would be to extract the common logic for parsing server configurations into a shared helper function. This function could be called by both mcp_tool_desc_transform_v2 and mcp_tool_desc_transform_v2_reuse, each then handling their specific logic for server connection management (one using AsyncExitStack, the other using the cache).
| global MCP_SERVERS_CONFIG | ||
| MCP_SERVERS_CONFIG = config |
There was a problem hiding this comment.
Modifying a global variable (MCP_SERVERS_CONFIG) within a function is a practice that should be avoided. It creates hidden dependencies and side effects, making the code harder to understand, test, and debug. The configuration should be passed explicitly as a parameter to the functions that require it. It appears the functions that might use this global already accept an mcp_config parameter, making this global assignment likely redundant.
| global MCP_SERVERS_CONFIG | |
| MCP_SERVERS_CONFIG = config | |
| # global MCP_SERVERS_CONFIG | |
| # MCP_SERVERS_CONFIG = config |
| if _mcp_openai_tools: | ||
| mcp_openai_tools.extend(_mcp_openai_tools) | ||
| logger.info(f"✅ server ({server_name}) connected success") | ||
| except BaseException as err: |
There was a problem hiding this comment.
Catching BaseException is too broad as it includes system-exiting exceptions like KeyboardInterrupt and SystemExit. This can prevent the application from shutting down cleanly. It's better to catch the more specific Exception to handle application-level errors while allowing system exceptions to propagate.
| except BaseException as err: | |
| except Exception as err: |
| # Success, break out of retry loop | ||
| break | ||
|
|
||
| except (asyncio.TimeoutError, BaseException) as e: |
There was a problem hiding this comment.
Catching BaseException is too broad and can mask critical system-level issues. It includes exceptions like SystemExit and KeyboardInterrupt which should generally not be caught. Please catch Exception instead to handle standard application errors.
| except (asyncio.TimeoutError, BaseException) as e: | |
| except (asyncio.TimeoutError, Exception) as e: |
| if self._should_reuse(): | ||
| self.tool_list = await mcp_tool_desc_transform_v2_reuse( | ||
| tools=self.mcp_servers, | ||
| mcp_config=self.mcp_config, | ||
| context=context, | ||
| server_instances=self.server_instances, | ||
| black_tool_actions=self.black_tool_actions, | ||
| sandbox_id=sandbox_id, | ||
| tool_actions=self.tool_actions | ||
| ) | ||
| else: | ||
| self.tool_list = await mcp_tool_desc_transform_v2( | ||
| tools=self.mcp_servers, | ||
| mcp_config=self.mcp_config, | ||
| context=context, | ||
| server_instances=self.server_instances, | ||
| black_tool_actions=self.black_tool_actions, | ||
| sandbox_id=sandbox_id, | ||
| tool_actions=self.tool_actions | ||
| ) |
There was a problem hiding this comment.
The if/else block contains duplicated code, as the arguments passed to mcp_tool_desc_transform_v2_reuse and mcp_tool_desc_transform_v2 are identical. You can make this more concise and maintainable by selecting the function first and then calling it once.
transform_func = (
mcp_tool_desc_transform_v2_reuse
if self._should_reuse()
else mcp_tool_desc_transform_v2
)
self.tool_list = await transform_func(
tools=self.mcp_servers,
mcp_config=self.mcp_config,
context=context,
server_instances=self.server_instances,
black_tool_actions=self.black_tool_actions,
sandbox_id=sandbox_id,
tool_actions=self.tool_actions,
)| if self._should_reuse(): | ||
| # Reuse mode: use cached server instances (delegated to utils.py) | ||
| call_result_raw = await call_mcp_tool_with_reuse( | ||
| server_name=server_name, | ||
| tool_name=tool_name, | ||
| parameter=parameter, | ||
| server_instances=self.server_instances, | ||
| mcp_config=self.mcp_config, | ||
| context=context, | ||
| sandbox_id=sandbox_id, | ||
| progress_callback=progress_callback, | ||
| max_retry=3, | ||
| timeout=120.0 | ||
| ) | ||
|
|
||
| if not call_result_raw: | ||
| call_mcp_e = Exception("Failed to call tool after all retry attempts") | ||
| else: | ||
| # Non-reuse mode: use AsyncExitStack (delegated to utils.py) | ||
| call_result_raw = await call_mcp_tool_with_exit_stack( | ||
| server_name=server_name, | ||
| tool_name=tool_name, | ||
| parameter=parameter, | ||
| mcp_config=self.mcp_config, | ||
| context=context, | ||
| sandbox_id=sandbox_id, | ||
| progress_callback=progress_callback, | ||
| max_retry=3, | ||
| timeout=120.0 | ||
| ) | ||
|
|
||
| if not call_result_raw: | ||
| call_mcp_e = Exception("Failed to call tool after all retry attempts") | ||
|
|
There was a problem hiding this comment.
The check for if not call_result_raw: is duplicated in both branches of the if self._should_reuse(): condition. You can refactor this to remove the duplication by moving the check to after the if/else block.
if self._should_reuse():
# Reuse mode: use cached server instances (delegated to utils.py)
call_result_raw = await call_mcp_tool_with_reuse(
server_name=server_name,
tool_name=tool_name,
parameter=parameter,
server_instances=self.server_instances,
mcp_config=self.mcp_config,
context=context,
sandbox_id=sandbox_id,
progress_callback=progress_callback,
max_retry=3,
timeout=120.0
)
else:
# Non-reuse mode: use AsyncExitStack (delegated to utils.py)
call_result_raw = await call_mcp_tool_with_exit_stack(
server_name=server_name,
tool_name=tool_name,
parameter=parameter,
mcp_config=self.mcp_config,
context=context,
sandbox_id=sandbox_id,
progress_callback=progress_callback,
max_retry=3,
timeout=120.0
)
if not call_result_raw:
call_mcp_e = Exception("Failed to call tool after all retry attempts")
No description provided.