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

Docker + CUDA documentation #1232

Merged
merged 4 commits into from
Dec 12, 2023
Merged

Conversation

gedoensmax
Copy link
Contributor

@gedoensmax gedoensmax commented Sep 5, 2023

Actually the base image is a bit overkill. CUDA Toolkit is only required for compilation but not for execution. It should be sufficient to compile in a CUDA docker but copy everything over to the respective Ubuntu base image. Any help with reducing the image into a build image and a final image would be much appreciated ! If image size is not a concern this is working as is.

@kylophone
Copy link
Collaborator

I've been thinking that it might be easier to use ffnvcodec inside libvmaf rather than maintain a special Dockerfile for building this. I would like to continue being cross-platform as possible, meaning in addition to Linux, we still have support for Windows, FreeBSD, etc. Thoughts?

@gedoensmax
Copy link
Contributor Author

While I agree with you this is much more work that simply doing a separate Dockerfile. I am not sure when or if I can get to do this.
Let's leave this PR as is to serve as a public documentation until then.
When using the ffnvcodec I would also recommend doing a 2 stage build - compiling inside a CUDA devel container and then copying the binaries over. CUDA container are not small by any means ...

--enable-ffnvcodec \
--disable-stripping \
--extra-cflags="-I/usr/local/cuda/include" \
--extra-ldflags="-L/usr/local/cuda/lib64 -L/usr/local/cuda/lib64/stubs/"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Linking against stubs as during build the driver is not available.

@kylophone
Copy link
Collaborator

@gedoensmax I will merge this now, and we can look at non-Docker alternatives in the future. Anything else you want to add before I merge?

@gedoensmax
Copy link
Contributor Author

@MorkTheOrk please take a look if you think something is missing otherwise I am happy with it.

(add `-Denable_float=true` flag in the rare case if you want to use the floating-point feature extractors.)
Special cases:
- add `-Denable_float=true` flag in the rare case if you want to use the floating-point feature extractors.
- add `-Denable_avx512=true` to support wider SIMD instructions to achieve the fastest processing on supported CPUs
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: this is now enabled by default so this wording may be misleading. We can fix this later though.

@MorkTheOrk
Copy link
Contributor

@gedoensmax For the Docker and Documentation part: Looks fine to me.
There are a few bugs I would like to fix soonish, but I dont know if it needs to be part of this PR, we might want to open a new one for more fixes.

@kylophone kylophone merged commit 0f90e27 into Netflix:master Dec 12, 2023
8 checks passed
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.

4 participants