add support for read-only s3 virtual driver #1010

gszep · 2022-09-11T22:27:39Z

Related to #552; We would like the equivalent functionality as h5py

h5open("s3://..."; driver=Drivers.ROS3(aws_region="us-west-2", key="", id="")) do file
    file
end

mkitti

Thanks for the pull request.

We need to pass a pointer to H5Pset_fapl_ros3. Perhaps we should box the struct in a Ref or concretely as a RefValue.

src/drivers/drivers.jl

src/api/types.jl

src/drivers/drivers.jl

mkitti · 2022-09-11T22:56:57Z

src/drivers/drivers.jl

+This is the read-only virtual driver that enables access to HDF5 objects stored in AWS S3
+"""
+struct ROS3 <: Driver
+    fa::API.H5FD_ros3_fapl_t


This probably should be a Base.RefValue{API.H5FD_ros3_fapl_t}. It can then be converted to a pointer by ccall.

gszep · 2022-09-11T23:47:20Z

@mkitti thank you for the helpful review! Do you have a recommendation for a unit test for this? We need a public HDF5 file hosted on S3.. It appears that some of the builds do not pass raising

could not find function H5Pset_fapl_ros3 in library libhdf5

suggesting that these libhdf5 builds to not have ros3

mkitti · 2022-09-12T00:12:19Z

You might need to register this lazily like the mpio driver rather than during __init__. Not all of the HDF5 binaries we bundle have this driver enabled.

mkitti · 2022-09-12T00:14:52Z

The modifications to functions.jl directly are not going to work. They will be removed the next time someone calls the API generator.

I will provide a suggestion t as an alternate fix.

mkitti

Here are some suggestions.

Please regenerate functions.jl.

src/drivers/drivers.jl

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

src/file.jl

test/plain.jl

mkitti · 2022-09-12T21:16:56Z

test/ros3.jl

+    h5open("http://s3.us-east-2.amazonaws.com/hdf5ros3/GMODO-SVM01.h5"; driver=HDF5.Drivers.ROS3()) do f
+        @test keys(f) == ["All_Data", "Data_Products"]
+    end


Nice. Can you add a test to see if you can actually read data correctly?

mkitti · 2022-09-12T21:17:34Z

docs/src/api_bindings.md

@@ -477,6 +477,7 @@ h5pl_size
 - [`h5p_get_fapl_mpio32`](@ref h5p_get_fapl_mpio32)
 - [`h5p_get_fapl_mpio64`](@ref h5p_get_fapl_mpio64)
 - [`h5p_get_fapl_multi`](@ref h5p_get_fapl_multi)
+- [`h5p_get_fapl_ros`](@ref h5p_get_fapl_ros)


Hmm... I think we may need to the patch the generator.

agreed. the name should be h5p_get_fapl_ros3

I added a bind exception for ros3 in cc6625e

mkitti

Just a few changes. If you could just comment out the failing tests and mark it for later that would be fine. It would be even better if you could narrow the situation to remove the test / error case.

mkitti · 2022-09-13T14:13:11Z

This is getting closer. I need to examine the lazy loading mechanism more closely.

Basically it should only try to load the driver when the user tries to use it rather than eagerly loading it during initialization.

musm · 2022-09-13T15:53:24Z

src/drivers/ros3.jl

+    return (ROS3 ∘ API.H5FD_ros3_fapl_t)(
+        version,
+        auth,
+        Base.unsafe_convert(Cstring, region),


Can this be a regular convert or does it have to be an unsafe_convert ?

It needs to be an unsafe_convert:

julia> convert(Cstring, "hello") ERROR: MethodError: Cannot `convert` an object of type String to an object of type Cstring Closest candidates are: convert(::Type{Cstring}, ::Union{Ptr{Int8}, Ptr{Nothing}, Ptr{UInt8}}) at ~/src/julia-1.7.1/share/julia/base/c.jl:167 convert(::Type{T}, ::T) where T at ~/src/julia-1.7.1/share/julia/base/essentials.jl:218 Stacktrace: [1] top-level scope @ REPL[55]:1 julia> Base.unsafe_convert(Cstring, "hello") Cstring(0x00007fc5c5e06838)

you need to first call cconvert(Cstring, region) (this converts to String, and ensures it is nul-terminated)

you then need to keep a reference to the result of this whenever you pass it to a ccall

Probably the best option is to add them as extra fields to the ROS3 struct?

Couldn't we just do unsafe_convert(Cstring, String(region))?

I think I missed the original point of this all. Please correct me if I'm still wrong in my thinking
So the reason we need this is because we use the ROS3 instance in further calls (specifically h5p_set_fapl_ros3 h5p_get_fapl_ros3) and those need to convert the outward driver struct ROS3 to the internal libhdf5 struct H5FD_ros3_fapl_t and right now there's a mismatch in that ROS3 doesn't agree with the layout of H5FD_ros3_fapl_t?

We need to convert the Julia struct to the C struct. Part of the issue is that the Julia struct and its Strings could be garbage collected. Meanwhile, the Cstring is an unmanaged pointer to a null terminated character array.

Anyways, it turns out that the C struct definition here is wrong. The correct definition is

https://github.com/HDFGroup/hdf5/blob/100b22e6c23c44a082fd69b8c05a63c7492083f7/src/H5FDros3.h#L82-L88

typedef struct H5FD_ros3_fapl_t { int32_t version; hbool_t authenticate; char aws_region[H5FD_ROS3_MAX_REGION_LEN + 1]; char secret_id[H5FD_ROS3_MAX_SECRET_ID_LEN + 1]; char secret_key[H5FD_ROS3_MAX_SECRET_KEY_LEN + 1]; } H5FD_ros3_fapl_t;

In Julia, that would be

struct H5FD_ros3_fapl_t version::Int32 authenticate::Bool aws_region::NTuple{H5FD_ROS3_MAX_REGION_LEN + 1,Cchar} secret_id ::NTuple{H5FD_ROS3_MAX_SECRET_ID_LEN + 1, Cchar} secret_key::NTuple{H5FD_ROS3_MAX_SECRET_KEY_LEN + 1, Cchar}; end

I've started to rewrite this. Also, I think we need to check our definition of hbool_t. Currently it is UInt32, but I think it probably should be UInt8.

That's probably not the first time there has been a mismatch in definitions. Do you want to push your changes on this branch?

Yes, I will later today. I just started it on a different computer than I am on right now. I didn't quite get the tests to pass yet on the branch I was working on.

I quickly saw this comment on the hbool issue aldanor/hdf5-rust#28
but perhaps it doesn't affect us since we dont' use msvc

mkitti · 2022-09-14T21:33:45Z

I think this is about ready now. Is it?

mkitti · 2022-09-18T01:47:45Z

@simonbyrne , could you take a look at this pull request?

mkitti · 2022-09-18T01:48:25Z

@gszep is there any reason this is still in draft form?

mkitti · 2022-09-21T07:06:58Z

I think we should move forward with this.

simonbyrne · 2022-09-21T20:15:42Z

src/drivers/ros3.jl

@@ -0,0 +1,45 @@
+"""
+    ROS3()


Update the docstring signature

I added additonal signatures below this line.

simonbyrne · 2022-09-22T18:02:29Z

Do we have a way to test this?

mkitti · 2022-09-22T19:35:20Z

There is a test already, but without authentication.

There seems to be an example here:
https://www.hdfgroup.org/solutions/cloud-amazon-s3-storage-hdf5-connector/

musm · 2022-09-23T16:41:17Z

src/drivers/ros3.jl

 ROS3(region::AbstractString, id::AbstractString, key::AbstractString) =
    ROS3(1, true, region, id, key)
-ROS3() = ROS3(1, false, "", "", "")
+_ntuple_to_string(x) = unsafe_string(Ptr{Cchar}(pointer_from_objref(Ref(x))), length(x))


This doesn't convert the ntuple to a string back for me

_ntuple_to_string(x) = unsafe_string(Ptr{Cchar}(pointer_from_objref(Ref(x))), length(x)) x = "Dfsddfs" a = ntuple(i -> i <= length(x) ? Cchar(x[i]) : Cchar(0x0), 10 + 1) julia> _ntuple_to_string(a) "\xc8\x18\xb2\n\0\0\0\0X\xd0\xc3"

Shouldn't this actually look like something like the following

function _ntuple_to_string(buf) len = findfirst(iszero, buf) -1 unsafe_string(Base.unsafe_convert(Ptr{Int8}, collect(buf[1:len]))) end

Looks like this can also be written as

function _ntuple_to_string(buf) len = findfirst(iszero, buf) -1 unsafe_string(Base.unsafe_convert(Ptr{Int8}, collect(buf)), len) end

unsafe_string already searches for a null itself.

julia> function _ntuple_to_string(x) r = Ref(x) GC.@preserve r unsafe_string(Ptr{Cchar}(pointer_from_objref(r))) end _ntuple_to_string (generic function with 1 method) julia> _ntuple_to_string(a) "Dfsddfs"

We also probably should use ncodeunits and codeunits like you suggested before:
#818 (comment)

Why not use FixedString, which is already in the package?

HDF5.jl/src/typeconversions.jl

Lines 99 to 105 in 0120cb2

struct FixedString{N,PAD}

data::NTuple{N,UInt8}

end

Base.length(::Type{FixedString{N,PAD}}) where {N,PAD} = N

Base.length(str::FixedString) = length(typeof(str))

pad(::Type{FixedString{N,PAD}}) where {N,PAD} = PAD

pad(x::T) where {T<:FixedString} = pad(T)

FixedString is not an AbstractString.

I started working on a version of InlineStrings.jl called NStrings.jl over the weekend that basically is a full blown general implementation of this and is an AbstractString.

This is basically an implementation of this idea:
JuliaStrings/InlineStrings.jl#6

I'll publish that repo soon.

I was actually about to suggest moving it to a separate repo! Nice!

The initial version of NTuple based Strings is here:
https://github.com/mkitti/NStrings.jl

The current NTuple based Strings are now here and registered:

https://github.com/mkitti/StaticStrings.jl

mkitti · 2022-11-18T00:01:25Z

Let's merge just merge this. I'll follow up with a StaticStrings.jl pull request.

add ros3 functions

ef57f4d

gszep marked this pull request as draft September 11, 2022 22:28

mkitti requested changes Sep 11, 2022

View reviewed changes

fix julia julia to C struct passing

007b49d

mkitti requested changes Sep 12, 2022

View reviewed changes

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

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

Grisha Szep and others added 7 commits September 12, 2022 10:37

Update src/drivers/drivers.jl

ff4517c

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

Update src/drivers/drivers.jl

29f99d5

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

re-generate functions.jl and remove cchar hardcode

b7b4c18

remove local file existance check

bbbb1e4

remove user-defined cconvert

02eff7d

remove h5file existance test

692ded0

lazy tests

aa89a7f

mkitti reviewed Sep 12, 2022

View reviewed changes

src/file.jl Show resolved Hide resolved

obey formatter

97f0113

mkitti reviewed Sep 12, 2022

View reviewed changes

test/plain.jl Outdated Show resolved Hide resolved

mkitti reviewed Sep 12, 2022

View reviewed changes

mkitti requested changes Sep 12, 2022

View reviewed changes

gszep added 3 commits September 12, 2022 22:19

obey formatter

6aa4810

restore file existance tests

877f0ac

obey formatter

6fdeaa7

musm reviewed Sep 13, 2022

View reviewed changes

Make a bind exception for h5p_get_fapl_ros3 and h5p_set_fapl_ros3

cc6625e

mkitti marked this pull request as ready for review September 21, 2022 08:12

simonbyrne reviewed Sep 21, 2022

View reviewed changes

mkitti added 2 commits September 23, 2022 03:27

Refactor ROS3 driver interface to match C definitions

b904ce6

Update docstring, reorganize

6d41c99

musm reviewed Sep 23, 2022

View reviewed changes

mkitti approved these changes Nov 18, 2022

View reviewed changes

mkitti mentioned this pull request Nov 18, 2022

Merge strategy #1025

Open

musm merged commit 9891593 into JuliaIO:master Nov 21, 2022

	struct FixedString{N,PAD}
	data::NTuple{N,UInt8}
	end
	Base.length(::Type{FixedString{N,PAD}}) where {N,PAD} = N
	Base.length(str::FixedString) = length(typeof(str))
	pad(::Type{FixedString{N,PAD}}) where {N,PAD} = PAD
	pad(x::T) where {T<:FixedString} = pad(T)

add support for read-only s3 virtual driver #1010

add support for read-only s3 virtual driver #1010

Conversation

gszep commented Sep 11, 2022 • edited Loading

mkitti left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gszep commented Sep 11, 2022 • edited Loading

mkitti commented Sep 12, 2022

mkitti commented Sep 12, 2022

mkitti left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkitti left a comment

Choose a reason for hiding this comment

mkitti commented Sep 13, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkitti commented Sep 14, 2022

mkitti commented Sep 18, 2022

mkitti commented Sep 18, 2022

mkitti commented Sep 21, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simonbyrne commented Sep 22, 2022

mkitti commented Sep 22, 2022

musm Sep 23, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkitti Sep 26, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkitti commented Nov 18, 2022

gszep commented Sep 11, 2022 •

edited

Loading

gszep commented Sep 11, 2022 •

edited

Loading

musm Sep 23, 2022 •

edited

Loading

mkitti Sep 26, 2022 •

edited

Loading