zarr-developers / numcodecs

A Python package providing buffer compression and transformation codecs for use in data storage and communication applications.
http://numcodecs.readthedocs.io
MIT License
125 stars 87 forks source link

Fix ZFPY import error #540

Closed px39n closed 2 months ago

px39n commented 3 months ago

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:

normanrz commented 3 months ago

We want to support numpy 2.0. Unfortunately, that means we need to wait until zfpy has been updated. See https://github.com/zarr-developers/numcodecs/pull/535

jakirkham commented 3 months ago

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

https://github.com/zarr-developers/numcodecs/blob/342754d736aa5a0c79024f87e0a5b8d1576c1e28/numcodecs/zfpy.py#L3-L5

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

https://github.com/zarr-developers/numcodecs/blob/342754d736aa5a0c79024f87e0a5b8d1576c1e28/pyproject.toml#L65-L67

jakirkham commented 3 months ago

Thanks! 🙏

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

https://github.com/zarr-developers/numcodecs/blob/342754d736aa5a0c79024f87e0a5b8d1576c1e28/docs/release.rst?plain=1#L27-L33

codecov[bot] commented 3 months ago

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 ```diff @@ 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](https://app.codecov.io/gh/zarr-developers/numcodecs/pull/540?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=zarr-developers) | Coverage Δ | | |---|---|---| | [numcodecs/zfpy.py](https://app.codecov.io/gh/zarr-developers/numcodecs/pull/540?src=pr&el=tree&filepath=numcodecs%2Fzfpy.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=zarr-developers#diff-bnVtY29kZWNzL3pmcHkucHk=) | `96.00% <83.33%> (-4.00%)` | :arrow_down: |
px39n commented 3 months ago

pre-commit.ci autofix

normanrz commented 3 months ago

This looks like a great solution. Thank you!

px39n commented 3 months ago

@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

rabernat commented 2 months ago

It looks like this PR was merged without hitting the coverage target. Now all subsequent PRs to numcodecs are failing that test (see https://github.com/zarr-developers/numcodecs/pull/544#issuecomment-2215898925).

We should fix this.

normanrz commented 2 months ago

Fixed in #546

jakirkham commented 2 months ago

Thanks all! 🙏