-
Notifications
You must be signed in to change notification settings - Fork 2.8k
Add TTL for sandbox creation in AgentEngineSandboxCodeExecutor #4245
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?
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
Summary of ChangesHello @ArthurCRodrigues, 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 addresses a previously identified TODO by implementing Time-To-Live (TTL) functionality for sandboxes created by 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
|
|
Response from ADK Triaging Agent Hello @ArthurCRodrigues, thank you for creating this PR! This is a great feature enhancement. Could you please associate a GitHub issue with this PR? If there is no existing issue, could you please create one? This will help us to track the feature request. Also, it looks like you haven't signed the Contributor License Agreement (CLA) yet. Please make sure to sign it so we can proceed with reviewing your contribution. You can find more information about the CLA in the "Before you begin" section of our contribution guidelines. Thanks! |
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.
Code Review
This pull request successfully implements the TODO to add a Time-To-Live (TTL) for sandbox creation in AgentEngineSandboxCodeExecutor. The changes include adding a sandbox_ttl parameter to the initializer, updating the class and method docstrings, and passing the TTL value during sandbox creation. The implementation is supported by new unit tests that verify the behavior both with and without a TTL specified.
The code is well-structured and the changes are clear. I have one suggestion regarding the assignment of the sandbox_ttl attribute to improve consistency, but overall this is a solid contribution.
| ) | ||
| # @TODO - Add TTL for sandbox creation after it is available | ||
| # in SDK. | ||
| self.sandbox_ttl = sandbox_ttl |
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 sandbox_ttl attribute is only set when a new sandbox is created (i.e., when agent_engine_resource_name is provided). If an executor is initialized with an existing sandbox_resource_name, the sandbox_ttl parameter is ignored and self.sandbox_ttl remains None. This could be confusing for a user who provides the sandbox_ttl parameter in that case.
To improve consistency and predictability, consider setting self.sandbox_ttl = sandbox_ttl unconditionally at the beginning of the __init__ method. This would ensure the attribute always reflects the value passed to the constructor, even though it's only used during creation.
Please ensure you have read the contribution guide before creating a pull request.
Link to Issue or Description of Change
1. Link to an existing issue (if applicable):
2. Or, if no issue exists, describe the change:
There was a TODO at
agent_engine_sandbox_code_executor.py:If applicable, please follow the issue templates to provide as much detail as
possible.
Problem:
There was a TODO at
agent_engine_sandbox_code_executor.py:Solution:
Did the TODO :)
Testing Plan
Unit Tests:
Please include a summary of passed
pytestresults.Manual End-to-End (E2E) Tests:
Set up Vertex AI credentials:
Create a sandbox with TTL:
Verify the sandbox expires after the TTL period (or check in Vertex AI console)
Checklist
Additional context
Usage Example
TTL Format
The TTL follows the Google Cloud duration format:
"3600s"- 1 hour"86400s"- 1 day"604800s"- 1 week