Skip to content
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

Update pyav_reader.py to make generator threadsafe #395

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

Callum-Welsh
Copy link

I had an issue where code using pyav_reader.py would randomly break and throw a "ValueError: Generator Already Executing Error"; the stack indicated that the function _gen_frames was the culprit; I added a function and a decorator (line for line copied from the sources below with one change, re-naming "next" to "next") that makes a non-threadsafe generator threadsafe. This resolved the issue on both Windows and Mac. I thought I'd send it over to you guys for your perusal! Thanks for the great package!

The original error message: Screen Shot 2021-08-28 at 3 40 56 PM

Sources:
https://anandology.com/blog/using-iterators-and-generators/
https://stackoverflow.com/questions/41194726/python-generator-thread-safety-using-keras

I had an issue where code using pyav_reader.py would randomly break and throw a "ValueError: Generator Already Executing Error"; the stack indicated that the function _gen_frames was the culprit; I added a function and a decorator (line for line copied from the sources below with one change, re-naming "next" to "__next__") that makes a non-threadsafe generator threadsafe. This resolved the issue on both Windows and Mac.

Sources:
https://anandology.com/blog/using-iterators-and-generators/
https://stackoverflow.com/questions/41194726/python-generator-thread-safety-using-keras
@@ -2,6 +2,7 @@
unicode_literals)

import numpy as np
import threading
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Forgot to add the import for threading to the original pull request!

Sometimes the self.it.next() method was breaking; looks like that method might be an artifact of older versions of Python? Fixed it to modern spec to prevent future errors.
@tacaswell tacaswell closed this Apr 7, 2022
@tacaswell tacaswell reopened this Apr 7, 2022
@tacaswell
Copy link
Member

"powercycled" to get CI.

@codecov-commenter
Copy link

Codecov Report

❗ No coverage uploaded for pull request base (main@68c560e). Click here to learn what that means.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main     #395   +/-   ##
=======================================
  Coverage        ?   63.01%           
=======================================
  Files           ?       31           
  Lines           ?     4597           
  Branches        ?        0           
=======================================
  Hits            ?     2897           
  Misses          ?     1700           
  Partials        ?        0           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 68c560e...53f2518. Read the comment docs.

def threadsafe_generator(f):
"""A decorator that takes a generator function and makes it thread-safe.
"""
def g(*a, **kw):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add functools.wraps(f) here as well?

@tacaswell
Copy link
Member

@BakudanKame Sorry for the very slow review!

This seems reasonable, but I am a bit wary of tacking locks onto things, is there a reasonable way to let the user opt-into the locking/thread safety?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants