-
Notifications
You must be signed in to change notification settings - Fork 461
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
base: main
Are you sure you want to change the base?
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?
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.