-
-
Notifications
You must be signed in to change notification settings - Fork 284
Add video file support #405
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
Merged
Merged
Changes from 9 commits
Commits
Show all changes
14 commits
Select commit
Hold shift + click to select a range
7a8daa1
Add video file support to attachments
arnodirlam 81c8aba
Update docs
arnodirlam a218e70
Add tests to RubyLLM::Chat (instead of RubyLLM::ActiveRecord::ActsAs)…
arnodirlam 4a8a5f5
Add models `supports_video?` helper; Docs: Clarify vision vs video su…
arnodirlam 710c382
add documentation in appropriate files; apply review comments
altxtech bf490b8
Add gemini-2.5-flash to video models to test
altxtech feeb133
changes as per PR review comment on #260
altxtech 7d1d5f4
remove unnecessary rubocop:disable rule
altxtech a97e059
remove trailing comma
altxtech 56ece26
Merge branch 'main' into feature/video-file-support
crmne 27064cc
Merge branch 'main' into feature/video-file-support
altxtech c95c4d1
Merge branch 'main' into feature/video-file-support
crmne 68b27cd
Added VertexAI tests
crmne ff96a42
Merge branch 'main' into feature/video-file-support
crmne File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Binary file not shown.
91 changes: 91 additions & 0 deletions
91
...s/vcr_cassettes/chat_video_models_gemini_gemini-2_0-flash_can_understand_local_videos.yml
Large diffs are not rendered by default.
Oops, something went wrong.
156 changes: 156 additions & 0 deletions
156
...t_video_models_gemini_gemini-2_0-flash_can_understand_remote_videos_without_extension.yml
Large diffs are not rendered by default.
Oops, something went wrong.
90 changes: 90 additions & 0 deletions
90
...s/vcr_cassettes/chat_video_models_gemini_gemini-2_5-flash_can_understand_local_videos.yml
Large diffs are not rendered by default.
Oops, something went wrong.
155 changes: 155 additions & 0 deletions
155
...t_video_models_gemini_gemini-2_5-flash_can_understand_remote_videos_without_extension.yml
Large diffs are not rendered by default.
Oops, something went wrong.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Can we add an expectation that it recognizes the actual content of the video, like at least includes the words "woman" and "beach"?
Uh oh!
There was an error while loading. Please reload this page.
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 don't think so, 2 reasons:
A bit out of scope. No other tests of this spec do this. If we were to make that change, it would better to do it for all tests for consistency, probably on a separate PR
My understanding is these are more of a boundary interface test. In other words, we are testing if the lib correctly interacts with the providers (sends valid requests), and obtains responses. We are not testing the capability of the models themselves.
But I'll wait on more comments. If more people thinks it makes sense, I can add the assertions.
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.
You're right about it not existing in this spec file. I'm a bit surprised about this though as it does exist in the spec for the text models.
ruby_llm/spec/ruby_llm/chat_spec.rb
Line 16 in fa10f0c
ruby_llm/spec/ruby_llm/chat_spec.rb
Line 37 in fa10f0c
ruby_llm/spec/ruby_llm/chat_spec.rb
Line 40 in fa10f0c
I do like how this makes the specs ensure that the models being used actually accomplish the user's intended purpose. But not a showstopper for me.
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.
You have a good point, but there is also some common wisdom that a project does not need to test the functionality of its external dependencies.
This depends a little on the testing philosophy of this project, I prefer to wait on maintainer feedback before making this change.
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 question to ponder. There's value in adding the checks for content as it tests if the LLM has actually received the file. The only problem is understanding: we don't want to test that. Since I don't think we can separate the two I'm leaning towards checking the content too. We should probably add similar content checks to the rest of the spec, but perhaps in another PR.