feat: allow thinking_config in generate_content_config (#4108) #4117
Conversation
Summary of ChangesHello @Akshat8510, 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 enhances the 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. 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
|
adk-bot
added
the
core
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request correctly implements the feature to allow thinking_config in generate_content_config. The changes in llm_agent.py to relax validation and add a warning for conflicting configurations are well-implemented. However, I've found a couple of issues in built_in_planner.py related to logging that need to be addressed. Specifically, an incorrect logger is being imported from the venv module. I've also pointed out a minor issue with a duplicate import in llm_agent.py. Please see my detailed comments.
|
@gemini-code-assist/gemini review |
There was a problem hiding this comment.
Code Review
This pull request correctly implements the feature to allow thinking_config to be set directly in generate_content_config. The changes are well-aligned with the description: the validation in LlmAgent is relaxed, a UserWarning is added for conflicting configurations at initialization, and an INFO log is added in BuiltInPlanner to signal when a user-provided configuration is being overwritten at runtime. The implementation is solid. I have one suggestion to improve the readability of the condition for the new warning.
|
@gemini-code-assist/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a requested feature to allow thinking_config to be set directly in generate_content_config. The changes are well-implemented, removing the previous validation and adding appropriate warnings and logs to guide the user when conflicting configurations are provided.
I've added one suggestion to make the checks for existing configurations more explicit and robust by using is not None instead of relying on truthiness.
As a follow-up, you might want to update the docstring for LlmAgent.generate_content_config (line 288) which currently states that thinking_config must be configured via the planner. This is no longer strictly true with the changes in this PR.
|
@gemini-code-assist/gemini review |
There was a problem hiding this comment.
Code Review
This PR successfully enables thinking_config in generate_content_config. The logic is sound, and the UserWarning for conflicting settings is a thoughtful addition.
However, a critical issue exists: the unit tests have not been updated. Specifically, test_validate_generate_content_config_thinking_config_throw in tests/unittests/agents/test_llm_agent_fields.py will now fail as it expects a ValueError that is no longer raised. This test must be fixed.
Furthermore, new unit tests should be added to cover the new warning and logging behaviors to ensure long-term maintainability, rather than relying solely on a manual reproduction script.
|
@gemini-code-assist/gemini review |
There was a problem hiding this comment.
Code Review
This pull request correctly implements the feature to allow thinking_config in generate_content_config. The changes are well-structured, including relaxing the validation, adding a UserWarning for conflicting configurations, and logging when a configuration is overwritten. My feedback includes a suggestion to improve the clarity of a docstring to better inform users about configuration precedence.
|
@gemini-code-assist/gemini review |
There was a problem hiding this comment.
Code Review
This pull request effectively implements the requested feature to allow thinking_config directly in generate_content_config. The logic for handling precedence with a planner and providing a UserWarning is well-implemented. The addition of an informational log message when the planner's configuration takes precedence is also a nice touch for improving observability.
My main feedback, as detailed in the review comments, is the need to add unit tests for the new functionality. Additionally, please note that this change will likely break the existing test test_validate_generate_content_config_thinking_config_throw, which should be updated or removed. Ensuring comprehensive test coverage is crucial for maintaining code quality.
|
@gemini-code-assist/gemini review |
There was a problem hiding this comment.
Code Review
This pull request successfully implements the requested feature to allow thinking_config in generate_content_config. The changes in llm_agent.py and built_in_planner.py are well-implemented, including the warning for conflicting configurations and the informational logging on overwrite. However, the associated test file, tests/unittests/agents/test_llm_agent_fields.py, has several critical issues that need to be addressed. There are multiple duplicated test functions, incorrectly indented tests that will cause syntax errors, and leftover code from a deleted test that will cause a runtime error. I've left specific comments on these issues. Please clean up the test file to ensure the test suite is correct and maintainable.
|
@gemini-code-assist/gemini review |
There was a problem hiding this comment.
Code Review
This pull request successfully implements the requested feature to allow thinking_config in generate_content_config. The changes are logical, including the removal of the validation error and the addition of a warning for conflicting configurations. The new logging in the planner is also a good addition for transparency.
My review includes a suggestion to refactor a boolean check for better maintainability and points out duplicated test functions that should be removed. Overall, the implementation is solid and the accompanying tests are thorough, aside from the duplication.
|
@gemini-code-assist/gemini review |
There was a problem hiding this comment.
Code Review
This pull request successfully implements the feature to allow thinking_config to be set directly in generate_content_config. The changes are logical and well-tested with new unit tests. I have two main points of feedback. First, an existing test that is now obsolete due to the changes needs to be removed to prevent CI failures. Second, a new test function has been placed inside a class definition, which harms readability and should be moved. Please see the detailed comments for specifics.
I am having trouble creating individual review comments. Click here to see my feedback.
src/google/adk/agents/llm_agent.py (852-853)
With the removal of this validation, the test test_validate_generate_content_config_thinking_config_throw in tests/unittests/agents/test_llm_agent_fields.py is now obsolete and will fail. Please ensure it is removed as part of this pull request.
tests/unittests/agents/test_llm_agent_fields.py (350-368)
This new test function test_builtin_planner_overwrite_logging has been inserted in the middle of the TestCanonicalTools class definition. While this is syntactically valid, it makes the code very difficult to read and maintain. Please move this function to be outside the class definition, for example, after the TestCanonicalTools class block concludes.
|
@gemini-code-assist/gemini review |
There was a problem hiding this comment.
Code Review
This pull request successfully implements the requested feature to allow thinking_config within generate_content_config. The changes are logical, including the relaxed validation, the warning for conflicting configurations, and the informative logging when a planner overwrites a setting. The accompanying tests are thorough and cover the new behaviors well.
I have identified one critical issue in the new model_post_init method that could lead to an AttributeError during agent initialization under certain conditions. I've also noted a minor improvement for one of the new test functions. Please see the specific comments for details.
|
@gemini-code-assist/gemini review |
There was a problem hiding this comment.
Code Review
This pull request successfully implements the ability to set thinking_config directly in generate_content_config, which is a great feature for consistency with other SDKs. The changes are well-implemented: the validation has been correctly relaxed, a helpful UserWarning is now issued in case of conflicting configurations, and the planner's behavior of overwriting the config is now clearly logged. The accompanying tests are thorough and cover all new logic paths, ensuring the changes are robust. My only suggestions are minor style improvements in the test file to maintain consistency.
|
@gemini-code-assist/gemini review |
There was a problem hiding this comment.
Code Review
This pull request effectively implements the requested feature to allow thinking_config in generate_content_config. The changes are logical, with appropriate warnings for conflicting configurations and logging for overrides. The new functionality is also well-covered by unit tests. I have one suggestion to improve code clarity in built_in_planner.py by removing a redundant check.
|
@gemini-code-assist/gemini review |
There was a problem hiding this comment.
Code Review
This pull request successfully implements the feature to allow thinking_config to be set directly in generate_content_config. The changes are well-structured: the validation in LlmAgent is correctly relaxed, a helpful UserWarning is added for cases where both planner and generate_content_config provide a thinking_config, and the BuiltInPlanner now logs when it overwrites a configuration, improving transparency. The accompanying tests are thorough, covering the new success path, the warning mechanism, and the logging behavior. The code quality is high, and I have no further suggestions for improvement. Great work!
|
I have addressed all the feedback from the gemini-code-assist bot, including refactoring the precedence checks, cleaning up imports, adding unit tests, and removing redundant logic. The bot has given a final approval with no further suggestions. Ready for final maintainer review! Thanks |
src/google/adk/agents/llm_agent.py
Outdated
|
|
||
| # Check if thinking_config is set in both the model config and the planner. | ||
| # Using getattr for consistency in checking optional attributes. | ||
| has_manual_thinking_config = ( |
There was a problem hiding this comment.
"manual": This is not a good naming in my opinion. But see the other comment.
|
Please double check your code, the isort and Pyink are still failing. Also add |
ryanaiagent
added
the
request clarification
|
@gemini-code-assist/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a useful feature by allowing thinking_config to be set directly in generate_content_config, which improves consistency with other SDKs. The changes are well-implemented: the validation logic is correctly relaxed, and a helpful UserWarning is added to inform users about configuration precedence when both a planner and a manual thinking_config are provided. Additionally, the BuiltInPlanner now logs an informative message when it overwrites an existing configuration, which enhances debuggability. The accompanying tests are thorough and correctly validate the new functionality, including the warning and logging behavior. Overall, this is a high-quality contribution that improves the developer experience.
|
/gemini summary |
Summary of ChangesThis pull request introduces the capability to directly specify 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
|
Signed-off-by: Akshat Kumar <[email protected]>
Akshat8510
force-pushed
the
feat/allow-thinking-config-4108
branch
from
33a9789 to
7d3c06f
Compare
|
@gemini-code-assist/gemini review |
|
/gemini summary |
There was a problem hiding this comment.
Code Review
This pull request effectively implements the requested feature to allow thinking_config in generate_content_config. The changes are well-structured, including relaxing validation, adding a warning for conflicting configurations, and logging overrides in the planner. The accompanying tests are thorough and cover the new logic well.
I have a couple of minor suggestions for improvement: one regarding the stacklevel of the new warning to improve developer experience, and another to simplify the code in the BuiltInPlanner by removing a redundant check. Overall, this is a solid contribution.
Summary of ChangesThis pull request enhances the 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
|
…ialization Signed-off-by: Akshat Kumar <[email protected]>
Akshat8510
force-pushed
the
feat/allow-thinking-config-4108
branch
from
5deeb89 to
39b55ec
Compare
|
@gemini-code-assist/gemini review |
|
/gemini summary |
There was a problem hiding this comment.
Code Review
This pull request successfully implements the feature to allow thinking_config to be set directly in generate_content_config. The changes are well-implemented, including relaxing the validation, adding a UserWarning for conflicting configurations, and logging an informational message when the planner's configuration takes precedence. The accompanying tests are thorough and cover all new logic and edge cases effectively. The code quality is high, and the changes improve the SDK's flexibility and alignment with other implementations.
Summary of ChangesThis pull request introduces the capability to directly specify 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
|
|
Hi @wuliang229, final cleanup is done. Fixed import sorting for isort/pyink, added from future import annotations, updated the planner logger, and refactored the LlmAgent logic as requested. |
Merge #4117 **Overview** This PR implements the feature request in #4108 to allow `thinking_config` to be set directly within `generate_content_config`, bringing the Python SDK in line with the Go implementation. **Changes** - **llm_agent.py**: Relaxed the validation logic in `validate_generate_content_config` to remove the `ValueError` for `thinking_config`. - **Precedence Warning**: Added an override of `model_post_init` in `LlmAgent` to issue a `UserWarning` if both a `planner` and a manual `thinking_config` are provided. - **built_in_planner.py**: Updated `apply_thinking_config` to log an `INFO` message when the planner overwrites an existing configuration on the `LlmRequest`. **Testing** Verified with a reproduction script covering: 1. Successful initialization of an agent with direct `thinking_config`. 2. Validation of `UserWarning` during initialization when conflicting configurations are present. 3. Confirmation of logger output when the planner performs an overwrite. Closes: #4108 Tagging @invictus2010 for visibility. Co-authored-by: Liang Wu <[email protected]> COPYBARA_INTEGRATE_REVIEW=#4117 from Akshat8510:feat/allow-thinking-config-4108 5deeb89 PiperOrigin-RevId: 856821447
|
Thank you @Akshat8510 for your contribution! 🎉 Your changes have been successfully imported and merged via Copybara in commit e162bb8. Closing this PR as the changes are now in the main branch. |
4 participants
Overview
This PR implements the feature request in #4108 to allow
thinking_configto be set directly withingenerate_content_config, bringing the Python SDK in line with the Go implementation.Changes
validate_generate_content_configto remove theValueErrorforthinking_config.model_post_initinLlmAgentto issue aUserWarningif both aplannerand a manualthinking_configare provided.apply_thinking_configto log anINFOmessage when the planner overwrites an existing configuration on theLlmRequest.Testing
Verified with a reproduction script covering:
thinking_config.UserWarningduring initialization when conflicting configurations are present.Closes: #4108
Tagging @invictus2010 for visibility.