fix(plugins): Add 20MB file size validation to SaveFilesAsArtifactsPl…#3781
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 @AakashSuresh2003, 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 enhances the 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
|
There was a problem hiding this comment.
Code Review
This pull request effectively addresses the issue of unhandled large file uploads in SaveFilesAsArtifactsPlugin. By introducing a 20MB file size validation, it prevents cryptic errors and provides clear, actionable feedback to the user. The implementation is straightforward, and the accompanying unit tests are comprehensive, covering various edge cases. I have one minor suggestion to improve the maintainability of the error message construction. Overall, this is a great fix.
64a1208 to
85f7c7c
Compare
|
Hi @ryanaiagent, Could you please review my PR and provide any suggestions? |
|
Hi @AakashSuresh2003 I am not sure if we want to impose a 20MB limit, I was looking at this problem and thinking about using google file storage api. I think this would be a better direction if it works! |
|
Sure, Will start working on this direction |
|
Hi @GWeale , could you please review my PR when you have time? |
|
Hi @AakashSuresh2003 , we appreciate your patience and support. Can you please fix the failing unit tests before we can proceed with the review. |
…ugin with clear error messages and Files API guidance. Fixes google#3751
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request effectively addresses the issue of handling large file uploads in the SaveFilesAsArtifactsPlugin by introducing file size validation and routing to the Google Files API for files exceeding the inline data limit. The implementation is logical, and the addition of comprehensive unit tests is commendable. I've provided a few suggestions to enhance maintainability and robustness, including using the mimetypes library for better file extension handling, removing a hardcoded value in an error message, and refactoring a unit test to adhere to best practices.
|
Hi @AakashSuresh2003 , can you please address the suggestions. |
|
Sure @ryanaiagent |
…)" This reverts commit aa5d6a4.
…)" This reverts commit aa5d6a4.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a valuable fix for handling large file uploads in SaveFilesAsArtifactsPlugin. By routing files larger than 20MB through the Google Files API, it addresses a limitation of inline data uploads and provides much clearer error handling for users. The changes are well-structured and include comprehensive unit tests for various file size scenarios.
My review focuses on a few areas to improve the implementation:
- A misleading log message that could cause confusion during debugging.
- A performance improvement by avoiding repeated instantiation of the
Client. - Removing a hardcoded value from an error message to improve maintainability.
Overall, this is a solid contribution that significantly improves the user experience for file uploads.
aa5d6a4 to
e99625c
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request enhances the SaveFilesAsArtifactsPlugin to support larger file uploads by integrating with the Gemini Files API. Previously, files were handled as inline_data, but now files exceeding 20MB will be uploaded via the Files API, while files over 2GB will be rejected with an error message. The changes include adding os, tempfile, and google.genai.Client imports, defining new constants for maximum inline data and Files API sizes, and implementing a new _upload_to_files_api method to manage temporary file creation, upload, and cleanup. The on_user_message_callback method was updated to incorporate file size checks and conditionally route files to either the existing inline data handling or the new Files API upload process, including error handling for upload failures. Corresponding unit tests were added to cover various scenarios, including files exceeding the 20MB limit, files at the limit, mixed file sizes, Files API upload failures, and files exceeding the 2GB limit. Review comments suggest minor improvements such as reordering imports, making file size calculations more robust and efficient by avoiding repetition, promoting a mime_to_ext dictionary to a class-level constant, and refining test patching strategies for clarity and robustness.
…eFilesAsArtifactsPlugin with robust error handling and improved test mocking (google#3781)
Link to Issue or Description of Change
1. Link to an existing issue (if applicable):
Problem:
The
SaveFilesAsArtifactsPluginfails with a cryptic400 INVALID_ARGUMENTerror when users attempt to upload files larger than approximately 50MB. This error message provides no guidance to users on what went wrong or how to resolve the issue. According to the Gemini API documentation, inline_data uploads have a 20MB size limit, and files larger than this must use the Files API.Solution:
Added file-size routing and validation to automatically handle files of all sizes. The plugin now:
This prevents the cryptic
400 INVALID_ARGUMENTerror from reaching users and provides actionable guidance on how to handle large files properly.Testing Plan
test_file_size_exceeds_limit - Validates that a 50MB file is automatically uploaded via Files API instead of using inline_data
test_file_size_at_limit - Validates that a 20MB file (at the limit) uses inline_data as expected
test_file_size_just_over_limit - Validates that a 21MB file (just over the 20MB limit) is routed to Files API
test_mixed_file_sizes - Validates that multiple files with different sizes (small and large) are handled independently and correctly
test_files_api_upload_failure - Validates graceful error handling when Files API upload fails with proper error messages
test_file_exceeds_files_api_limit - Validates that a 3GB file is rejected with a clear error message about the 2GB limit, and Files API is not called
Unit Tests:
Please include a summary of passed
pytestresults.Manual End-to-End (E2E) Tests:
Created a Python script to test the plugin with different file sizes:
Test Results:
Test 1: Small file upload (5MB)
Result: Clear, actionable error message instead of
400 INVALID_ARGUMENT.Checklist
Additional context
Implementation Details:
_MAX_INLINE_DATA_SIZE_BYTES = 20 * 1024 * 1024(20MB) for inline_data limit_MAX_FILES_API_SIZE_BYTES = 2 * 1024 * 1024 * 1024(2GB) for Files API limitChanges Made:
src/google/adk/plugins/save_files_as_artifacts_plugin.py:tests/unittests/plugins/test_save_files_as_artifacts.py:test_file_size_exceeds_limit- Validates 50MB file uploads via Files APItest_file_size_at_limit- Validates 20MB file uses inline_datatest_file_size_just_over_limit- Validates 21MB file uses Files APItest_mixed_file_sizes- Validates mixed sizes handled independentlytest_files_api_upload_failure- Validates error handling for Files API failurestest_file_exceeds_files_api_limit- Validates 3GB file rejected with 2GB limitReference:
Before this fix:
Users received:
400 INVALID_ARGUMENT: {'error': {'code': 400, 'message': 'Request contains an invalid argument.', 'status': 'INVALID_ARGUMENT'}}After this fix:
[Upload Error: File huge_video.mp4 (3.00 GB) exceeds the maximum supported size of 2GB. Please upload a smaller file.]