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

Add virtual dataset support #1012

Merged
merged 2 commits into from
Oct 17, 2022
Merged

Conversation

simonbyrne
Copy link
Collaborator

@simonbyrne simonbyrne commented Sep 15, 2022

Adds an interface for supporting HDF5 virtual datasets.

Copy link
Member

@mkitti mkitti left a comment

Choose a reason for hiding this comment

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

Could provide a way to set the virtual view property of the DatasetAccessPropertyList using h5p_set_virtual_view?

src/api/helpers.jl Show resolved Hide resolved
src/virtual.jl Outdated Show resolved Hide resolved
@mkitti
Copy link
Member

mkitti commented Sep 21, 2022

Could you please rebase this?

@mkitti
Copy link
Member

mkitti commented Sep 21, 2022

I'm also adding the cross reference to #617 and https://github.com/AStupidBear/HDF5Utils.jl/blob/master/src/virtual.jl as prior attempts to do this.

@mkitti
Copy link
Member

mkitti commented Sep 22, 2022

Could we add a property API on DatasetAccessProperties via H5Pset_virtual_view() and H5Pget_virtual_view() for completeness?

@simonbyrne
Copy link
Collaborator Author

I'm also adding the cross reference to #617 and https://github.com/AStupidBear/HDF5Utils.jl/blob/master/src/virtual.jl as prior attempts to do this.

Ah, I wasn't aware of these. Thanks!

@mkitti
Copy link
Member

mkitti commented Oct 16, 2022

Where are we on this?

@simonbyrne
Copy link
Collaborator Author

I've been pretty busy, so haven't had time to look at it further.

Are we happy with the interface? I was hoping to have a simpler higher level interface, but I guess it's okay for now.

It does need some docs as well.

Any other changes?

@mkitti
Copy link
Member

mkitti commented Oct 16, 2022

I would focus on writing some minimal documentation and get this merged. I would prefer these pull requests not hang around for a long time. As noted previously, we've already been through several versions of this without it having been merged in.

The interface is pretty much as straightforward as it could be, so let's proceed with that.

@simonbyrne simonbyrne marked this pull request as ready for review October 17, 2022 05:13
@simonbyrne simonbyrne merged commit 88da0ed into JuliaIO:master Oct 17, 2022
@simonbyrne simonbyrne deleted the sb/virtual-dataset branch October 17, 2022 16:38
"x",
datatype(Float64),
vspace;
virtual=[HDF5.VirtualMapping(vspace, "./sub-%0b.hdf5", "x", srcspace)]
Copy link
Member

@musm musm Oct 17, 2022

Choose a reason for hiding this comment

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

I'm getting the following error:

julia> d = create_dataset(
           f,
           "x",
           datatype(Float64),
           vspace;
           virtual=[HDF5.VirtualMapping(vspace, "./sub-%0b.hdf5", "x", srcspace)]
       )
ERROR: HDF5.API.H5Error: Error setting virtual
libhdf5 Stacktrace:
 [1] H5D_virtual_parse_source_name: Invalid arguments to routine/Bad value
     invalid format specifier
  
Stacktrace:
  [1] macro expansion
    @ C:\Users\Mus\.julia\packages\HDF5\5P16B\src\api\error.jl:18 [inlined]
  [2] h5p_set_virtual(dcpl_id::HDF5.DatasetCreateProperties, vspace_id::HDF5.Dataspace, src_file_name::String, src_dset_name::String, src_space_id::HDF5.Dataspace)
    @ HDF5.API C:\Users\Mus\.julia\packages\HDF5\5P16B\src\api\functions.jl:3960
  [3] push!
    @ C:\Users\Mus\.julia\packages\HDF5\5P16B\src\virtual.jl:43 [inlined]
  [4] append!
    @ C:\Users\Mus\.julia\packages\HDF5\5P16B\src\virtual.jl:50 [inlined]
  [5] set_virtual!
    @ C:\Users\Mus\.julia\packages\HDF5\5P16B\src\properties.jl:507 [inlined]
  [6] class_setproperty!(#unused#::Type{HDF5.DatasetCreateProperties}, p::HDF5.DatasetCreateProperties, name::Symbol, val::Vector{HDF5.VirtualMapping})
    @ HDF5 C:\Users\Mus\.julia\packages\HDF5\5P16B\src\properties.jl:550
  [7] setproperty!
    @ C:\Users\Mus\.julia\packages\HDF5\5P16B\src\properties.jl:59 [inlined]
  [8] (::HDF5.var"#4#5"{Tuple{HDF5.DatasetCreateProperties, HDF5.DatasetTransferProperties, HDF5.DatasetAccessProperties}})(::Pair{Symbol, Vector{HDF5.VirtualMapping}})
    @ HDF5 C:\Users\Mus\.julia\packages\HDF5\5P16B\src\properties.jl:139
  [9] filter
    @ .\abstractdict.jl:472 [inlined]
 [10] #setproperties!#3
    @ C:\Users\Mus\.julia\packages\HDF5\5P16B\src\properties.jl:135 [inlined]
 [11] create_dataset(parent::HDF5.File, path::String, dtype::HDF5.Datatype, dspace::HDF5.Dataspace; dcpl::HDF5.DatasetCreateProperties, dxpl::HDF5.DatasetTransferProperties, dapl::HDF5.DatasetAccessProperties, pv::Base.Pairs{Symbol, Vector{HDF5.VirtualMapping}, Tuple{Symbol}, NamedTuple{(:virtual,), Tuple{Vector{HDF5.VirtualMapping}}}})
    @ HDF5 C:\Users\Mus\.julia\packages\HDF5\5P16B\src\datasets.jl:57
 [12] top-level scope
    @ REPL[20]:1

Copy link
Member

Choose a reason for hiding this comment

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

julia> versioninfo()
Julia Version 1.8.2
Commit 36034abf26 (2022-09-29 15:21 UTC)
Platform Info:
  OS: Windows (x86_64-w64-mingw32)
  CPU: 16 × AMD Ryzen 9 5900HS with Radeon Graphics
  WORD_SIZE: 64
  LIBM: libopenlibm
  LLVM: libLLVM-13.0.1 (ORCJIT, znver3)
  Threads: 16 on 16 virtual cores
Environment:
  JULIA_EDITOR = code.cmd
  JULIA_NUM_THREADS = auto

Copy link
Member

Choose a reason for hiding this comment

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

Oops, we did not add virtual_dataset.jl to runtests.jl

julia> HDF5.API.SHORT_ERROR[] = false
false

julia> d = create_dataset(
           f,
           "x",
           datatype(Float64),
           vspace;
           virtual=[HDF5.VirtualMapping(vspace, "./sub-%0b.hdf5", "x", srcspace)]
       )
ERROR: HDF5.API.H5Error: Error setting virtual
libhdf5 Stacktrace:
 [1] H5D_virtual_parse_source_name: Invalid arguments to routine/Bad value
     invalid format specifier
 [2] H5Pset_virtual: Property lists/Unable to initialize object
     can't parse source file name

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah, it should be just %b: the docs are wrong, they never actually added support for the dimension specifier. I'll open a PR

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.

3 participants