Skip to content

Issue 699: add a workaround for read_chunk in hdf5 2.0#700

Open
jkotan wants to merge 13 commits intomasterfrom
issue_699b
Open

Issue 699: add a workaround for read_chunk in hdf5 2.0#700
jkotan wants to merge 13 commits intomasterfrom
issue_699b

Conversation

@jkotan
Copy link
Copy Markdown
Collaborator

@jkotan jkotan commented Apr 1, 2026

It adds a workaround for read_chunk in hdf5 2.0 and adds missing V200 definition.
To make full support of read_chunk we would need to add either

  • size to dataspace and datatype traits
  • or adds read_chunk with additional size argument

@jkotan jkotan requested review from ggoneiESS and yuelongyu April 1, 2026 18:19
@ggonei
Copy link
Copy Markdown

ggonei commented Apr 1, 2026

I don't see any issue with the HDF2 stuff because it's all conditional, but maybe it would be good to pull out the PIXI stuff into a different MR to make either easier to revert? Would be good to get @milnowak or André to add some thoughts there on the CI

@jkotan
Copy link
Copy Markdown
Collaborator Author

jkotan commented Apr 2, 2026

I don't see any issue with the HDF2 stuff because it's all conditional, but maybe it would be good to pull out the PIXI stuff into a different MR to make either easier to revert? Would be good to get @milnowak or André to add some thoughts there on the CI

I've added issue_699c branch to illustrate the problem. After commenting workaround you get https://github.com/ess-dmsc/h5cpp/actions/runs/23888511368/job/69656363675

 [ 90%] Building CXX object test/node/CMakeFiles/node_test.dir/dataset_direct_chunk_test.cpp.o
 │ │ In file included from $SRC_DIR/src/h5cpp/hdf5.hpp:88,
 │ │                  from $SRC_DIR/test/node/dataset_direct_chunk_test.cpp:32:
 │ │ $SRC_DIR/src/h5cpp/node/dataset.hpp: In instantiation of 'uint32_t hdf5::node::Dataset::read_chunk(T&, const hdf5::datatype::Datatype&, std::vector<long unsigned int>&, const hdf5::property::DatasetTransferList&) const [with T = std::vector<short unsigned int>; uint32_t = unsigned int]':
 │ │ $SRC_DIR/src/h5cpp/node/dataset.hpp:940:20:   required from 'uint32_t hdf5::node::Dataset::read_chunk(T&, std::vector<long unsigned int>, const hdf5::property::DatasetTransferList&) const [with T = std::vector<short unsigned int>; uint32_t = unsigned int]'
 │ │   940 |   return read_chunk(data, mem_type_holder.get(data), offset, dtpl);
 │ │       |          ~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 │ │ $SRC_DIR/test/node/dataset_direct_chunk_test.cpp:117:24:   required from here
 │ │   117 |             dataset.read_chunk(read_chunk_value, {i, 0, 0});
 │ │       |             ~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 │ │ $SRC_DIR/src/h5cpp/node/dataset.hpp:953:23: error: too few arguments to function 'herr_t H5Dread_chunk2(hid_t, hid_t, const hsize_t*, uint32_t*, void*, size_t*)'
 │ │   953 |       if(H5Dread_chunk(static_cast<hid_t>(*this),
 │ │       |                       ^
 │ │ In file included from $BUILD_PREFIX/include/hdf5.h:24,
 │ │                  from $SRC_DIR/src/h5cpp/core/hdf5_capi.hpp:44,
 │ │                  from $SRC_DIR/src/h5cpp/hdf5.hpp:27:
 │ │ $BUILD_PREFIX/include/H5Dpublic.h:1320:15: note: declared here
 │ │  1320 | H5_DLL herr_t H5Dread_chunk2(hid_t dset_id, hid_t dxpl_id, const hsize_t *offset, uint32_t *filters,
 │ │       |               ^~~~~~~~~~~~~~
 │ │ make[2]: *** [test/node/CMakeFiles/node_test.dir/build.make:457: test/node/CMakeFiles/node_test.dir/dataset_direct_chunk_test.cpp.o] Error 1
 │ │ make[1]: *** [CMakeFiles/Makefile2:1394: test/node/CMakeFiles/node_test.dir/all] Error 2
 │ │ make: *** [Makefile:146: all] Error 2

Basically, in the new version of read_chunk i.e. read_chunk2 there is an additional parameter size_t* which contains byte size of the buffer which should protect overwriting in memory (if the buffer is too small the data is not read)

@jkotan
Copy link
Copy Markdown
Collaborator Author

jkotan commented Apr 2, 2026

I've just added the read_chunk implementation with the byte_size parameter. In case we add the size method we can replace the H5Dread_chunk1 call by read_chunk with the byte_size calculated from the corresponding traits

@jkotan
Copy link
Copy Markdown
Collaborator Author

jkotan commented Apr 2, 2026

The pixi stuff was added to test the current PR (with hdf5 2.10). The pixi tests would fail separetrly. On the other hand the PR cannot be fully tested without pixi test. Other option would be to upgrade conan tests to hdf5 2

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.

2 participants