update cython defs to v1.6 compatibility in v1.12 #1536
Conversation
HDF5 v1.12 provides a compatibility feature, however it seems to not have covered everything, i.e. the compatibility-v1.6 is not the same as v1.6, so fixup the differences. Note: with HDF5 v1.12.0, a further patch is required to H5version.h (generated from H5vers.txt), to fix missing functions/types in the v1.6 compatibility spec (not sure whether it's a bug in upstream or not; it seems that the information about the API version in which a function is introduced might be inaccurate). Fixes issues in h5py#1504 and PR h5py#1506.
Codecov Report
@@ Coverage Diff @@
## master #1536 +/- ##
=======================================
Coverage 85.32% 85.32%
=======================================
Files 17 17
Lines 2296 2296
=======================================
Hits 1959 1959
Misses 337 337 Continue to review full report at Codecov.
|
|
Thanks for working this out! Compile-time conditionals based on the HDF5 version should be possible like this: Lines 48 to 57 in c69fc62 |
|
Not quite sure but it seems thirty lines above should also be changed. |
|
But even then it still does not build for me: Some fix in the line of e.g. csound/csound#1314 will be required. |
|
@acolinisi Sorry, I missed the fact that HDF5 itself has to be patched in order for this to work. I’ll try and see. |
|
Actually I still see the same issue after recompiling HDF5 1.12.0 with your patch… |
|
The patch for HDF5 is patching a generated header file. What I did was install HDF5 1.12.0 as is, then applied the patch to that header file installed in /usr/include, then built h5py. From your errors above, it looks like the header was not patched, can you take a look at that header file? |
|
I patched before building HDF5 actually, I will check the packaged file. |
|
Ok, I'm just worried that the header is auto-generated, but I'm not sure whether it is (re-)generated during the build or not. |
|
Seems so indeed, the packaged version does not include your changes. I will patch post-build and retry. |
|
OK, so that did the trick but I got a bunch of tests failures (note that I’m using 2.10.0 release): Most of them seems related to 110 not being the latest version anymore. |
|
Ok, I did built the (patched) 2.10.0 too, but I didn't run the test suite on 2.10.0; I only ran the testsuite in master (same as the CI in this PR). Since test suite passes in master, presumably there's some commit(s) on the master branch since the release that fix the above errors -- maybe something stands out in git log? |
|
All, The HDF Group developers started looking into the issues with porting h5py to 1.12. We need to talk to @takluyver and whoever is interested on possible approaches before taking on the task of porting h5py to 1.12 and preserving compatibility with 1.8 and 1.10 versions (if required). In a few words the problem is that h5py uses a mixture of HDF5 APIs from different versions of HDF5 and defaulting APIs to 1_6 doesn't work anymore with the HDF5 1.12 library. h5py needs to be converted to use versioned names and be consistent in that versioning within a set of HDF5 functions that use the same data structures. For example, if an application uses version 1 of H5Oget_info_by_name, then H5Lvisit, H5Ovisit, H5Lvisit_by_name, H5Literate, H5Literate_by_name, H5Lget_info, H5Lget_info_by_idx, and data structures H5O_info_t, H5L_info_t, H5L_iterate_t have to be of version 1. (I will not be surprised if I missed some functions already ;-) The HDF Group will document and post the analysis of the problem and proposed solution, but first, we need to understand h5py maintenance principles and an extent to which this community needs compatibility with the old versions of HDF5 before we dive into the work. |
@epourmal In src/H5Version.h, why is |
|
I am definitely interested in being involved.
Was this intentional?
We need to keep at least 1.10 and I would prefer to keep 1.8 (if it is not too difficult). We no longer support 1.6 so it is not a problem to drop that (we just have not put the effort into migrating the bindings to 1.8) |
API versioning was introduced in the 1.8.0 release. Since there was no H5Fget_info function in 1.6 (see H5Fget_info), library maps it to the latest. This is clearly described in RM entry I am pointing too. Confusing? Yes... We tried to simplify the path for applications to convert to new APIs, but backfired in 1.12.0 ;-( I would not dismiss that we may have bugs in HDF5 :-) |
|
|
Closing because this should be done instead and the respective errors resolved by moving the bindings to 1_8 API:
|
|
@acolinisi thanks - it's useful to have this change for reference as the first step for 1.12 compatibility. We can't start testing against 1.12 until we can compile against it. :-) @epourmal yes, let's set up a time to talk about this. I think you're in UTC-5 during the summer? How about 10am your time tomorrow (15.00 UTC / 11am New York / 4pm UK)? @tacaswell , does that work for you? If that's too short notice, we'll find a time next week. I agree with @tacaswell about compatibility. It looks like the last release of HDF5 1.8 (1.8.21) was in June 2018, so it's reaching the point where we could require 1.10, but if it's practical to keep 1.8 support, I'm still inclined to do so. |
|
@takluyver Thank you for getting back! Sorry, I cannot meet tomorrow at 10:00 am Central. Will 11:00 am Central work? If this doesn't work, morning of 5/5/ from 8:00 am - 2:00 pm will work. |
|
Thanks. The later time (16:00 UTC) works for me. Let's wait to hear if @tacaswell can make it, and if not, find a time on Tuesday. |
|
Sure! Thanks! |
|
Yes, I can make 1600 utc work. |
|
Thomas and Thomas, I just sent you an invite for May 1, 2020 11:00 am Central. Please feel free to forward the invite to anyone who may be interested. To other members: if you like to participate in the discussion, please contact me and I'll send you GTM info. |
NOT READY TO BE MERGED because these changes probably need to be under a conditional based on detected hdf5 version, similar to VOL_MIN_HDF5_VERSION (but I couldn't figure out the right way to do that).
HDF5 v1.12 provides a compatibility feature, however it seems to not have covered everything, i.e. the compatibility-v1.6 is not the same as v1.6, so this PR fixes up the differences. (A fix to a stop-gap until h5py moves to API v1.12 when available).
Note: with HDF5 v1.12.0, a further patch (attached) is required to H5version.h, to fix missing functions/types in the v1.6 compatibility spec. Not sure whether it's a bug in upstream or not; it seems that the information about the API version in which a function is introduced might be inaccurate in H5vers.txt -- @epourmal what do you think?. (I also tried bumping to H5_USE_18_API in setup_build.py, but then other function prototypes are mismatched, which is expected since h5py bindings are presumably written against v1.6).
Fixes issues in #1504 and PR #1506.
A test fails for me even on master even with HDF5 v1.10.6, so appears unrelated:
skipping that test, the rest of the test suite passes with this PR on top of master and HDF5 v1.12.0 with the attached HDF5 patch applied:
= 545 passed, 18 skipped, 17 deselected, 3 xfailed in 11.83s =hdf5-1.12.0-compat-1.6.patch.txt
Note: if you are looking to apply this PR to h5py v2.10.0 release, then you also need this patch:
h5py-2.10.0-hdf5-v1.12-h5i.patch.txt