search

enthought / mayavi

3D visualization of scientific data in Python
http://docs.enthought.com/mayavi/mayavi/
Other
1.28k stars 282 forks source link

MAINT: Add NumPy include dir during build #1291

Closed larsoner closed 3 months ago

larsoner commented 4 months ago

Needed to get https://github.com/conda-forge/mayavi-feedstock/pull/77 builds working. Not sure why it wasn't required before/elsewhere! But should be safe/more complete to always have it I think.

larsoner commented 4 months ago

Okay this was much harder than I thought. NumPy on Python 3.12 has dropped numpy.distutils, so we need to get rid of that. Might as well refactor for full setuptools use. This PR now takes a stab at that. I'll try to keep iterating until it works, and also adjust the CIs to have a Python 3.12 run.

larsoner commented 4 months ago

cherry-picked 3 commits from https://github.com/enthought/mayavi/pull/1290 in an effort to get CIs green (can remove/revert later if that PR isn't merged before this one)

larsoner commented 4 months ago

GREEN!

larsoner commented 4 months ago

Just kidding... 3.12 (well really VTK 9.3) is still problematic. Based on some traceback and some local testing I think something is overwriting some memory where it shouldn't or something because the default trait it's trying to set are junk :scream: I vaguely recall having to fix something like this a while ago, I'll need to look up how I did it.

opoplawski commented 4 months ago

Thank you for working on this - need this for Fedora support. However it seems like the array_ext module isn't getting generated at all for me. Is this intentional?

larsoner commented 4 months ago

However it seems like the array_ext module isn't getting generated at all for me. Is this intentional?

No that's not intentional, it should be :( Locally I get that it is generated, and on CIs you can see it for example here. It should show up at the very least if you do python setup.py build_ext for example. On my macOS machine (though I got something similar on Linux last I looked):

$ python setup.py build_ext
running build_ext
building 'tvtk.array_ext' extension
creating build
creating build/temp.macosx-11.0-arm64-cpython-311
creating build/temp.macosx-11.0-arm64-cpython-311/tvtk
creating build/temp.macosx-11.0-arm64-cpython-311/tvtk/src
clang -DNDEBUG -fwrapv -O2 -Wall -fPIC -O2 -isystem /Users/larsoner/Applications/MNE-Python/1.6.0_0/.mne-python/include -arch arm64 -fPIC -O2 -isystem /Users/larsoner/Applications/MNE-Python/1.6.0_0/.mne-python/include -arch arm64 -I/opt/OpenBLAS/include -I/opt/homebrew/opt/llvm/include -I/Users/larsoner/Applications/MNE-Python/1.6.0_0/.mne-python/lib/python3.11/site-packages/numpy/core/include -I/Users/larsoner/Applications/MNE-Python/1.6.0_0/.mne-python/include/python3.11 -c tvtk/src/array_ext.c -o build/temp.macosx-11.0-arm64-cpython-311/tvtk/src/array_ext.o
...
$ ls build/lib.macosx-11.0-arm64-cpython-311/tvtk/
array_ext.cpython-311-darwin.so
opoplawski commented 4 months ago

Okay, I think it's an issue with the Fedora pyproject build macros, but I can stick with the classic python build macros for now. This fixes the Fedora build so it would be nice to see it merged. FWIW - we are still at VTK 9.2.6.

larsoner commented 4 months ago

I opened upstream https://gitlab.kitware.com/vtk/vtk/-/issues/19252 but I'll try to find a way to patch it here...

larsoner commented 4 months ago

Okay:

  1. Added a workaround for the vtkCylinderSource problem
  2. Added the shiny new macos-14 / arm64 runner for testing purposes

Everything is finally green for real on VTK 9.2.6 and 9.3 and Python 3.12. The corresponding mayavi-feedstock is also green so I think we should be good here!

I know the +7,129 −3,542 is scary but this is mostly the result of running Cython 3.0.8 locally to get things to be 3.12-compatible -- the rest is pretty straightforward I think.

larsoner commented 4 months ago

Okay, I think it's an issue with the Fedora pyproject build macros, but I can stick with the classic python build macros for now.

Could be -- but mayavi also does non-standard stuff like only including the tvtk_classes.zip when installing. They're not in the wheel for example if you build it (or the source dist). This is because there's a dependence on the VTK version being built against. But this is really an edge case as far as Python packages go I think so maybe you don't have to look into fixing anything with the Fedora build process knowing that mayavi has this behavior.

larsoner commented 3 months ago

@prabhuramachandran when you get a chance can you

  1. (Review if you want and) approve this PR
  2. Change the branch protection rules to reflect those in this PR
  3. Merge
prabhuramachandran commented 3 months ago

@prabhuramachandran when you get a chance can you

  1. (Review if you want and) approve this PR

Done.

  1. Change the branch protection rules to reflect those in this PR

I don't see these options yet on the branch protection rules page, I suspect I will need to merge this first and do it.

  1. Merge

Will do now.