fix(tools): support Python 3.10+ pipe union syntax in function parameter parser#4806
fix(tools): support Python 3.10+ pipe union syntax in function parameter parser#4806Raman369AI wants to merge 1 commit into
Conversation
…ter parser The parameter parser only handled Union[X, Y] (typing.Union) for complex types like Optional[list[str]]. When a function used the modern | syntax (e.g. list[str] | None), the annotation resolved to types.UnionType and fell through to a ValueError, breaking the adk web builder assistant. Extend the isinstance guard and origin check in _parse_schema_from_parameter to also handle types.UnionType so that list[str] | None is parsed identically to Optional[list[str]]. Fixes google#3591
Summary of ChangesHello, 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 critical bug in the function parameter parser that prevented the correct interpretation of Python 3.10+ pipe union type hints, such as list[str] | None. By extending the parser's type recognition capabilities, it ensures that tools relying on automatic function calling, like the ADK web builder assistant, can properly process function signatures, thereby restoring functionality and improving compatibility with modern Python syntax. 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. Changelog
Activity
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 correctly fixes an issue where the function parameter parser did not support Python 3.10+ pipe union syntax (|). The changes appropriately extend the parser to handle types.UnionType. The new unit test effectively verifies the fix. I have one suggestion to improve the robustness of the logic for handling Optional[T] types, which is made more relevant by this change.
| _raise_if_schema_unsupported(variant, schema) | ||
| return schema | ||
| if origin is Union: | ||
| if origin in (Union, typing_types.UnionType): |
There was a problem hiding this comment.
While this change is correct to include UnionType, the subsequent logic for handling Optional[T] types (which results in len(schema.any_of) == 1) is a bit fragile and could be improved for robustness.
The current implementation at lines 369-371 only copies the type from the wrapped schema, and relies on a special-case handling for list types at lines 352-362 to copy the items. This means that for other complex Optional types, like Optional[pydantic.BaseModel], properties will be lost during the unwrapping.
Consider refactoring the unwrapping logic to be more generic and robust. For example:
# at lines 369-371
if len(schema.any_of) == 1:
# This is an Optional[T] type, so unwrap it.
unwrapped_schema = schema.any_of[0]
schema.type = unwrapped_schema.type
schema.items = unwrapped_schema.items
schema.properties = unwrapped_schema.properties
schema.enum = unwrapped_schema.enum
schema.any_of = NoneThis would handle various Optional types correctly and would also allow removing the special-case logic for lists.
|
Closing as a similar fix has already been accepted. Thanks! |
Summary
Fixes #3591
The function parameter parser in
_parse_schema_from_parameteronly handledUnion[X, Y](typing.Union) for complex types. When a function used the modern Python 3.10+ pipe syntax (e.g.list[str] | None), the annotation resolved totypes.UnionTypeat runtime and fell through to aValueError:This broke the adk web builder assistant because
cleanup_unused_files(a built-in tool) useslist[str] | Nonein its signature.Changes
isinstanceguard in_parse_schema_from_parameterto includetypes.UnionTypeorigincheck to handletypes.UnionTypealongsidetyping.Unionlist[str] | Nonenow parses identically toOptional[list[str]]→ARRAY(items=STRING, nullable=True)Test plan
test_get_declaration_with_pipe_union_optional_listintests/unittests/tools/test_function_tool.pyverifieslist[str] | Noneandstr | Noneparse correctlyuv run pytest tests/unittests/tools/test_function_tool.py tests/unittests/tools/test_function_tool_pydantic.py)autoformat.sh— no changesmypy— no new errors introduced