Skip to content
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

Tool call result summary message #4755

Merged
merged 13 commits into from
Dec 20, 2024

Conversation

jspv
Copy link
Contributor

@jspv jspv commented Dec 18, 2024

Why are these changes needed?

Creates a ToolCallResultSummaryMessage type more clearly differentiate when an agent is responding with a tool summary
vs. a TextMessage response from the LLM.

Related issue number

Closes #4754

Checks

Copy link
Contributor

@husseinmozannar husseinmozannar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • We recently updated the naming of toolcallmessage and toolcallresultmessage, look to pull from main and then add your changes, there is no real conflict
  • The naming "ToolCallResultSummaryMessage" is a bit too long, I would just do "ToolCallSummaryMessage" perhaps, i'm sure @ekzhu has opinions

@jspv
Copy link
Contributor Author

jspv commented Dec 19, 2024

Code updated per request

@jspv jspv requested a review from husseinmozannar December 19, 2024 01:31
@husseinmozannar
Copy link
Contributor

Thanks!

See the failing tests, I believe ChatMessage should contain ToolCallSummaryMessage because of the Response type

@husseinmozannar
Copy link
Contributor

@victordibia This is related to #4725 I wonder what you think we should do

@ekzhu ekzhu requested a review from victordibia December 19, 2024 17:55
Copy link
Collaborator

@ekzhu ekzhu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jspv you can read about how to run tests in CONTRIBUTING.md

In some group chat orchestrator classes, you need to update the handling of message thread:

def _thread_to_context(self) -> List[LLMMessage]:
"""Convert the message thread to a context for the model."""

if isinstance(msg, TextMessage | StopMessage | HandoffMessage):
message += f" {msg.content}"

Bonus: would be great to add test code to verify the text content has been added to the prompt that gets sent to the monkey patched model client, for SelectorGroupChat.

Also declare message types for assistant agent, only when there are tools.

def produced_message_types(self) -> List[type[ChatMessage]]:
"""The types of messages that the assistant agent produces."""

@jspv jspv requested a review from ekzhu December 19, 2024 21:18
@ekzhu
Copy link
Collaborator

ekzhu commented Dec 19, 2024

@husseinmozannar I believe the recent changes to this PR has already resolved your requests.

@husseinmozannar husseinmozannar self-requested a review December 20, 2024 02:23
Copy link
Contributor

@husseinmozannar husseinmozannar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@victordibia I believe this will break connection.py in AGS, specifically line 328 we should add ToolCallSummary

@victordibia
Copy link
Collaborator

@victordibia This is related to #4725 I wonder what you think we should do

It is related, we might defer to a seperate PR.

I feel #4725 might be better served by extending ToolCallExecutionEvent and FunctionExecutionResult to return data on data types returned.

I'll try to see how I can address the ags breaks

@ekzhu
Copy link
Collaborator

ekzhu commented Dec 20, 2024

@husseinmozannar @victordibia please take over this PR from now on to address any changes necessary

@victordibia
Copy link
Collaborator

I have taken a look .. I'll address any changes needed in a seperate PR. (mostly to add checks for ToolCallSummaryMessage types). Its not exactly breaking for now (those messages may not show up in the UI)

@husseinmozannar husseinmozannar merged commit a271708 into microsoft:main Dec 20, 2024
48 checks passed
@jspv jspv deleted the ToolCallResultSummaryMessage branch December 23, 2024 12:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ToolCallResultSummaryMessage message type in AgentChat
4 participants