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

Drop ffmpeg-python dependency and call ffmpeg directly. #1242

Merged
merged 10 commits into from
May 4, 2023

Conversation

petterreinholdtsen
Copy link
Contributor

The last ffmpeg-python module release was in 2019[1], upstream seem to be unavailable[2] and the project development seem to have stagnated[3]. As the features it provide is trivial to replace using the Python native subprocess module, drop the dependency.

[1] <URL: https://github.com/kkroening/ffmpeg-python/tags >
[2] <URL: kkroening/ffmpeg-python#760 >
[3] <URL: https://openhub.net/p/ffmpeg-python >

The last ffmpeg-python module release was in 2019[1], upstream seem to be
unavailable[2] and the project development seem to have stagnated[3].  As
the features it provide is trivial to replace using the Python native
subprocess module, drop the dependency.

 [1] <URL: https://github.com/kkroening/ffmpeg-python/tags >
 [2] <URL: kkroening/ffmpeg-python#760 >
 [3] <URL: https://openhub.net/p/ffmpeg-python >
Copy link

@glangford glangford left a comment

Choose a reason for hiding this comment

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

Good idea, but should this use run() rather than Popen()?

The recommended approach to invoking subprocesses is to use the run() function for all use cases it can handle. For more advanced use cases, the underlying Popen interface can be used directly.

@petterreinholdtsen
Copy link
Contributor Author

petterreinholdtsen commented Apr 16, 2023 via email

@petterreinholdtsen
Copy link
Contributor Author

I added a commit to switch from Popen() to run(). Feel free to use the approach you like.

@szelenka
Copy link

We're running into some issues with ffmpeg-python when running on FIPS environment, it'd be good to have the ffmpeg-python removed. Any word on when this PR can be merged?

@petterreinholdtsen
Copy link
Contributor Author

petterreinholdtsen commented Apr 26, 2023 via email

@szelenka
Copy link

szelenka commented May 4, 2023

@petterreinholdtsen do we have any method to contact the maintainers of openai/whisper to merge this PR?

@glangford is this PR good to go now?

@petterreinholdtsen
Copy link
Contributor Author

petterreinholdtsen commented May 4, 2023 via email

@jongwook jongwook merged commit 8035e9e into openai:main May 4, 2023
@jongwook
Copy link
Collaborator

jongwook commented May 4, 2023

Thanks! Other than tidying up the dependencies, did you have any missing features or security concerns from ffmpeg-python?

@petterreinholdtsen
Copy link
Contributor Author

petterreinholdtsen commented May 4, 2023 via email

@szelenka
Copy link

szelenka commented May 4, 2023

@jongwook ffmpeg-python is not a FIPS compatible library, due to it's use of md5 without the usedforsecurity parameter introduced in Python 3.9, which means you cannot run it (or whisper) on FIPS hardware.

It'd be an easy fix to ffmpeg-python, but as @petterreinholdtsen mentioned, this is an abandoned project .. so nobody can merge the PRs there.

zackees pushed a commit to zackees/whisper that referenced this pull request May 5, 2023
* Drop ffmpeg-python dependency and call ffmpeg directly.

The last ffmpeg-python module release was in 2019[1], upstream seem to be
unavailable[2] and the project development seem to have stagnated[3].  As
the features it provide is trivial to replace using the Python native
subprocess module, drop the dependency.

 [1] <URL: https://github.com/kkroening/ffmpeg-python/tags >
 [2] <URL: kkroening/ffmpeg-python#760 >
 [3] <URL: https://openhub.net/p/ffmpeg-python >

* Rewrote to use subprocess.run() instead of subprocess.Popen().

* formatting changes

* formatting update

* isort fix

* Error checking

* isort 🤦🏻

* flake8 fix

* minor spelling changes

---------

Co-authored-by: Jong Wook Kim <jongwook@openai.com>
ilanit1997 pushed a commit to ilanit1997/whisper that referenced this pull request May 16, 2023
* Drop ffmpeg-python dependency and call ffmpeg directly.

The last ffmpeg-python module release was in 2019[1], upstream seem to be
unavailable[2] and the project development seem to have stagnated[3].  As
the features it provide is trivial to replace using the Python native
subprocess module, drop the dependency.

 [1] <URL: https://github.com/kkroening/ffmpeg-python/tags >
 [2] <URL: kkroening/ffmpeg-python#760 >
 [3] <URL: https://openhub.net/p/ffmpeg-python >

* Rewrote to use subprocess.run() instead of subprocess.Popen().

* formatting changes

* formatting update

* isort fix

* Error checking

* isort 🤦🏻

* flake8 fix

* minor spelling changes

---------

Co-authored-by: Jong Wook Kim <jongwook@openai.com>
abyesilyurt pushed a commit to abyesilyurt/whisper that referenced this pull request Nov 13, 2023
* Drop ffmpeg-python dependency and call ffmpeg directly.

The last ffmpeg-python module release was in 2019[1], upstream seem to be
unavailable[2] and the project development seem to have stagnated[3].  As
the features it provide is trivial to replace using the Python native
subprocess module, drop the dependency.

 [1] <URL: https://github.com/kkroening/ffmpeg-python/tags >
 [2] <URL: kkroening/ffmpeg-python#760 >
 [3] <URL: https://openhub.net/p/ffmpeg-python >

* Rewrote to use subprocess.run() instead of subprocess.Popen().

* formatting changes

* formatting update

* isort fix

* Error checking

* isort 🤦🏻

* flake8 fix

* minor spelling changes

---------

Co-authored-by: Jong Wook Kim <jongwook@openai.com>
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.

5 participants