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

Numpy2 #982

Merged
merged 4 commits into from
Nov 14, 2024
Merged

Numpy2 #982

merged 4 commits into from
Nov 14, 2024

Conversation

mwrona77
Copy link
Contributor

@mwrona77 mwrona77 commented Nov 13, 2024

This is my first pull request on GitHub, so if I did something wrong, consider it a rookie mistake.

Since part of my issue involved changing np.product to np.prod, I made that adjustment separately from @giammarc.

Although this change was enough for all the tests to pass, there were a lot of warnings, mostly about the deprecation of certain functions. All other code changes I made are specifically to eliminate these warnings.

I understand that the next step is CI, but I assume that to run the tests, I need to make this pull request. Or should I have done this earlier?

Closes https://github.com/phoebe-project/phoebe-development/issues/110

Reverts #930

@mwrona77 mwrona77 requested a review from aprsa November 13, 2024 01:02
Copy link
Member

@kecnry kecnry left a comment

Choose a reason for hiding this comment

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

The diff looks reasonable to me - thanks! For some reason there is one build that is still failing - any idea why that is?

phoebe/dependencies/ligeor/models/polyfit.py Show resolved Hide resolved
phoebe/frontend/io.py Show resolved Hide resolved
@mwrona77
Copy link
Contributor Author

For some reason there is one build that is still failing - any idea why that is?

I'm not entirely sure, but there's a high chance that NumPy no longer supports building i686 wheels. See, for example: duckdb/duckdb#9101

This would make sense, as the CI fails specifically at
Building pp310-manylinux_i686 wheel
PyPy 3.10 manylinux i686.

Honestly, I have no idea how to resolve this. One option is to enforce using NumPy < 2.0 when building wheels for i686, or to drop support for i686 entirely.

Here's what I found in NumPy's pyproject.toml on GitHub (https://github.com/numpy/numpy/blob/060c28aa1920f0699ef7c3725e59dcd8e99723ca/pyproject.toml#L143):

[tool.cibuildwheel]
# Note: the below skip command doesn't do much currently, the platforms to
# build wheels for in CI are controlled in `.github/workflows/wheels.yml` and
# `tools/ci/cirrus_wheels.yml`.
build-frontend = "build"
skip = "cp36-* cp37-* cp-38* pp37-* *-manylinux_i686 *_ppc64le *_s390x *_universal2"
before-build = "bash {project}/tools/wheels/cibw_before_build.sh {project}"
# The build will use openblas64 everywhere, except on arm64 macOS >=14.0 (uses Accelerate)
config-settings = "setup-args=-Duse-ilp64=true setup-args=-Dallow-noblas=false build-dir=build"
before-test = "pip install -r {project}/requirements/test_requirements.txt"
test-command = "bash {project}/tools/wheels/cibw_test_command.sh {project}"

*-manylinux_i686 * is in skip

@kecnry
Copy link
Member

kecnry commented Nov 13, 2024

ok, then I'm ok with just adding to skip as well in this case. Anyone with that scenario can try to build from source as we used to do before #952

@mwrona77
Copy link
Contributor Author

ok, then I'm ok with just adding to skip as well in this case. Anyone with that scenario can try to build from source as we used to do before #952

Done! This time it passes all the tests. I added *-manylinux_i686 in the publish.yml to the CIBW_SKIP line.

Copy link
Member

@kecnry kecnry left a comment

Choose a reason for hiding this comment

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

LGTM - thanks for the fix!

@kecnry kecnry merged commit 0c6af3d into phoebe-project:bugfix-2.4.17 Nov 14, 2024
29 checks passed
This was referenced Nov 20, 2024
kecnry added a commit that referenced this pull request Nov 20, 2024
* Fix support for numpy 2.0. [#982]

---------

Co-authored-by: mwrona77 <125769279+mwrona77@users.noreply.github.com>
Co-authored-by: Marcin Wrona <mwrona@villanova.edu>
aprsa added a commit that referenced this pull request Dec 18, 2024
commit 7a2d8fb
Author: Kyle Conroy <kyleconroy@gmail.com>
Date:   Wed Nov 20 16:06:44 2024 -0500

    prepare readme and changelog for 2.4.18 bugfix

commit 84f9585
Author: Kyle Conroy <kyleconroy@gmail.com>
Date:   Wed Nov 20 16:05:30 2024 -0500

    2.4.17 release (#985)

    * Fix support for numpy 2.0. [#982]

    ---------

    Co-authored-by: mwrona77 <125769279+mwrona77@users.noreply.github.com>
    Co-authored-by: Marcin Wrona <mwrona@villanova.edu>
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