TomographicImaging / CCPi-Regularisation-Toolkit

The set of CPU/GPU optimised regularisation modules for iterative image reconstruction and other image processing tasks
Apache License 2.0
49 stars 25 forks source link

build error #178

Closed dkazanc closed 5 months ago

dkazanc commented 12 months ago

I'm getting this error when trying to import a module:

   from ccpi.filters.cpu_regularisers import TV_ROF_CPU, TV_FGP_CPU, TV_PD_CPU, TV_SB_CPU, dTV_FGP_CPU, TNV_CPU, NDF_CPU, Diff4th_CPU, TGV_CPU, LLT_ROF_CPU, PATCHSEL_CPU, NLTV_CPU
ImportError: /home/algol/miniconda3/conda-bld/ccpi-regulariser_1694256493616/_test_env_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold/lib/python3.10/site-packages/ccpi/filters/cpu_regularisers.cpython-310-x86_64-linux-gnu.so: undefined symbol: _Z14SB_TV_CPU_mainPfS_S_fifiiii

Seems that it is related to SB_TV_CPU_main function, but I cannot find any issues with it. Any ideas @paskino? Many thanks

dkazanc commented 10 months ago

UPD: It is not related to the Compiled code actually.

I'm seeing the same problem consistently across other packages where distutils being used. I can only assume that the build is broken, possibly due to deprecation of distutils which comes from setuptools. And since we're using significantly newer setuptools in our environment than the recommended version (<60), this might be because of that.

Worth thinking upgrading to skbuild, for instance?

paskino commented 9 months ago

I found the same error somewhere else https://github.com/SyneRBI/SIRF-SuperBuild/issues/832

paskino commented 9 months ago

Probably it's better to start by changing this line

https://github.com/vais-ral/CCPi-Regularisation-Toolkit/blob/a3dc5201a75705c1a8e6f840f85d7d811023ba71/src/Python/CMakeLists.txt#L109

into using pip install . like here

paskino commented 8 months ago

On CI the current build process has the warning:

        ********************************************************************************
        Please avoid running ``setup.py`` directly.
        Instead, use pypa/build, pypa/installer or other
        standards-based tools.

        See https://blog.ganssle.io/articles/2021/10/setup-py-deprecated.html for details.
        ********************************************************************************
casperdcl commented 8 months ago

Worth thinking upgrading to skbuild, for instance?

or the successor scikit-build-core now :) - would also fix #114 -> #115

dkazanc commented 8 months ago

I'm currently refactoring TomoPhantom into Ctypes only package. Cmake still triggers the build of the C code into shared library but it is a totally independent entity from the Python code of the package, which can be simply pip installed. This is how TomoPy group does the build as well. They've got libtomo library for C/Cuda-C code and then just Python stuff. This is very convenient when you're working as a developer as you don't need to rebuild the package all the time when you're concerned only with the Python code. I guess the down side is that you need two conda recipes, but that can be done easily (Tomopy uses conda-forge feedstock for that).

For Regularisation Toolkit, as there is not much of Python, may be Cython build would work OK, but yes skbuild could be also an option. I've done a mini version of the toolkit slightly differently with Cython bindings and it seems to avoid the errors here. I can share it bellow. This is a pure play so far, but I guess we can potentially re-factor it in a similar way? sofia.zip

casperdcl commented 6 months ago

https://docs.cython.org/en/latest/src/userguide/source_files_and_compilation.html#configuring-the-c-build looks like we need to replace Cython.Distutils.build_ext with Cython.Build.cythonize