-
Notifications
You must be signed in to change notification settings - Fork 2k
fix: #3036 improve parameter filtering for CrewAI functions with **kwargs #3037
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
base: main
Are you sure you want to change the base?
fix: #3036 improve parameter filtering for CrewAI functions with **kwargs #3037
Conversation
- Fix FunctionTool parameter filtering to support CrewAI-style tools - Functions with **kwargs now receive all parameters except 'self' and 'tool_context' - Maintains backward compatibility with explicit parameter functions - Add comprehensive tests for **kwargs functionality Fixes parameter filtering issue where CrewAI tools using **kwargs pattern would receive empty parameter dictionaries, causing search_query and other parameters to be None. #non-breaking
Summary of ChangesHello @omarcevi, 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 issue where 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
|
Hello @omarcevi, thank you for your contribution! To help us review this PR, could you please:
This information will help the reviewers to review your PR more efficiently. Thanks! Response from ADK Triaging Agent |
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 aims to improve parameter filtering for functions with **kwargs
, which is a valuable addition. However, the current implementation introduces a critical bug: if a function's signature contains both an explicit tool_context
parameter and **kwargs
, the logic incorrectly removes the tool_context
before the function call, which would cause a TypeError
. I have provided a detailed comment and a code suggestion in function_tool.py
to correct this. Additionally, while the new tests are a good start, they do not cover this specific failing edge case. I've added another comment in the test file recommending a new test case to ensure the fix is validated and to prevent future regressions.
- Fix critical bug where tool_context was being removed for functions with both explicit tool_context and **kwargs - Start with original args instead of args_to_call to avoid removing injected tool_context - Re-inject tool_context only if it's an explicit parameter in the function signature - Add comprehensive tests for edge case: functions with both tool_context and **kwargs - Maintains all existing functionality while fixing the TypeError issue This fixes the scenario where functions like: def my_func(tool_context: ToolContext, **kwargs): would fail with TypeError due to missing tool_context argument. #non-breaking
Hey @seanzhou1023 i was wondering when youll be available to review this? this fixes the issues with adk not working with crewai components |
/gemini review |
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 effectively addresses the issue of parameter filtering for CrewAI-style tools that use **kwargs
. The logic change correctly passes all necessary arguments to these functions while maintaining backward compatibility. The addition of comprehensive unit tests is also excellent, ensuring the new functionality is well-covered.
My review includes a couple of suggestions to improve the code's maintainability: one to simplify the argument filtering logic in function_tool.py
and another to reduce code duplication in the new tests by using a pytest fixture. Overall, this is a solid fix.
…and updating tests with mock ToolContext - Updated FunctionTool to defensively remove 'self' from arguments when handling **kwargs. - Refactored tests to utilize a mock ToolContext for better isolation and clarity. - Ensured backward compatibility with existing functionality while enhancing test coverage. #non-breaking
/gemini review |
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 correctly addresses an issue with parameter filtering for functions that use **kwargs
, ensuring compatibility with CrewAI-style tools. The implementation correctly passes all arguments to functions with **kwargs
while preserving the original filtering behavior for functions with explicit parameters, thus maintaining backward compatibility. The accompanying tests are comprehensive, covering synchronous and asynchronous functions, backward compatibility, and edge cases with tool_context
. I have one minor suggestion to improve a code comment for better clarity and long-term maintainability.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
/gemini review |
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 effectively resolves the parameter filtering issue for CrewAI-style functions that use **kwargs
. The implementation in function_tool.py
is clean, correctly identifying functions with **kwargs
and passing arguments appropriately, while ensuring backward compatibility for functions with explicit parameters. The accompanying tests are comprehensive, covering various scenarios like sync/async functions and the interaction with tool_context
. My feedback includes suggestions to refactor the new tests to reduce code duplication, which will enhance their long-term maintainability.
…ext handling - Introduced synchronous and asynchronous CrewAI-style tools for testing. - Refactored tests to utilize parameterized inputs for better coverage of **kwargs handling. - Ensured both synchronous and asynchronous functions are tested with tool_context and **kwargs. - Improved clarity and maintainability of test cases while preserving existing functionality. #non-breaking
/gemini review |
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 introduces a great improvement by adding support for CrewAI-style tool functions that use **kwargs
, enhancing flexibility. The change correctly identifies functions with **kwargs
and avoids filtering their arguments, while maintaining backward compatibility for functions with explicit parameters. The accompanying tests are comprehensive and cover the new functionality well.
My main feedback is regarding a subtle issue in the handling of the tool_context
parameter for functions with **kwargs
. The implementation doesn't fully match the PR description, potentially allowing tool_context
from the args
dictionary to be passed through, which could lead to unexpected behavior. I've provided a suggestion to align the code with the intended logic and recommended an additional test case.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
/gemini review |
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 enhances the FunctionTool
class to correctly handle functions that accept **kwargs
, addressing an issue where such functions would receive empty parameter dictionaries. The changes include modifications to the run_async
method to detect and appropriately pass arguments to functions with **kwargs
, along with the addition of comprehensive tests to ensure the new functionality works as expected and maintains backward compatibility. The code changes look good and the tests are comprehensive.
- Added an assertion to ensure that unexpected parameters are filtered out and not passed to the function in the test for backward compatibility with **kwargs. - This enhancement improves the robustness of the tests by explicitly checking for parameter handling. #non-breaking
/gemini review |
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 effectively addresses an issue with parameter filtering for functions that use **kwargs
, a pattern common in frameworks like CrewAI. The implementation correctly identifies functions with **kwargs
and ensures all relevant arguments are passed, while also preserving backward compatibility for functions with explicitly defined parameters. The accompanying tests are comprehensive, covering synchronous and asynchronous cases, backward compatibility, and interactions with tool_context
. My feedback includes a suggestion to refactor a logic block for improved clarity and a minor cleanup in the test suite to remove a redundant assertion. Overall, this is a solid contribution that enhances the tool's flexibility.
@seanzhou1023 Hello any chance you can review this? :) |
Hey @seanzhougoogle , :) please let me know if anything is missing here. without this fix users cant work with crew ai tools with adk. made sure to also test for backwards compatibility. glad to help if there are any edits necessary here. im including screen shots of the test results: ![]() |
fix: #3036
Fixes parameter filtering issue where CrewAI tools using **kwargs pattern would receive empty parameter dictionaries, causing search_query and other parameters to be None.
#non-breaking