I/O performance improvements

[iguinn]

• Use low level h5py API to for read. Note that this resulted in moving the handling of lists of files from _serializers.read.composite to store.read and core.read. Only the _serializers functions are using the low level API; read itself is still using h5py.File to open the file get the top level group.
• Require h5py >= 3.10. Not sure why, but there is a noticeable performance improvement starting at this version
• Encode string attributes to utf-8 before writing. This is something that happens implicitly anyway, so I'm not sure why doing this helps, but it does
• Write files using paged aggregation. This is a setting that you use when opening a file to write. Right now I have it hard-coded to use 64 kB pages, which seemed to be optimal in terms of file size and read speed, although this was only tested using a file with many small datasets. Also use latest version of file for writing
  Among these changes, the low level API made the biggest difference (a factor of almost 2), while the other two changes combine for an improvement of maybe 1.5 or so. The low level API makes a difference without reprocessing our files, while the other two changes happen at file write time, so will only be noticed after reprocessing

[codecov]

Codecov Report

Attention: Patch coverage is 81.13208% with 40 lines in your changes missing coverage. Please review.

Project coverage is 76.69%. Comparing base (7e2c9ee) to head (c134065).
Report is 36 commits behind head on main.

Files with missing lines	Patch %	Lines
...rc/lgdo/lh5/_serializers/read/vector_of_vectors.py	59.09%	9 Missing ⚠️
src/lgdo/lh5/_serializers/read/composite.py	69.56%	7 Missing ⚠️
src/lgdo/lh5/_serializers/read/ndarray.py	75.86%	7 Missing ⚠️
src/lgdo/lh5/store.py	82.92%	7 Missing ⚠️
src/lgdo/lh5/_serializers/read/utils.py	84.21%	3 Missing ⚠️
src/lgdo/lh5/core.py	91.42%	3 Missing ⚠️
src/lgdo/lh5/_serializers/read/encoded.py	83.33%	2 Missing ⚠️
src/lgdo/lh5/_serializers/read/array.py	88.88%	1 Missing ⚠️
src/lgdo/lh5/_serializers/read/scalar.py	87.50%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #100      +/-   ##
==========================================
+ Coverage   76.28%   76.69%   +0.41%     
==========================================
  Files          46       46              
  Lines        3036     3133      +97     
==========================================
+ Hits         2316     2403      +87     
- Misses        720      730      +10     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[gipert]

@oschulz anything to be worried about from the Julia side?

[oschulz]

@oschulz anything to be worried about from the Julia side?

Probably not, but can you give a test output file to @apmypb for testing?

[oschulz]

Probably not, but can you give a test output file to @apmypb for testing?

The best thing would be to replace the HDF5 files (read and write again with these improvements) in the legend-testdata repo as part of a PR there. Then we can test on Julia and finally merge into legend-testdata.

[iguinn]

Added a change to read files with locking=False (see #78 (comment))

[gipert]

I wonder if we really want to hardcode locking=False... Isn't it a useful feature?

[iguinn]

Yeah, perhaps we should make this an argument that defaults to True for write and False for read?

There are some other settings that I hardcoded here:

                {
                    "fs_strategy": "page",
                    "fs_page_size": 65536,
                    "fs_persist": True,
                    "fs_threshold": 1,
                    "libver": ("latest", "latest"),
                }

which I can change.

[gipert]

I'm a bit reluctant about hardcoding settings that might produce unexpected behavior or sub-optimal performance on a different system than your test one. Looking at the h5py defaults:

https://github.com/h5py/h5py/blob/959604fe7f6ee1f2bdc5328e9036b96c0ec44323/h5py/_hl/files.py#L376

seems like your fs_threshold and libver values are the same? Should we avoid hardcoding then?

Why did you change fs_persist?

About fs_page_size: maybe we should override the page size value in the dataflow only? Or do you feel that this value should be universally good? Again, I'm worried about unexpected behavior in user applications.

[iguinn]

I un-hardcoded these things. I added a locking option for read, which will default to False (I think this is ok in most of our use-cases and it helps with using the ro filesystem), and a page_buffer option for write which I am now defaulting to 0

[gipert]

This code is duplicated in this file and in core.py, can it be defined in only one place?

[iguinn]

Back in this pull request #97, I moved a lot of the code that opened files and found the group being read in into store.read and core.read, which created a lot of redundancy. I think both of these functions should be revisited. I actually tried fixing this at some point by having store.read use core.read, but it caused an error that I didn't have time to sort through; as I recall it had to do with how the output is sometimes an lgdo object and some times a tuple.

[gipert]

Looks good to me, I'll merge. Thanks a lot Ian for all this!