-
Notifications
You must be signed in to change notification settings - Fork 1.3k
feat(bedrock): add VideoUrl input for BedrockConverseModel #1435
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
feat(bedrock): add VideoUrl input for BedrockConverseModel #1435
Conversation
PR Change SummaryEnhanced the BedrockConverseModel to support video input through the new VideoUrl class, along with updated documentation and tests.
Modified Files
How can I customize these reviews?Check out the Hyperlint AI Reviewer docs for more information on how to customize the review. If you just want to ignore it on this PR, you can add the Note specifically for link checks, we only check the first 30 links in a file and we cache the results for several hours (for instance, if you just added a page, you might experience this). Our recommendation is to add |
'format': format, | ||
'source': {'bytes': response.content}, | ||
} | ||
content.append({'image': video}) # type: ignore - Verify |
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 probably need to understand a bit more about content
. It's complaining because VideoBlockTypeDef
supports extra keys for S3 location.
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.
It's because you are passing 'image'
instead of 'video'
.
The CI is failing, let me see what's going on. |
from ._utils import generate_tool_call_id as _generate_tool_call_id, now_utc as _now_utc | ||
from .exceptions import UnexpectedModelBehavior | ||
|
||
AudioMediaType: TypeAlias = Literal['audio/wav', 'audio/mpeg'] |
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 just moved this whole block for better readability.
I don't know why the test coverage is failing. I can't have the same scenario in the local environment, only in CI. I need guidance here. |
'format': format, | ||
'source': {'bytes': response.content}, | ||
} | ||
content.append({'image': video}) # type: ignore - Verify |
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.
It's because you are passing 'image'
instead of 'video'
.
Hi @Kludex, thanks for updating the PR! I forgot to update the |
Thanks for this. Finally a contribution that uses VCR :) (Maybe we need to document better how to use VCR...) |
Thanks a lot for your feedback and review! I'll start working on another PR to support more Bedrock configuration fields, and I can try to improve the VCR documentation... or if you prefer, I can open another issue with that. Valeu!! |
I think there is some random error in the pipeline. Maybe re-run will solve it. |
PRs welcome :) |
I'm checking. It's recent... |
Closes #1411
Summary
This pull request adds a new VideoUrl class that will enable models to use video in their agents. This also include this video support to the
BedrockConverseModel
Changes
Type of Change
Checklist
Hi @Kludex, I'm uploading a short video showing this working.
Screen.Recording.2025-04-10.at.08.34.16.mov