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

Fix ZFPY import error #540

Merged
merged 11 commits into from
Jul 8, 2024
Merged

Fix ZFPY import error #540

merged 11 commits into from
Jul 8, 2024

Conversation

px39n
Copy link
Contributor

@px39n px39n commented Jun 29, 2024

When

numpy >=2.0.0
from numcodecs.zfpy import ZFPY

raise error [ValueError: numpy.ndarray size changed, may indicate binary incompatibility. Expected 88 from C header, got 80 from PyObject]

The error is caused by zfpy library crashing with recent ABI breaking of numpy udpate. So we'd better force numpy <2.0 unless zfpy update again.

Reference: (https://stackoverflow.com/questions/66060487/valueerror-numpy-ndarray-size-changed-may-indicate-binary-incompatibility-exp)

[Description of PR]

TODO:

  • Unit tests and/or doctests in docstrings
  • Tests pass locally
  • Docstrings and API docs for any new/modified user-facing classes and functions
  • Changes documented in docs/release.rst
  • Docs build locally
  • GitHub Actions CI passes
  • Test coverage to 100% (Codecov passes)

When 

numpy >=2.0.0
from numcodecs.zfpy import ZFPY

raise error [ValueError: numpy.ndarray size changed, may indicate binary incompatibility. Expected 88 from C header, got 80 from PyObject]


The error is caused by zfpy library crashing with recent ABI breaking of numpy udpate. So we'd better force numpy <2.0 unless zfpy update again.


Reference: (https://stackoverflow.com/questions/66060487/valueerror-numpy-ndarray-size-changed-may-indicate-binary-incompatibility-exp)
@normanrz
Copy link
Contributor

normanrz commented Jul 1, 2024

We want to support numpy 2.0. Unfortunately, that means we need to wait until zfpy has been updated. See #535

@jakirkham
Copy link
Member

Agreed

That said, do understand why this error is confusing

Perhaps we can add some logic here to check NumPy version and disable the extension with an appropriate message to the user

_zfpy = None
with suppress(ImportError):
import zfpy as _zfpy

We could also ensure that the zfpy optional dependency has this constraint

numcodecs/pyproject.toml

Lines 65 to 67 in 342754d

zfpy = [
"zfpy>=1.0.0",
]

px39n added 3 commits July 2, 2024 11:02
Add warning when user import zfpy but with numpy >=2.0.0
ensure that the zfpy optional dependency has this constraint.
Copy link
Member

@jakirkham jakirkham left a comment

Choose a reason for hiding this comment

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

Thanks! 🙏

Am wondering if we should check whether zfpy exists before warning

Also wondering if we should limit which zfpy versions warn (since at some point this will be fixed)

What do you think? 🙂

numcodecs/zfpy.py Outdated Show resolved Hide resolved
numcodecs/zfpy.py Outdated Show resolved Hide resolved
px39n and others added 2 commits July 2, 2024 11:27
@jakirkham
Copy link
Member

Thanks! 🙏

Please add a release note as well. Perhaps under fix?

Fix
~~~
* Fix skip of entry points backport tests
By :user:`Elliott Sales de Andrade <QuLogic>`, :issue:`487`.
* Fix Upgrade to Zstd 1.5.5 due to potential corruption.
By :user:`Mark Kittisopikul <mkitti>`, :issue:`429`

Copy link

codecov bot commented Jul 2, 2024

Codecov Report

Attention: Patch coverage is 83.33333% with 2 lines in your changes missing coverage. Please review.

Project coverage is 99.82%. Comparing base (342754d) to head (577ca5a).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #540      +/-   ##
==========================================
- Coverage   99.91%   99.82%   -0.09%     
==========================================
  Files          59       59              
  Lines        2319     2329      +10     
==========================================
+ Hits         2317     2325       +8     
- Misses          2        4       +2     
Files Coverage Δ
numcodecs/zfpy.py 96.00% <83.33%> (-4.00%) ⬇️

@px39n
Copy link
Contributor Author

px39n commented Jul 2, 2024

pre-commit.ci autofix

@normanrz
Copy link
Contributor

normanrz commented Jul 2, 2024

This looks like a great solution. Thank you!

@px39n
Copy link
Contributor Author

px39n commented Jul 2, 2024

@jakirkham Thank you for constructive suggestions, and thanks @normanrz for mention #535

It helps the numpy 2.0 users like me who would like to use zfpy but no clues where is going wrong with import error

@px39n px39n requested a review from jakirkham July 2, 2024 12:48
@normanrz normanrz merged commit 4929b35 into zarr-developers:main Jul 8, 2024
24 of 26 checks passed
@rabernat
Copy link
Contributor

rabernat commented Jul 9, 2024

It looks like this PR was merged without hitting the coverage target. Now all subsequent PRs to numcodecs are failing that test (see #544 (comment)).

We should fix this.

@normanrz
Copy link
Contributor

normanrz commented Jul 9, 2024

Fixed in #546

@jakirkham
Copy link
Member

Thanks all! 🙏

DimitriPapadopoulos pushed a commit to DimitriPapadopoulos/numcodecs that referenced this pull request Aug 10, 2024
* Update pyproject.toml

When 

numpy >=2.0.0
from numcodecs.zfpy import ZFPY

raise error [ValueError: numpy.ndarray size changed, may indicate binary incompatibility. Expected 88 from C header, got 80 from PyObject]


The error is caused by zfpy library crashing with recent ABI breaking of numpy udpate. So we'd better force numpy <2.0 unless zfpy update again.


Reference: (https://stackoverflow.com/questions/66060487/valueerror-numpy-ndarray-size-changed-may-indicate-binary-incompatibility-exp)

* Update zfpy.py

Add warning when user import zfpy but with numpy >=2.0.0

* Update pyproject.toml

ensure that the zfpy optional dependency has this constraint.

* Update zfpy.py

* Update numcodecs/zfpy.py

Co-authored-by: jakirkham <[email protected]>

* Update numcodecs/zfpy.py

Co-authored-by: jakirkham <[email protected]>

* Update zfpy.py

* Update release.rst

* Update release.rst

* style: pre-commit fixes

* Update zfpy.py

---------

Co-authored-by: jakirkham <[email protected]>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
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.

4 participants