-
Notifications
You must be signed in to change notification settings - Fork 74
test: add cppapi test to give TorchCodecConfig.cmake a try #1087
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
base: main
Are you sure you want to change the base?
Conversation
|
Thank you for triggering CI. Ok, we have the whole bunch of errors which are circling around the pattern that we need not only runtime, but development environment as well, i.e. we need
Across these errors the most worrisome seems to be missing CUDA. @NicolasHug, is that reasonable to request having CUDA fully installed on the runner environment for these tests or we better change a strategy? For example, we can consider to avoid integrating this test into regular torchcodec pytest suite and run it separately within |
Actually, it's installed, but was not detected. I guess that due to OS differences in cmake behavior. On ubuntu it get's detected from /usr/local, on AlmaLinux it seems not: |
|
Thanks for the PR @dvrogozh ! I have a few questions / comments:
Right now, the newly added test are ran under this
install-and-test, that still depends on the build job above, but where we can install the required dev dependencies safely. This new job doesn't even need to rely on pytest, we could have some inline bash commands directly calling into cmake, which might be simpler too? Does that make sense?
|
Signed-off-by: Dmitry Rogozhkin <[email protected]>
Yes, ultimate goal is to support the latter - that's the intended scenario. This being said, it still should work if someone builds from source.
On XPU side we plan to support Windows for we are not their yet (with torchcodec). To be honest, I don't feel that right now we are at the point when focusing just on Linux will make things easier. I still believe we can make things just work across all 3 OSes fluently.
I think that connecting things to pytest will just make things for developers so much easier. Otherwise they will start seeing issues not on the point when they've prepared a code locally, but when CI completed after a push. Creating a separate job won't actually much help if we want to give test a run on the CUDA environment because as of now I still propose to try finish with the idea to integrate test into
I've did these changes in the last update of the PR. @NicolasHug, don't you mind to trigger CI once again to see if it works now? |
That's a test for the
TorchCodecConfig.cmakewe've introduced in dc87228. Test is integrated into TorchCodec pytest suite. It assumes that ffmpeg is installed and can be used for both runtime and compilation (i.e. .pc and include files are available). Test builds an executable which is linked to TorchCodec core library. There are 2 kind of tests executed:pkg-configTORCHCODEC_FFMPEGn_INSTALL_PREFIXenvironment variableTest which relies on environment variable assumes that the ffmpeg install layout will match expectations hardcoded in the test and in the
TorchCodecConfig.cmake. It potentially might fail - we need to check in the CI.@NicolasHug, please, help to review paying attention on the above. I hope this will allow us to have test naturally integrated into TorchCodec test suite. If not, let's discuss what we need to modify.
CC: @NicolasHug