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

Add development CI for NumPy 2.0 beta testing #5450

Merged
merged 43 commits into from
Apr 14, 2024

Conversation

MatthewFlamm
Copy link
Contributor

@MatthewFlamm MatthewFlamm commented Jan 10, 2024

Overview

This PR and branch serve as the development branch for numpy 2.0 testingfixes. Any fixes for numpy 2.0 should be use branch maint/add-tests-for-numpy2.0 as the base in a PR.

Details

https://numpy.org/devdocs/dev/depending_on_numpy.html#testing-against-the-numpy-main-branch-or-pre-releases

@github-actions github-actions bot added the maintenance Low-impact maintenance activity label Jan 10, 2024
@MatthewFlamm
Copy link
Contributor Author

I'm not sure that we will need to test 2.0 against all versions of vtk+python in our CI in an ongoing manner, but I think it would be good to test all of them up front.

Copy link

codecov bot commented Jan 10, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.83%. Comparing base (0db6f41) to head (8adf393).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #5450   +/-   ##
=======================================
  Coverage   96.83%   96.83%           
=======================================
  Files         139      139           
  Lines       24051    24051           
=======================================
  Hits        23290    23290           
  Misses        761      761           

@MatthewFlamm
Copy link
Contributor Author

MatthewFlamm commented Jan 10, 2024

I set this up so that the jobs still pass so it isn't confusing for other PRs. This means that to tell if changes work against numpy v 2.0, you have to click "Details" and look for failures in the logs themselves.

@MatthewFlamm
Copy link
Contributor Author

MatthewFlamm commented Jan 10, 2024

Currently tests for pyvista/core have 48 failures and many look like simple variable name substitutions even for vtk == 9.1. This looks promising. Tests for pyvista/plotting (and pv.Report() for that matter) don't run because at least one plotting depedency, cmocean is not compatible.

@MatthewFlamm MatthewFlamm marked this pull request as ready for review January 10, 2024 21:55
@banesullivan
Copy link
Member

because at least one plotting depedency, cmocean is not compatible.

If its just cmocean, I'll advocate just dropping that dependency here

Copy link
Member

@banesullivan banesullivan left a comment

Choose a reason for hiding this comment

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

Generally this looks well implemented though I'm concerned that this will significantly increase our GitHub actions runner usage and we should look into our current usage vs the quota before merging.

I'm wondering if instead of merging this soon here, if we should have a numpy-2.0 development branch until we are ready to support numpy2? Then we'd land changes, fixes, etc for numpy2 compatibility as separable PRs against this branch until we are ready to support numpy2 at which point we could merge this branch into main.

@banesullivan
Copy link
Member

banesullivan commented Jan 11, 2024

^ is just a suggestion/thought. @MatthewFlamm, I'm happy to go with what you think will provide the least friction since this sort of upgrade is likely to have enough friction already. Let me know and I'd happy to approve this PR as code-owner so as not to block the merge

@banesullivan
Copy link
Member

banesullivan commented Jan 11, 2024

we should look into our current usage vs the quota before merging.

I took a quick look and think we'll be under our quota but let's not leave these on forever

@MatthewFlamm
Copy link
Contributor Author

If its just cmocean, I'll advocate just dropping that dependency here

Let's give packages some time. We just started preparing for this and I put in a PR in cmocean. I'm more worried about meshio.

@MatthewFlamm
Copy link
Contributor Author

I'm wondering if instead of merging this soon here, if we should have a numpy-2.0 development branch until we are ready to support numpy2?

I'm subconsciously already treating this branch as the development branch. #5453 points here. So let's go with that idea. I will update the description here and change the CI here to show the fails. Any 2.0 fix PRs should use this branch as the base, and additionally we will keep this branch up to date with main. Before we merge this PR with main we will prune the CI to be more manageable, but I think testing on all versions would be useful to do while working out the initial bugs.

@MatthewFlamm MatthewFlamm marked this pull request as draft January 11, 2024 12:03
@MatthewFlamm MatthewFlamm changed the title Add tests for numpy 2.0 numpy 2.0 Jan 11, 2024
@MatthewFlamm
Copy link
Contributor Author

Locally testing #5453 with fixed versions of cmocean and meshio, I don't get any errors in our tests. So, I think we can merge #5453 and then wait for cmocean and meshio.

@MatthewFlamm
Copy link
Contributor Author

#5460 shows that downstream packages also need us to fix this. So, it might be good to prune the numpy 2.0 tests to something reasonable and merge to main. We will have to ignore failing numpy 2.0 tests in every PR though.

@banesullivan
Copy link
Member

The most practical way to go here might be to keep this branch for testing and validation and merge small changes like #5460 to main as they come

@tkoyama010 tkoyama010 disabled auto-merge March 27, 2024 20:04
@tkoyama010 tkoyama010 enabled auto-merge (squash) April 14, 2024 21:31
@tkoyama010 tkoyama010 requested a review from banesullivan April 14, 2024 21:38
@tkoyama010 tkoyama010 merged commit b84a679 into main Apr 14, 2024
30 checks passed
@tkoyama010 tkoyama010 deleted the maint/add-tests-for-numpy2.0 branch April 14, 2024 23:45
@banesullivan banesullivan mentioned this pull request Jul 7, 2024
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance Low-impact maintenance activity
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants