Modify handleFunctionCalls to execute beforeToolCallbacks despite tool existing #385
Conversation
…l existing. Add retry callback example.
Summary of ChangesHello @baptmont, 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 LLM agent's tool execution flow by ensuring that 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 introduces a significant enhancement by allowing beforeToolCallbacks to execute even when a tool does not exist. This is useful for implementing custom error handling or retry logic, as demonstrated by the new retrytool example. The core logic in handleFunctionCalls has been refactored to support this, and the changes are well-covered by new unit tests. The documentation for BeforeToolCallback and AfterToolCallback has also been updated to reflect the new behavior where the tool parameter can be nil. My review identifies a few minor typos in a variable name within the new example and in error messages within the tests, for which I've suggested corrections.
| if !ok { | ||
| return nil, fmt.Errorf("tool %q is not a function tool", curTool.Name()) | ||
| } | ||
| curTool, toolExists := toolsDict[fnCall.Name] |
There was a problem hiding this comment.
Could the framework just do this?
if !toolExists {
return map[string]any{"error": fmt.Errorf("tool %q does not exist, please choose another tool", fnCall.Name)}, nil
}
There was a problem hiding this comment.
Is this the ADK-Python equivalent? https://github.com/google/adk-python/blob/main/src/google/adk/flows/llm_flows/functions.py#L323-L336
There was a problem hiding this comment.
This seems like a good option. Simply notify the llm instead of returning the err all the way back to the runner.
| spans := telemetry.StartTrace(ctx, "execute_tool "+fnCall.Name) | ||
|
|
||
| result := f.callTool(funcTool, fnCall.Args, toolCtx) | ||
| result, err := f.invokeBeforeToolCallbacks(funcTool, fnCall.Args, toolCtx) |
There was a problem hiding this comment.
With this change, it seems the contract with the client is changed: now the tool can be nil.
This could cause runtime errors with existing clients.
ADK-Python seems to have a different callback for tool errors - and it generates a fake tool to hold the name of the tool that had the error. https://github.com/google/adk-python/blob/main/src/google/adk/flows/llm_flows/functions.py#L326-L332
Should ADK-Go do something similar?
Add retry callback example.
Add base_flow tests.