-
Notifications
You must be signed in to change notification settings - Fork 70
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
Videolab: Minimum Viable Class #231
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #231 +/- ##
===========================================
- Coverage 95.63% 84.06% -11.58%
===========================================
Files 16 17 +1
Lines 986 1123 +137
Branches 194 214 +20
===========================================
+ Hits 943 944 +1
- Misses 22 158 +136
Partials 21 21 ☔ View full report in Codecov by Sentry. |
pyproject.toml
Outdated
dependencies = [ | ||
"Pillow>=9.3", | ||
"av>=10.0.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.
dependencies = [ | |
"Pillow>=9.3", | |
"av>=10.0.0", | |
dependencies = [ # optional: av>=10.0.0 | |
"Pillow>=9.3", |
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.
suggest making av
an optional dependency, so the package still works for image data without it installed
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 looks good, instead of av
, you could use the video
keyword.
src/cleanvision/videolab.py
Outdated
from pathlib import Path | ||
from typing import Any, Dict, Generator, List, Optional | ||
|
||
import av |
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.
import av |
src/cleanvision/videolab.py
Outdated
# create frame samples sub directory | ||
sample_sub_dir = self._create_frame_sample_sub_dir(video_file, output_dir) | ||
|
||
# open video file for streaming |
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.
# open video file for streaming | |
# open video file for streaming | |
try: | |
import av | |
except ImportError as error: | |
raise ImportError( | |
"Cannot import package `av`. " | |
"Please install it via `pip install av` and then try again." | |
) from error | |
import av |
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 lazy import the optional dependency av
only where it's required
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 can move this line of code wherever is the first time a user would do something related to videolab. I.e. when they construct the object.
The reason I don't think this line of code can be at the top of the file is because the file will be imported with: import cleanvision
And thus this exception will be raised, even for users who didn't want to use Videolab.
So you can really put this code anywhere that makes it possible for users to run Imagelab without having av installed (which you can easily test yourself), while ensuring the exception gets raised as soon as such users try to run Videolab.
@@ -0,0 +1,260 @@ | |||
"""Videolab is an extension of Imagelab for finding issues in a video dataset.""" |
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.
amazing stuff!
Could you comment:
A minimal example script of how to use this new Videolab and what the outputs look like?
A comment with comprehensive list of limitations of this code?
Eg:
- What video file-type requirements are there.
- What happens if some videos are really long some really short.
- any efficiency bottlenecks where the code feels slow
- edge cases where the results might not be good or code might crash.
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 great! That will super useful while our team is reviewing the current code
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.
Great callout, ya replacing the FrameSampler will certainly need to be improved before we publicly announce this module to users. But I don't think it is critical to this PR being merged and could be addressed in a future follow-up PR.
Thanks for the comprehensive benchmarking/profiling, very helpful! Our team will get to the code review as soon as we're able to
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.
Great job, if you need me to optimise this, let me know, there are some avenues to explore here.
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.
@lafemmephile Do you want to make one of your notebooks here:
https://github.com/lafemmephile/videolab/tree/master/notebooks
the official quickstart tutorial for Videolab?
Here's a GH issue detailing what we need for our quick-start tutorial:
#239
Should be a simple PR of one of your existing notebooks, which you can make separately from the Videolab source code PR in parallel, if you're interested!
Hi @lafemmephile ! This PR is a great start to implementing Videolab. I looked at the PR and will be posting my thoughts here. For the question of how to keep filenames for images, hashing, etc. I suggest we use a VideoDataset class, that maintains the mapping between numeric indices like [0,1,2,3] to video_paths. This would help us in supporting both Using this index you can retrieve, video path, hence, video frames and the video itself. For saving frames, you could use the dir structure
|
I think frame sampler is okay for now. We can take up the performance of frame sampler and also the strategy to sample frames in a separate PR, when we benchmark this on different datasets. |
@lafemmephile Is there a specific reason you inherited Videolab from Imagelab? I think we could just have an |
@lafemmephile |
For visualization of videos with issues, we could construct gifs from the extracted frames, I got the idea from here. |
@lafemmephile for faster iteration over the code and checking if the issues make any sense, I suggest you create a test dataset for videos similar to the one we have for images. For starters just add one video of each issue type and see how it works. |
That's correct @lafemmephile |
It would be nice to have all the public methods in |
Hi @lafemmephile ! That is some amazing work. Could you elaborate on what difficulty you are facing in making videos playable? And does playable means you click and play, or it plays as soon as it is plotted? It could be the case that it requires some frontend support from jupyter notebook. We could start with frames, where we don't plot all the say |
@lafemmephile We want to wrap up this PR soon, as I will be on PTO from second week of December and wouldn't be available during that time. Would it be okay if I take the reins of this PR from here? I think you have done some phenomenal contribution. Could you please tell if there are any pending in-progress contributions from your side? |
Abstract
The purpose of issue #215 was to implement a minimal extension of the
Imagelab
class. I believe that at this point, per the criteria listed here, we are now very close to achieving the aforementioned extension. While I fully expect there will be specific issues that thecleanvision
maintainers will want to address, the extension is mature enough to begin the review process.Implementation Strategy
In an effort to keep to the original goal of the extension laid out by @jwmueller, I have not attempted any additional features (including any default arguments, or error checking). I have only implemented exactly that which is necessary to fulfill the three tenets:
1. just extract every k-th frame from the video,
2. run cleanvision on all those images,
3. aggregate the results across the frames extracted per video, into scores per video.
Tenet 1 was implemented in quite a simple way, with the
FrameSampler
class (which is a simplified version of the VideoSampler written by @LemurPwned). Again you will notice I limited any excess features except where necessary, in order to keep to the minimal design and limited scope of issue #215.Tenet 2 and 3 were achieved with the
Videolab
class. Again, where possible I have limited any excess features or additional design choices, preferring to defer to thecleanvision
maintainers for specifics.Conclusion
Thank you to @smttsp @LemurPwned @sanjanag and @jwmueller for their contributions and perspective on extending Cleanvision to run on video data.