Skip to content

fix: #2839 argument reference error #2877

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

fix: #2839 argument reference error #2877

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/google/adk/models/lite_llm.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -391,8 +391,8 @@ def _function_declaration_to_tool_param(
},
},
}

if function_declaration.parameters.required:
if function_declaration.parameters and function_declaration.parameters.required:
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

high: This change adds a check for function_declaration.parameters before accessing the required attribute. This prevents an AttributeError when function_declaration.parameters is None. This is good defensive programming, as it prevents the code from crashing when encountering unexpected input. However, it would be better to add a comment explaining why this check is necessary, and what kind of input would cause function_declaration.parameters to be None.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

medium: Consider simplifying this condition by using the walrus operator to reduce redundancy.

if params := function_declaration.parameters and params.required:
Suggested change
if function_declaration.parameters and function_declaration.parameters.required:
if params := function_declaration.parameters and params.required:

tool_params["function"]["parameters"][
"required"
] = function_declaration.parameters.required
Expand Down
17 changes: 17 additions & 0 deletions tests/unittests/models/test_litellm.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -731,6 +731,23 @@ def test_maybe_append_user_content(
},
},
),
(
"no_arguments_function",
types.FunctionDeclaration(
name="function_no_args"
),
{
"type": "function",
"function": {
"name": "function_no_args",
"description": "",
"parameters": {
"type": "object",
"properties": {},
},
},
},
)
Comment on lines +734 to +750
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

high: Adding a test case for a function with no arguments is a good way to ensure that the fix works as expected. This test case covers the scenario where function_declaration.parameters is None, which is the root cause of the bug. It would be better to add a comment explaining what this test case is testing, and why it is important.

]


Expand Down