feat(media): add S3 location support to image, document, and video content types#11
feat(media): add S3 location support to image, document, and video content types#11github-actions[bot] wants to merge 2 commits intomainfrom
Conversation
src/strands/models/llamacpp.py
Outdated
| def _has_s3_source(self, content: ContentBlock) -> bool: | ||
| """Check if a content block contains an S3 source. | ||
|
|
||
| Args: | ||
| content: Content block to check. | ||
|
|
||
| Returns: | ||
| True if the content block contains an S3 source, False otherwise. | ||
| """ | ||
| if "image" in content: | ||
| return "s3Location" in content["image"].get("source", {}) | ||
| if "document" in content: | ||
| return "s3Location" in content["document"].get("source", {}) | ||
| if "video" in content: | ||
| return "s3Location" in content["video"].get("source", {}) | ||
| return False | ||
|
|
There was a problem hiding this comment.
Can you turn this into a centralized helper method, and update the pr to use it
There was a problem hiding this comment.
+1. Consider adding this as a helper function in src/strands/types/media.py since it operates on the media content types defined there, or in a shared utilities module under src/strands/models/. That would keep the model implementations DRY and ensure consistent behavior across all providers.
There was a problem hiding this comment.
Done - centralized as _has_s3_source() in src/strands/models/_validation.py and all providers now use it.
tests/strands/models/test_openai.py
Outdated
|
|
||
| def test_format_request_filters_s3_source_image(model, caplog): | ||
| """Test that images with S3 sources are filtered out with warning.""" | ||
| import logging |
tests/strands/models/test_openai.py
Outdated
|
|
||
| def test_format_request_filters_s3_source_document(model, caplog): | ||
| """Test that documents with S3 sources are filtered out with warning.""" | ||
| import logging |
tests/strands/models/test_openai.py
Outdated
|
|
||
| def test_has_s3_source_detection(): | ||
| """Test _has_s3_source class method correctly detects S3 sources.""" | ||
| from strands.models.openai import OpenAIModel |
|
/strands |
|
/strands review |
Review SummaryAssessment: Request Changes Key Themes:
The feature implementation looks solid - the Bedrock S3 handling is correct, and the warning/skip pattern for unsupported providers is a good approach. Once the above items are addressed, this PR will be ready to merge. Nice work on the comprehensive coverage across all model providers! 🎉 |
|
/strands |
8019d5e to
107837a
Compare
107837a to
625d338
Compare
625d338 to
2e47315
Compare
2e47315 to
e17e8bb
Compare
|
/strands can you add unit tests to this pull request to cover the missing model providers? You should be able to add a simple test to most model providers that passes in an s3location content block, and asserts that a warning is thrown and the content is removed in the api call. |
9c2aca3 to
a4874cb
Compare
e85b6eb to
b590357
Compare
b590357 to
4029cf4
Compare
4029cf4 to
2193161
Compare
2193161 to
76b1b0b
Compare
76b1b0b to
c1c9cc4
Compare
664745d to
ab51706
Compare
Summary
Adds native S3 location support for media content types in Bedrock, allowing users to reference images, documents, and videos stored in S3 directly rather than requiring base64 encoding.
Why
Currently users must download media from S3 and base64-encode it before sending to Bedrock. This feature enables direct S3 references, reducing memory usage and simplifying workflows for S3-hosted content. This is particularly valuable for large media files where base64 encoding is impractical.
What Changed
Type System Updates
ImageSource,DocumentSource, andVideoSourceintypes/media.pyto accepts3Locationas an alternative tobytesS3LocationTypedDict with requiredurifield and optionalbucketOwnerfor cross-account accessProvider Behavior
Code Consolidation
_validation.has_s3_source()helper function_has_s3_source()methods from individual model providersUsage Example
Testing
Resolves #10