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

ENH: Remove ITKCudaCommon from the source tree #492

Conversation

AlexyPellegrini
Copy link

Now builds with the remote module CudaCommon of ITK. Builds both as a remote module of ITK and as a standalone library.
This MR replaces https://github.com/SimonRit/RTK/pull/427

@AlexyPellegrini
Copy link
Author

@LucasGandel

@SimonRit
Copy link
Collaborator

Thanks @AlexyPellegrini! I think that the CI must be updated to account for the ITKCudaCommon dependency. See this comment to know how to do it:
InsightSoftwareConsortium/ITKModuleTemplate#123 (comment)

@AlexyPellegrini AlexyPellegrini force-pushed the external-cuda-common branch 2 times, most recently from 522aed4 to 58efe1d Compare June 3, 2022 07:22
@LucasGandel LucasGandel self-requested a review June 3, 2022 07:40
@SimonRit
Copy link
Collaborator

SimonRit commented Jun 3, 2022

FYI, my plan is to finalize SimonRit/ITKCudaCommon#15. I'll update then #495. I will ask you then to rebase your PR on it... It's taking longer than expected because I have uncovered some issues by setting up the CI, sorry for the delay.

@@ -95,7 +98,7 @@ jobs:
mkdir ITK-build
cd ITK-build
call "C:\Program Files (x86)\Microsoft Visual Studio\2019\Community\VC\Auxiliary\Build\vcvars64.bat"
cmake -DCMAKE_C_COMPILER:FILEPATH="${{ matrix.c-compiler }}" -DBUILD_SHARED_LIBS:BOOL=ON -DCMAKE_CXX_COMPILER="${{ matrix.cxx-compiler }}" -DCMAKE_BUILD_TYPE:STRING=${{ matrix.cmake-build-type }} -DBUILD_TESTING:BOOL=OFF -GNinja ../ITK
cmake -DCMAKE_C_COMPILER:FILEPATH="${{ matrix.c-compiler }}" -DBUILD_SHARED_LIBS:BOOL=ON -DCMAKE_CXX_COMPILER="${{ matrix.cxx-compiler }}" -DCMAKE_BUILD_TYPE:STRING=${{ matrix.cmake-build-type }} -DModule_CudaCommon:BOOL=ON -DBUILD_TESTING:BOOL=OFF -GNinja ../ITK
Copy link
Collaborator

Choose a reason for hiding this comment

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

There seems to be a problem wtith itkCudaSquareImage.cu see the log. Maybe you should try to compile the master version of ITKCudaCommon by setting Module_CudaCommon_GIT_TAG here?

@LucasGandel
Copy link
Collaborator

@SimonRit The windows cuda python packages seems to work well. However there is an issue with docker for the linux cuda python packages (see here). Do you have any clue about what is happening with the linux self-hosted runner?

@SimonRit
Copy link
Collaborator

SimonRit commented Jul 4, 2022

@SimonRit The windows cuda python packages seems to work well. However there is an issue with docker for the linux cuda python packages (see here). Do you have any clue about what is happening with the linux self-hosted runner?

Sorry, the docker service was no longer running. It should be fixed now, I have scheduled a new run or all jobs. However, shouldn't this trigger a job failure? I'm not sure to understand why it doesn't.

@SimonRit
Copy link
Collaborator

SimonRit commented Jul 5, 2022

The compilation is still struggling due to missing CudaCommonExport.h, see here. This file should be generated when compiling CudaCommon, any clue why it can't find it? I'm not sure I understand this manual operation but for sure CudaCommonExport.h is not there but in the binary directory.

@AlexyPellegrini
Copy link
Author

To be honest, we copied it from ITKUltrasound's source code, all remote module are build this way with this copy of their include at the end. Maybe we need to also copy that generated config header to that include directory to make it accessible, or maybe it is not the good way of doing this and we should find a way to build it without any copy.
Can you check the path of the directory that contains the config header ?

@SimonRit
Copy link
Collaborator

SimonRit commented Jul 7, 2022

I can't find the file... I can find ./_work/RTK/RTK/_skbuild/linux-x86_64-3.7/cmake-build/include/RTKExport.h but _work/RTK/RTK/ITKCudaCommon/_skbuild is empty. I'm lost...

@SimonRit
Copy link
Collaborator

SimonRit commented Jul 7, 2022

As far as I can see, the remote modules BSplineGradient, HigherOrderAccurateGradient, SplitComponents, Strain, from which Ultrasound depends, do not need this generated file (grep Export in their source repo was empty).

@AlexyPellegrini
Copy link
Author

I found a typo in what I wrote, and it was one of the include dir copy so I could make sense, can you try to launch build-linux-cuda-python-packages (37) once again ?

@SimonRit
Copy link
Collaborator

FYI, the problem with CudaCommonExport.h is still here. During the build, the file is in ./_work/RTK/RTK/ITKCudaCommon/_skbuild/linux-x86_64-3.7/cmake-build/include/CudaCommonExport.h but it seems to be cleaned automatically. I guess it should be installed here and I'm not sure why it is not?

@SimonRit
Copy link
Collaborator

I'd like to quickly do a release now that #507 is merged (see InsightSoftwareConsortium/ITK#3528). Do you think we should finalize this PR before? What is its status?

@LucasGandel
Copy link
Collaborator

I'd like to quickly do a release now that #507 is merged (see InsightSoftwareConsortium/ITK#3528). Do you think we should finalize this PR before? What is its status?

Yes I will rebase this on master and it should be ready

@LucasGandel
Copy link
Collaborator

@SimonRit This should finally pass once InsightSoftwareConsortium/ITKPythonPackage#200 is merged and new ITKPythonBuilds are released

@SimonRit
Copy link
Collaborator

SimonRit commented Sep 7, 2022

@SimonRit This should finally pass once InsightSoftwareConsortium/ITKPythonPackage#200 is merged and new ITKPythonBuilds are released

Following @thewtex' comment, I have relaunched the checks. But it will likely fail for manylinux2014...

@SimonRit
Copy link
Collaborator

Can you rebase on master to retrigger the tests based on #510?

@LucasGandel
Copy link
Collaborator

Can you rebase on master to retrigger the tests based on #510?

Done! Thanks for the update!

Please make sure to use "squash and merge" button if everything goes well.

@LucasGandel
Copy link
Collaborator

@SimonRit It looks like the pipelines are passing 🥳 🎉 We should test the produced wheels and merge if everything is fine

@SimonRit
Copy link
Collaborator

Can you please rebase on master? I have updated the CI infrastructure for compability with the latest ITK builds, see 7cda37a.

@SimonRit
Copy link
Collaborator

I have tested the Linux 3.10 CI package (from ITKCudaCommon and this PR) and it works! There are a bunch of warnings at execution to address, see below. I also think that there is a missing install requirement for itk-cudacommon-cuda116 in setup.py?

(rtk_cudacommon_split) srit@linux-b3vn:~/src/rtk/rtk/examples/FirstReconstruction> LD_LIBRARY_PATH=/home/srit/Downloads/cuda116/lib64:$LD_LIBRARY_PATH python ~/src/rtk/rtk/examples/FirstReconstruction/FirstCudaReconstruction.py a.mha a.xml
Template itk::ImageSource<itk::CudaImage<float,2>>
 already defined as <class 'itk.itkImageSourceCudaCommonPython.itkImageSourceCIF2'>
 is redefined as {cl}
Warning: template already defined 'itk::ImageSource<itk::CudaImage<float,2>>'
Template itk::ImageSource<itk::CudaImage<float,3>>
 already defined as <class 'itk.itkImageSourceCudaCommonPython.itkImageSourceCIF3'>
 is redefined as {cl}
Warning: template already defined 'itk::ImageSource<itk::CudaImage<float,3>>'
Template itk::ImageSource<itk::CudaImage<float,4>>
 already defined as <class 'itk.itkImageSourceCudaCommonPython.itkImageSourceCIF4'>
 is redefined as {cl}
Warning: template already defined 'itk::ImageSource<itk::CudaImage<float,4>>'
Template itk::ImageSource<itk::CudaImage<itk::Vector<float,2>,2>>
 already defined as <class 'itk.itkImageSourceCudaCommonPython.itkImageSourceCIVF22'>
 is redefined as {cl}
Warning: template already defined 'itk::ImageSource<itk::CudaImage<itk::Vector<float,2>,2>>'
Template itk::ImageSource<itk::CudaImage<itk::Vector<float,2>,3>>
 already defined as <class 'itk.itkImageSourceCudaCommonPython.itkImageSourceCIVF23'>
 is redefined as {cl}
Warning: template already defined 'itk::ImageSource<itk::CudaImage<itk::Vector<float,2>,3>>'
Template itk::ImageSource<itk::CudaImage<itk::Vector<float,2>,4>>
 already defined as <class 'itk.itkImageSourceCudaCommonPython.itkImageSourceCIVF24'>
 is redefined as {cl}
Warning: template already defined 'itk::ImageSource<itk::CudaImage<itk::Vector<float,2>,4>>'
Template itk::ImageSource<itk::CudaImage<itk::Vector<float,3>,2>>
 already defined as <class 'itk.itkImageSourceCudaCommonPython.itkImageSourceCIVF32'>
 is redefined as {cl}
Warning: template already defined 'itk::ImageSource<itk::CudaImage<itk::Vector<float,3>,2>>'
Template itk::ImageSource<itk::CudaImage<itk::Vector<float,3>,3>>
 already defined as <class 'itk.itkImageSourceCudaCommonPython.itkImageSourceCIVF33'>
 is redefined as {cl}
Warning: template already defined 'itk::ImageSource<itk::CudaImage<itk::Vector<float,3>,3>>'
Template itk::ImageSource<itk::CudaImage<itk::Vector<float,3>,4>>
 already defined as <class 'itk.itkImageSourceCudaCommonPython.itkImageSourceCIVF34'>
 is redefined as {cl}
Warning: template already defined 'itk::ImageSource<itk::CudaImage<itk::Vector<float,3>,4>>'
Template itk::ImageSource<itk::CudaImage<itk::Vector<float,4>,2>>
 already defined as <class 'itk.itkImageSourceCudaCommonPython.itkImageSourceCIVF42'>
 is redefined as {cl}
Warning: template already defined 'itk::ImageSource<itk::CudaImage<itk::Vector<float,4>,2>>'
Template itk::ImageSource<itk::CudaImage<itk::Vector<float,4>,3>>
 already defined as <class 'itk.itkImageSourceCudaCommonPython.itkImageSourceCIVF43'>
 is redefined as {cl}
Warning: template already defined 'itk::ImageSource<itk::CudaImage<itk::Vector<float,4>,3>>'
Template itk::ImageSource<itk::CudaImage<itk::Vector<float,4>,4>>
 already defined as <class 'itk.itkImageSourceCudaCommonPython.itkImageSourceCIVF44'>
 is redefined as {cl}
Warning: template already defined 'itk::ImageSource<itk::CudaImage<itk::Vector<float,4>,4>>'
Warning: Unknown parameter 'itk::CudaImage<itk::Vector<float,5>,4>' in template 'itk::ImageSource'
Warning: Unknown parameter 'itk::CudaImage<itk::Vector<float,5>,4>' in template 'itk::ImageToImageFilter'
Warning: Unknown parameter 'itk::CudaImage<itk::Vector<float,5>,4>' in template 'itk::CudaImageToImageFilter'
Warning: Unknown parameter 'itk::CudaImage<itk::Vector<float,5>,4>' in template 'itk::InPlaceImageFilter'
Warning: template already defined 'itk::InPlaceImageFilter<itk::CudaImage<itk::Vector<float,5>,4>,itk::CudaImage<itk::Vector<float,5>,4>>'
Warning: Unknown parameter 'itk::CudaImage<itk::Vector<float,5>,4>' in template 'itk::CudaInPlaceImageFilter'
Reconstructing...
Writing output image...
Done!

AlexyPellegrini and others added 2 commits March 14, 2023 06:46
Now builds with the remote module CudaCommon of ITK. Builds both as a
remote module of ITK and as a standalone library.
@SimonRit
Copy link
Collaborator

I have been working on this PR and inadvertently modified @AlexyPellegrini's branch, sorry. The reference PRs are now RTKConsortium/ITKCudaCommon#20 and #528.

@SimonRit SimonRit closed this Mar 14, 2023
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.

None yet

3 participants