Feat/code tool #756
Feat/code tool #756
Conversation
Summary of ChangesHello @alcholiclg, 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 significantly enhances the system's capability by integrating a comprehensive CodeExecutionTool. This tool enables secure and isolated execution of code within sandboxed environments, offering both stateful and stateless options. It provides a suite of specialized tools for various operations, from Python and shell command execution to file manipulation, and includes built-in resilience features like port conflict resolution and sandbox creation retries. Highlights
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 the CodeExecutionTool, enhancing the agent's capabilities by enabling secure code execution within isolated sandboxes. The tool supports both HTTP and local sandbox environments, accommodating docker and docker-notebook types. It includes several executor tools (notebook, python, shell, file operation, reset sandbox, get sandbox info), a retry mechanism for sandbox creation, and automatic port scanning. The changes also include documentation and dependency updates. The review focuses on correctness and maintainability, with suggestions for improved error handling and code clarity.
| elif info.status == SandboxStatus.ERROR: | ||
| error_msg = info.metadata.get('error', 'Unknown error') | ||
| raise RuntimeError(f'Sandbox failed to start: {error_msg}') |
There was a problem hiding this comment.
When the sandbox enters an error state, the code retrieves the error message from info.metadata.get('error', 'Unknown error'). However, it does not include the original exception or any other context about why the sandbox entered the error state. It would be helpful to log the entire info.metadata for debugging purposes.
| except OSError as e: | ||
| if e.errno == 98 or e.errno == 48: # Address already in use | ||
| return False # Port is occupied |
There was a problem hiding this comment.
The OSError exception handling only checks for errno 98 and 48, but there might be other relevant errno values indicating address already in use or permission issues. It would be more robust to check for a broader range of potential port binding errors.
Consider including EACCES (13) which indicates permission denied.
| return result != 0 # 0 means connection successful, port is occupied | ||
| except Exception as e: | ||
| logger.warning(f'Error checking port {port}: {e}') | ||
| return False # Be conservative: assume occupied if we can't check reliably |
There was a problem hiding this comment.
When the port check fails, the function conservatively assumes the port is occupied. However, the log message indicates that the check might be unreliable. It would be better to raise an exception or return a specific error code to allow the calling function to handle this ambiguous state appropriately, rather than assuming the port is occupied.
| logger.error( | ||
| f'Could not find available port in range {start_port}-{start_port + max_attempts - 1}' | ||
| ) |
There was a problem hiding this comment.
The log message indicates that no available port was found, but the function simply returns None. It would be more informative to raise an exception with a descriptive message, providing more context to the caller about why a port could not be found.
| except Exception as e: | ||
| error_msg = str(e).lower() | ||
| last_error = e | ||
|
|
There was a problem hiding this comment.
The code converts the exception e to a lowercase string (error_msg = str(e).lower()) for checking port conflict. This might lead to incorrect detection if the error message contains uppercase letters or if the relevant keywords are not consistently cased. A case-insensitive search would be more reliable.
error_msg = str(e).lower()
is_port_conflict = any(keyword in error_msg.lower()
ms_agent/tools/code/code_executor.py
Outdated
| raise RuntimeError( | ||
| f'Port conflict and no available ports found: {e}' | ||
| ) |
There was a problem hiding this comment.
If no available ports are found, a RuntimeError is raised, but the original exception e is not included in the new exception's message. It would be helpful to include the original exception for debugging purposes.
raise RuntimeError(
f'Port conflict and no available ports found: {e}'
) from e
ms_agent/tools/code/code_executor.py
Outdated
| raise RuntimeError( | ||
| f'Sandbox initialization failed after {max_retries} attempts: {last_error}' | ||
| ) |
There was a problem hiding this comment.
If the sandbox initialization fails after multiple retries, a RuntimeError is raised, but the original exception last_error is not chained to the new exception. This makes it harder to trace the root cause of the failure. Chain the original exception to preserve the context.
raise RuntimeError(
f'Sandbox initialization failed after {max_retries} attempts: {last_error}'
) from last_error
ms_agent/tools/code/code_executor.py
Outdated
| logger.error(f'Failed to initialize sandbox: {e}', exc_info=True) | ||
| raise RuntimeError(f'Sandbox initialization failed: {e}') |
There was a problem hiding this comment.
When the sandbox initialization fails, the code logs the exception with exc_info=True, which is good for debugging. However, the RuntimeError that is raised does not include the original exception as the cause. It would be better to chain the original exception to the RuntimeError to preserve the exception context.
raise RuntimeError(f'Sandbox initialization failed: {e}') from e
…at/code_tool Merge branch 'main' of https://github.com/modelscope/ms-agent into feat/code_tool
2 participants
Change Summary
Added full support for CodeExecutionTool (based on ms-enclave), enabling secure execution of code blocks in isolated sandboxes with file operation and shell command capabilities, significantly improving reliability and safety of code execution.
Related issue number
Checklist
pre-commit installandpre-commit run --all-filesbefore git commit, and passed lint check.