refactor!: Update to Protocol v1.0.0-rc and fix all tests#665
refactor!: Update to Protocol v1.0.0-rc and fix all tests#665muscariello wants to merge 4 commits intoa2aproject:1.0-a2a_proto_refactorfrom
Conversation
Summary of ChangesHello @muscariello, 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 introduces a significant refactoring of the Python SDK to align with Protocol V1 specifications. The core change involves migrating from Pydantic-based data models to native Protobuf message objects for all A2A types, which improves performance and consistency. This transition necessitated updates to field names, data structures like PushNotificationConfig, and the underlying serialization/deserialization logic across both client and server components. Additionally, the PR streamlines the code generation process and removes outdated legacy client implementations, ensuring the SDK is modern and fully compliant with the latest protocol standards. Highlights
Ignored Files
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
|
5584d97 to
70316d9
Compare
There was a problem hiding this comment.
Code Review
This is a substantial and well-executed pull request that refactors the Python SDK to align with Protocol V1. The changes are extensive, touching nearly every part of the codebase, and appear to be implemented correctly and consistently with the goals outlined in the description.
Key improvements include:
- A shift from Pydantic models to using Protobuf-generated classes directly, which simplifies the code by removing the
proto_utilsconversion layer. - A new, clearer exception hierarchy defined in
a2a.utils.errors. - The introduction of a robust signing and verification mechanism for Agent Cards.
- Renaming of fields and methods (e.g.,
resubscribetosubscribe,get_cardtoget_extended_agent_card) for better clarity and consistency with the protocol. - Comprehensive updates to tests to cover the new protocol version and features.
The overall quality of the refactoring is high. I have only one minor stylistic suggestion.
src/a2a/client/client_factory.py
Outdated
|
|
||
| ) |
7d64a4a to
d1b8162
Compare
Signed-off-by: Luca Muscariello <muscariello@ieee.org>
d1b8162 to
898119c
Compare
| params: SubscribeToTaskRequest, | ||
| context: ServerCallContext | None = None, | ||
| ) -> AsyncGenerator[StreamResponse]: | ||
| ) -> AsyncGenerator[Event, None]: |
There was a problem hiding this comment.
I'm not sure we want to revert this change
| Requires the task and its queue to still be active. | ||
| """ | ||
| task_id = _extract_task_id(params.name) | ||
| task_id = _extract_task_id(params.id) |
There was a problem hiding this comment.
Not need for _extract_task_id(params.id) any longer, task.id is the ID.
| raise ServerError(error=UnsupportedOperationError()) | ||
|
|
||
| task_id = _extract_task_id(params.parent) | ||
| task_id = _extract_task_id(params.task_id) |
| @@ -476,7 +476,7 @@ | |||
|
|
|||
| async def on_set_task_push_notification_config( | |||
There was a problem hiding this comment.
We should rename "on_set" to "on_create" to align with the protocol.
| push_id = request.path_params['push_id'] | ||
| params = GetTaskPushNotificationConfigRequest( | ||
| name=f'tasks/{task_id}/pushNotificationConfigs/{push_id}' | ||
| task_id=f'tasks/{task_id}', |
| Parse(body, params) | ||
| # Set the parent to the task resource name format | ||
| params.parent = f'tasks/{task_id}' | ||
| params.task_id = f'tasks/{task_id}' |
src/a2a/server/models.py
Outdated
| return MessageToDict(value, preserving_proto_field_name=False) | ||
| if isinstance(value, BaseModel): | ||
| return value.model_dump(mode='json') | ||
| return cast('BaseModel', value).model_dump(mode='json') |
There was a problem hiding this comment.
I don't think you need a cast here normally mypy is happy with just the isinstance(...) check.
| return [ | ||
| MessageToDict(part.data.data) for part in parts if part.HasField('data') | ||
| ] | ||
| return [MessageToDict(part.data) for part in parts if part.HasField('data')] |
There was a problem hiding this comment.
part.data is of type protobuf.Value which can contain any JSON compatible type, dict, string, array etc
So the return type list[dict[str, Any]] isn't correct, and also I don't think we want to use MessageToDict because part.data isn't a message type.
Honestly I think with the changes to Part, both get_data_parts and get_file_parts functions probably need refactoring out of the code because they don't mean what they used to mean.
| """Provides a sample TaskPushNotificationConfig object.""" | ||
| return TaskPushNotificationConfig( | ||
| name='tasks/task-1', | ||
| task_id='tasks/task-1', |
There was a problem hiding this comment.
| task_id='tasks/task-1', | |
| task_id='task-1', |
|
|
||
| # Use GetTaskRequest with name (AIP resource format) | ||
| params = GetTaskRequest(name=f'tasks/{GET_TASK_RESPONSE.id}') | ||
| params = GetTaskRequest(id=f'tasks/{GET_TASK_RESPONSE.id}') |
There was a problem hiding this comment.
| params = GetTaskRequest(id=f'tasks/{GET_TASK_RESPONSE.id}') | |
| params = GetTaskRequest(id=GET_TASK_RESPONSE.id) |
Signed-off-by: Luca Muscariello <muscariello@ieee.org>
Signed-off-by: Luca Muscariello <muscariello@ieee.org>
…ests Signed-off-by: Luca Muscariello <muscariello@ieee.org>
Updates the Python SDK to match Protocol v1.0.0-rc specifications.
Changes include: