-
Notifications
You must be signed in to change notification settings - Fork 535
fix: add helper to handle base64 conversion to raw bytes for boto3 converse_stream
#940
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
Conversation
converse_stream
|
@Unshure I think it is a simple fix. Could you kindly have a look? |
| else: # "auto" | ||
| return any(model in self.config["model_id"] for model in _MODELS_INCLUDE_STATUS) | ||
|
|
||
| def _coerce_to_bytes(self, value: Any, *, expected_fmt: Optional[str] = None) -> bytes: |
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'm not so sure this logic should exist in the BedrockModel provider. The expected type of ["source"]["bytes"] is bytes (src) and so users should be configuring this before passing the payload into Strands. Also, this would be a problem for other model providers as well.
I'm thinking that this logic should go into https://github.com/strands-agents/sdk-python/blob/main/src/strands/multiagent/a2a/executor.py if we are trying to resolve #850.
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 started with executor.py but moved it to bedrock.py because this is a requirement from the Bedrock API and has nothing to do with the a2a protocol. The point about other model provides is a good one so may be we should check what provider it is before coercing it. WDYT?
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 would say this does have to do with A2A as the bytes coming in through the payloads are likely to be base64 encoded.
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.
Indeed, the bytes are Base64-encoded since JSON-RPC is used as the serialization protocol for A2A server. But their conversion to raw bytes is a requirement of Bedrock. Hence implementing it on the Bedrock side. Or did I miss your point?
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.
@pgrayy any thoughts?
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.
What I was proposing is now addressed in #1195. This solution will work for all model providers and does not affect the typing contract, which specifies that source is of type bytes.
Note, it may be still worthwhile to accept a b64 source string, but that would require more updates than provided here. We would also need to update the source content block type, which would also require updates across all model providers.
|
I’m encountering the same issue as well. I’m sending the PDF content in base64-encoded format to the Anthropic model via Bedrock and I receive the this error: "Failed to generate permutations: An error occurred (ValidateException) when calling the ConverseStream operation: The detected filetype is PLAIN_TEXT, but the provided filetype was PDF." It seems it is interpreting the base64 string as plain text rather than recognizing it as a PDF file. When i try to pass byte directly, it tells that the content must be in base64 format. |
Description
document,image, andvideopayloads to raw bytes before sending them to Bedrock so base64 strings no longer trigger “detected filetype is PLAIN_TEXT”.document.sourceinput types and fail fast on plain text.Related Issues
Type of Change
Bug fix
Testing
hatch testChecklist
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.