InsightSoftwareConsortium / ITKModuleTemplate

A template to start an ITK Module
https://itk.org/ITKSoftwareGuide/html/Book1/ITKSoftwareGuide-Book1ch9.html#x50-1430009
Apache License 2.0
12 stars 16 forks source link

ENH: Update CI to build against ITK 5.3 RC 3 #123

Closed thewtex closed 2 years ago

thewtex commented 2 years ago

@tbirdso @LucasGandel @dzenanz we can test GitHub Actions builds for recent remote module changes in 5.3rc03 :1234:

LucasGandel commented 2 years ago

@tbirdso @LucasGandel @dzenanz we can test GitHub Actions builds for recent remote module changes in 5.3rc03 🔢

Thanks a lot @thewtex 🥇 ! The Linux wheels produced by the CI seem to work fine for RTK with Cuda. Only the driver installation is required to use the package, but the CUDA toolkit is provided by the wheel.

Here is the WIP change using the new scripts arguments.

This first test was produced with the CudaCommon sources still included in the RTK sources (old approach). We now have to try to produce the same wheel using the dependency between external modules. Meaning with have to build CudaCommon (new remote module) and RTK consecutively in the ITKPythonPackage build. Please share your thoughts if you have already think of this use case.

We also have to work on reducing the size of the produced wheel, as RTK requires cuBLAS which is huge in CUDA 11. Removing the TBB lib that gets included for each module could be a good start.

Stay tuned!

dzenanz commented 2 years ago

ITKUltrasound depends on 4 other remote modules, here is how we build the packages: https://github.com/KitwareMedical/ITKUltrasound/blob/v0.5.0/.github/workflows/build-test-package.yml#L170-L206 You can model this for RTK's dependence on CudaCommon.

LucasGandel commented 2 years ago

ITKUltrasound depends on 4 other remote modules, here is how we build the packages: https://github.com/KitwareMedical/ITKUltrasound/blob/v0.5.0/.github/workflows/build-test-package.yml#L170-L206 You can model this for RTK's dependence on CudaCommon.

Awesome! Thanks a lot @dzenanz, we'll do the same for RTK and CudaCommon

We also wonder whether packaging the cuda libs into the wheel is the right approach. Some of shared libraries are >100Mo. We could expect the user system to provide the required shared libraries at runtime, but then it's no longer a "manylinux" wheel. I'd love to hear your insight on this too if possible.

Thanks again!

dzenanz commented 2 years ago

then it's no longer a "manylinux" wheel.

I think it is reasonable to expect CUDA to be installed, even if it is technically not self-contained package. For example, cupy-cuda package explicitly asks that a user installs CUDA themselves.

LucasGandel commented 2 years ago

I think it is reasonable to expect CUDA to be installed, even if it is technically not self-contained package. For example, cupy-cuda package explicitly asks that a user installs CUDA themselves.

Agreed. We checked other packages and they all seem to require the user to install CUDA first. So we will remove the cuda libs from the wheel. Thanks a lot @dzenanz !