-
Notifications
You must be signed in to change notification settings - Fork 325
fix: Remove status field from toolResult for non-claude 3 models in Bedrock model provider #686
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?
Conversation
…edrock model provider
…edrock model provider
Note: During my testing, I was able to test with multiple models and I had varying results in comparison with the issue reporter in #554. For example, I was able to get successful tool execution with the
More interestingly, after this change I am seeing the model
I suspect the issue is that Writer Palmyra X4 and X5 have different tool calling implementations in Amazon Bedrock. Palmyra X4 properly supports tool results and can process toolResult blocks with just toolUseId and content fields, while Palmyra X5 appears to have a different tool calling implementation that fails to process tool results entirely, regardless of the field |
I am able to use the status field with |
…Bedrock model provider
…Bedrock model provider
Hey @mehtarac thanks a lot for working on this. Out of curiosity, when did you run these tests with Palmyra X5? We swapped the version of X5 on Bedrock on Friday the 15th at some point. Could you retest if it was on or before that day and see if it changes anything? EDIT: We were able to reproduce the issue on our side and have RCA'd it. There has been a fix provided upstream to AWS. Once AWS updates the container we should be good with this. |
…edrock model provider
…edrock model provider
src/strands/types/tools.py
Outdated
@@ -91,7 +91,7 @@ class ToolResult(TypedDict): | |||
""" | |||
|
|||
content: list[ToolResultContent] | |||
status: ToolResultStatus | |||
status: NotRequired[ToolResultStatus] |
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.
Do we need to modify this? It seems like globally we always want ToolResult to contain status - and consumers may depend on status being present.
Can we just perform the filtering within the bedrock when we send the data to bedrock without propagating the issue throughout our code.
Meaning if from a tool I want to pass a ToolResult, we internally still require status to be provided
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.
Not sure I follow fully, but based on what I understand and experienced during testing -- there are some models available on bedrock that don't support the status
field. In order to mitigate that, we do the filtering in bedrock.py
. The change here to the type was needed to pass the linter where the status
field is required in all cases. But since we have models that don't require status field, we don't require the status field. We DO return the status field for all models that accept the status
field.
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 agree with @dbschmigelski here. ToolResult
is the type that any result of a "StrandsTool" needs to return. If someone implements a custom tool, we want to require them to return the status
field as part of the tools execution. We can leave this code unchanged, and still filter out status when calling bedrock.
status: NotRequired[ToolResultStatus] | |
status: ToolResultStatus |
Hey all, just to update: we've successfully updated Palmyra X5 on Bedrock and are seeing tool calling working as expected. Thank you for the catch on this! We are really hoping to be able to use Palmyra via Bedrock in Strands, so thanks for this. |
Thank you for the fix @samjulien!!! I'll test the model again! |
…edrock model provider
…edrock model provider
src/strands/types/tools.py
Outdated
@@ -91,7 +91,7 @@ class ToolResult(TypedDict): | |||
""" | |||
|
|||
content: list[ToolResultContent] | |||
status: ToolResultStatus | |||
status: NotRequired[ToolResultStatus] |
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 agree with @dbschmigelski here. ToolResult
is the type that any result of a "StrandsTool" needs to return. If someone implements a custom tool, we want to require them to return the status
field as part of the tools execution. We can leave this code unchanged, and still filter out status when calling bedrock.
status: NotRequired[ToolResultStatus] | |
status: ToolResultStatus |
…edrock model provider
…edrock model provider
…edrock model provider
Description
Removed the
status
field from thetoolResult
block for non claude-3 models in the Bedrock Model provider. This change is motivated by the linked issue. Furthermore, this change is supported by the bedrock documentation where it explicitly states that thestatus
field is only supported in claude 3 models.Related Issues
#554
Documentation PR
Type of Change
Bug fix
Testing
Script used to test:
hatch run prepare
Checklist
Sources:
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.