-
Notifications
You must be signed in to change notification settings - Fork 645
Feature: Allow s3Location as Document, Image, and Video location source #1572
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
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
2e47315 to
e17e8bb
Compare
9c2aca3 to
a4874cb
Compare
a4874cb to
b4a8f0a
Compare
b4a8f0a to
97f435d
Compare
97f435d to
b34f588
Compare
b34f588 to
27d9463
Compare
1b60ecc to
e85b6eb
Compare
e85b6eb to
b590357
Compare
b590357 to
4029cf4
Compare
4029cf4 to
2193161
Compare
2193161 to
76b1b0b
Compare
76b1b0b to
c1c9cc4
Compare
mkmeral
left a comment
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.
Looks good, based on agent feedback, I'd just change the title before merging
Agent feedback:
Minor Suggestions (Non-blocking)
-
PR title should follow conventional commit format:
feat(models): add S3 location support... -
Add note in docstrings that
s3Locationis Bedrock-only for now -
Consider helper method to reduce code duplication in
bedrock.py
Description
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.
One decision I made in this pr is the choice between a generic
Locationsource, vs the more specificS3Locationsource. Some model providers support url for a document location (ref), so it could make sense to make this generic for s3 as well. But since S3 also has an optionalbucketOwner, I thought it would make more sense to have a separateS3Location, and then later add something like aURLLocationto cover the openai case.What Changed
Type System Updates
ImageSource,DocumentSource, andVideoSourceintypes/media.pyto accepts3Locationas an alternative tobytesS3LocationTypedDict with requiredurifield and optionalbucketOwnerfor cross-account accessProvider Behavior
Updated Integ Tests
I added integ tests which create an s3 bucket (post-fixed with the accountId) where the local file resources are uploaded for testing. Then they are referenced in the tests to ensure the model can recognize and identify the content in them.
Ive also added a short 5 second video resource to the integ tests to cover video modalities
Usage Example
Notes on backwards compatibility:
This change updates
ImageSource,DocumentSource, andVideoSourceto usetotal=Falsein their typed dict definition, ultimately making thebytesparameter optional now when it was required before. This is considered safe since all existing code should not be impacted by this. However, going forward, since these fields are essentially union types now, they need to be treated as such by customers going forward.Related Issues
#1482
Documentation PR
TODO
Type of Change
New feature
Testing
How have you tested the change? Verify that the changes do not break functionality or introduce warnings in consuming repositories: agents-docs, agents-tools, agents-cli
hatch run prepareChecklist
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.