conda-forge / vmtk-feedstock

A conda-smithy repository for vmtk.
BSD 3-Clause "New" or "Revised" License
6 stars 5 forks source link

Add osx #4

Closed ramtingh closed 2 years ago

ramtingh commented 2 years ago

Checklist

conda-forge-linter commented 2 years ago

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe) and found it was in an excellent condition.

ramtingh commented 2 years ago

@conda-forge-admin, please rerender

conda-forge-linter commented 2 years ago

Hi! This is the friendly automated conda-forge-linting service.

I wanted to let you know that I linted all conda-recipes in your PR (recipe) and found some lint.

Here's what I've got...

For recipe:

conda-forge-linter commented 2 years ago

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe) and found it was in an excellent condition.

conda-forge-linter commented 2 years ago

Hi! This is the friendly automated conda-forge-linting service.

I wanted to let you know that I linted all conda-recipes in your PR (recipe) and found some lint.

Here's what I've got...

For recipe:

ramtingh commented 2 years ago

@conda-forge-admin, please lint

conda-forge-linter commented 2 years ago

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe) and found it was in an excellent condition.

ramtingh commented 2 years ago

@conda-forge-admin, please rerender

ramtingh commented 2 years ago

@conda-forge-admin, please rerender

hkjeldsberg commented 2 years ago

Hi @ramtingh,

I appreciate all the work you have put in so far, and wondered if there has been any progress on this? I'm using VMTK for multiple projects of my own and am very eager to see it being released on conda-forge.

Tell me if I can assist in any way. What seems to be the problem when deploying to osx?

ramtingh commented 2 years ago

Windows/linux builds are already available on conda-forge. I'm not quite sure what is going with osx, it compiles correctly and then runs into segmentation fault when testing. I don't have much experience with osx so I haven't gotten around to figuring out why that is.

hkjeldsberg commented 2 years ago

@ramtingh I see. I am mostly working on osx at the moment, although my experience with compilation and segfaults on mac is lacking. Do the logs give any insight; any indication at what point during testing it fails?

Also, are you able to re-run the build on Azure? Seems like the previous error logs have been deprecated.

conda-forge-linter commented 2 years ago

Hi! This is the friendly automated conda-forge-linting service.

I was trying to look for recipes to lint for you, but it appears we have a merge conflict. Please try to merge or rebase with the base branch to resolve this conflict.

Please ping the 'conda-forge/core' team (using the @ notation in a comment) if you believe this is a bug.

conda-forge-linter commented 2 years ago

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe) and found it was in an excellent condition.

ramtingh commented 2 years ago

@conda-forge-admin, please rerender

ramtingh commented 2 years ago

@hkjeldsberg no particular error that I can see, just "Segmentation fault: 11" when trying to import anything from vtkvmtk ( along with a lot of ELF warnings that as far as I know can be ignored). At some point I will need to pinpoint exactly what script is causing that

hkjeldsberg commented 2 years ago

@ramtingh Yeah, I also think the ELF warnings can be ignored. Seems like the segfault occurs during the call to from vmtk.vtkvmtkCommonPython import * based on the logs. Would be interesting to investigate this further.

I can try to build this locally (osx) and see if I'm able to reproduce the error, or if it is something Azure specific.

hkjeldsberg commented 2 years ago

Update: the build fails locally at the same point. Here is the last output before segfault:

import: 'vmtk'
+ python importtests.py
Fatal Python error: Segmentation fault

Current thread 0x0000000109284600 (most recent call first):
  File "<frozen importlib._bootstrap>", line 219 in _call_with_frames_removed
  File "<frozen importlib._bootstrap_external>", line 1043 in create_module
  File "<frozen importlib._bootstrap>", line 583 in module_from_spec
  File "<frozen importlib._bootstrap>", line 670 in _load_unlocked
  File "<frozen importlib._bootstrap>", line 967 in _find_and_load_unlocked
  File "<frozen importlib._bootstrap>", line 983 in _find_and_load
  File "importtests.py", line 12 in <module>

Note that line 12 in my importtests.py file is where from vmtk.vtkvmtkCommonPython import * is called.

ramtingh commented 2 years ago

@hkjeldsberg Thanks! If you get a chance can you also try it without the patch I added? (I assume remove line 13/14 in meta.yaml) The code base itself works fine in Slicer's osx builds, so presumably the only possible error would be in the recipe/patch I've added.

hkjeldsberg commented 2 years ago

@ramtingh I tried without the patch locally, but it resulted in termination errors during compilation. Seems like your patch is necessary for it to build VMTK successfully.

Here is the last output during compilation:

[301/598] Python Wrapping - generating vtkvmtkPolyDataLaplaceBeltramiStencilPython.cxx
[302/598] Python Wrapping - generating vtkvmtkPolyDataFELaplaceBeltramiStencilPython.cxx
[303/598] Building CXX object vtkVmtk/Utilities/vtkvmtkITK/CMakeFiles/vtkvmtkITK.dir/vtkvmtkITKArchetypeImageSeriesScalarReader.cxx.o
[304/598] Building CXX object vtkVmtk/Utilities/vtkvmtkITK/CMakeFiles/vtkvmtkITK.dir/vtkvmtkITKImageWriter.cxx.o
ninja: build stopped: subcommand failed.
Traceback (most recent call last):
  File "/Users/henriakj/miniconda3/bin/conda-build", line 11, in <module>
    sys.exit(main())
  File "/Users/henriakj/miniconda3/lib/python3.7/site-packages/conda_build/cli/main_build.py", line 488, in main
    execute(sys.argv[1:])
  File "/Users/henriakj/miniconda3/lib/python3.7/site-packages/conda_build/cli/main_build.py", line 479, in execute
    verify=args.verify, variants=args.variants, cache_dir=args.cache_dir)
  File "/Users/henriakj/miniconda3/lib/python3.7/site-packages/conda_build/api.py", line 195, in build
    variants=variants
  File "/Users/henriakj/miniconda3/lib/python3.7/site-packages/conda_build/build.py", line 3093, in build_tree
    notest=notest,
  File "/Users/henriakj/miniconda3/lib/python3.7/site-packages/conda_build/build.py", line 2212, in build
    cwd=src_dir, stats=build_stats)
  File "/Users/henriakj/miniconda3/lib/python3.7/site-packages/conda_build/utils.py", line 410, in check_call_env
    return _func_defaulting_env_to_os_environ('call', *popenargs, **kwargs)
  File "/Users/henriakj/miniconda3/lib/python3.7/site-packages/conda_build/utils.py", line 390, in _func_defaulting_env_to_os_environ
    raise subprocess.CalledProcessError(proc.returncode, _args)
subprocess.CalledProcessError: Command '['/bin/bash', '-o', 'errexit', '/Users/henriakj/miniconda3/conda-bld/vmtk_1646634849188/work/conda_build.sh']' returned non-zero exit status 1.
ramtingh commented 2 years ago

@hkjeldsberg Fair enough, thanks for the help. Yeah I think that patch is necessary with conda compilers otherwise it overrides important settings. I will be able to look into this in more detail over the next couple of weeks, until then I'd appreciate any ideas of what might be going on.

Similar problems on windows ended up being slightly different hdf5 settings between the vtk and itk packages used, but as far as I've checked that shouldn't be a problem with the current versions

rupertnash commented 2 years ago

I also would like this to work on macOS. I've never tried to use the conda-forge stuff before, so apologies for any misunderstandings there.

I've built this branch using the build-locally.py script. When I test I get:

$ python3.7 -c 'import vmtk.vmtkcenterlines'
Traceback (most recent call last):
  File "<string>", line 1, in <module>
  File "/Users/rnash2/work/hemelb-dev/vmtk-feedstock/miniforge3/conda-bld/debug_1649756879502/_h_env/lib/python3.7/site-packages/vmtk/vmtkcenterlines.py", line 20, in <module>
    from vmtk import vtkvmtk
  File "/Users/rnash2/work/hemelb-dev/vmtk-feedstock/miniforge3/conda-bld/debug_1649756879502/_h_env/lib/python3.7/site-packages/vmtk/vtkvmtk.py", line 9, in <module>
    from .vtkvmtkCommonPython import *
ImportError: dlopen(/Users/rnash2/work/hemelb-dev/vmtk-feedstock/miniforge3/conda-bld/debug_1649756879502/_h_env/lib/python3.7/site-packages/vmtk/vtkvmtkCommonPython.so, 2): Library not loaded: libvtkvmtkCommon.dylib
  Referenced from: /Users/rnash2/work/hemelb-dev/vmtk-feedstock/miniforge3/conda-bld/debug_1649756879502/_h_env/lib/python3.7/site-packages/vmtk/vtkvmtkCommonPython.so
  Reason: image not found

Inspecting the dylib shows:

otool -l /Users/rnash2/work/hemelb-dev/vmtk-feedstock/miniforge3/conda-bld/debug_1649756879502/_h_env/lib/python3.7/site-packages/vmtk/vtkvmtkCommonPython.so
<snip />
Load command 9
          cmd LC_LOAD_DYLIB
      cmdsize 48
         name libvtkvmtkCommon.dylib (offset 24)
   time stamp 2 Thu Jan  1 01:00:02 1970
      current version 0.0.0
compatibility version 0.0.0
Load command 10
          cmd LC_LOAD_DYLIB
      cmdsize 56
         name @rpath/libpython3.7m.dylib (offset 24)
   time stamp 2 Thu Jan  1 01:00:02 1970
      current version 3.7.0
compatibility version 3.7.0
<snip />
Load command 14
          cmd LC_RPATH
      cmdsize 112
         path /Users/rnash2/work/hemelb-dev/vmtk-feedstock/miniforge3/conda-bld/debug_1649756879502/_h_env/lib (offset 12)
<snip />

which reveals that the VMTK dylib has not been linked correctly: load command 9 should point to @rpath/libvtkvmtkCommon.dylib. I'll see if I can tweak the CMake flags to fix this.

rupertnash commented 2 years ago

Setting -DCMAKE_MACOSX_RPATH:BOOL=ON reproduces the segfault on import. Debugger shows it's in the module init function. I'm rebuilding with CMAKE_BUILD_TYPE=Debug while I get lunch....

rupertnash commented 2 years ago

OK - something is messed up in the python/libpython binaries installed by the build system. Debugger shows that the segfault occurs in PyModule_Create2, which isn't super surprising.

* thread #1, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x10)
  * frame #0: 0x000000011961fd9b libpython3.7m.dylib`PyModule_Create2 + 11
    frame #1: 0x000000011959fa48 vtkvmtkCommonPython.so`::real_initvtkvmtkCommonPython((null)=<unavailable>) at vtkvmtkCommonPythonInitImpl.cxx:34:17 [opt]

However! Note that the version of that function is inside the libpython dylib. The python executable provided by conda does NOT link against that library. Indeed setting a breakpoint on PyModule_Create2 shows that there are two version of that function in the running process:

(lldb) breakpoint set -n PyModule_Create2                                                                                                            Breakpoint 1: 2 locations.
(lldb) breakpoint list
Current breakpoints:
1: name = 'PyModule_Create2', locations = 2, resolved = 2, hit count = 0
  1.1: where = python3.7`PyModule_Create2, address = 0x0000000100086740, resolved, hit count = 0 
  1.2: where = libpython3.7m.dylib`PyModule_Create2, address = 0x000000011961fd90, resolved, hit count = 0 

So, I'm not surprised this is dying when the interpreter is using one set of functions while the extension module is using another

rupertnash commented 2 years ago

I had another look at this just now. The problem is that the vtkMacroKitPythonWrap macro (in a .cmake of the same name) downloaded from https://github.com/Slicer/vtkAddon incorrectly links against the python library:

target_link_libraries(${MY_KIT_NAME}Python
      PRIVATE
        ${MY_KIT_NAME}
        ${VTK_PYTHON_LIBRARIES} #error
        VTK::WrappingPythonCore
        VTK::Python
        )

Any link that is needed should be picked up through the link against VTK::Python (assuming its INTERFACE_LINK_LIBRARIES property is correctly set - VTK is usually quite good about that)

Since this is an external dependency, I will make a PR there. When accepted (I realise that Slicer may have requirements making this harder...) we can update VMTK's CMake/CMakeLists.txt to point to that and then this recipe should work as-is.

EDIT: PR is https://github.com/Slicer/vtkAddon/pull/27 in case anyone here is also maintainer over there?

ramtingh commented 2 years ago

Thanks! this looks very promising. That is the exact opposite of what I was expecting; I thought as the code works fine for slicer builds it couldn't be anything upstream related, but I don't have nearly enough osx/cmake experience to figure out what is going on.

I guess there's three levels this could be fixed at, either the slicer repository, patched in vmtk's git repository or as a patch in this repository (as @rupertnash has already added). @lassoan I think that depends on your preference.

ramtingh commented 2 years ago

@conda-forge-admin, please rerender

ramtingh commented 2 years ago

@conda-forge-admin, please rerender

ramtingh commented 2 years ago

@rupertnash After some minor fixes to the test script, your changes seem to work very well. Only 1 out of 542 tests is failing on osx. For some reason vmtkcentrelinesnetwork hangs on osx+python 3.8/3.9, I strongly suspect joblib is running into an issue and stalling, but I think the worst case scenario if I can't figure out what is going wrong is I'll just disable joblib on osx for now (This test has historically been very finicky anyway).

I'll wait for the disucussion in https://github.com/Slicer/vtkAddon/pull/27 to figure out what is the best way of addressing this. Logistically I'd be perfectly happy patching it here to avoid too much disruption to other build systems.

rupertnash commented 2 years ago

Great!

ramtingh commented 2 years ago

@conda-forge-admin, please rerender

rupertnash commented 2 years ago

Hi @ramtingh - I've pushed a new version to the vtkAddon PR. I'm waiting to see what the maintainers think. In the meantime, I've verified (in PR #5) that it works for us. Agree that waiting on vtkAddon decision is the correct course for this repo.

ramtingh commented 2 years ago

@rupertnash In the interest of not delaying this too much, I'm thinking of merging it as is so osx packages are available on conda-forge and once vtkAddon is updated it can be fixed for future builds. I can't think of any issues that can cause (besides some technical debt I'd have to fix later), do you think I should wait?

rupertnash commented 2 years ago

Aye, you're probably right. Suggest creating an issue here to track the TODO. Thanks!

ramtingh commented 2 years ago

@rupertnash Fair enough, I appreciate all the work you've done to make this work! I'll merge this for now