-
Notifications
You must be signed in to change notification settings - Fork 67
Remove hard dep on nvcuvid.so and fallback to CPU if it is not available.
#996
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
Conversation
|
Hey @traversaro, I saw you already opened #983 for this, thank you!! We're getting more reports related to This is my current attempt, it seems to be working fine locally, let's see what the CI says. I'm only handling linux for now but Windows will come, if this works. |
Great, thanks for opening the PR. That was just an initial attempt I did not had time to follow up, so it is great if you can work on this, thanks! |
|
@NicolasHug has imported this pull request. If you are a Meta employee, you can view this in D85561942. |
|
@traversaro is there any chance you could try to see if the current PR works fine on Windows GPUs? I've added the code to dynamically load the dll at runtime, but we don't have Windows GPU CI jobs, so I can't verify myself that it's working as intended |
| return true; | ||
| } | ||
| } // namespace facebook::torchcodec | ||
| #else |
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 confirm fbcode build and tests are fine with the above. Also, FBCODE_CAFFE2 is very public on pytorch's GH repo: https://github.com/search?q=repo%3Apytorch%2Fpytorch%20FBCODE_CAFFE2&type=code
| else() | ||
| message(FATAL_ERROR "Could not find NVCUVID library") | ||
| endif() | ||
|
|
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.
Seeing a bunch of cmake code removed makes me irrationally happy. :)
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.
Wait until you see the dlopen() stuff 😅
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.
Great improvement, agreed this is worth a fixpack.
|
QQ @traversaro do you need this PR to be merged (and maybe released as a bugfix) to test it? IIRC in a previous comment you mentioned that testing the build was a lot easier for you on stable releases, so I just want to make sure |
I am testing it now with a local modified version of conda-forge/torchcodec-feedstock#38, I will report the results here. No need for a release for the tests. |
|
Just tested on Windows, the test pass fine and the GPU is used, so I guess we are good to go w.r.t. to that! |
|
Great, thank you so much for testing! I'll push a bugfix release later this week |
Closes #978
TL;DR:
libnvcuvid.so.import torchcodecnow fails iflibnvcuvid.soisn't available.libnvcuvid.soisn't usually installed with the normal CUDA toolkit installations, so it's not rare for it not to be available. So, it's not rare forimport torchcodecto fail. That's bad - it's my fault, I missed that.libnvcuvid.soat runtime only when necessary (i.e. only when the user explicitly requests the beta backend). This makesimport torchcodecwork, like before. We fallback to the CPU interface if it's not available , that's what FFmpeg does too!