Support for push notifications in non-streaming mode#560
Conversation
- Simplified the test structure by removing unnecessary comments and variables. - Enhanced the webhook handler to support GET requests for connectivity checks. - Improved the timing for server startup and notification assertions. - Updated the agent configuration to use a default model ID for consistency. - Ensured proper cleanup of the webhook server after tests. This makes the test more robust and easier to understand! 🎉
| else: | ||
| logger.debug("Task already exists: %s", task.model_dump_json(indent=2)) | ||
| logger.info("Task already exists: %s", task.model_dump_json(indent=2)) | ||
| updater = TaskUpdater(event_queue, task.id, task.contextId) |
There was a problem hiding this comment.
Yes: when we support streaming mode and callbacks etc I'll want to pass things into the updater as a part of running the agent. This re-order is to support this planned usage.
| request_handler = DefaultRequestHandler( | ||
| agent_executor=AnyAgentExecutor(agent, task_manager), | ||
| task_store=InMemoryTaskStore(), | ||
| push_notifier=InMemoryPushNotifier(httpx_client=httpx.AsyncClient()), |
There was a problem hiding this comment.
@njbrake it looks like this would soon be broken: a2aproject/a2a-python#212
There was a problem hiding this comment.
Good to know. I'll be ready to patch once that new release drops 👍
| first_message_id = str(uuid4()) | ||
|
|
||
| # Configure push notifications in the initial message/send request | ||
| # following the A2A specification example |
There was a problem hiding this comment.
I am not really understanding how users would use this feature from the any-agent perspective. Would it be possible to use push notifications when using a2a_as_tool?
There was a problem hiding this comment.
Let's discuss this next week. I see uses, but a human user won't normally have a defined domain/ip to set up a callback. Unless you have a notification service like a queue to which notifications are sent and the the user would poll them out.
I see it ok as an example of another way of notifying, but maybe we need a more compelling narrative of why one would use this and in which kind of setup.
The a2a_as_tool case could be such a setup. But it's very specific since the original agent would need to set up the callback, plus be able to handle asynchronous notifications from other agents.
There was a problem hiding this comment.
Push notification support is purely to support people who wish to contact an A2A served any-agent agent using their own client, we have no use for it in a2a_tool.
Right now, it has low importance because with a2a streaming=False, the only updates sent out for push notification are the last taskupdate when on_message_send completes. (failed/completed/cancelled/input_required).
This feature is to enable further customization of notifications for complex tasks as well as push notifications to be sent when using the newly available 'callbacks' feature. We still may not have any use for it in the a2a-as-tool multi-agent setup, but by implementing this we are moving towards a more complete implementation of the A2A spec.
There was a problem hiding this comment.
I'd explore the possibility of exploring the grpc transport if we feel that notifications and async tasks will be important for users later on. Push notifications, as I mentioned, are ok in a distributed setting, but probably not so easy for individual users (unless maybe when using local containers talking among themselves).
There was a problem hiding this comment.
LGTM, but not sure how useful it is to have this if it has been already broken by a new a2a version.
Overall, I feel like we are investing a lot of time in A2A features while they are still half-backed and we didn't really got a signal of people needing / using those.
I know we don't want to release saying that we support A2A with less than half of the features, but feels like they are still trying to figure out stuff on their side and we just suffer the side effects of trying to keep up.
| request_handler = DefaultRequestHandler( | ||
| agent_executor=AnyAgentExecutor(agent, task_manager), | ||
| task_store=InMemoryTaskStore(), | ||
| push_notifier=InMemoryPushNotifier(httpx_client=httpx.AsyncClient()), |
There was a problem hiding this comment.
similar to #318 , this should probably be exposed as part of the config
There was a problem hiding this comment.
Good reminder. I'll take care of the push notifier in this PR and I'll take care of 318 later today.
In this case, the breaking change in the new A2A version is an easy upgrade. But point taken that now our a2a version pin is restrictive because we'll need ==0.2.11 instead of 0.2.8<=n<=0.2.10.
Totally agree. I think it makes sense to put a pause on our new A2A feature development (which will align nicely with upcoming paternity leave 😆 ) to focus on supporting and stabilizing the features that are currently available, and can prioritize new features as we get feedback about their usefulness. |
3 participants
Clients can now subscribe to tasks to be updated when their states change.