-
Notifications
You must be signed in to change notification settings - Fork 370
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
Small updates to enable Cython 3 #1145
Conversation
Mostly it seems that cython wants you to declare things as `noexcept` Closes PyAV-Org#1140 I'm going to see if I can report the bug to cython, but it is hard for me to create a minimum reproducible example
Tested locally and cython 0.29.36 and it seems to compile and function just as well. |
av/video/format.pyx
Outdated
# Cython 3 seems to have trouble with cdef tuples, so we use a list | ||
# it complains about some const identifier not being able to get assigned |
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.
FWIW, this issue was fixed in Cython 3.0.1, so this workaround can probably be removed.
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.
If this isn't needed can we prune it please?
It works for me locally, too. I hope this gets merged soon! |
What's needed to get this merged? |
With PyAV's bindings to FFMPEG it's trivial to offload decoding to FFMPEG. While fun, it wasn't feasible to write a decoder for every format used in QTVR files. The self writter decoders also had slight color differences with respect to FFMPEG. Note: PyAV needs to be installed by hand, see the README for more information. Long story short, there are two issues: 1. Regular PyAV from PyPI doesn't expose `bits_per_coded_sample` on codecs. PR 1162 resolves the issue: PyAV-Org/PyAV#1162 2. PyAV can't be build easily. Missing is Cython 3 support from the source. Apply this PR PyAV-Org/PyAV#1145
Co-maintainers, see #1186 |
@@ -107,9 +107,16 @@ def extract(path, **kwargs): | |||
c_string_encoding='ascii', | |||
) | |||
|
|||
context = options.create_context() | |||
context = Context( |
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.
How do these changes relate to the 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.
cython3 removed create_context, not sure when so I looked at the old implementation of create_context and copied it 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.
Do we have to add this in the dev folder? There no such folder available inside transformers
Can you point us to some Cython docs explaining why these changes are required, and if there are others we need to look out for? |
I'm not sure there is much to point to:
|
It's explained here: https://cython.readthedocs.io/en/latest/src/userguide/migrating_to_cy30.html#exception-values-and-noexcept |
i'm not sure why the tests are failing. |
To pull in changes PyAV-Org/PyAV#1145 that fixes bug in installation on Cython 3.02 (Fedora 39 and python 3.12).
Mostly it seems that cython wants you to declare things as
noexcept
Closes #1140
I'm going to see if I can report the bug to cython, but it is hard for me to create a minimum reproducible example