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

build error #178

Closed
dkazanc opened this issue Sep 9, 2023 · 7 comments · Fixed by #183
Closed

build error #178

dkazanc opened this issue Sep 9, 2023 · 7 comments · Fixed by #183
Assignees

Comments

@dkazanc
Copy link
Collaborator

dkazanc commented Sep 9, 2023

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
Copy link
Collaborator Author

dkazanc commented Nov 1, 2023

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
Copy link
Contributor

paskino commented Nov 24, 2023

I found the same error somewhere else SyneRBI/SIRF-SuperBuild#832

@paskino
Copy link
Contributor

paskino commented Nov 24, 2023

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
Copy link
Contributor

paskino commented Dec 19, 2023

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
Copy link
Member

casperdcl commented Dec 19, 2023

Worth thinking upgrading to skbuild, for instance?

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

@dkazanc
Copy link
Collaborator Author

dkazanc commented Dec 19, 2023

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
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants