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

Build with Numpy 2.0 #231

Open
wants to merge 22 commits into
base: develop
Choose a base branch
from
Open

Build with Numpy 2.0 #231

wants to merge 22 commits into from

Conversation

jaimergp
Copy link

Investigating #210

@jaimergp jaimergp mentioned this pull request Jun 18, 2024
@jaimergp
Copy link
Author

CI available at https://github.com/jaimergp/zfp/actions/runs/9568406928/job/26378542726. It passes but I do see some warnings about deprecated APIs:

[  8%] Building C object python/CMakeFiles/zfpy.dir/zfpy.c.o
In file included from /opt/hostedtoolcache/Python/3.12.3/x64/lib/python3.12/site-packages/numpy/_core/include/numpy/ndarraytypes.h:1909,
                 from /opt/hostedtoolcache/Python/3.12.3/x64/lib/python3.12/site-packages/numpy/_core/include/numpy/ndarrayobject.h:12,
                 from /opt/hostedtoolcache/Python/3.12.3/x64/lib/python3.12/site-packages/numpy/_core/include/numpy/arrayobject.h:5,
                 from /home/runner/work/zfp/zfp/build/python/zfpy.c:1232:
/opt/hostedtoolcache/Python/3.12.3/x64/lib/python3.12/site-packages/numpy/_core/include/numpy/npy_1_7_deprecated_api.h:17:2: warning: #warning "Using deprecated NumPy API, disable it with " "#define NPY_NO_DEPRECATED_API NPY_1_7_API_VERSION" [-Wcpp]
   17 | #warning "Using deprecated NumPy API, disable it with " \
      |  ^~~~~~~
[ 98%] Building C object tests/python/CMakeFiles/test_utils.dir/test_utils.c.o
In file included from /opt/hostedtoolcache/Python/3.12.3/x64/lib/python3.12/site-packages/numpy/_core/include/numpy/ndarraytypes.h:1909,
                 from /opt/hostedtoolcache/Python/3.12.3/x64/lib/python3.12/site-packages/numpy/_core/include/numpy/ndarrayobject.h:12,
                 from /opt/hostedtoolcache/Python/3.12.3/x64/lib/python3.12/site-packages/numpy/_core/include/numpy/arrayobject.h:5,
                 from /home/runner/work/zfp/zfp/build/tests/python/test_utils.c:1232:
/opt/hostedtoolcache/Python/3.12.3/x64/lib/python3.12/site-packages/numpy/_core/include/numpy/npy_1_7_deprecated_api.h:17:2: warning: #warning "Using deprecated NumPy API, disable it with " "#define NPY_NO_DEPRECATED_API NPY_1_7_API_VERSION" [-Wcpp]
   17 | #warning "Using deprecated NumPy API, disable it with " \
      |  ^~~~~~~

@rgommers
Copy link

Those warnings are harmless, you can safely ignore them. Would be nice to fix them eventually of course, but they've been there for many years and are unrelated to numpy 2.0

@seberg
Copy link

seberg commented Jun 18, 2024

Seems these builds are still using Python 3.8, in that case you can't force NumPy>=2 (not available). OTOH, if there is nothing else limiting the version, you'll get Numpy 2 anyway during the build.

Now, in principle if you still build for 3.8 you should still use oldest-supported-numpy unfortunately (although, not usre if you need to rebuild at all for 3.8 at this point).

@@ -1,2 +1,2 @@
cython>=0.22
numpy>=1.8.0
numpy>=2.0
Copy link
Contributor

Choose a reason for hiding this comment

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

is this a runtime requirements file or a build time requirements file?


- COMPILER: msvc
GENERATOR: Visual Studio 14 2015
PLATFORM: Win32
BUILD_TYPE: Release
PYTHON_VERSION: 38
PYTHON_VERSION: 39

- COMPILER: mingw
GENERATOR: MinGW Makefiles
Copy link
Contributor

Choose a reason for hiding this comment

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

Think these MinGW builds also need Python version set to 3.9. Appears they use Python 2.7 otherwise

@jakirkham
Copy link
Contributor

Think we also need someone to approve GHA so that it can run

@lindstro would you be able to help with that? 🙂

@jaimergp
Copy link
Author

The workflow triggers are not configured for pull requests. So you have to go to my branch and see the Actions tab there (push event triggers are active).

@jakirkham
Copy link
Contributor

I trust that you got it to work 🙂

Am now trying to figure out how we can work with the maintainers here to integrate it (though the suggested AppVeyor change above still seems advisable)

@lindstro
Copy link
Member

Think we also need someone to approve GHA so that it can run

@lindstro would you be able to help with that? 🙂

@jakirkham I'm happy to help out, but I must admit to being a Python novice at best. It seems reasonable to bump Python from 3.8 to 3.9, but are there no concerns about requiring NumPy 2.0?

There's also this lingering issue with setup.py that appears to not have gotten fixed. I'm not sure what the intent is. @GarrettDMorrison @da-wad Could either of you comment?

@jakirkham
Copy link
Contributor

Right now we are just focused on getting CI running. Think there is a button a repo owner can click

@lindstro
Copy link
Member

@jakirkham Thanks for the pointer. However, the "approve and run" button in question does not appear visible to me. I'm no t sure if that's some kind of repo configuration issue.

I also don't know why we're not running workflows on pull requests. Let me look into this. I think for now you'll have to consult @jaimergp's fork (see link to CI above).

@jaimergp
Copy link
Author

jaimergp commented Jul 1, 2024

I can add the PR support here, I think. Then you'll see the button John mentions.

@lindstro
Copy link
Member

lindstro commented Jul 1, 2024

I can add the PR support here, I think. Then you'll see the button John mentions.

@jaimergp I should have mentioned that I already added workflows on PRs on develop. We actually want this for our feature branches as well, not just develop. But I didn't do anything about concurrency. Can you please explain what this does?

I do see the "Approve and run" button now and tests are running...

@william-silversmith
Copy link
Contributor

Just looking at AppVeyor, it kinda looks like the failing runs are using python2.7 (EOL Jan. 1, 2020) and an old pip and so are not finding numpy 2.0 wheels. If its reconfigured to use Python3 and a newer pip is used, it'll probably work.

@jaimergp
Copy link
Author

jaimergp commented Jul 6, 2024

We actually want this for our feature branches as well, not just develop.

I was trying to have the workflows triggered for every push to develop (usually a merge), and then every PR targetting develop (from either a branch or a fork). If you don't restrict the push branches, then on PRs you'd duplicate every run because they'd be triggered by both push and pull_request.

But I didn't do anything about concurrency. Can you please explain what this does?

It'll cancel ongoing runs for a given PR (a new push to the PR will cancel the previous runs triggered by previous pushes if they are still running). On develop, it makes sure that only workflow is running per commit (just a safeguard, this is usually the case) anyway.

@jaimergp
Copy link
Author

jaimergp commented Jul 6, 2024

Also I'd like to see if you are interested in using Appveyor, given that we can simply configure Github Actions to run on Windows too. Thoughts?

@lindstro
Copy link
Member

lindstro commented Jul 6, 2024

We actually want this for our feature branches as well, not just develop.

I was trying to have the workflows triggered for every push to develop (usually a merge), and then every PR targetting develop (from either a branch or a fork). If you don't restrict the push branches, then on PRs you'd duplicate every run because they'd be triggered by both push and pull_request.

I'm not sure I understand. Clearly workflows are not triggered currently on PRs, which adding pull_request is supposed to address (i.e., what I just did on develop).

But I didn't do anything about concurrency. Can you please explain what this does?

It'll cancel ongoing runs for a given PR (a new push to the PR will cancel the previous runs triggered by previous pushes if they are still running). On develop, it makes sure that only workflow is running per commit (just a safeguard, this is usually the case) anyway.

Gotcha.

@lindstro
Copy link
Member

lindstro commented Jul 6, 2024

Also I'd like to see if you are interested in using Appveyor, given that we can simply configure Github Actions to run on Windows too. Thoughts?

I'd be the first to drop AppVeyor in favor of something better. I just don't have the cycles at the moment to investigate how to transition to GitHub Actions (which compilers and versions of CMake and Python to use, how/whether to deal with Fortran, how to interface with CDash, ...). Keep in mind that we're currently testing both MSVC and MinGW through AppVeyor.

If someone else has time to help out with this, I'd be very grateful.

@jaimergp
Copy link
Author

jaimergp commented Jul 7, 2024

I'm not sure I understand. Clearly workflows are not triggered currently on PRs, which adding pull_request is supposed to address (i.e., what I just did on develop).

I reverted the branch filter so you can see what happens when you run the workflows. You will see duplicated (redundant) entries for push and pull_request, which is usually a waste of CI resources.

@jaimergp
Copy link
Author

jaimergp commented Jul 8, 2024

I don't know why it can't find the Python.h headers. They are there in include. Those CMake functions are now deprecated so maybe it would be a good idea to upgrade them, but I don't have the time nor the expertise to dive there, sorry.

@hmaarrfk
Copy link
Contributor

hmaarrfk commented Jul 8, 2024

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.

7 participants