-
Notifications
You must be signed in to change notification settings - Fork 2.5k
Adds ADK EventActions to A2A response #3631
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
Summary of ChangesHello @hoonji, 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 ADK to A2A event conversion by incorporating the 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.
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 adds the Event.actions field to the A2A event metadata, ensuring that action-related information is passed along during event conversion. The changes are straightforward and include corresponding unit tests to verify the new behavior.
My review includes two main points for consideration. First, I've noted a pre-existing inconsistency with type hints related to metadata serialization, which this PR's test changes make more apparent. Second, I've pointed out that due to the current data model, empty EventActions objects will always be included in the metadata, which is inefficient. I've suggested that these be addressed in follow-up work to improve code clarity and efficiency.
| ("custom_metadata", event.custom_metadata), | ||
| ("usage_metadata", event.usage_metadata), | ||
| ("error_code", event.error_code), | ||
| ("actions", event.actions), |
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.
The actions field on an Event object is initialized with default_factory=EventActions, so event.actions will never be None. As a result, it will always be serialized and included in the metadata, even if it's an empty object representing no actions. This adds unnecessary data to every A2A event.
To make this more efficient, the actions field should only be included when it contains meaningful data. The ideal solution would be to change the definition in src/google/adk/events/event.py to actions: Optional[EventActions] = None. This would allow the existing if field_value is not None: check to correctly filter out cases where no actions are present. Since that change is outside this PR's scope, this is a good candidate for a follow-up task.
| assert result[f"{ADK_METADATA_KEY_PREFIX}actions"] == { | ||
| "test_actions": "value" | ||
| } |
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.
This test assertion highlights a pre-existing inconsistency in event_converter.py. The _get_context_metadata function is type-hinted to return Dict[str, str], but this test correctly asserts that the value for the actions key is a dictionary, not a string.
While the A2A server expects Dict[str, Any] for metadata and this will work at runtime, the incorrect type hints are misleading and harm code maintainability.
To resolve this, the type hints in event_converter.py should be updated in a follow-up:
_get_context_metadata's return type should beDict[str, Any]._serialize_metadata_value's name, docstring, and return type hint should be changed to reflect that it can return non-string values (e.g., return typeAny).
|
Hi @DeanChensj , can you please review this. |
Link to Issue or Description of Change
1. Link to an existing issue (if applicable):
Solution:
Adds Event.actions to A2A event metadata during ADK -> A2A event conversion (to allow A2A event consumers to access important info like state_delta, artifact delta, etc)
Testing Plan
pytest ./tests/unittestspasses