Fix track_order usage in do block

[t-bltg]

Follow up of #912.

• fix track_order only used with FileIO & OrderedDict #939;
• fix Use task_local_storage as a context instead of global state. #985.

Remove IDX_TYPE and ORDER globals (non thread safe global states).

And a little cleanup / simplifications.

[mkitti]

Did the method definitions need to move?

[t-bltg]

Did the method definitions need to move?

Yes, because src/fileio.jl is conditionally loaded by Requires.jl:

HDF5.jl/src/HDF5.jl

Line 208 in ccdb3e9

@require OrderedCollections="bac558e1-5e72-5ebc-8fee-abe8a469f55d" include("fileio.jl")

[mkitti]

We may eventually want to move IDX_TYPE into task_local_storage, but that is outside the scope of this pull request.

[t-bltg]

We may eventually want to move IDX_TYPE into task_local_storage, but that is outside the scope of this pull request.

Not needed, I'm reworking the pr to remove unsafe globals IDX_TYPE and ORDER.

[mkitti]

Not needed, I'm reworking the pr to remove unsafe globals IDX_TYPE and ORDER.

Honestly, I think this is pretty close to mergeable. Perhaps this second refactor should be another pull request?

Here, I'm mainly waiting on someone else, perhaps @musm to review before merging.

[mkitti]

This is close to mergeable. The one thing that is missing is checking the reversion of IDX_TYPE after the do syntax.

[t-bltg]

Currently stuck on one failing test. Will continue tomorrow.

[mkitti]

track_order seems to conflate attribute order and link order. However, a dataset only has attribute order.

Thus you need to have a distinct implementation of the track_order accessors for dataset creation property lists.

This makes me wonder what happens when link track order and attribute track order differ.

[mkitti]

Conflating attribute and link order seems a bit problematic. We might need to address that in the next minor version.

[t-bltg]

Thus you need to have a distinct implementation of the track_order accessors for dataset creation property lists.

Yes, or support track_order only for Groups and Files.

I'm currently struggling with the property lists for Groups and Files when reading since get_create_properties(...) through h5g_get_create_plist or h5f_get_create_plist doesn't seem to return the correct values.
Currently, fcpl is used only upon writing a new file:

HDF5.jl/src/file.jl

Line 35 in ccdb3e9

fid = API.h5f_create(filename, flag, fcpl, fapl)

thus the track_order property of fcpl is lost when reading.

It doesn't seem to be possible to modify the property list after the call to h56f_open here:

HDF5.jl/src/file.jl

Line 43 in ccdb3e9

fid = API.h5f_open(filename, flag, fapl)

This is why I've added a fcpl or gcpl named field to the File and Group structs and try to work with that now.

What remains is a failing test which occurs when h5object is used for iterating the File objects:

HDF5.jl/src/objects.jl

Line 25 in ccdb3e9

obj_type == API.H5I_GROUP ? Group(obj_id, file(parent)) :

[musm]

Generally looks good, still reviewing some of the PR.

Could you give a brief summary of how the task local changes are an improvement and operate?

One crude way to probe the value of the order would be to have global const refs, but I could easily imagine those values getting garbled with repeated calls in independent threads, even if things don't work 100% in multi-threaded contexts yet when using HDF5

[mkitti]

My biggest issue at the moment is that the get_create_properties and get_access_properties API is being repurposed while we still need the original functionality. I would leave those as they were.

Instead I might create an internal API to query the context directly. If the context property list is invalid, we can always initialize it to obtain a default value.

[mkitti]

The task local storage (TLS) is actually my idea, so I'll explain it. The (TLS) is basically a global namespace specific to each Task. If you start a new Task, you have a new TLS.

In our case, we need to temporarily change the global space for some fixed duration. To do so we can launch a new Task and then populate the TLS with the modified "global" context. Functions running within the Task will see the modified variables. These changes will be undone upon completion of the Task.

Specifically in our case those local variables are property lists and their properties. In this PR, we are mainly thinking about index order. In the future, I think we should use these property lists as a template when creating new property lists. When we need a new property list, we copy it from the local context rather than creating one de novo.

This is better than using a global constant Ref to store state in that the changes are never seen outside the Task. Although multithreading does not currently apply here, this also happens to be compatible with that case.

I have a working draft here of what I think this should look like:

https://github.com/mkitti/HDF5.jl/blob/be385088adef7d1a83b994b0b140e75b530f7272/src/context.jl

[mkitti]

Merge master to get #993 if you have not already. #993 auto-initializes the property lists. Thus we do not need a fallback to the object's property list since the context will always have a valid property list on demand.

If someone wants to change the global index type or order, they can modify the global CONTEXT. Otherwise, they can use the do syntax for h5open to locally change the value.

[t-bltg]

Looks like the failure on hdf5 with MPI support is spurious:

1. https://github.com/JuliaIO/HDF5.jl/runs/7544045615?check_suite_focus=true ✔
2. https://github.com/JuliaIO/HDF5.jl/runs/7544232871?check_suite_focus=true ✘

Even with the same failure, outcome of 1. is a passing test.

[mkitti]

  MethodError: no method matching h5o_get_info_by_name(::HDF5.File, ::String, ::Base.RefValue{HDF5.API.H5O_info1_t}, ::Int64)

That method error seems real though.

[t-bltg]

Yes, but unrelated to this PR, since also present on master:
https://github.com/JuliaIO/HDF5.jl/runs/7429724952?check_suite_focus=true

[mkitti]

I'm going to try to squash the test failure on this branch.

[mkitti]

I fixed the tests here in f5910a6 and in #1000

[musm]

The task local storage (TLS) is actually my idea, so I'll explain it. The (TLS) is basically a global namespace specific to each Task. If you start a new Task, you have a new TLS.

In our case, we need to temporarily change the global space for some fixed duration. To do so we can launch a new Task and then populate the TLS with the modified "global" context. Functions running within the Task will see the modified variables. These changes will be undone upon completion of the Task.

Specifically in our case those local variables are property lists and their properties. In this PR, we are mainly thinking about index order. In the future, I think we should use these property lists as a template when creating new property lists. When we need a new property list, we copy it from the local context rather than creating one de novo.

This is better than using a global constant Ref to store state in that the changes are never seen outside the Task. Although multithreading does not currently apply here, this also happens to be compatible with that case.

I have a working draft here of what I think this should look like:

https://github.com/mkitti/HDF5.jl/blob/be385088adef7d1a83b994b0b140e75b530f7272/src/context.jl

Do you plan on opening a PR after this is merged with your working draft as I find that those changes are important improvements?

[musm]

@t-bltg I'd recommend rebasing on master and cleaning up the commits. Then we can finally finalize merging in this PR. I'm not sure it would be best to squash all the commits or preserve the logical changes into several commits with more descriptive names. In either case a rebase would be helpful.

[mkitti]

Do you plan on opening a PR after this is merged with your working draft as I find that those changes are important improvements?

I would probably start another branch and send that in. I would also like to try implementing some of the applications. In particular, copying the context property lists to create default property lists for object creation.

For now, I've just documented this as an internal API that's under development.

[mkitti]

I would just squash it.

[t-bltg]

I fixed the tests here in f5910a6 and in #1000

Thanks for fixing.

@t-bltg I'd recommend rebasing on master and cleaning up the commits.

As you wish, rebased.

[musm]

Thanks for sticking to this and sorry for the long review!

[t-bltg]

Thanks all for helping finalizing this PR.