ENH: Add CI Checks for CPU builds + GPU test

tbirdso commented 3 years ago

Adds Github Actions continuous integration:

cxx project builds on Windows, MacOS, Linux CPU-only environments and passes KWStyle and Doxygen style tests
Python module wheels build on Windows, MacOS, Linux CPU-only environments
cxx project builds and all tests succeed on Linux GPU environment in AWS EC2 cloud, spun up with Cirun.io

tbirdso commented 2 years ago

Build/test is passing on GPU but failing on CPU. The CTest dashboard and Github Actions logs show that configure+cbuild passes in both instances, but no tests are found for CPU regardless of whether BUILD_TESTING is enabled (note that we expect KWStyleTest to be built and run regardless of this flag). All tests are found and pass on GPU.

CPU runner results

CPU dashboard results

GPU dashboard results

Python package builds fail in dockcross image due to absence of setup.py?

/opt/python/cp36-cp36m/bin/python setup.py bdist_wheel --build-type Release -G Ninja -- -DITK_DIR:PATH=/work/ITK-cp36-cp36m-manylinux2014_x64 -DITK_USE_SYSTEM_SWIG:BOOL=ON -DWRAP_ITK_INSTALL_COMPONENT_IDENTIFIER:STRING=PythonWheel -DSWIG_EXECUTABLE:FILEPATH=/work/ITK-cp36-cp36m-manylinux2014_x64/Wrapping/Generators/SwigInterface/swig/bin/swig -DCMAKE_CXX_COMPILER_TARGET:STRING=x86_64-linux-gnu -DBUILD_TESTING:BOOL=OFF -DPython3_EXECUTABLE:FILEPATH=/opt/python/cp36-cp36m/bin/python -DPython3_INCLUDE_DIR:PATH=/opt/python/cp36-cp36m/bin/../include/python3.6m /opt/python/cp36-cp36m/bin/python: can't open file 'setup.py': [Errno 2] No such file or directory

tbirdso commented 2 years ago

@thewtex Have you seen this behavior before where tests are built but not found?

tbirdso commented 2 years ago

It looks like our base Windows tests pass but 50+ warnings were thrown based on the clci dependency:

D:\a\ITKCLEsperanto\build_deps\clci-src\clic\src\core\cleDeviceManager.cpp(79): warning C4244: '=': conversion from '__int64' to 'int', possible loss of data

D:\a\ITKCLEsperanto\build_deps\clci-src\tests\sobel_test.cpp(35): warning C4244: '=': conversion from 'double' to '_Ty', possible loss of data

etc.

Full CTest results here

StRigaud commented 2 years ago

Hi Those are mainly conversion type issues apparently, mainly due to me not really focusing on those warning (also they are not highlight by my compiler ...) I tried to fix some of those on my side, do not hesitate to @ me if needed in future.

tbirdso commented 2 years ago

Thank you @StRigaud !!

For reference, those build warnings were coming from the Ninja build system + MSVC C/C++ compiler. I'll loop you in next time issues appear 😃

tbirdso commented 2 years ago

Windows and MacOS Python builds are failing to ~either fetch or~ include the CLIc dependency:

In file included from /Users/svc-dashboard/D/P/ITKPythonPackage/scripts/../ITK-3.7-macosx_x86_64/Wrapping/itkMinimalStandardRandomVariateGenerator.cxx:16:
/Users/runner/work/ITKCLEsperanto/ITKCLEsperanto/include/itkMinimalStandardRandomVariateGenerator.h:27:10: fatal error: 'clesperanto.hpp' file not found
#include "clesperanto.hpp"
         ^~~~~~~~~~~~~~~~~
1 error generated.

This file should be found in the CLIc dependency: ITKCLEsperanto-build\_deps\clci-src\clic\includes\core\clesperanto.hpp.

Following up on an apparent mounting issue in Linux Python build:

+ itk_build_dir='/work/ITK-cp36-cp36*-manylinux2014_x64'
+ ln -fs '/ITKPythonPackage/ITK-cp36-cp36*-manylinux2014_x64' '/work/ITK-cp36-cp36*-manylinux2014_x64'
+ [[ ! -d /work/ITK-cp36-cp36*-manylinux2014_x64 ]]
+ echo 'ITK build tree not available!'
ITK build tree not available!
+ exit 1

tbirdso commented 2 years ago

I was able to reproduce the include error locally on Linux, it looks like the castxml step fails to include the CLIc directory from _deps:

[ 90%] Built target swig [ 93%] Built target castxml [ 94%] Generating /home/tom/builds/ITK/Wrapping/itkMinimalStandardRandomVariateGenerator.xml In file included from /home/tom/builds/ITK/Wrapping/itkMinimalStandardRandomVariateGenerator.cxx:16: /home/tom/repos/ITKCLEsperanto/include/itkMinimalStandardRandomVariateGenerator.h:27:10: fatal error: 'clesperanto.hpp' file not found

include "clesperanto.hpp"

^~~~~ 1 error generated. make[2]: [Wrapping/Modules/CLEsperanto/CMakeFiles/CLEsperantoCastXML.dir/build.make:76: /home/tom/builds/ITK/Wrapping/itkMinimalStandardRandomVariateGenerator.xml] Error 1 make[1]: [CMakeFiles/Makefile2:3310: Wrapping/Modules/CLEsperanto/CMakeFiles/CLEsperantoCastXML.dir/all] Error 2

This failure only occurs when Python wrapping is turned on. A cxx-only build succeeds, implying that it was able to correctly retrieve the clesperanto.hpp template header from _deps.

@thewtex @dzenanz Do you have any experience with Python wrapping failing to include headers where the cxx build otherwise succeeds?

tbirdso commented 2 years ago

It looks like ITK Python 3.6 sources were not packaged in the Linux v5.3rc01 tarball, hence the issue in finding the ITK build tree for the Python 3.6 test specifically. Currently re-running packaging steps to see if the issue is resolved.

dzenanz commented 2 years ago

I think that we have dropped Python 3.6 support recently.

tbirdso commented 2 years ago

@StRigaud Just so you know it looks like we're still getting 33 build warnings on the Windows platform, still mostly due to type conversion warnings (see https://open.cdash.org/build/7508117). I've verified we are fetching content from the master branch. At a glance it looks like these are new warnings after recent fixes, likely ones that were being ignored by the compiler before rather than resulting directly from the fixes.

tbirdso commented 2 years ago

@dzenanz Do you know where I could confirm this? 3.6 sources are still being packaged in the Windows tarball.

dzenanz commented 2 years ago

In ITKPythonPackage. Yes, it was removed: https://github.com/InsightSoftwareConsortium/ITKPythonPackage/commit/3927c51d3ef4bc145936f13c908cbaf9a108767d https://github.com/InsightSoftwareConsortium/ITKPythonPackage/commit/5ab4922e10f73e4f33cf93837f8a2f1a685fe713 https://github.com/InsightSoftwareConsortium/ITKPythonPackage/commit/64b79cb1ca5f926fa07e324621abaf3c22f95449

tbirdso commented 2 years ago

Perfect, thanks for tracking this down @dzenanz, I was looking in ITK. I will update our CI here to remove the 3.6 test, and revisit the v5.3rc01 Windows packaging...

tbirdso commented 2 years ago

Was able to temporarily work around castxml linking errors by copying headers from _deps/clic-src, _deps/thirdparty, and OpenCL-ICD-Loader to ITKCLEsperanto/includes.

Another error arose from .a shared libraries but was resolved by adding -fPIC to CMake CXX compiler options:

[ 95%] Linking CXX shared module /home/tom/builds/ITK/Wrapping/Generators/Python/itk/_CLEsperantoPython.so /usr/bin/ld: /home/tom/builds/ITK/lib/libCLIc.a(clesperanto.cpp.o): relocation R_X86_64_PC32 against symbol `_ZTVN3cle24MaximumOfAllPixelsKernelE' can not be used when making a shared object; recompile with -fPIC /usr/bin/ld: final link failed: bad value collect2: error: ld returned 1 exit status make[2]: [Wrapping/Modules/CLEsperanto/CMakeFiles/CLEsperantoPython.dir/build.make:289: /home/tom/builds/ITK/Wrapping/Generators/Python/itk/_CLEsperantoPython.so] Error 1 make[1]: [CMakeFiles/Makefile2:3367: Wrapping/Modules/CLEsperanto/CMakeFiles/CLEsperantoPython.dir/all] Error 2 make: *** [Makefile:146: all] Error 2

With these changes the Python build succeeded locally on Linux.

thewtex commented 2 years ago

@tbirdso in the top level CMakeLists.txt, we need to add, e.g.

set(CLEsperanto_INCLUDE_DIRS ${clci_SOURCE_DIR})

after it has been defined. <module-name>_INCLUDE_DIRS is used by the module configuration system to export the include directories needed by a module. CastXML will use it.

See an example here:

https://github.com/InsightSoftwareConsortium/ITK/blob/94dfea440baecc15d6a91399dd3a4a7adb64707e/Modules/ThirdParty/JPEG/CMakeLists.txt#L18

Re: Python 3.6, yes it was dropped in 5.3 RC 1. As a heads up, Python 3.10 is coming in 5.3 RC 2 :-).

tbirdso commented 2 years ago

Thanks @thewtex . Setting CLEsperanto_INCLUDE_DIRS works, but I had to add additional subdirectories to resolve pathing issues. Is there a better way to recursively include header directories for an ITK module?

Local build is completing successfully, if Python CI is green I will squash and mark ready for review.

tbirdso commented 2 years ago

Hi again @StRigaud , I've submitted a patch to fix remaining MSVC compiler warnings in CLIc_prototype here: https://github.com/clEsperanto/CLIc_prototype/pull/54

By relying on the cl.hpp header file retrieved with CLIc we are also getting a configuration warning in CMakeLists.txt:

"WARNING: Using project version of OpenCL-CLHPP."

See the full build log here.

By relying on OpenCL-ICD-Loader for cl.h we are trying to reduce dependence on OpenCL system headers. Do you have suggestions on the best path for mitigating this issue?

My understanding from https://github.com/KhronosGroup/OpenCL-CLHPP is that the project OpenCL C++ bindings should be adequate. Does this need to be a warning at all, or maybe it could be downgraded in severity?

tbirdso commented 2 years ago

Interestingly, Windows Python CI is passing but Linux Python CI fails as position independent code is not build for CLIc despite being enabled (and also passing locally on Linux):

2021-10-12T20:11:26.4522256Z -- CMAKE_POSITION_INDEPENDENT_CODE IS ON 2021-10-12T20:12:57.8447903Z FAILED: ../../../ITK-cp38-cp38-manylinux2014_x64/Wrapping/Generators/Python/itk/_CLEsperantoPython.so 2021-10-12T20:12:57.8577160Z : && /opt/rh/devtoolset-10/root/usr/bin/g++ -fPIC -O3 -DNDEBUG -flto -fno-fat-lto-objects -shared -o ../../../ITK-cp38-cp38-manylinux2014_x64/Wrapping/Generators/Python/itk/_CLEsperantoPython.so Wrapping/Modules/CLEsperanto/CMakeFiles/CLEsperantoPython.dir/CLEsperantoPython.cpp.o Wrapping/Modules/CLEsperanto/CMakeFiles/CLEsperantoPython.dir/itkMinimalStandardRandomVariateGeneratorPython.cpp.o ... 2021-10-12T20:12:57.8657560Z /opt/rh/devtoolset-10/root/usr/libexec/gcc/x86_64-redhat-linux/10/ld: _deps/clci-build/clic/libCLIc.a(clesperanto.cpp.o): relocation R_X86_64_32 against symbol `__pthread_key_create@@GLIBC_2.2.5' can not be used when making a shared object; recompile with -fPIC ... 2021-10-12T20:12:57.8998742Z /opt/rh/devtoolset-10/root/usr/libexec/gcc/x86_64-redhat-linux/10/ld: _deps/clci-build/clic/libCLIc.a(cleCloseIndexGapsInLabelMapKernel.cpp.o): relocation R_X86_64_32S against symbol `_ZNSs4_Rep20_S_empty_rep_storageE@@GLIBCXX_3.4' can not be used when making a shared object; recompile with -fPIC 2021-10-12T20:12:57.9000720Z collect2: error: ld returned 1 exit status

StRigaud commented 2 years ago

Hi again @StRigaud , I've submitted a patch to fix remaining MSVC compiler warnings in CLIc_prototype here: clEsperanto/CLIc_prototype#54

Thanks! I will merge it asap.

By relying on the cl.hpp header file retrieved with CLIc we are also getting a configuration warning in CMakeLists.txt:

"WARNING: Using project version of OpenCL-CLHPP."

Yes, I've put it here. The main reason is that in my head users should actually have it install on their own system along with OpenCL headers for a clean install. If it was not detected, the warning was a reminder for possible issue encountered with OpenCL, possible rise of warning during compilation, etc.

By relying on OpenCL-ICD-Loader for cl.h we are trying to reduce dependence on OpenCL system headers. Do you have suggestions on the best path for mitigating this issue?

My understanding from https://github.com/KhronosGroup/OpenCL-CLHPP is that the project OpenCL C++ bindings should be adequate. Does this need to be a warning at all, or maybe it could be downgraded in severity?

I tried to avoid at the maximum to mingle with OpenCL installation in CLIc, first because I am not very good on this part, and also I assume this is not the job of the project and users should have this part cleared. Mainly because most GPU platform provider come with opencl headers. However OpenCL-CLHPP isn't provided, hence the OpenCL-CLHPP submodule part and warning.

OpenCL-ICD-Loader is clearly the adapted option though not knowing much how to use it, I went for the easy solution for now and skipped this part but I would gladly take any improvement on this. If relying on the ICD and providing the C++ Binding with CLIc isn't an issue, we can remove the warning and change the installation process.

If there is a better cleaner way for managing OpenCL headers and C++ bindings, I am all for it and will gladly help you there !

thewtex commented 2 years ago

I had to add additional subdirectories to resolve pathing issues. Is there a better way to recursively include header directories for an ITK module?

This is good - it is best to be explicit with the directories.

tbirdso commented 2 years ago

@StRigaud

By relying on the cl.hpp header file retrieved with CLIc we are also getting a configuration warning in CMakeLists.txt:

"WARNING: Using project version of OpenCL-CLHPP."

Yes, I've put it here. The main reason is that in my head users should actually have it install on their own system along with OpenCL headers for a clean install. If it was not detected, the warning was a reminder for possible issue encountered with OpenCL, possible rise of warning during compilation, etc.

Okay, I think this is reasonable, thank you for the explanation. For this project we are relying on OpenCL-ICD-Loader's OpenCL headers to ensure building across platforms. Following that premise we can independently pull OpenCL-CLHPP as part of our CI and then provide those OpenCL C++ bindings to CLIc so that CLIc does not need to provide the bindings.

I'm not sure yet what the scope of those changes would be in regards to OpenCL CMake variables for ITK, so I will bump that investigation to another PR.

tbirdso commented 2 years ago

Marking this as ready to review. Known CI failures that are getting beyond the scope of this PR but should be subsequently addressed:

build-cxx fails on Windows due to CMake warning from CLIc dependency. As discussed in the thread above, this warning is intentional and signifies that OpenCL C++ bindings were expected to be supplied to the project by the user but are missing and instead provided by CLIc. I will investigate fetching OpenCL-CLHPP sources on a given CI system in the same step as OpenCL-ICD-Loader and subsequently providing to the project.
build-macos-opencl-python-packages fails because the script lacks permissions for removing wheels in cleanup. This has been addressed in https://github.com/InsightSoftwareConsortium/ITKPythonPackage/commit/bff7c53aed9b0edc5b4b9abd96a3c1775f5c381b and will be resolved in the v5.3rc02 release.

StRigaud commented 2 years ago

@tbirdso

I didn't understood that the warning was blocking the CI. I am fine in lowering the status to a simple message display for now. Especially if that make all this simpler.

On the long run, It is possible that we also switch to OpenCL-ICD-Loader as it may simplify installation issues for very high level users.

tbirdso commented 2 years ago

Thanks @StRigaud . I've submitted https://github.com/clEsperanto/CLIc_prototype/pull/55 which will reduce the severity of the CLHPP message while also opening up the OpenCL_CLHPP_HEADERS variable so that the user can specify a location for C++ bindings other than the OpenCL header directory from OpenCL-ICD-Loader. This should fix the CI failure while still leaving open the potential for supplying headers from a different source if needed in the future.

StRigaud commented 2 years ago

Great! :)

thewtex commented 2 years ago

@StRigaud thanks for the quick integrations! :pray:

@tbirdso can we update to upstream CLIc for clean builds?

tbirdso commented 2 years ago

@thewtex

@tbirdso can we update to upstream CLIc for clean builds?

We are referencing the master branch of CLIc by default for building ITKCLEsperanto. The patches to CLIc have resolved the configuration warnings in regards to CLHPP, but it looks like there are still MSVC compiler warnings regarding type conversions. Unlike the earlier ones these aren't showing up in my IDE even when increasing compiler warning level with -W, so it's going to take more time to iteratively track these down. I've submitted one patch for visible warnings at https://github.com/clEsperanto/CLIc_prototype/pull/56 but these are new warnings after resolving others in previous patches.

In short, the remaining build-cxx Windows errors we're seeing are due to minimal-impact compiler warnings from the CLIc and I don't think they should prevent us from moving forward with these changes.

StRigaud commented 2 years ago

Happy to help, also learning a few things on the go so clearly a win-win for me :+1:

tbirdso commented 2 years ago

Checks are green 💚

InsightSoftwareConsortium / ITKCLEsperanto

ENH: Add CI Checks for CPU builds + GPU test #18

include "clesperanto.hpp"