-
Notifications
You must be signed in to change notification settings - Fork 2.9k
feat(plugins): wire on_state_change_callback into plugin framework #4395
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?
Conversation
Add plumbing so that plugins are notified when an event carries session state changes (non-empty state_delta). This closes a gap where BasePlugin had no default method, PluginManager had no dispatcher, and the runner never triggered the callback. Fixes google#4393
Summary of ChangesHello @mportdata, 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 significantly enhances the plugin framework by introducing a new callback mechanism, 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.
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 introduces the on_state_change_callback into the plugin framework, providing a much-needed mechanism for plugins to react to state changes. The implementation is well-structured across the BasePlugin, PluginManager, and Runner layers, and is accompanied by a comprehensive set of unit tests. My review includes a couple of suggestions to improve docstring accuracy and type hint consistency, which will enhance clarity for future developers using this new callback.
- Clarify docstring: non-None return short-circuits subsequent plugins - Fix return type: Optional[None] -> Optional[Any] to match _run_callbacks
bac6922 to
7c4ef94
Compare
- Add log_state_changes config flag (default False) to BigQueryLoggerConfig for explicit opt-in to STATE_DELTA logging via the existing after_tool_callback inline path - Add event ID dedup guard in Runner._exec_with_plugin to prevent the same event from triggering on_state_change_callback twice - Add tests for toggle enabled and disabled behavior
7c4ef94 to
3115954
Compare
Remove the log_state_changes config field and guard from on_state_change_callback. The toggle is a separate feature and can be added in a follow-up PR. This keeps the change focused on framework wiring only.
The dedup guard was only needed when STATE_DELTA had two code paths (inline after_tool_callback + on_state_change_callback). Now that the inline path is removed, each event passes through the loop once, making the guard unnecessary.
Invoke on_state_change_callback in _handle_new_message when a caller passes state_delta to run_async. This ensures all state mutations — both caller-supplied and tool-generated — trigger the callback.
Description
Fixes #4393
Problem
Changes to state don't trigger plug-in methods the same way other events do.
Additionally state changes can also be caused by
_append_new_message_to_sessionhowever these never trigger plug-in methods as this happens separately toafter_tool_callback(the only place state changes are currently captured).Pattern for events triggering plug-in methods
Each plug-in in Google ADK inherits the
BasePluginclass (BigQueryAgentAnalyticsPlugin,ContextFilterPlugin, etc.).BasePluginhas several no-op methods such asbefore_run_callback,on_event_callback, etc. that are inherited by all Google ADK plug-ins. Each of these methods is associated with an event that can occur in Google ADK. Since they functionally do nothing, they are overwritten by plug-ins that inherit them when an action needs to be taken before, on or after an event.For example,
on_user_message_callbackis a no-op method defined onBasePlugin. This is overwritten when definingBigQueryAgentAnalyticsPluginso that the event is logged (using_log_event, anotherBigQueryAgentAnalyticsPluginmethod).This pattern is adhered to when having plug-in methods triggered by events.
The
PluginManagerclass is used to call a specific method on all registered plug-ins. This works as methods associated with events are uniformly named across plug-ins.The
Runnerclass is what is used to call plug-in methods (viaPluginManager) upon each event.How
state_deltais currently managedThis pattern is currently not followed when capturing changes to state in
BigQueryAgentAnalyticsPlugin.Currently, to capture state changes in the
BigQueryAgentAnalyticsPlugin, whenafter_tool_callbackis triggered we check to see iftool_context.actions.state_deltaexists. If it does then aSTATE_DELTAevent is logged.Solution
BasePlugin— Add default no-opon_state_change_callbackmethod (consistent with all other callbacks)PluginManager— Add"on_state_change_callback"toPluginCallbackNameand addrun_on_state_change_callbackdispatcherRunner._exec_with_plugin— After yielding each event, detect non-emptystate_deltaand invoke the callbackBigQueryAgentAnalyticsPlugin— Remove inlineSTATE_DELTAlogging fromafter_tool_callbackand route it throughon_state_change_callbackinsteadRunner._handle_new_message— After handling a new message, detect non-emptystate_deltaand invoke the callbackDesign decisions:
dict()copy — prevents plugins from mutating the event'sstate_deltaSTATE_DELTAwas logged inline withinafter_tool_callback; this has been removed in favour of routing allSTATE_DELTAlogging throughon_state_change_callback, consistent with how the framework handles other event typesTest plan
on_state_change_callbackoverride toFullOverridePluginintest_base_plugin.pytest_base_plugin_default_callbacks_return_noneverifying default returnsNonetest_base_plugin_all_callbacks_can_be_overriddenverifying override workson_state_change_callbackhandler toTestPluginintest_plugin_manager.pytest_all_callbacks_are_supportedto include new callbacktest_run_on_state_change_callback— basic invocation returnsNone, callback loggedtest_run_on_state_change_callback_calls_all_plugins— both plugins' call_logs contain the callbacktest_run_on_state_change_callback_wraps_exceptions— exception wrapped inRuntimeErrorwith chained causetest_after_tool_callback_no_inline_state_delta— verifiesafter_tool_callbackno longer logsSTATE_DELTAinlinetest_on_state_change_callback_logs_correctly— verifiesSTATE_DELTAis logged viaon_state_change_callbacktest_state_delta_in_run_async_triggers_on_state_change_callback— verifies caller-suppliedstate_deltainrun_asynctriggerson_state_change_callbacktest_no_state_delta_does_not_trigger_on_state_change_callback— verifies callback is not called when nostate_deltais providedADK Web and BigQuery testing
When tested locally with ADK web I have verified STATE_DELTA events are still written to BigQuery the expected number of times with the fields and values as they were before (inline with what is described in the documentation also):
_append_new_message_to_sessiontestingBelow is a scrip that instantiates an agent with an initial state and then runs the agent provoking an additional state change during run time, I have including a screenshot of how the
BigQueryAgentAnalyticsPluginplug-in has written these to BigQuery. You can see that STATE_DELTA is only logged once for each state change as expected.Checklist
autoformat.shfor formatting