-
Notifications
You must be signed in to change notification settings - Fork 808
feat(bedrock-model): Support content and text DocumentSource #2135
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?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -534,16 +534,22 @@ def _format_request_message_content(self, content: ContentBlock) -> dict[str, An | |||||||||||||||
| if "format" in document: | ||||||||||||||||
| result["format"] = document["format"] | ||||||||||||||||
|
|
||||||||||||||||
| # Handle source - supports bytes or location | ||||||||||||||||
| # Handle source - supports bytes, content, location, or text | ||||||||||||||||
| if "source" in document: | ||||||||||||||||
| source = document["source"] | ||||||||||||||||
| formatted_document_source: dict[str, Any] | None | ||||||||||||||||
| if "location" in source: | ||||||||||||||||
| formatted_document_source = self._handle_location(source["location"]) | ||||||||||||||||
| document_source = document["source"] | ||||||||||||||||
| formatted_document_source: dict[str, Any] | None = None | ||||||||||||||||
| if "location" in document_source: | ||||||||||||||||
| formatted_document_source = self._handle_location(document_source["location"]) | ||||||||||||||||
| if formatted_document_source is None: | ||||||||||||||||
| return None | ||||||||||||||||
| elif "bytes" in source: | ||||||||||||||||
| formatted_document_source = {"bytes": source["bytes"]} | ||||||||||||||||
| elif "bytes" in document_source: | ||||||||||||||||
| formatted_document_source = {"bytes": document_source["bytes"]} | ||||||||||||||||
| elif "text" in document_source: | ||||||||||||||||
| formatted_document_source = {"text": document_source["text"]} | ||||||||||||||||
| elif "content" in document_source: | ||||||||||||||||
| formatted_document_source = { | ||||||||||||||||
| "content": [{"text": item["text"]} for item in document_source["content"] if "text" in item] | ||||||||||||||||
| } | ||||||||||||||||
|
Comment on lines
+550
to
+552
|
||||||||||||||||
| formatted_document_source = { | |
| "content": [{"text": item["text"]} for item in document_source["content"] if "text" in item] | |
| } | |
| formatted_content = [{"text": item["text"]} for item in document_source["content"] if "text" in item] | |
| if not formatted_content: | |
| return None | |
| formatted_document_source = {"content": formatted_content} |
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.
When an empty content list is passed in, botocore throws an exception:
botocore.errorfactory.ValidationException: An error occurred (ValidationException) when calling the ConverseStream operation: The model returned the following errors: messages.0.content.1.document.source.content.content: Field required
I see value in not passing content (returning None) when there is actually no content.
Not sure what the use case of a user providing an empty source document is, so it can be filtered out. I think if this path is chosen then the same approach should be used for the document source being None (dropping, since there is no content). I don't think a user would want this to error out, but also would be good to know that malformed documents are being provided and not being silently hidden.
Copilot
AI
Apr 16, 2026
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.
formatted_image_source is only assigned in the location/bytes branches. If an image block includes a source without either key (e.g., empty dict), this will raise UnboundLocalError when building result. Initialize formatted_image_source to None and either return None (skip block) or raise a TypeError when neither supported key is present.
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.
This leaves current behavior to keep change set smaller, this area was only updated to address mypy linter concerns. If needed, I can update this area, but will increase the logical scope of the changes.
Copilot
AI
Apr 16, 2026
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.
formatted_video_source is only assigned in the location/bytes branches. If a video block includes a source without either key, this will raise UnboundLocalError when building result. Initialize formatted_video_source to None and either return None (skip block) or raise a TypeError when neither supported key is present.
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.
This leaves current behavior to keep change set smaller, this area was only updated to address mypy linter concerns. If needed, I can update this area, but will increase the logical scope of the changes.
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.
When
document["source"]is present but does not contain any of the recognized keys (location,bytes,text,content),formatted_document_sourceremainsNoneand the request is sent with"source": None. This will violate Bedrock’s schema (source should be an object) and can lead to a runtime validation error. Consider either (a) returningNoneto drop the whole document block (consistent with the unsupported-location path), or (b) only settingresult["source"]whenformatted_document_sourceis notNoneand otherwise raising aTypeError/logging + skipping.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.
This is a good point to raise, the
sourcefield is always required in a DocumentSource (per docs), and whensourceis passed asNonea botocore exception is raised:This is the current behavior which I was not modifying, so based on any review provided, I can follow the
locationhandling of dropping, raising an exception in Strands, or allowing botocore to handle it. I personally think leaving the current implementation is best, allowing botocore/aws to bubble up the correct exception, not dealing with that in Strands itself.