search

LLNL / zfp

Compressed numerical arrays that support high-speed random access
http://zfp.llnl.gov
BSD 3-Clause "New" or "Revised" License
768 stars 155 forks source link

Build with Numpy 2.0 #231

Open jaimergp opened 3 months ago

jaimergp commented 3 months ago

Investigating https://github.com/LLNL/zfp/issues/210

jaimergp commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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

jakirkham commented 3 months ago

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

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

jaimergp commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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

lindstro commented 3 months ago

@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 commented 3 months ago

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

lindstro commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

See #227 or I believe I had a patch: https://github.com/conda-forge/zfpy-feedstock/blob/main/recipe/fix_find_python_stuff.patch