Generalize README benchmark #327
Conversation
facebook-github-bot
added
the
CLA Signed
|
|
||
| wait(futures) | ||
| for f in futures: | ||
| assert f.result() |
There was a problem hiding this comment.
This ensures that if we encounter an error while generating a video, we stop.
| shutil.rmtree(videos_path) | ||
| ffmpeg_path = "ffmpeg" | ||
| videos_path = "/tmp/torchcodec_benchmarking_videos" | ||
| shutil.rmtree(videos_path, ignore_errors=True) |
There was a problem hiding this comment.
We don't want to fail if the tmp directory does not exist already.
| videos_path, | ||
| ) | ||
| video_paths = glob.glob(f"{videos_path}/*.mp4") | ||
| video_files = glob.glob(f"{videos_path}/*.mp4") |
There was a problem hiding this comment.
I was confused by how close the old variable names were to each other.
There was a problem hiding this comment.
See comment above about rename
ahmadsharif1
left a comment
ahmadsharif1
left a comment
There was a problem hiding this comment.
Minor comment about renames
| videos_path = "/tmp/videos" | ||
| shutil.rmtree(videos_path) | ||
| ffmpeg_path = "ffmpeg" | ||
| videos_path = "/tmp/torchcodec_benchmarking_videos" |
There was a problem hiding this comment.
Maybe this should be renamed to videos_dir_path
I like path over files because it tells the user it's a full path and not the filename only
There was a problem hiding this comment.
Yeah, good point, I'll make one use dir and the other file.
| videos_path, | ||
| ) | ||
| video_paths = glob.glob(f"{videos_path}/*.mp4") | ||
| video_files = glob.glob(f"{videos_path}/*.mp4") |
There was a problem hiding this comment.
See comment above about rename
ahmadsharif1
left a comment
ahmadsharif1
left a comment
There was a problem hiding this comment.
Minor comment about renames
3 participants
I could not run the benchmarks at first; these are the changes I had to make to get them to run fresh.