Bring back compatibility with HDF5 < v1.12 #756
Conversation
Tests fail with HDF5.jl v0.14.2, which requires HDF5 binaries v1.12. See JuliaIO/HDF5.jl#756 for details.
|
Right now, the binaries we ship with are restricted to v0.12 through HDF5_jll. I see where you are coming from. I think from my perspective, in an ideal world, we would have BinaryProvider distribute all compatible HDF5 versions and using the new Preference system we could figure out a way to distribute MPI compatible binaries along side. This would eliminate the need for system binaries and entirely solve the problem. The only challange here is that cross-compiling the hdf5 has proven to be quite difficult. There are v0.12 features we would like to incorporate and update the library to, and specifying a minimum to v0.10 would restrict this work. I'm not too keen on having to supporting multiple hdf5 version in a backward compatible way. One solution would be to allow any system built hdf5 version with no guarantee that HDF5.jl is fully compatible with that release. I could be convinced otherwise, but those are my current thoughts. |
|
Thanks for your comment. I understand your point, and I agree that supporting only the most recent version of the HDF5 libraries would be ideal from a developer and maintainer's point of view. It would be great if we could distribute the latest HDF5 binaries built for different systems, and ideally with support for the different possible MPI libraries. Unfortunately I don't see that happening anytime soon, as the MPI libraries vary wildly between different systems. In particular, HPC clusters usually provide MPI builds optimised for those systems, along with custom builds of HDF5 adapted for those MPI libraries. I am definitely not a specialist here, and I really appreciate the huge effort in distributing MPI, HDF5 and other libraries via BinaryProvider (from which I have greatly benefited), but it seems to me that removing the option for custom HDF5 binaries would make it very difficult to use HDF5.jl in many HPC systems. If I'm not wrong, right now, the only two ways of using HDF5.jl with MPI are:
While not ideal, I agree with this solution if it removes maintainer's burden. It's actually what I was thinking about when I proposed option (2) in the original post. And as this PR shows, very little changes are needed for tests to pass with older HDF5 libraries (but I'm aware that this may change in the future). |
| @test sprint(show, dspace_irrg) == "HDF5.Dataspace: (100, 4) [irregular selection]" | ||
|
|
||
| if HDF5.libversion ≥ v"1.12" | ||
| dspace_irrg = HDF5.Dataspace(HDF5.h5s_combine_select( |
There was a problem hiding this comment.
Is it the HDF5.h5s_combine_select that's failing here?
Apparently that should be available on 1.10.7
There was a problem hiding this comment.
Good point... I was testing with 1.10.4, which is why it fails for me.
Note that 1.10.7 is relatively recent, as it was released in September.
| @@ -35,9 +35,6 @@ else | |||
| if libhdf5_size != filesize(Libdl.dlpath(libhdf5)) | |||
| error("HDF5 library has changed, re-run Pkg.build(\\\"HDF5\\\")") | |||
| end | |||
| if libversion < v"1.12" | |||
| error("HDF5.jl requires ≥ v1.12 of the HDF5 library.") | |||
There was a problem hiding this comment.
@warn "HDF5.jl requires ... use at your own risk ...
Or something along those lines?
There was a problem hiding this comment.
Sounds good.
Maybe it would be better if it's only printed when HDF5.jl is built (here it would be printed every time HDF5.jl is loaded), but I don't know if that's possible.
Or maybe we could read an environment variable which would disable warnings, as in:
if libversion < v"1.12" && get(ENV, "JULIA_HDF5_NOWARN", "0") == "0"
@warn "HDF5.jl requires ≥ v1.12 of the HDF5 library, but v\$libversion was detected. Use at your own risk..."
endThere was a problem hiding this comment.
Probably better to check if JULIA_HDF5_LIBRARY_PATH is not set to nothing
There was a problem hiding this comment.
Or actually move this warning up in the file instead. (i.e. don't' write this out)
I don't think that will actually work, since the library would need to be loaded first.
There was a problem hiding this comment.
In that case we could load it at build time, just to get the library version. Note that this will only be done when JULIA_HDF5_LIBRARY_PATH is set.
There was a problem hiding this comment.
I just tried this, but the problem is that by default nothing is printed to the terminal when building a package. This is unless --verbose is explicitly passed, e.g. ]build --verbose HDF5.
that's right. Yeah this in general is going to be tricky, I don't even know if we have a good way to interact with the MPI interface JuliaPackaging/Yggdrasil#550 This will also stall PR's like #741 If we look at conda-forge, they provide mpich backd hdf5 binaries https://anaconda.org/conda-forge/hdf5/files It looks like the only real solution is to specify a minimum hdf5 library version and work on that? But then the question is which minimum version and at what point can we upgrade? We are essential beholden to arbitrary versions on some HPC clusters. It doesn't seem like a great long-term solution. |
I agree, that seems to be the only realistic solution. In h5py they seem to be able to support different HDF5 versions, but yeah, for sure it's some extra effort. But right now I think it would be reasonable to support HDF5 1.10, since it's not that old. I really don't want to prevent PRs like #741 from happening, but maybe one could add a small compatibility layer for those functions that were introduced in 1.12. People using unsupported HDF5 versions, like myself, could take care of that after those PRs are merged. |
|
How about v1.10.5 as a guaranteed minimum? HDF5.jl may support lower version, but I cannot confirm. @jipolanco which patch version did you test as your minimum? |
|
@jmert Do you have any thoughts on this? My current thinking is that we specify a minimum version and offer a warning for users using custom built binaries that we cannot guarantee compatibility, and to user at their own risk. The minimum should be some variant of 1.10 (patch number pending). I'm tempted to specify it as version 1.10.5, because that's the minimum version we offer BB binaries for. |
| @@ -35,9 +35,6 @@ else | |||
| if libhdf5_size != filesize(Libdl.dlpath(libhdf5)) | |||
There was a problem hiding this comment.
https://github.com/JuliaIO/HDF5.jl/blob/master/Project.toml will also need ~1.10.5. I'd also update https://github.com/JuliaIO/HDF5.jl/blob/master/HISTORY.md and add a new section with v0.14.3 to mention these recent changes that we brought back v1.10.5 BB support and the minimum guaranteed version.
I tested 1.10.4. I'd say that this is a reasonable minimum, since it's the version that is installable from the GitHub Actions images running Ubuntu 20.04. More recent versions would be difficult to test in CI... |
I'll have to re-review this thread (quickly glanced at the summaries in my email as they came through), but I'm definitely sympathetic to the hassles HPC clusters have — I mostly worked on my HPC cluster without any MPI, but I'm intimately familiar with the struggle from trying to get other software all working together. I think we can probably work toward having both 1.10 and 1.12 supported simultaneously, though that's going to either slow down adoption of e.g. the newer reference API or make their implementation more complicated. But since we're still using the older interface for now, I guess that just argues to not adopting the newer interface yet. An immediate action might be to tweak the CI again — maybe get rid of a few combos (all 3 OSs on only one Julia version maybe?) and then add in a new run that uses the Ubuntu system libhdf5. The other thing to think about right now is the example of |
|
@jipolanco thanks for the PR, I'm going to go ahead and relax the requirement for now in the PR #781, until we have a better plan for adoption or at least a better BB process. |
Since #739, HDF5 versions less than v1.12 are no longer supported. This is not a problem when one uses the default binaries provided by HDF5_jll. However, it kind of limits the use of custom binaries (as described in the docs), since the HDF5 v1.12 libraries are not that common in existent systems, and one would need to build them from source there. For instance, the latest versions of Fedora and Ubuntu both default to HDF5 v1.10. This is particularly inconvenient when combining HDF5 with MPI.
This PR is a proposal to bring back compatibility with HDF5 library versions less than v1.12. I checked that, with these changes, tests now pass when using HDF5 v1.10.
Probably further changes are needed to make sure that either (1) all tests pass on HDF5 v1.10, or (2) the user is aware that HDF5 < v1.12 is not officially supported.
For option (1), I can add an extra test run that uses the HDF5 1.10 binaries from the Ubuntu repos (I've done something similar in one of my packages). For option (2), one could include a comment in the docs and the README, and maybe a warning when HDF5 < v1.12 is first detected.