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.

Sign up for GitHub

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

Jump to bottom

MAINT: Add NumPy include dir during build #1291

Merged
merged 26 commits into from
Mar 14, 2024
Merged

MAINT: Add NumPy include dir during build #1291

merged 26 commits into from
Mar 14, 2024

Conversation

larsoner
Copy link
Collaborator

@larsoner larsoner commented Feb 8, 2024

Needed to get conda-forge/mayavi-feedstock#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 larsoner mentioned this pull request Feb 8, 2024
Closed
@larsoner
Copy link
Collaborator Author

larsoner commented Feb 8, 2024

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
larsoner commented Feb 8, 2024
View reviewed changes
setup.py
Comment on lines -351 to -366
class MyClean(clean.clean):
"""Reimplements to remove the extension module array_ext to guarantee a
fresh rebuild every time. The module hanging around could introduce
problems when doing develop for a different vtk version."""
def run(self):
MY_DIR = os.path.dirname(__file__)

ext_file = os.path.join(
MY_DIR,
"tvtk",
"array_ext" + (".pyd" if sys.platform == "win32" else ".so")
)

if os.path.exists(ext_file):
print("Removing in-place array extensions {}".format(ext_file))
os.unlink(ext_file)
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This wasn't working anyway, on my system the generated file is tvtk/array_ext.cpython-311-x86_64-linux-gnu.so for example. We can revisit at some point if needed

@larsoner
Copy link
Collaborator Author

larsoner commented Feb 8, 2024

cherry-picked 3 commits from #1290 in an effort to get CIs green (can remove/revert later if that PR isn't merged before this one)

@larsoner
Copy link
Collaborator Author

GREEN!

@larsoner larsoner mentioned this pull request Feb 14, 2024
Merged
@larsoner
Copy link
Collaborator Author

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 😱 I vaguely recall having to fix something like this a while ago, I'll need to look up how I did it.

@opoplawski
Copy link

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

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

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

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

@larsoner
Copy link
Collaborator Author

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

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

@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
prabhuramachandran approved these changes Mar 14, 2024
View reviewed changes
Copy link
Member

@prabhuramachandran prabhuramachandran left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow, extensive changes. Thank you so much @larsoner, looks good to me.

@prabhuramachandran
Copy link
Member

@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.

@prabhuramachandran prabhuramachandran merged commit db518f8 into enthought:master Mar 14, 2024
10 checks passed
@larsoner larsoner deleted the include branch March 14, 2024 14:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@prabhuramachandran prabhuramachandran prabhuramachandran approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@larsoner @opoplawski @prabhuramachandran @bnavigator