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

ENH: avoid deprecation warnings from numpy 2 #2416

Merged
merged 4 commits into from
May 8, 2024

Conversation

neutrinoceros
Copy link
Contributor

Fixes a couple deprecation warnings caught while running the test suite with pytest -Werror against numpy dev.

Namely

DeprecationWarning: numpy.core is deprecated and has been renamed to numpy._core. The numpy._core namespace contains private NumPy internals and its use is discouraged, as NumPy internals can change without warning in any release. In practice, most real-world usage of numpy.core is to access functionality in the public NumPy API. If that is the case, use the public NumPy API. If not, you are using NumPy internals. If you would still like to access an internal attribute, use numpy._core.records.

xref: numpy/numpy#24634

And

DeprecationWarning: __array__ implementation doesn't accept a copy keyword, so passing copy=False failed. __array__ must implement 'dtype' and 'copy' keyword arguments.

xref: numpy/numpy#25168

@neutrinoceros neutrinoceros changed the title TST: avoid a numpy deprecation warning in test_write_empty_vlen ENH: avoid deprecation warnings from numpy 2 Apr 24, 2024
h5py/_hl/dataset.py Outdated Show resolved Hide resolved
h5py/tests/test_dtype.py Outdated Show resolved Hide resolved
Copy link

codecov bot commented Apr 26, 2024

Codecov Report

Attention: Patch coverage is 94.11765% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 89.58%. Comparing base (c8972f1) to head (12c62ab).
Report is 5 commits behind head on master.

Files Patch % Lines
h5py/_hl/dataset.py 94.11% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##           master    #2416   +/-   ##
=======================================
  Coverage   89.58%   89.58%           
=======================================
  Files          17       17           
  Lines        2391     2401   +10     
=======================================
+ Hits         2142     2151    +9     
- Misses        249      250    +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@neutrinoceros
Copy link
Contributor Author

@tacaswell should I add tests for any/all new (uncovered) paths ?

@tacaswell
Copy link
Member

Yes please if it is not excessively complicated to make the tests work with both np1 and np2.

@neutrinoceros
Copy link
Contributor Author

Took me longer than I anticipated but I added tests to cover most new paths (I couldn't figure out how to use Dataset.fields properly so FieldsWrapper.__array__ still has an uncovered branch).
In the process I learned more about what these various classes do and I changed my approach. I think supporting copy=False in numpy 2.0 (which means: "raise an exception if you cannot avoid a copy/allocation) doesn't make sense for these classes since they always return newly allocated memory (as far as I understand), so I'm raising an exception where due.

Copy link
Member

@takluyver takluyver left a comment

Choose a reason for hiding this comment

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

Thanks! I think the NumPy copy stuff can be simplified a bit, I've put some comments about it inline.

h5py/_hl/dataset.py Outdated Show resolved Hide resolved
h5py/_hl/dataset.py Outdated Show resolved Hide resolved
h5py/_hl/dataset.py Outdated Show resolved Hide resolved
h5py/_hl/dataset.py Outdated Show resolved Hide resolved
h5py/_hl/dataset.py Outdated Show resolved Hide resolved
h5py/_hl/dataset.py Outdated Show resolved Hide resolved
h5py/_hl/dataset.py Outdated Show resolved Hide resolved
Co-authored-by: Thomas Kluyver <thomas.kluyver@xfel.eu>
h5py/_hl/dataset.py Outdated Show resolved Hide resolved
Co-authored-by: Thomas Kluyver <thomas.kluyver@xfel.eu>
@takluyver takluyver added this to the 3.12 milestone May 8, 2024
@takluyver
Copy link
Member

Thanks!

@takluyver takluyver merged commit b2a83a0 into h5py:master May 8, 2024
42 checks passed
@neutrinoceros neutrinoceros deleted the numpy2_compat branch May 8, 2024 10:03
@neutrinoceros
Copy link
Contributor Author

@takluyver these deprecation warnings are starting to affect downstream CI (I've heard of astropy, but there may be others). My assumption is that we'll release h5py 3.12 sometimes in the next 2 months for Python 3.13 wheels anyway, and I don't think that's a problem if not, but could you confirm ? Thank you !

@takluyver
Copy link
Member

Yup, I think we can do a new release fairly soon. I'd be tempted to wait for Python 3.13, but we can also go before that if we need to.

@neutrinoceros
Copy link
Contributor Author

with the rc1 out we should be able to ship wheels for it soon. This is happening in #2470

@takluyver
Copy link
Member

Ah, yes, that's right. But possibly we should wait for numpy to make wheels first?

@neutrinoceros
Copy link
Contributor Author

We don't have to. In fact matplotlib 3.9.2 already has cp313 wheels on PyPI, even though neither numpy or contourpy does, at the moment.
That said it's absolutely reasonable to wait for numpy if you'd like.

@takluyver
Copy link
Member

I doubt there's much pressure until numpy has wheels ready, and it gives us a chance to catch any random errors from numpy's wheels being built differently from how numpy gets built in our CI, so I probably would wait for that.

Python 3.13 also includes the option to disable the GIL, doesn't it? Do you know if we need to do anything around that? Using h5py you don't get much benefit from disabling the GIL, because HDF5 has it's own big lock, but I imagine someone will want to try.

@neutrinoceros
Copy link
Contributor Author

Yes there's a couple small things we can do to at least not get in the way of downstream testers. I've been learning about this topic lately and I should be able to expedite such changes in the coming days.

@takluyver
Copy link
Member

Thanks!

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.

None yet

3 participants