Pass s3:// file URLs directly to API in BedrockConverseModel
#3663
Conversation
DouweM
changed the title
s3:// file URLs directly to API in BedrockConverseModel
| format = item.media_type.split('/')[1] | ||
| assert format in ('jpeg', 'png', 'gif', 'webp'), f'Unsupported image format: {format}' | ||
| image: ImageBlockTypeDef = {'format': format, 'source': {'bytes': downloaded_item['data']}} | ||
| image: ImageBlockTypeDef = {'format': format, 'source': cast(Any, source)} |
There was a problem hiding this comment.
Instead of casting this to Any, can we fix the source type hint to be DocumentSourceTypeDef?
| 'name': name, | ||
| 'format': item.format, | ||
| 'source': {'bytes': downloaded_item['data']}, | ||
| 'source': cast(Any, source), |
| """Test that s3:// image URLs are passed directly to Bedrock API without downloading.""" | ||
| m = BedrockConverseModel('us.amazon.nova-pro-v1:0', provider=bedrock_provider) | ||
| agent = Agent(m, system_prompt='You are a helpful chatbot.') | ||
| image_url = ImageUrl(url='s3://my-bucket/images/test-image.jpg', media_type='image/jpeg') |
There was a problem hiding this comment.
I'm guessing these are not real files in S3 and the cassettes were manually modified? That'll get us into trouble if we re-record the cassettes :) So I'd rather either use a real S3 URL (may be hard), or directly test the result of _map_messages as we do in other tests already
There was a problem hiding this comment.
Yeah cassettes were manual (with help of AI of course). I have updated the tests to directly test _map_messages result. Not sure whether I should update the cassettes.
There was a problem hiding this comment.
I think the cassettes can be deleted then
| format = downloaded_item['data_type'] | ||
| source: dict[str, Any] | ||
| if item.url.startswith('s3://'): | ||
| source = {'s3Location': {'uri': item.url}} |
There was a problem hiding this comment.
There's also a bucketOwner field that users may want to set. Maybe we can tell them to encode it as a query param on the URL, and parse it out here?
There was a problem hiding this comment.
Do you mean something like s3://my-bucket/key?bucketOwner=owner?
There was a problem hiding this comment.
Yep that's what I was thinking
| if item.url.startswith('s3://'): | ||
| source = {'s3Location': {'uri': item.url}} | ||
| else: | ||
| downloaded_item = await download_item(item, data_format='bytes', type_format='extension') |
There was a problem hiding this comment.
download_item currently has logic gating for gs:// URLs; let's check s3:// URLs there as well
There was a problem hiding this comment.
It seems the existing code in download_item checks for gs:// and youtube URLs:
if item.url.startswith('gs://'):
raise UserError('Downloading from protocol "gs://" is not supported.')
elif isinstance(item, VideoUrl) and item.is_youtube:
raise UserError('Downloading YouTube videos is not supported.')
What check do you mean for s3:// here?
There was a problem hiding this comment.
Same check raising an error saying that download_item does not support s3:// URLs
There was a problem hiding this comment.
We should document this feature in input.md. At the bottom there's already a section on uploaded files to Google, can you mention S3 files + BedrockConverseModel there as well please?
2 participants
Closes #3621