Add support for the new RNTuple format #395

jmcarcell · 2023-03-22T15:06:02Z

Add a writer and a reader using the podio Frame format. All tests pass and it doesn't break the other writes but the cost for that is a couple of ugly workarounds (see #393 for some of the issues that were found while doing this). My idea is that this is merged first and then we worry about the workarounds if fixes have not been found in time, otherwise this PR will get quite complicated.

For running with this a recent version of ROOT is needed, 6.28.00 or newer (I'm not sure 100% if it works with 6.28.00 since I have been running off master). There is a new option in the CMakeLists to enable support for this by asking cmake to find the relevant ROOT library. When this option is enabled (-DENABLE_RNTUPLE), the writer reader and tests will be compiled, otherwise nothing should have changed.

The writer and reader have a very similar interface to the current TTree-based frame ones. The tests are the same for both. How the writer and reader work is a bit different, of course, from the TTree-based. For example, RNTuple doesn't support mapped types right now so for the generic parameters there is a 'disentangling' from the map types to simple vector types that are saved in the rntuple and then reconstructed in the reader (from vector to map types). I tried to make it as simple as possible by not using extra types and trying to pull few headers from podio.

It's still not fully ready for merging as cleanups need to be done and more comments are needed in several places. Also the support for reading from multiple files is not complete right now. It is a bit more tricky than the TTree case since there is no equivalent to a TChain as far as I know.

BEGINRELEASENOTES

Add support for the new RNTuple format by adding a writer, reader and tests.

ENDRELEASENOTES

Supersedes #247

tmadlener · 2023-03-24T17:24:34Z

clang-tidy in pre-commit workflow is failing because the environment doesn't come with a new enough ROOT version. We will have to think about something there.

tmadlener

Very nice work, thanks for this. My questions/comments are either minor things, or some design questions. I guess the most important one is, whether it would make sense to introduce similar helper structs as for the TTree versions to keep category information together and remove a few of the maps that all have the same keys at the moment (as far as I understood).

Another one, would be the repetition around the GenericParameter reading/writing, where we should be able to play some template games to drastically reduce it (and additionally make it almost automatic if there should ever by a new type added to them).

include/podio/ROOTNTupleReader.h

tmadlener · 2023-03-24T17:28:13Z

include/podio/CollectionBuffers.h

@@ -25,6 +25,7 @@ using VectorMembersInfo = std::vector<std::pair<std::string, void*>>;
 */
 struct CollectionWriteBuffers {
  void* data{nullptr};
+  void* vecPtr{nullptr};


Not entirely happy with this one. But at the moment we seem to not have any other option(?).

Not yet, it's in my todo list

include/podio/ROOTNTupleReader.h

src/CMakeLists.txt

src/ROOTNTupleReader.cc

include/podio/ROOTNTupleWriter.h

src/ROOTNTupleReader.cc

src/rootUtils.h

jmcarcell · 2023-04-17T09:31:04Z

I have implemented almost all the comments. The simplification for writing and reading the generic parameters adds a couple of constexpr to rootUtils (to get the "branch" name for each parameter) and also makes one getMap function public instead of private

tmadlener

Thanks. Mainly a few questions for my understanding and minor variable/function name nitpicking for better readability.

Making the second getMap<T> public for the GenericParameters is not really an issue, since they have effectively been public before as well.

tmadlener · 2023-04-17T10:14:26Z

src/ROOTNTupleReader.cc

+  auto keys = keyView(entNum);
+  auto values = valueView(entNum);


Just for my understanding. These are really just views to the data, right? There is no way we can "steal" them without destroying things. I.e. we cannot simply read this things once and then move them into the GenericParameter maps?

Yep. Do you mean reading them at once for all entNum and then moving? I don't think that would change anything, values[i] here is already a vector and we are writing in a map<string, vector<T>> so we need to do it one by one anyway. But one thing that can be done is to cast to an rvalue so that values can be moved in the next line:

for (size_t i = 0; i < keys.size(); ++i) { params.getMap<T>()[keys[i]] = std::move(values[i]); }

Yeah, my thinking was that we could provide some functionality for GenericParameters that simply takes the keys vector<string>&& and the values vector<vector<T>>&& and then we move them into that function and inside that function we would move the individual elements into the internal map. But maybe something like:

params.getMap<T>().emplace(std::move(keys[i]), std::move(values[i]));

would be good enough. In this way we would also move the keys instead of copying them. Anyhow, all of this only works if we don't accidentally steal the things from the RNTuple view. I am not sure about that, maybe we have to make copies in any case.

I checked and the values are being moved inside the function because the vectors are left empty but if you call GetView again then you get the values again which means that at least one copy is being made (I assume it's not reading twice). So I'm actually not sure the moves will do anything. But I guess there is no downside to std::move. I guess we are only changing when the copies are made

Yeah, I suppose the GetView will have to do some reading and potentially uncompressing. Maybe it skips this if the thing it views is non-empty. In any case, since we call GetView only once per event, and we don't break things by move-ing, I agree that we keep std::move here and at least avoid unnecessary copies.

Ok this is done now moving both keys and values, I also removed the intermediate variables since they are not really used more times than that one

src/ROOTNTupleReader.cc

tmadlener · 2023-04-17T10:26:13Z

src/ROOTNTupleReader.cc

+  ROOTFrameData::BufferMap buffers;
+  auto dentry = m_readers[category][0]->GetModel()->GetDefaultEntry();
+
+  std::map<std::pair<std::string, int>, std::vector<podio::ObjectID>*> tmp;


If I understand correctly here: This map of vector<ObjectID>* is necessary, because we cannot directly "connect" the buffers.references[i].get() to the vector* that is expected in CaptureValueUnsafe?

If that is the case, could you put a small comment here stating why we need this map?

This is related to buffers.references[i] being a unique_ptr I think. I'll double check but I think passing either the buffers.references[i] or the buffers.references[i].get() both crash and that's why I had to choose that not very elegant solution since I don't know better 🤷

Hmm, pity. Would have been nice if the get() version would have worked. But if both crash, then this is the way it has to be for the moment.

Yeah, it even was commented out above. I also found another way of doing this that works which consisted on starting with making an unique_ptr:

refCollections->at(j) = std::make_unique<std::vector<podio::ObjectID>>();

and then reading (on a new vector so it's not deleted) and then making the unique_ptr point to it (relying on 0-sized unique_ptrs) but I think I settled for the current way because it's less confusing; it's probably there in the history.

src/ROOTNTupleWriter.cc

src/rootUtils.h

jmcarcell · 2023-04-19T12:54:19Z

Ok now this should be properly formatted

jmcarcell · 2023-05-15T06:08:31Z

Updated after #394

jmcarcell · 2023-05-30T08:20:08Z

Writer updated to use a bare model from the comments from Jakob in e7e7e7e. It isn't that elegant since not everything was being written through void*, although when support for std::map is there it will be simplified. I have tried to do a similar thing for the reader but it doesn't seem to be trivial since the tests end up failing later on

tmadlener

Thanks for looking into this. Does this mean we can drop one of the void* in the CollectionBuffers? Or do we have to keep them around for now? If there is no easy way around it, then I would probably be OK with merging this for now and properly documenting that the buffers are meant for internal use only and that they are subject to changes.

I once more also have a question about some RNTuple details that I missed before.

tmadlener · 2023-05-30T09:45:40Z

src/ROOTNTupleWriter.cc

+        const auto typeName = "vector<" + type + ">";
+        const auto brName = root_utils::vecBranch(name, i++);
+        auto ptr = *(std::vector<int>**)vec;


Suggested change

const auto typeName = "vector<" + type + ">";

const auto brName = root_utils::vecBranch(name, i++);

auto ptr = *(std::vector<int>**)vec;

const auto brName = root_utils::vecBranch(name, i++);

auto ptr = *(std::vector<int>**)vec;

The typeName seems to be unused, so I suppose this works because the necessary type information has been passed to the whole thing when we create the fields? And the vector<int> cast works for all types that potentially come along here and is simply necessary for getting to the correct void*?

Yeah I don't think there is a typename for the other members, must have forgotten to remove it when copying and pasting from the TTree reader. I should double check that it works for every type, but in principle it's the same three star cast used in other places:

podio/include/podio/CollectionBuffers.h

Line 41 in 930e600

return *static_cast<std::vector<T>**>(raw);

jmcarcell · 2023-06-14T20:25:44Z

No way of getting rid of the extra pointer that I know of, the last changes were to change how the RNTuple model was created, now it is a bare one which doesn't do instantiation (before it was a default one which does). This is unrellated to the void* pointers.

hegner · 2023-06-23T07:23:16Z

@jmcarcell - can you update your PR ? would like to see whether we can merge today

tmadlener · 2023-06-27T09:02:08Z

clang-tidy seems to break, because the root version in that LCG release is not yet recent enough: https://github.com/AIDASoft/podio/actions/runs/5386875055/jobs/9777453794?pr=395#step:4:416

I think now that we have LLVM in the key4hep stack we could also switch to that for the CI. However, we should do that in a separate PR, after we have merged this.

There are also some clang-format complaints. I suppose these are due to different versions running on your machine and the CI(?). Can you fix those? That would make it easier to have one global commit for when we switch to a newer version of clang-format.

jmcarcell · 2023-06-28T19:46:25Z

Now clang-format is happy

hegner · 2023-07-05T07:34:15Z

@jmcarcell - can you spell out the branch name issues we discussed yesterday? Thanks!

jmcarcell · 2023-07-10T20:31:11Z

Updated now to conform to #405, which was the last thing pending, at least that I remember. The reader needs some cleanup of these changes but I think that will have to wait for tomorrow 😪
Some test results on my PC since I have them handy:

    Start 15: write_rntuple
1/2 Test #15: write_rntuple ....................   Passed    0.17 sec
    Start 16: read_rntuple
2/2 Test #16: read_rntuple .....................   Passed    0.63 sec

writing and reading the same frame:

    Start 12: write_frame_root
1/2 Test #12: write_frame_root .................   Passed    1.25 sec
    Start 11: read_frame_root
2/2 Test #11: read_frame_root ..................   Passed    1.26 sec

hegner · 2023-07-11T07:05:46Z

@jmcarcell - thanks a lot! Do you think that's it for now? If yes, I will redo the review

jmcarcell · 2023-07-11T07:25:47Z

Yes I don't think I have anything else (other than the void* and the three * workarounds that are discussed above, but I think that's maybe better in another PR, certainly I don't have the fixes right now)

hegner · 2023-07-11T10:18:35Z

@jmcarcell - thanks a lot. I think it is fine as it is now. We will do the cleanups in another PR then :-)

* Add a RNTuple writer * Cleanup and add a reader * Add compilation instructions for RNTuple * Add tests * Fix the reader and writer so that they pass most of the tests * Commit missing changes in the header * Add support for Generic Parameters * Add an ugly workaround to the unique_ptr issue * Read also vector members and remove some comments * Do a bit of cleanup * Do more cleanup, also compiler warnings * Add names in rootUtils.h, fix a few compiler warnings * Add a few minor changes * Add missing changes in the headers * Change map -> unordered_map and use append in CMakeLists.txt * Simplify writing and reading of generic parameters * Only create the ID table once * Add CollectionInfo structs * Add a ROOT version check * Add missing endif() * Add Name at the end of some names * Add missing Name at the end * Cast to rvalue * Cache entries and reserve * Add comment and remove old comments * Remove a few couts * Remove intermediate variables and use std::move * Run clang-format * Use clang-format on tests too * Enable RNTuple I/O in Key4hep CI * Check if dev3 workflows come with recent enough ROOT * Change MakeField to the new signature * Update the RNTuple reader and writer to use the buffer factory * Run clang-format * Update the RNTuple writer to use a bare model * Add friends for Generic Parameters * Update changes after the changes in the collectionID and string_view * Run clang-format * Update the reader and writer to conform to AIDASoft#405 * Reorganize and clean up code in the reader * Run clang-format * Simplify how the references are filled --------- Co-authored-by: jmcarcell <[email protected]> Co-authored-by: tmadlener <[email protected]>

tmadlener reviewed Mar 24, 2023

View reviewed changes

tmadlener mentioned this pull request Mar 24, 2023

[WIP] Add ROOTNTuple{Reader|Writer} I/O Backends #247

Closed

4 tasks

tmadlener reviewed Apr 17, 2023

View reviewed changes

jmcarcell force-pushed the new-rntuple branch from e6b98f2 to 8947a79 Compare May 15, 2023 06:07

jmcarcell force-pushed the new-rntuple branch from 8947a79 to e7e7e7e Compare May 30, 2023 08:17

jmcarcell force-pushed the new-rntuple branch from e7e7e7e to cca2e85 Compare May 30, 2023 09:33

tmadlener reviewed May 30, 2023

View reviewed changes

jmcarcell added 16 commits June 27, 2023 08:35

Add a RNTuple writer

7f2826f

Cleanup and add a reader

78d1162

Add compilation instructions for RNTuple

69d50c8

Add tests

f4cc923

Fix the reader and writer so that they pass most of the tests

4dbbf7f

Commit missing changes in the header

49167c9

Add support for Generic Parameters

4bc7ed7

Add an ugly workaround to the unique_ptr issue

9fc8da3

Read also vector members and remove some comments

529dffc

Do a bit of cleanup

8dcdf5a

Do more cleanup, also compiler warnings

27e7b87

Add names in rootUtils.h, fix a few compiler warnings

5ea4a97

Add a few minor changes

bdf4335

Add missing changes in the headers

f119026

Change map -> unordered_map and use append in CMakeLists.txt

2aa55ef

Simplify writing and reading of generic parameters

5edd00e

jmcarcell and others added 10 commits June 27, 2023 08:44

Run clang-format

7379ade

Use clang-format on tests too

027925b

Enable RNTuple I/O in Key4hep CI

2629d66

Check if dev3 workflows come with recent enough ROOT

f8c58d1

Change MakeField to the new signature

a4ecb55

Update the RNTuple reader and writer to use the buffer factory

beaf236

Run clang-format

ab5cfb2

Update the RNTuple writer to use a bare model

1cd727e

Add friends for Generic Parameters

f34ebc7

Update changes after the changes in the collectionID and string_view

659e051

jmcarcell force-pushed the new-rntuple branch from cca2e85 to 659e051 Compare June 27, 2023 07:13

Run clang-format

a471949

Update the reader and writer to conform to AIDASoft#405

178e6a1

jmcarcell added 3 commits July 11, 2023 08:38

Reorganize and clean up code in the reader

5be7223

Run clang-format

ec5bcf3

Simplify how the references are filled

b0f654c

hegner self-requested a review July 11, 2023 10:17

hegner approved these changes Jul 11, 2023

View reviewed changes

hegner merged commit 1e8a8f9 into AIDASoft:master Jul 11, 2023

tmadlener mentioned this pull request Jul 11, 2023

Cleanup double void* in CollectionBuffers #444

Open

jmcarcell mentioned this pull request Jul 11, 2023

podio: Add a variant for rntuple spack/spack#38851

Closed

tmadlener mentioned this pull request Jul 13, 2023

Fix the pre-commit workflow #449

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for the new RNTuple format #395

Add support for the new RNTuple format #395

jmcarcell commented Mar 22, 2023 •

edited

Loading

tmadlener commented Mar 24, 2023

tmadlener left a comment

tmadlener Mar 24, 2023

jmcarcell Apr 11, 2023

jmcarcell commented Apr 17, 2023

tmadlener left a comment

tmadlener Apr 17, 2023

jmcarcell Apr 17, 2023 •

edited

Loading

tmadlener Apr 17, 2023

jmcarcell Apr 17, 2023 •

edited

Loading

tmadlener Apr 17, 2023

jmcarcell Apr 18, 2023

tmadlener Apr 17, 2023

jmcarcell Apr 17, 2023 •

edited

Loading

tmadlener Apr 17, 2023

jmcarcell Apr 17, 2023

jmcarcell commented Apr 19, 2023

jmcarcell commented May 15, 2023

jmcarcell commented May 30, 2023 •

edited

Loading

tmadlener left a comment

tmadlener May 30, 2023

jmcarcell Jun 14, 2023

jmcarcell commented Jun 14, 2023

hegner commented Jun 23, 2023

tmadlener commented Jun 27, 2023

jmcarcell commented Jun 28, 2023

hegner commented Jul 5, 2023

jmcarcell commented Jul 10, 2023

hegner commented Jul 11, 2023

jmcarcell commented Jul 11, 2023

hegner commented Jul 11, 2023

		auto keys = keyView(entNum);
		auto values = valueView(entNum);

Add support for the new RNTuple format #395

Add support for the new RNTuple format #395

Conversation

jmcarcell commented Mar 22, 2023 • edited Loading

tmadlener commented Mar 24, 2023

tmadlener left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmcarcell commented Apr 17, 2023

tmadlener left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmcarcell Apr 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmcarcell Apr 17, 2023 • 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

jmcarcell Apr 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmcarcell commented Apr 19, 2023

jmcarcell commented May 15, 2023

jmcarcell commented May 30, 2023 • edited Loading

tmadlener left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmcarcell commented Jun 14, 2023

hegner commented Jun 23, 2023

tmadlener commented Jun 27, 2023

jmcarcell commented Jun 28, 2023

hegner commented Jul 5, 2023

jmcarcell commented Jul 10, 2023

hegner commented Jul 11, 2023

jmcarcell commented Jul 11, 2023

hegner commented Jul 11, 2023

jmcarcell commented Mar 22, 2023 •

edited

Loading

jmcarcell Apr 17, 2023 •

edited

Loading

jmcarcell Apr 17, 2023 •

edited

Loading

jmcarcell Apr 17, 2023 •

edited

Loading

jmcarcell commented May 30, 2023 •

edited

Loading