-
Notifications
You must be signed in to change notification settings - Fork 168
Fix: Ensure proper format for Bedrock deepseek tool result #3933
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
Fix: Ensure proper format for Bedrock deepseek tool result #3933
Conversation
Signed-off-by: Pavan Yekbote <pybot@amazon.com>
toolMessage.getContent().add(Map.of("text", Map.of(TOOL_CALL_ID, toolUseId, TOOL_RESULT, toolResult.get(TOOL_RESULT)))); | ||
|
||
String textJson = StringUtils.toJson(Map.of(TOOL_CALL_ID, toolUseId, TOOL_RESULT, toolResult.get(TOOL_RESULT))); | ||
toolMessage.getContent().add(Map.of("text", textJson)); |
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.
No unit test?
unit test is failing, check this out
|
toolMessage.getContent().add(Map.of("text", Map.of(TOOL_CALL_ID, toolUseId, TOOL_RESULT, toolResult.get(TOOL_RESULT)))); | ||
|
||
String textJson = StringUtils.toJson(Map.of(TOOL_CALL_ID, toolUseId, TOOL_RESULT, toolResult.get(TOOL_RESULT))); | ||
toolMessage.getContent().add(Map.of("text", textJson)); |
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.
does the value of 'text' has to be string? I saw the return allowing List<Map<String, Object>> toolResults
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.
Supply method returns List<LLMMessage>
This is how the supply result is used
List<LLMMessage> llmMessages = functionCalling.supply(toolResults);
interactions.add(llmMessages.getFirst().getResponse());
This is the getResponse method in Bedrock message (which is an implemention of LLMMessage)
public String getResponse() {
return StringUtils.toJson(Map.of("role", role, "content", content));
}
At the end we are converting back to a string, so it's okay
CI failed,
|
I fixed the failing test, build is running on my local. I'll push the changes once it builds successfully. |
Signed-off-by: Pavan Yekbote <pybot@amazon.com>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3933 +/- ##
============================================
- Coverage 80.37% 80.36% -0.01%
+ Complexity 7917 7915 -2
============================================
Files 693 693
Lines 34864 34865 +1
Branches 3878 3878
============================================
- Hits 28021 28019 -2
- Misses 5103 5104 +1
- Partials 1740 1742 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
* fix: deepseek function calling tool result Signed-off-by: Pavan Yekbote <pybot@amazon.com> * fix: unit test Signed-off-by: Pavan Yekbote <pybot@amazon.com> --------- Signed-off-by: Pavan Yekbote <pybot@amazon.com> (cherry picked from commit 3322ac5)
Description
3.1 introduced function calling support via interface implementations.
Bedrock deepseek does not naturally support function calling and is simulated via prompts.
In order to support this, we need to ensure that the tool result supplied to the model is a json escaped string rather than a proper json itself.
Current (trimmed payload to show what the change achieves)
Expected
Related Issues
Resolves #[Issue number to be closed when this PR is merged]
Check List
--signoff
.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.