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

Lock native float16 dtype only if available #2422

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ajelenak
Copy link
Contributor

@ajelenak ajelenak commented May 5, 2024

I've created this PR to start discussion how to solve the #2419 issue. Not sure if the current fix is the right approach but locking the H5T_NATIVE_FLOAT16 datatype now happens only if it's not equal to H5I_INVALID_HID.

Copy link

codecov bot commented May 5, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.58%. Comparing base (c75aac4) to head (a27a1f4).

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #2422   +/-   ##
=======================================
  Coverage   89.58%   89.58%           
=======================================
  Files          17       17           
  Lines        2391     2391           
=======================================
  Hits         2142     2142           
  Misses        249      249           

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

@jhendersonHDF
Copy link

Thanks for this proposed fix @ajelenak! I have a branch I'm working on that fixes the crashing issues in #2419, but I'm still hitting a corrupted heap/use after free issue that I suspect comes from some interaction between h5py's custom datatype conversion methods and the changes to type conversion in 1.14.4 that avoids unnecessary copying of datatypes. It appears the library is ending up with multiple IDs pointing to the same datatype pointer, leading to issues when the library is terminating and closing IDs. Still tracking this down.

@takluyver
Copy link
Member

Is only H5T_NATIVE_FLOAT16 affected by this, or do H5T_IEEE_F16BE & ...LE need similar checks?

@jhendersonHDF
Copy link

Is only H5T_NATIVE_FLOAT16 affected by this, or do H5T_IEEE_F16BE & ...LE need similar checks?

The H5T_IEEE_F16BE and H5T_IEEE_F16LE macros will always map to a valid HDF5 datatype. Only H5T_NATIVE_FLOAT16 could be invalid, depending on the build.

@ajelenak
Copy link
Contributor Author

ajelenak commented May 7, 2024

Perhaps h5py needs a special Cython class for unavailable datatypes in h5t.pyx for any future cases like this one? I haven't checked whether H5Tget_class(H5I_INVALID_HID) would throw an error or not.

@ajelenak
Copy link
Contributor Author

ajelenak commented May 9, 2024

I haven't checked whether H5Tget_class(H5I_INVALID_HID) would throw an error or not.

Update:

import h5py
a = h5py.h5t.TypeID(-1)
a.get_class()

Produces an exception:

ValueError: Not a datatype (not a datatype)

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