Use Preferences for using custom hdf5 library #1061

JoshuaLampert · 2023-04-19T13:18:47Z

This switches to Preferences.jl for using a system-provided hdf5 library and replaces the additional build step. Additionally, I allowed additional names of the library binaries as in my case, e.g., the binaries are located in /usr/lib/x86_64-linux-gnu/libhdf5_openmpi.so and /usr/lib/x86_64-linux-gnu/libhdf5_openmpi_hl.so.

closes #1037

cc @ranocha, @sloede

JoshuaLampert · 2023-04-19T13:32:34Z

Honestly, I don't understand why the tests complain about Package HDF5 does not have HDF5_JLL in its dependencies. Isn't HDF5_jll listed as a dependency here or am I missing something stupid?

Project.toml

src/api/api.jl

Co-authored-by: Michael Schlottke-Lakemper <[email protected]>

Project.toml

docs/src/index.md

docs/src/mpi.md

src/api/api.jl

mkitti · 2023-04-19T18:13:55Z

Please run the formatter. See the contrib folder for a utility.

Also this is breaking so I think we will want to version bump to 0.17 unless we implement a fallback. I'm also wondering if we could include a migration utility.

sloede · 2023-04-20T04:17:34Z

Also this is breaking so I think we will want to version bump to 0.17 unless we implement a fallback. I'm also wondering if we could include a migration utility.

MPI.jl had performed a similar migration last year, from configuration via environment variables to using Preferences. Back then they solved this by adding a migration guide to the docs and included notes for HPC administrators on how to configure this on clusters with managed MPI/HDF5 installations. Maybe this can serve as a blueprint for this PR here as well?

ranocha · 2023-04-20T12:05:44Z

Also this is breaking so I think we will want to version bump to 0.17 unless we implement a fallback. I'm also wondering if we could include a migration utility.

This should probably be mentioned in https://github.com/JuliaIO/HDF5.jl/blob/master/HISTORY.md, too, I guess?

mkitti · 2023-04-20T12:55:00Z

Yes, please document this as much as possible. If we do provide migration instructions, that should also be tested.

JoshuaLampert · 2023-04-21T11:27:11Z

I provided some migration instructions.

sloede · 2023-04-21T11:43:45Z

I provided some migration instructions.

This reads already very well! One suggestion from my side: Maybe you can give an example for admins of clusters what the Preferences file could look like and maybe explicitly mention that for backward compatibility it is recommended to both set the environment variables and use the Preferences file for the time being. But these are just suggestions, they are not critical IMHO. Other than that, this PR LGTM!

JoshuaLampert · 2023-04-21T12:08:09Z

I've also added an example of how the LocalPreferences.toml could look like and added a recommendation to also set the environment variable for backward compatibility.

sloede · 2023-04-21T12:10:36Z

I've also added an example of how the LocalPreferences.toml could look like and added a recommendation to also set the environment variable for backward compatibility.

Great, thanks!

JoshuaLampert · 2023-05-31T12:20:33Z

For me, it's also failing on master with the same error using a local libhdf5 v1.10.6 on

julia> versioninfo()
Julia Version 1.9.0
Commit 8e630552924 (2023-05-07 11:25 UTC)
Platform Info:
  OS: Linux (x86_64-linux-gnu)
  CPU: 8 × Intel(R) Core(TM) i7-7700HQ CPU @ 2.80GHz
  WORD_SIZE: 64
  LIBM: libopenlibm
  LLVM: libLLVM-14.0.6 (ORCJIT, skylake)
  Threads: 1 on 8 virtual cores
Environment:
  JULIA_HDF5_PATH = /usr/lib/x86_64-linux-gnu/hdf5/openmpi

I'm rather confused why it's not failing in the other CI tests from other PRs.

simonbyrne · 2023-05-31T21:49:17Z

I think the problem is that h5d_get_space is broken for virtual datasets for libhdf5 1.10. At the moment, we only test custom hdf5 build on Julia 1.3, and where we disable the virtual dataset tests.

We previously thought it was a buggy JLL, but it could just be all v1.10. I was able to replicate the error with a local build using libhdf5 1.10.7.

I suggest we just disable the virtual dataset tests if using libhdf5 <1.12.

JoshuaLampert · 2023-06-01T07:32:38Z

That totally makes sense. Thanks, @simonbyrne! I disabled the virtual datasets tests for libhdf5 < 1.12.

JoshuaLampert · 2023-06-01T13:03:13Z

Loading MPI.jl before HDF5.jl is not required anymore with Preferences.jl, right? I changed the docs accordingly.

ranocha · 2023-06-01T13:08:36Z

Status of this PR is that HDF5.jl still uses Requires.jl to load the MPI code, doesn't it? If so, it's still necessary to first load MPI.jl as far as I know

ranocha

Thanks! Looks good to me. Let's keep fingers crossed that tests pass 🤞

mkitti · 2023-06-01T15:29:29Z

Could we issue a warning if we detect that the environment variable JULIA_HDF5_PATH is set?

JoshuaLampert · 2023-06-01T15:46:11Z

Could we issue a warning if we detect that the environment variable JULIA_HDF5_PATH is set?

Done in 8e98087.

mkitti

The warning should be done during __init__ since the environment can change after precompilation occurs. Also, since we are encouraging people to set both the enviornment variable and Preferences.jl we should not warn if they are in fact using both.

I'm also wondering if we should just add a convenience method HDF5.API.set_libraries!.

const HDF5_JL_UUID = UUID("f67ccb44-e63f-5c2f-98bd-6dc0ccc4ba2f")

function set_libraries!(libhdf5 = nothing, libhdf5_hl = nothing; force = true)
   if isnothing(libhdf5) && isnothing(libhdf5_hl)
      delete_preferences!(HDF5_JL_UUID, "libhdf5")
      delete_preferences!(HDF5_JL_UUID, "libhdf5_hl")
      delete_preferences!("HDF5_jll", "libhdf5", "libhdf5_hl")
      @info "The libraries from HDF5_jll will be used."
    elseif isnothing(libhdf5) && isnothing(libhdf5_hl)
        throw(ArgumentError("Specify either no positional arguments or both positional arguments."))
   else
       isfile(libhdf5) || throw(ArgumentError("$libhdf5 is not a file that exists.")
       isfile(libhdf5_hl) || throw(ArgumentError("$libhdf5_hl is not a file that exists.")
       set_preferences!(HDF5_JL_UUID, "libhdf5" => libhdf5; force)
       set_preferences!(HDF5_JL_UUID, "libhdf5_hl" => libhdf5_hl; force)
       # Also set the HDF5_jll override settings in case some other package tries to use HDF5_jll
       set_preferences!(
           "HDF5_jll",
           "libhdf5_path" => libhdf5,
           "libhdf5_jl_path" => libhdf5_jl
       )
   end
   @info "Please restart Julia and reload HDF5.jl for the library changes to take effect"
end

See https://github.com/jheinen/GR.jl/blob/master/src/preferences.jl for further inspiration. Perhaps that is for another pull request.

src/api/api.jl

mkitti · 2023-06-01T16:16:51Z

test/configure_packages.jl

+import UUIDs, Preferences
+Preferences.set_preferences!(
+    UUIDs.UUID("f67ccb44-e63f-5c2f-98bd-6dc0ccc4ba2f"), # UUID of HDF5.jl
+    "libhdf5" => joinpath(JULIA_HDF5_PATH, "libhdf5.so");


Could libhdf5.so also be in a lib or lib64 subdirectory?

Not as it is now. Primarily, I created this file for CI, where we know, where the binaries are located. Now, that we suggest the user to have a look at this file, it would probably be nice to also allow the binaries to be in joinpath(JULIA_HDF5_PATH, "lib") or joinpath(JULIA_HDF5_PATH, "lib64").

test/configure_packages.jl

docs/src/index.md

mkitti · 2023-06-01T16:55:55Z

I think we should also document configuring HDF5_jll as well in case some other package tries to load that.

See https://docs.binarybuilder.org/stable/jll/#Overriding-specific-products

docs/src/mpi.md

mkitti · 2023-06-02T00:09:15Z

If you would prefer, I would be fine merging this as is and implementing the above later.

Co-authored-by: Simon Byrne <[email protected]> Co-authored-by: Mark Kittisopikul <[email protected]>

Co-authored-by: Mark Kittisopikul <[email protected]>

JoshuaLampert · 2023-06-02T08:13:39Z

I addressed most of the suggestions. The remaining parts can be done in a separate PR.

sloede · 2023-06-02T09:26:25Z

🥳

JoshuaLampert added 2 commits April 19, 2023 14:53

use system library using Preferences.jl

f7dc5c2

allow specific library filenames

7d226ae

sloede reviewed Apr 19, 2023

View reviewed changes

Project.toml Outdated Show resolved Hide resolved

sloede reviewed Apr 19, 2023

View reviewed changes

src/api/api.jl Outdated Show resolved Hide resolved

fix typo

3261f29

sloede reviewed Apr 19, 2023

View reviewed changes

src/api/api.jl Outdated Show resolved Hide resolved

JoshuaLampert and others added 3 commits April 19, 2023 16:02

Apply suggestions from code review

0489e4d

Co-authored-by: Michael Schlottke-Lakemper <[email protected]>

import dlsym

d60a8e6

remove deps folder from Format check

93601ac

ranocha reviewed Apr 19, 2023

View reviewed changes

Project.toml Outdated Show resolved Hide resolved

set julia compat to 1.6

7c34b18

sloede reviewed Apr 19, 2023

View reviewed changes

docs/src/index.md Show resolved Hide resolved

docs/src/mpi.md Show resolved Hide resolved

src/api/api.jl Outdated Show resolved Hide resolved

extend docs

f292a51

JoshuaLampert marked this pull request as ready for review April 19, 2023 15:33

JoshuaLampert added 3 commits April 20, 2023 10:53

format

efbda60

bump version

0571a1d

bump HDF5 compat versions of filters to 0.17

99f695a

update HISTORY.md

6eb754c

add note on migration instructions

2e5239b

add example of LocalPreferences.toml in docs

d190097

ranocha mentioned this pull request Apr 21, 2023

MPI support via package extensions (weak dependency) #1062

Merged

disable virtual dataset tests for libhdf5 version < 1.12

bd14d4f

JoshuaLampert force-pushed the preferences branch from d05bb58 to bd14d4f Compare June 1, 2023 13:15

fix typo in docs

13ec550

ranocha approved these changes Jun 1, 2023

View reviewed changes

mkitti requested a review from simonbyrne June 1, 2023 15:29

raise warning if JULIA_HDF5_PATH is detected

8e98087

format

a156e60

mkitti requested changes Jun 1, 2023

View reviewed changes

simonbyrne reviewed Jun 1, 2023

View reviewed changes

docs/src/mpi.md Outdated Show resolved Hide resolved

JoshuaLampert and others added 4 commits June 2, 2023 08:57

Apply suggestions from code review

5304ba5

Co-authored-by: Simon Byrne <[email protected]> Co-authored-by: Mark Kittisopikul <[email protected]>

set both preferences in one call

b89dda3

set both preferences in one call

7e5eec7

Apply suggestions from code review

37dde47

Co-authored-by: Mark Kittisopikul <[email protected]>

JoshuaLampert requested review from mkitti and simonbyrne June 2, 2023 08:14

mkitti merged commit 4d446fc into JuliaIO:master Jun 2, 2023

JoshuaLampert deleted the preferences branch June 2, 2023 09:28

JoshuaLampert mentioned this pull request Jun 2, 2023

Update docs for parallel HDF5 trixi-framework/Trixi.jl#1504

Merged

JoshuaLampert mentioned this pull request Sep 8, 2023

Convenience function to use custom or system provided HDF5 binaries #1114

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use Preferences for using custom hdf5 library #1061

Use Preferences for using custom hdf5 library #1061

JoshuaLampert commented Apr 19, 2023 •

edited

Loading

JoshuaLampert commented Apr 19, 2023

mkitti commented Apr 19, 2023

sloede commented Apr 20, 2023

ranocha commented Apr 20, 2023

mkitti commented Apr 20, 2023

JoshuaLampert commented Apr 21, 2023

sloede commented Apr 21, 2023

JoshuaLampert commented Apr 21, 2023

sloede commented Apr 21, 2023

JoshuaLampert commented May 31, 2023 •

edited

Loading

simonbyrne commented May 31, 2023

JoshuaLampert commented Jun 1, 2023

JoshuaLampert commented Jun 1, 2023

ranocha commented Jun 1, 2023

ranocha left a comment

mkitti commented Jun 1, 2023

JoshuaLampert commented Jun 1, 2023

mkitti left a comment

mkitti Jun 1, 2023

JoshuaLampert Jun 2, 2023

mkitti commented Jun 1, 2023 •

edited

Loading

mkitti commented Jun 2, 2023

JoshuaLampert commented Jun 2, 2023

sloede commented Jun 2, 2023

Use Preferences for using custom hdf5 library #1061

Use Preferences for using custom hdf5 library #1061

Conversation

JoshuaLampert commented Apr 19, 2023 • edited Loading

JoshuaLampert commented Apr 19, 2023

mkitti commented Apr 19, 2023

sloede commented Apr 20, 2023

ranocha commented Apr 20, 2023

mkitti commented Apr 20, 2023

JoshuaLampert commented Apr 21, 2023

sloede commented Apr 21, 2023

JoshuaLampert commented Apr 21, 2023

sloede commented Apr 21, 2023

JoshuaLampert commented May 31, 2023 • edited Loading

simonbyrne commented May 31, 2023

JoshuaLampert commented Jun 1, 2023

JoshuaLampert commented Jun 1, 2023

ranocha commented Jun 1, 2023

ranocha left a comment

Choose a reason for hiding this comment

mkitti commented Jun 1, 2023

JoshuaLampert commented Jun 1, 2023

mkitti left a comment

Choose a reason for hiding this comment

mkitti Jun 1, 2023

Choose a reason for hiding this comment

JoshuaLampert Jun 2, 2023

Choose a reason for hiding this comment

mkitti commented Jun 1, 2023 • edited Loading

mkitti commented Jun 2, 2023

JoshuaLampert commented Jun 2, 2023

sloede commented Jun 2, 2023

JoshuaLampert commented Apr 19, 2023 •

edited

Loading

JoshuaLampert commented May 31, 2023 •

edited

Loading

mkitti commented Jun 1, 2023 •

edited

Loading