-
Notifications
You must be signed in to change notification settings - Fork 2
ML-984: Remove sorting of messages and simplify merging of tool calls #20
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: thousandeyes-fixes
Are you sure you want to change the base?
Conversation
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.
Added a few comments, thanks @jcruzmot-te
| return messages | ||
|
|
||
| # Function to move toolUse last to handle AWS Bedrock issues in EU | ||
| def moved_tooluse_last(messages): |
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.
Can you explain why this is not needed anymore? Where does this get handled in Semantic Kernel's latest release?
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 explained the reason in Slack (https://thousandeyes.slack.com/archives/C08APJUUQ14/p1759309509036729) I can add more info in the description of the PR if needed. But basically, the problem with the order of messages that we had was because of the fake TextContent messages that were generated when we modified the content here: https://github.com/thousandeyes/aia-gateway/pull/550/files#diff-8ba4fc98b14ac8dde48d0506bfca04ebb35795c780a90c803164e8f935ce46a8L352. These fake messages were messing up the order of the messages, which Semantic Kernel constructs correctly if we don't modify the content.
| tool_message_buffer["content"].extend( | ||
| (MESSAGE_CONVERTERS[message.role](message)).get("content", []) | ||
| ) | ||
| formatted_message = MESSAGE_CONVERTERS[message.role](message) |
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 this a re-write of the multiple tool calls change, and is new/different functionality added? The previous code was quite readable already :)
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.
As I mentioned in slack (https://thousandeyes.slack.com/archives/C08APJUUQ14/p1759319719260209?thread_ts=1759309509.036729&cid=C08APJUUQ14), the merge of "tool results" is in fact required because we are using Claude. This merge is done in the Semantic Kernel code when Anthropic is queried directly, but not when it is queried through Bedrock. I have modified the existing code to be closer to what is done in the code of SK for Anthropic (code available here: https://github.com/microsoft/semantic-kernel/blob/167308f53c3ea15aed5ed1140210eed56474c968/python/semantic_kernel/connectors/ai/anthropic/services/anthropic_chat_completion.py#L242) and from my point of view this version is more readable and concise. The diff is maybe complicated to read, but if you compare side by side, the new code is just this:
prev_message_role: AuthorRole | None = None
for message in chat_history.messages:
if message.role == AuthorRole.SYSTEM:
continue
formatted_message = MESSAGE_CONVERTERS[message.role](message)
if prev_message_role == AuthorRole.TOOL and message.role == AuthorRole.TOOL:
# Consecutive tool messages have to be appended in the same message
messages[-1][content_key] += formatted_message[content_key]
else:
# If previous message is not a tool message, we start a new message
messages.append(formatted_message)
prev_message_role = message.role
While the previous one was all this:
tool_message_buffer = None
for message in chat_history.messages:
if message.role == AuthorRole.SYSTEM:
continue
# If there are multiple tool responses, Bedrock expects one message
# with multiple content blocks, one for each response.
if message.role == AuthorRole.TOOL:
if tool_message_buffer is None:
tool_message_buffer = MESSAGE_CONVERTERS[message.role](message)
else:
tool_message_buffer["content"].extend(
(MESSAGE_CONVERTERS[message.role](message)).get("content", [])
)
else:
# If a non-tool message is encountered, flush the buffer
if tool_message_buffer:
messages.append(tool_message_buffer)
tool_message_buffer = None
messages.append(MESSAGE_CONVERTERS[message.role](message))
# Flush any remaining tool messages buffer
if tool_message_buffer:
messages.append(tool_message_buffer)
Motivation and Context
After fixing a problem with repeated messages in https://github.com/thousandeyes/aia-gateway, it was observed that the sorting of messages for having tool calls at the end was not needed anymore. Furthermore, it was also observed that the fix for merging multiple tool results was needed because we are using Claude (see code from Semantic Kernel for Anthropic here: https://github.com/microsoft/semantic-kernel/blob/167308f53c3ea15aed5ed1140210eed56474c968/python/semantic_kernel/connectors/ai/anthropic/services/anthropic_chat_completion.py#L242).
Description
This PR removes the sorting of tool use message, since it is not required anymore. Furthermore, the code that merges consecutive tool results has been simplified.