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

MAINT: Compile with Cython 3.0.10 #1303

Merged
merged 3 commits into from
Jul 23, 2024
Merged

Conversation

larsoner
Copy link
Collaborator

Compiled while on NumPy 2.0rc2 (though I doubt this matters) and Cython 3.0.10.

@larsoner
Copy link
Collaborator Author

@prabhuramachandran taking a stab at #1301 (comment), it seems like things are complicated if we try to support Python 3.8 since there is no NumPy 2.0 for Python 3.8. Okay to bump to 3.9?

@prabhuramachandran
Copy link
Member

prabhuramachandran commented May 29, 2024

@prabhuramachandran taking a stab at #1301 (comment), it seems like things are complicated if we try to support Python 3.8 since there is no NumPy 2.0 for Python 3.8. Okay to bump to 3.9?

Thanks for doing this. Yes, I am perfectly fine with dropping 3.8 and moving to 3.9 for the tests. I just changed the branch protection rules so the checks look OK now. So I will just go ahead and merge this after a quick review.

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.

I am not sure about the change to the build requirements as this will force a new numpy even if someone does not have it.

@@ -1,6 +1,6 @@
[build-system]
requires = [
"oldest-supported-numpy",
"numpy>=2.0.0rc2,<3",
Copy link
Member

Choose a reason for hiding this comment

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

Is this a problem? Should we reduce this requirement, what if someone is using 1.21.6 wants to install mayavi?

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 is for the build system. So if someone does pip install . or whatever, it should do an isolated build using NumPy 2+. This should be fine for them since wheels compiled with NumPy 2 are backward compatible with NumPy 1.x.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, but since numpy 2.x does not work with 3.8, those on an older Python cannot use Mayavi. Do we want that or is Python 3.8 ancient history?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Even though security support for 3.8 isn't over yet (it ends in 5 months), NumPy made the decision to drop it already. So to me it makes sense for Mayavi to drop it as well.

If it were trivial to support 3.8 I'd say "sure let's keep it", but I'm not sure how hard it would be to fix the errors seen here for example:

https://github.com/enthought/mayavi/actions/runs/9273915257/job/25515059891

  tvtk/src/array_ext.c:3467:13: error: call to undeclared function 'PyArray_MultiIter_ITERS'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]

I tried for a little while but it wasn't trivial for me at least.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the clarification, in any case people could use the previous release of mayavi if they really want to use Python 3.8.

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.

Approving.

@prabhuramachandran prabhuramachandran merged commit 964c95c into enthought:master Jul 23, 2024
11 checks passed
@larsoner larsoner deleted the cython branch September 6, 2024 17:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants