-
Notifications
You must be signed in to change notification settings - Fork 865
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 moviepy 2 compatibility #747
base: master
Are you sure you want to change the base?
Conversation
On second though using |
For the python 3.11 error, I might have misconfigured the coverage test token. Let me check. |
I've looked at the test and think they do not cover moviepy right? I've tested v2 vs v1; with the setup test it fails downstream in
Converting this back to a draft and checking what changed |
Thank you for the update. I think there are about possible ways to get around the problem. There is one thing that confuses me though. For moviepy v2 the reason is that the test gif is saved as RGB rather than grayscale making the file bigger, why the 1.0.3 is different from the current version I cannot tell. @lanpa could you maybe verify if you would generate a different file nowadays as well? I found different way that allows to pass a 2D grayscale tensor in moviepy v2 as well but that solution is more involved Minor note: The last commit theoretically allows to run the |
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 now modified the tests to not depend on the imageio/pillow versions.
return | ||
try: | ||
from moviepy import editor as mpy | ||
# moviepy v2+ | ||
from moviepy import ImageSequenceClip |
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.
In line 393, using the Version function is a good idea. Instead of trying to import a specific package, should we also use it here to explicitily determine the version of moviepy 1 and moviepy 2?
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've reworked the version parsing to use importlib.metadata
instead. Im undecided what to do with the actual import. I would like to avoid using from moviepy.editor import ...
as this might fail due to a missing requests
module.
We could just use from moviepy.video.io.ImageSequenceClip import ImageSequenceClip
as it will not enter the except clause with moviepy v1. Just in the event of another moviepy refactoring that statement will break.
tests/test_summary.py
Outdated
def _iter_gif(encoded_image): | ||
import io | ||
|
||
from PIL import Image, ImageSequence |
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 this be imported at the beginning of the file?
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.
Yes they should be installed with the test requirements. I put it down there as I was not 100% sure if pillow will be installed with the normal setup.
Hi, I have tested the newest commit with moviepy==2.1.2 (with |
Thank you for checking. Can you tell me you imagio and pillow versions. |
@@ -369,25 +368,49 @@ def video(tag, tensor, fps=4, dataformats="NTCHW"): | |||
|
|||
def make_video(tensor, fps): | |||
try: |
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 I move the imports to the top of the file? I am not sure how you like to handle it.
Python 3.12.0 full packages:
|
Currently moviepy v2+ cannot be used to add videos because the content of
moviepy.editor
was refactored tomoviepy.__init__.py
.This PR now uses just
from moviepy import ImageSequenceClip
instead ofmoviepy.editor.ImageSequenceClip
.Furthermore as fallback to
moviepy
< 2 it will use the direct locationfrom moviepy.video.io.ImageSequenceClip import ImageSequenceClip
instead viamoviepy.editor
which would import more modules and dependencies likerequests
which are not actually necessary and listed in the current error message.