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 flag to specify number of frames per-chunk in intermediate hdf5 data #367

Merged
merged 5 commits into from
Jul 25, 2024

Conversation

yousefmoazzam
Copy link
Collaborator

@yousefmoazzam yousefmoazzam commented Jun 13, 2024

Built on top of work done in #271

This adds the --frames-per-chunk flag which enables the ability to:

  • write the intermediate hdf5 data as chunked
  • specify the number of frames (projections or sinograms) per-chunk

A value of 0 will turn chunking off (ie, write the hdf5 data as contiguous), and values > 0 will be the number of frames per-chunk.

Acceptance criteria checklist:

  • Intermediate data is chunked as expected
  • Nicely handle the case when the frames per-chunk value provided exceeds the number of frames (h5py throws an error if this is the case, should be handled nicely in some way)
  • Add tests
  • All tests pass

@yousefmoazzam
Copy link
Collaborator Author

To address the problem of if the value given to the --frames-per-chunk flag exceeds the length of the slicing dim (ie, the chunk shape exceeds the boundaries of the data to be written), when this happens, the frames per-chunk value used is 1 instead of the flag value.

If other behaviour is preferred, feel free to mention, I'll open this for review now.

@yousefmoazzam yousefmoazzam marked this pull request as ready for review June 14, 2024 13:12
@@ -118,6 +118,12 @@ def check(yaml_config: Path, in_data_file: Path = None):
default=514,
help="Port on the host the syslog server is running on",
)
@click.option(
"--frames-per-chunk",
Copy link
Collaborator

Choose a reason for hiding this comment

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

@ptim0626 @yousefmoazzam
I wonder if we need more granular control over the chunk sizes here? For instance we would like to use a smaller chunk than the whole frame, e.g. (1, 150, 150)? Something like a tuple to pass instead of the number of frames. The first value in that tuple would be linked to the slicing dimension and the other two of non-slicing dimensions?
Could that help with more versatile optimisation around the better write performance? And also the read speed in Dawn?
Currently the run fails on Wilson if frames-per-chunk > 1

Copy link
Collaborator

@ptim0626 ptim0626 Jul 24, 2024

Choose a reason for hiding this comment

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

Considering performance, instead of decreasing the chunk size, I think increasing the chunk size would actually help.
(1) with compression, applying the filter to a single bigger chunk is usually quicker than multiple smaller chunks because of the overhead of threading for some filters llike Blosc
(2) larger chunks mean fewer metadata is required (the size of B-tree for looking up where the chunks are is smaller)

Choosing an optimal chunk size depends how we access the data, also a balance between read and write performance. I would say having one frame per chunk as the smallest unit with the option of increasing it via --frames-per-chunk is a good balance of performance and usability (does the general users know the implication of using a customised chunk shape?)

The recommended 1 MiB chunk size is about the default raw data chunk cache (rdcc) in hdf5, which is default to be 1 MiB. As long as we ensure we set the size of rdcc to be at least one chunk, it doesn't matter much for the chunk size, if it fits our access pattern. Even so, I don't think it impacts performance a lot, as most of the time (I guess) we just write/read the data once (so caching is not used).

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks @ptim0626 , it makes sense. The general user won't be playing with --frames-per-chunk I reckon so it would be nice to fix a default size that fits our needs best.

@dkazanc
Copy link
Collaborator

dkazanc commented Jul 24, 2024

Actually by enabling export OMPI_MCA_fcoll=dynamic fixed the PMIX related issue and I can confirm that I can run with more than one frame per chunk.

Setting the `--frames-per-chunk` flag to 0 will write the intermediate
data as contiguous, and any value > 0 will be the number of frames
(projections or sinograms) per-chunk.

Thus, this preserves the ability to turn chunking on/off when writing
intermediate data, but also adds the ability to choose the number of
frames per-chunk.
The method function now is standalone and has no reliance on the
framework's global variable `FRAMES_PER_CHUNK` to know the number of
frames per-chunk to write.

Instead, the associated method wrapper passes the framework's global
variable `FRAMES_PER_CHUNK` through to the method function.
If the value provided for the `--frames-per-chunk` flag is larger than
the length of the data's slicing dimension, then the number of frames
per-chunk will be set to 1 (and will be logged in the verbose logfile).

This is to avoid `h5py` errors with trying to create a chunk whose shape
exceeds the boundaries of the data being written.
@yousefmoazzam yousefmoazzam merged commit 4629a0f into main Jul 25, 2024
2 of 4 checks passed
@yousefmoazzam yousefmoazzam deleted the write-hdf5-chunked branch July 25, 2024 11:36
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.

None yet

3 participants