-
Notifications
You must be signed in to change notification settings - Fork 285
feat(client): allow specifying historyLength via call-site MessageSendConfiguration in BaseClient.send_message #500
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?
Changes from all commits
41f4456
cab68ed
234e23c
bb135cb
6afdbcd
c84c455
3502183
91781f9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -46,6 +46,7 @@ async def send_message( | |
| self, | ||
| request: Message, | ||
| *, | ||
| configuration: MessageSendConfiguration | None = None, | ||
| context: ClientCallContext | None = None, | ||
| ) -> AsyncIterator[ClientEvent | Message]: | ||
| """Sends a message to the agent. | ||
|
|
@@ -56,12 +57,13 @@ async def send_message( | |
|
|
||
| Args: | ||
| request: The message to send to the agent. | ||
| configuration: Optional per-call overrides for message sending behavior. | ||
| context: The client call context. | ||
|
|
||
| Yields: | ||
| An async iterator of `ClientEvent` or a final `Message` response. | ||
| """ | ||
| config = MessageSendConfiguration( | ||
| base_config = MessageSendConfiguration( | ||
| accepted_output_modes=self._config.accepted_output_modes, | ||
| blocking=not self._config.polling, | ||
| push_notification_config=( | ||
|
|
@@ -70,6 +72,16 @@ async def send_message( | |
| else None | ||
| ), | ||
| ) | ||
| if configuration is not None: | ||
| overrides = configuration.model_dump( | ||
| exclude_unset=True, | ||
| exclude_none=True, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let’s remove this (exclude_none=True) to give more control to the user. That way they can override values with “None”. |
||
| by_alias=False, | ||
| ) | ||
TadakiAsechi marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| config = base_config.model_copy(update=overrides) | ||
| else: | ||
| config = base_config | ||
TadakiAsechi marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| params = MessageSendParams(message=request, configuration=config) | ||
|
|
||
| if not self._config.streaming or not self._card.capabilities.streaming: | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -9,6 +9,7 @@ | |||||
| AgentCapabilities, | ||||||
| AgentCard, | ||||||
| Message, | ||||||
| MessageSendConfiguration, | ||||||
| Part, | ||||||
| Role, | ||||||
| Task, | ||||||
|
|
@@ -115,3 +116,109 @@ async def test_send_message_non_streaming_agent_capability_false( | |||||
| assert not mock_transport.send_message_streaming.called | ||||||
| assert len(events) == 1 | ||||||
| assert events[0][0].id == 'task-789' | ||||||
|
|
||||||
|
|
||||||
| @pytest.mark.asyncio | ||||||
| async def test_send_message_callsite_config_overrides_history_length_non_streaming( | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The new configuration parameter allows overriding any field in MessageSendConfiguration, not just history_length. Tests such as test_send_message_config_override_non_streaming which checks multiple fields provide better coverage of the feature. |
||||||
| base_client: BaseClient, mock_transport: MagicMock, sample_message: Message | ||||||
| ): | ||||||
| base_client._config.streaming = False | ||||||
| mock_transport.send_message.return_value = Task( | ||||||
| id='task-cfg-ns-1', | ||||||
| context_id='ctx-cfg-ns-1', | ||||||
| status=TaskStatus(state=TaskState.completed), | ||||||
| ) | ||||||
|
|
||||||
| cfg = MessageSendConfiguration(history_length=2) | ||||||
| events = [ | ||||||
| event | ||||||
| async for event in base_client.send_message( | ||||||
| sample_message, configuration=cfg | ||||||
| ) | ||||||
| ] | ||||||
|
|
||||||
| mock_transport.send_message.assert_called_once() | ||||||
| assert not mock_transport.send_message_streaming.called | ||||||
| assert len(events) == 1 | ||||||
| task, _ = events[0] | ||||||
| assert task.id == 'task-cfg-ns-1' | ||||||
|
|
||||||
| params = mock_transport.send_message.await_args.args[0] | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: consider changing to sent_params=mock_transport.send_message_streaming.call_args[0][0] to make it consistent with other tests already in the file:
a2a-python/tests/client/test_base_client.py Line 105 in fdb4731
That will improve overall readability of the file. |
||||||
| assert params.configuration.history_length == 2 | ||||||
| assert params.configuration.blocking == (not base_client._config.polling) | ||||||
| assert ( | ||||||
| params.configuration.accepted_output_modes | ||||||
| == base_client._config.accepted_output_modes | ||||||
| ) | ||||||
|
|
||||||
|
|
||||||
| @pytest.mark.asyncio | ||||||
| async def test_send_message_ignores_none_fields_in_callsite_configuration_non_streaming( | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Will no longer be needed after changes in the base_client.py file. |
||||||
| base_client: BaseClient, mock_transport: MagicMock, sample_message: Message | ||||||
| ): | ||||||
| base_client._config.streaming = False | ||||||
| mock_transport.send_message.return_value = Task( | ||||||
| id='task-cfg-ns-2', | ||||||
| context_id='ctx-cfg-ns-2', | ||||||
| status=TaskStatus(state=TaskState.completed), | ||||||
| ) | ||||||
|
|
||||||
| cfg = MessageSendConfiguration(history_length=None, blocking=None) | ||||||
TadakiAsechi marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
| events = [ | ||||||
| event | ||||||
| async for event in base_client.send_message( | ||||||
| sample_message, configuration=cfg | ||||||
| ) | ||||||
| ] | ||||||
|
|
||||||
| mock_transport.send_message.assert_called_once() | ||||||
| assert len(events) == 1 | ||||||
| task, _ = events[0] | ||||||
| assert task.id == 'task-cfg-ns-2' | ||||||
|
|
||||||
| params = mock_transport.send_message.await_args.args[0] | ||||||
| assert params.configuration.history_length is None | ||||||
| assert params.configuration.blocking == (not base_client._config.polling) | ||||||
| assert ( | ||||||
| params.configuration.accepted_output_modes | ||||||
| == base_client._config.accepted_output_modes | ||||||
| ) | ||||||
|
|
||||||
|
|
||||||
| @pytest.mark.asyncio | ||||||
| async def test_send_message_callsite_config_overrides_history_length_streaming( | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same as above. Test "test_send_message_config_override_streaming" where cfg includes more fields would provide better coverage of the feature. |
||||||
| base_client: BaseClient, mock_transport: MagicMock, sample_message: Message | ||||||
| ): | ||||||
| base_client._config.streaming = True | ||||||
| base_client._card.capabilities.streaming = True | ||||||
|
|
||||||
| async def create_stream(*args, **kwargs): | ||||||
| yield Task( | ||||||
| id='task-cfg-s-1', | ||||||
| context_id='ctx-cfg-s-1', | ||||||
| status=TaskStatus(state=TaskState.completed), | ||||||
| ) | ||||||
|
|
||||||
| mock_transport.send_message_streaming.return_value = create_stream() | ||||||
|
|
||||||
| cfg = MessageSendConfiguration(history_length=0) | ||||||
| events = [ | ||||||
| event | ||||||
| async for event in base_client.send_message( | ||||||
| sample_message, configuration=cfg | ||||||
| ) | ||||||
| ] | ||||||
|
|
||||||
| mock_transport.send_message_streaming.assert_called_once() | ||||||
| assert not mock_transport.send_message.called | ||||||
| assert len(events) == 1 | ||||||
| task, _ = events[0] | ||||||
| assert task.id == 'task-cfg-s-1' | ||||||
|
|
||||||
| params = mock_transport.send_message_streaming.call_args.args[0] | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: consider changing to sent_params=mock_transport.send_message_streaming.call_args[0][0] to make it consistent with other tests already in the file:
a2a-python/tests/client/test_base_client.py Line 105 in fdb4731
That will improve overall readability of the file. |
||||||
| assert params.configuration.history_length == 0 | ||||||
| assert params.configuration.blocking == (not base_client._config.polling) | ||||||
| assert ( | ||||||
| params.configuration.accepted_output_modes | ||||||
| == base_client._config.accepted_output_modes | ||||||
| ) | ||||||
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.
Nit: Consider renaming the override variable to update_data. Since this dictionary is used in config = base_config.model_copy(update=overrides), naming it update_data improves readability.