-
Notifications
You must be signed in to change notification settings - Fork 2.1k
fix: nullable types with gen ai bump #3280
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
fix: nullable types with gen ai bump #3280
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 @MarlzRana, 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 refines the schema sanitization process for Gemini, primarily focusing on how nullable types are interpreted and converted. It includes an update to 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
|
|
Response from ADK Triaging Agent Hello @MarlzRana, thank you for your contribution! Before we can review this PR, please address the following items from our contribution guidelines:
You can find more details in our contribution guide. Thank you! |
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 updates the google-genai dependency and refactors the schema sanitization logic to leverage improvements in the new version. The removal of the custom _sanitize_schema_type function simplifies the codebase, and the updated logic now correctly handles nullable and multi-typed fields by using anyOf, which is a significant improvement over the previous implementation. The test suite has been updated accordingly with new tests for nullable anyOf cases and adjustments to existing tests to reflect the more accurate schema conversion. The changes are well-implemented and improve both correctness and maintainability.
11623cb to
73b1c6c
Compare
This commit contains detailed analysis materials for investigating and reviewing the nullable fields bug with Google GenAI bump. Analysis Materials: - issue_3281_analysis.md: Root cause analysis and detailed bug flow - pr_3280_review.md: Comprehensive review of the proposed fix Test Scripts (for reproduction and validation): - test_nullable_issue.py: Basic reproduction of the Pydantic schema issue - test_sanitize_issue.py: Demonstrates the problematic transformation - test_exact_adk_flow.py: Traces the complete ADK processing flow - test_anyof_unwrap.py: Shows google-genai can unwrap correctly - test_unwrap_with_description.py: Tests unwrapping with metadata - test_gemini_api_error.py: Analyzes the schema structure problem - test_fix_proposal.py: Initial fix approach (tracking anyOf context) - test_pr_3280_fix.py: Validates the PR #3280 approach - test_pr_edge_cases.py: Comprehensive edge case testing Key Findings: - Root cause: _sanitize_schema_type transforms {"type": "null"} to {"type": ["object", "null"]} which breaks google-genai unwrapping - PR #3280 correctly fixes this by removing _sanitize_schema_type entirely and deferring to google-genai>=1.45.0 - All edge cases are properly handled by the new approach - The fix is minimal, clean, and improves type handling overall Recommendation: APPROVE PR #3280 Related: #3281, PR #3280
|
@speedstorm1 will take a look at this |
|
Thank you @speedstorm1, this PR is merged now. |
Link to Issue or Description of Change
1. Link to an existing issue (if applicable):
Problem:
In 1.45.0 of the
google-genaiPython SDK release, changes were made to handle null types out of the box. This conflicts with the code in thegoogle-adkto handle this, and causes the below exception:This is what the Gemini schema looks like when they collide:
As one can observe, this schema does have fields specified alongside
any_ofand it is also incorrect logically:nullableany_of, to define an extra schema for nullInstead, we should be seeing the absence of
any_ofwhere we have a nullable type. Instead we should see thetypespecified for the type, andnullableset to true.The line in the code that cause this is:
adk-python/src/google/adk/tools/_gemini_schema_util.py
Lines 96 to 97 in 0dbfb43
It doesn't correctly handle the situation were we define a
nullableusinganyOf(where one type is the non-nullable type, and the other is thenulltype), as it's recursion operates on the individualtypeproperty level - it does not consider atypevalue in the context of another, as one has withanyOf.Solution:
Let's defer to the changes to handle null types properly (one of the mechanisms being flattening an
any_oftype pair with one element being null).This results in the correct Gemini schema being produced:
Testing Plan
Unit Tests:
Manual End-to-End (E2E) Tests:
I utilized the ADK web, and no longer observed the error.
Checklist
Additional context
This was quite a tricky issue to debug, but here are the notes I took:
I hope this helps and I look forward to hearing back from you soon! 😄