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.
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
Add video models + functions #814
base: main
Are you sure you want to change the base?
Add video models + functions #814
Changes from 4 commits
75877d1
031b9df
548bbd5
b55149a
2cd6d62
5892ab9
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Check warning on line 236 in src/datachain/lib/file.py
Codecov / codecov/patch
src/datachain/lib/file.py#L236
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.
why do we need this separate model?
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.
so, how are these all modes connected with the helpers? how do I instantiate them? do I have to write my own UDFs to do that (just instantiate these classes?)
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'd use some impossible value like -1.0
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.
-1 and -1.0 as defaults?
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.
Should it be a subclass of VideoFile?
Check warning on line 544 in src/datachain/lib/file.py
Codecov / codecov/patch
src/datachain/lib/file.py#L544
Check warning on line 3 in src/datachain/lib/video.py
Codecov / codecov/patch
src/datachain/lib/video.py#L1-L3
Check warning on line 5 in src/datachain/lib/video.py
Codecov / codecov/patch
src/datachain/lib/video.py#L5
Check warning on line 18 in src/datachain/lib/video.py
Codecov / codecov/patch
src/datachain/lib/video.py#L14-L18
Check warning on line 25 in src/datachain/lib/video.py
Codecov / codecov/patch
src/datachain/lib/video.py#L25
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.
Could you please avoid using erm
meta
? How aboutfile_to_video(file: File)
?Btw... not just File as input type?
Check warning on line 36 in src/datachain/lib/video.py
Codecov / codecov/patch
src/datachain/lib/video.py#L35-L36
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 like this part, it looks like we are reading video file twice here. Need to check the other way to get video meta information.
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.
yep, also are we reading the whole file to get meta?
Check warning on line 41 in src/datachain/lib/video.py
Codecov / codecov/patch
src/datachain/lib/video.py#L38-L41
Check warning on line 43 in src/datachain/lib/video.py
Codecov / codecov/patch
src/datachain/lib/video.py#L43
Check warning on line 53 in src/datachain/lib/video.py
Codecov / codecov/patch
src/datachain/lib/video.py#L53
Check warning on line 65 in src/datachain/lib/video.py
Codecov / codecov/patch
src/datachain/lib/video.py#L65
Check warning on line 67 in src/datachain/lib/video.py
Codecov / codecov/patch
src/datachain/lib/video.py#L67
Check warning on line 70 in src/datachain/lib/video.py
Codecov / codecov/patch
src/datachain/lib/video.py#L70
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.
we usually use jpg in the codebase, not jpeg
Check warning on line 83 in src/datachain/lib/video.py
Codecov / codecov/patch
src/datachain/lib/video.py#L82-L83
Check warning on line 86 in src/datachain/lib/video.py
Codecov / codecov/patch
src/datachain/lib/video.py#L86
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.
Should we really support Path?
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.
jpg
Check warning on line 102 in src/datachain/lib/video.py
Codecov / codecov/patch
src/datachain/lib/video.py#L101-L102
Check warning on line 105 in src/datachain/lib/video.py
Codecov / codecov/patch
src/datachain/lib/video.py#L105
Check warning on line 124 in src/datachain/lib/video.py
Codecov / codecov/patch
src/datachain/lib/video.py#L124
Check warning on line 127 in src/datachain/lib/video.py
Codecov / codecov/patch
src/datachain/lib/video.py#L127
Check warning on line 129 in src/datachain/lib/video.py
Codecov / codecov/patch
src/datachain/lib/video.py#L129
Check warning on line 131 in src/datachain/lib/video.py
Codecov / codecov/patch
src/datachain/lib/video.py#L131
Check warning on line 135 in src/datachain/lib/video.py
Codecov / codecov/patch
src/datachain/lib/video.py#L135
Check warning on line 140 in src/datachain/lib/video.py
Codecov / codecov/patch
src/datachain/lib/video.py#L140
Check warning on line 142 in src/datachain/lib/video.py
Codecov / codecov/patch
src/datachain/lib/video.py#L142
Check warning on line 145 in src/datachain/lib/video.py
Codecov / codecov/patch
src/datachain/lib/video.py#L144-L145
Check warning on line 148 in src/datachain/lib/video.py
Codecov / codecov/patch
src/datachain/lib/video.py#L148
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 a lot of these helpers become part of the
Video*
classes?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.
Good question 👍 I was thinking about this and tried to implement it this way, but in the end I've checked other types and files in
lib
module (images, hf) and make it the same way.I was also thinking and trying to move all the models to the
datachain.model
module, but it turns out it needs more work and may be not backward compatible withFile
model. In is a subject for a separate PR.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.
yeah, we need all of theses to become methods of Video class. Should it be a followup or in this PR?
I'd appreciate more insights on the issues with this approach.
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.
jpg
Check warning on line 169 in src/datachain/lib/video.py
Codecov / codecov/patch
src/datachain/lib/video.py#L169
Check warning on line 172 in src/datachain/lib/video.py
Codecov / codecov/patch
src/datachain/lib/video.py#L172
Check warning on line 194 in src/datachain/lib/video.py
Codecov / codecov/patch
src/datachain/lib/video.py#L194
Check warning on line 200 in src/datachain/lib/video.py
Codecov / codecov/patch
src/datachain/lib/video.py#L197-L200
Check warning on line 203 in src/datachain/lib/video.py
Codecov / codecov/patch
src/datachain/lib/video.py#L203
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 looks like it needs to be renamed to
save_subvideo()
In the class names, we use term
Clip
for virtual videos (start-end) while in this case you are creating just another Video, not clip.So, it needs to be renamed or we need to avoid this Clip-as-virtual-reference terminology.
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 would be great to generalize the single and plural methods. We just need to come up with output format like
output="{name}{:06d}.{ext}")
and provide a string in case of a single file.Also, this method will require generalization for writing to cloud like
output={source}/tmp/{name}{:06d}.{ext}
Check warning on line 222 in src/datachain/lib/video.py
Codecov / codecov/patch
src/datachain/lib/video.py#L222
Check warning on line 225 in src/datachain/lib/video.py
Codecov / codecov/patch
src/datachain/lib/video.py#L225
Check warning on line 229 in src/datachain/lib/video.py
Codecov / codecov/patch
src/datachain/lib/video.py#L227-L229
Check warning on line 232 in src/datachain/lib/video.py
Codecov / codecov/patch
src/datachain/lib/video.py#L232
Check warning on line 253 in src/datachain/lib/video.py
Codecov / codecov/patch
src/datachain/lib/video.py#L252-L253
Check warning on line 255 in src/datachain/lib/video.py
Codecov / codecov/patch
src/datachain/lib/video.py#L255
Check warning on line 260 in src/datachain/lib/video.py
Codecov / codecov/patch
src/datachain/lib/video.py#L259-L260
Check warning on line 263 in src/datachain/lib/video.py
Codecov / codecov/patch
src/datachain/lib/video.py#L263
Check warning on line 266 in src/datachain/lib/video.py
Codecov / codecov/patch
src/datachain/lib/video.py#L266
Check warning on line 269 in src/datachain/lib/video.py
Codecov / codecov/patch
src/datachain/lib/video.py#L269
Check warning on line 271 in src/datachain/lib/video.py
Codecov / codecov/patch
src/datachain/lib/video.py#L271
Check warning on line 273 in src/datachain/lib/video.py
Codecov / codecov/patch
src/datachain/lib/video.py#L273