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

translator: toolCalls support in OpenAI to Bedrock #132

Merged
merged 19 commits into from
Jan 20, 2025

Conversation

wengyao04
Copy link
Contributor

@wengyao04 wengyao04 commented Jan 19, 2025

In this PR, I improve the translator to handle toolCalls. If toolCalls is not in the OpenAI message we append toolResultBlock for assistant role message. In the Response body, we add toolCalls if bedrock content has toolUse

I also refactor openAIMessageToBedrockMessage to have separate functions to convert message for different roles user, assistant, system and tool.

When dealing with image content in the roles message, add a function to parse data uri and add proper error handling.

@wengyao04 wengyao04 requested a review from a team as a code owner January 19, 2025 04:36
@mathetake
Copy link
Member

Could you shorten the title so that it fits in the commit title

@wengyao04 wengyao04 changed the title OpenAI To AWSBedrock Translator V1 ChatCompletion: add tool_calls in the message content OpenAI To AWSBedrock: add tool_calls in the message content Jan 19, 2025
@wengyao04 wengyao04 changed the title OpenAI To AWSBedrock: add tool_calls in the message content OpenAI To AWSBedrock: add toolCalls to message content Jan 19, 2025
@mathetake mathetake changed the title OpenAI To AWSBedrock: add toolCalls to message content translator: toolCalls support in OpenAI to Bedrock Jan 19, 2025
@wengyao04
Copy link
Contributor Author

@wengyao04 @yuzisun i guess the items list PR description might be a BB style but could you from now write messages in sentences? (i will add a PR template later, but for now could you follow my PR like #135)

Update the commit message with readable paragraph.

Copy link
Member

@mathetake mathetake left a comment

Choose a reason for hiding this comment

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

thanks for the multiple iterations. style wise LGTM, so deferring to @yuzisun for the actual logic final approval

Signed-off-by: yweng14 <[email protected]>
@wengyao04
Copy link
Contributor Author

thanks for the multiple iterations. style wise LGTM, so deferring to @yuzisun for the actual logic final approval

Thanks you for helping review, I will send another PR to add ToolCalls support in converse stream

Choices: []openai.ChatCompletionResponseChoice{
{
Index: 0,
FinishReason: openai.ChatCompletionChoicesFinishReasonStop,
Copy link
Contributor

Choose a reason for hiding this comment

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

The finish reason here should be “tool_calls” as the stop reason on ConverseOutput is set to toolUse in this case.

Copy link
Contributor Author

@wengyao04 wengyao04 Jan 20, 2025

Choose a reason for hiding this comment

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

update the unit test, for input bedrock converse response, the stop reason is tool_use for content with toolUseBlock. The output openai chat completion stop reason is expected to be tool_calls

Copy link
Contributor

@yuzisun yuzisun left a comment

Choose a reason for hiding this comment

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

Thanks @wengyao04 !!

@yuzisun yuzisun merged commit 9291fb3 into envoyproxy:main Jan 20, 2025
12 checks passed
@wengyao04 wengyao04 deleted the translator-unit-tests branch January 20, 2025 22:48
aabchoo pushed a commit that referenced this pull request Jan 21, 2025
In this PR, I improve the translator to handle toolCalls. If toolCalls
is not in the OpenAI message we append toolResultBlock for assistant
role message. In the Response body, we add toolCalls if bedrock content
has toolUse

I also refactor `openAIMessageToBedrockMessage` to have separate
functions to convert message for different roles `user`, `assistant`,
`system` and `tool`.

When dealing with image content in the roles message, add a function to
parse data uri and add proper error handling.

---------

Signed-off-by: yweng14 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants