-
Notifications
You must be signed in to change notification settings - Fork 345
refactor!: Update to Protocol v1.0.0-rc and fix all tests #665
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: 1.0-a2a_proto_refactor
Are you sure you want to change the base?
refactor!: Update to Protocol v1.0.0-rc and fix all tests #665
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
|
|
||
| ) |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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) |
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.
ditto
| @@ -476,7 +476,7 @@ async def _cleanup_producer( | |||
|
|
|||
| async def on_set_task_push_notification_config( | |||
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.
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}', |
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.
task_id should just be the 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}' |
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.
ditto
| 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| params = GetTaskRequest(id=f'tasks/{GET_TASK_RESPONSE.id}') | |
| params = GetTaskRequest(id=GET_TASK_RESPONSE.id) |
|
|
||
| [tool.pyright] | ||
| include = ["src"] | ||
| ignore = ["src/a2a/types"] |
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.
Is there a reason this is ignore instead of exclude?
Updates the Python SDK to match Protocol v1.0.0-rc specifications.
Changes include: