Moving RDataSource closer to Podio/EDM4hep #593

kjvbrt · 2024-05-03T11:38:12Z

BEGINRELEASENOTES

Adding ROOT RDataSource

ENDRELEASENOTES

include/podio/ROOTDataSource.h

tmadlener

Thanks @kjvbrt. I wasn't aware this was possible in this generality. Very nice. I am probably still missing quite a few things, but I tried to summarize my current understanding below and added and a few comments / questions inline

For my general understanding, what this does is basically:

Create a reader for each thread / slot
Keep a vector of Frames, again one per thread / slot
Populate a vector of collections per thread / slot, where
- each collection always is at the same position, so that we can simply index into it

I am wondering whether we could do with just one reader instead of multiple, but I suppose then we would probably have to do our own locking around that, because the RDataSource doesn't do that for us?

If I understand correctly, we could in the end even replace the current ROOTReader with the Reader that is introduced in #522 and effectively support all backends + RDataFrame?

include/podio/ROOTDataSource.h

src/ROOTDataSource.cc

tmadlener · 2024-05-03T12:18:49Z

include/podio/ROOTDataSource.h

+      template<typename T>
+      std::vector<T**> GetColumnReaders(std::string_view columnName);


If I understand the documentation of RDataSource correctly, we don't need this at all. Also this is not overriding anything from the RDataSource base class. I think all of the "magic" is happening in the GetColumnReadersImpl, right?

tmadlener · 2024-05-03T12:26:30Z

src/ROOTDataSource.cc

+      m_frames.emplace_back(
+          std::make_unique<podio::Frame>(
+              podio::Frame(m_podioReaders[i]->readEntry("events", 0))));
+    }


Aren't we reading the same event nSlots times this way? As far as I can see we read the correct entry in any case later in SetEntry. Can we not just simply put an empty Frame here? Or even just populate the vector with a bunch of nullptr?

tmadlener · 2024-05-06T12:59:37Z

The sanitizer workflows are most likely failing because the tests that produce the input files are not running. You can disable them by adding the new tests also to the list in CTestCustom.cmake

Zehvogel

to avoid the hardcoded strings :)

src/ROOTDataSource.cc

Zehvogel · 2024-05-07T13:54:42Z

src/ROOTDataSource.cc

+  // Get collections stored in the files
+  std::vector<std::string> collNames = frame.getAvailableCollections();
+  // std::cout << "podio::ROOTDataSource: Found following collections:\n";
+  for (auto& collName : collNames) {
+    const podio::CollectionBase* coll = frame.get(collName);
+    if (coll->isValid()) {
+      m_columnNames.emplace_back(collName);
+      m_columnTypes.emplace_back(coll->getValueTypeName());
+      // std::cout << "                - " << collName << "\n";
+    }
+  }


Can we also make the parameters of the frame available as columns, e.g. to access the cross section? :)

tmadlener

A few new minor suggestions for using the reader interface, now that it's available. I think I got all the places, but I might have missed some.

include/podio/ROOTDataSource.h

src/ROOTDataSource.cc

include/podio/ROOTDataSource.h

src/ROOTDataSource.cc

tmadlener · 2024-07-08T17:58:57Z

src/CMakeLists.txt

+target_link_libraries(podioRootIO PUBLIC podio::podio
+                                         ROOT::Core
+                                         ROOT::RIO
+                                         ROOT::Tree
+                                         ROOT::ROOTVecOps
+                                         ROOT::ROOTDataFrame)


I am contemplating whether we should move the ROOTDataSource into a separate library instead of making it part of the podioRootIO one. The main argument against it is that we will have one more library. However, if we really make use of the Reader interface internally, then we would need to link podioRootIO against podioIO (making a cyclic lib dependency) and potentially also against podioSioIO. Then we would have no separation of the libraries at all any longer.

Since it is probably easier to merge libraries down the line, than splitting them apart, I would suggest to leave the podioRootIO library untouched and instead make a new podioRDataSource library that links against podio::podioIO (and all the necessary things from ROOT).

Yes, I had to link against podio::podioIO in couple of places. Will create separate library...

kjvbrt · 2024-07-25T08:04:25Z

I continue with testing the changes made here in FCCAnalyses

tmadlener

There seems to be an issue with the installation now, because the EDM4hep tests are failing. As far as I can tell it has to do with header files that are no longer installed. Otherwise it's mainly minor comments.

tmadlener · 2024-07-24T13:03:52Z

CMakeLists.txt

+# Export compile commands --- used by the tools from clang family
+set(CMAKE_EXPORT_COMPILE_COMMANDS ON)
+


I have this in an alias for cmake. I don't think we want to change a default here and rather let the users decide whether they want this or not.

tmadlener · 2024-07-24T13:05:47Z

include/podio/ROOTDataSource.h

+  std::vector<void*> GetColumnReadersImpl(std::string_view name, const std::type_info& typeInfo) override;
+
+  std::string AsString() override {
+    return "Podio data source";


Are spaces a problem here? Or the fact that this is not the same as the class name?

tmadlener · 2024-07-25T14:31:26Z

src/CMakeLists.txt

 endif()
+if (NOT ENABLE_RNTUPLE)


Suggested change

if (NOT ENABLE_RNTUPLE)

if (NOT ENABLE_DATASOURCE)

At least that would make sense for the files that get filtered.

tmadlener · 2024-07-25T14:34:33Z

src/CMakeLists.txt

+file(GLOB headers_necessary
+     RELATIVE ${PROJECT_SOURCE_DIR}
+     "${PROJECT_SOURCE_DIR}/install/podio/*.h")


Something is fishy with this construct here or the filtering below, because no headers are installed now: https://github.com/AIDASoft/podio/actions/runs/10090563011/job/27900250138?pr=593#step:6:600

tmadlener · 2024-07-25T14:35:34Z

src/DataSource.cc

+  unsigned int nEventsInFiles = 0;
+  auto podioReader = podio::makeReader(m_filePathList);
+  nEventsInFiles = podioReader.getEntries(podio::Category::Event);
+  frame = podio::Frame(podioReader.readFrame(podio::Category::Event, 0));


Suggested change

frame = podio::Frame(podioReader.readFrame(podio::Category::Event, 0));

frame = podioReader.readFrame(podio::Category::Event, 0);

The Reader interface already gives us a fully usable Frame, we don't need to construct it any longer.

andresailer · 2024-07-30T07:39:32Z

src/DataSource.cc

+  }
+
+  for (size_t i = 0; i < m_nSlots; ++i) {
+    m_frames.emplace_back(std::make_unique<podio::Frame>(podio::Frame{}));


Suggested change

m_frames.emplace_back(std::make_unique<podio::Frame>(podio::Frame{}));

m_frames.emplace_back(std::make_unique<podio::Frame>());

make_unique forwards to c'tor, so there is no need to first create a frame and then make make_unique call the copy c'tor ?

andresailer · 2024-07-30T07:48:55Z

include/podio/DataSource.h

+};
+
+///
+/// Not used.


If this is not used, can you explain why it is needed to be here?

andresailer · 2024-07-30T07:52:52Z

include/podio/DataSource.h

+  std::vector<std::string> m_filePathList = {};
+
+  /// Total number of events
+  unsigned int m_nEvents = 0;


Suggested change

unsigned int m_nEvents = 0;

ULong64_t m_nEvents = 0;

Shouldn't this match with the ranges defined in the next lines?

tmadlener reviewed May 3, 2024

View reviewed changes

include/podio/ROOTDataSource.h Outdated Show resolved Hide resolved

tmadlener reviewed May 3, 2024

View reviewed changes

Zehvogel suggested changes May 7, 2024

View reviewed changes

src/ROOTDataSource.cc Outdated Show resolved Hide resolved

src/ROOTDataSource.cc Outdated Show resolved Hide resolved

src/ROOTDataSource.cc Outdated Show resolved Hide resolved

Zehvogel reviewed May 7, 2024

View reviewed changes

tmadlener mentioned this pull request Jun 4, 2024

GenericParameters should be stored the same way for TTree and RNTuple backends #590

Closed

kjvbrt force-pushed the r-data-source branch from e8b18e2 to bd129e8 Compare June 24, 2024 06:45

tmadlener reviewed Jun 24, 2024

View reviewed changes

include/podio/ROOTDataSource.h Outdated Show resolved Hide resolved

src/ROOTDataSource.cc Outdated Show resolved Hide resolved

src/ROOTDataSource.cc Outdated Show resolved Hide resolved

kjvbrt force-pushed the r-data-source branch from bd129e8 to c20eb40 Compare July 8, 2024 09:26

tmadlener reviewed Jul 8, 2024

View reviewed changes

kjvbrt added 7 commits July 23, 2024 15:39

Moving RDataSource closer to Podio/EDM4hep

1436966

Using /// and @ for the doxygen docs

e404002

Moving doc strings to header file

1d26bbf

Clang format

09f12c5

Adding podio::ROOTDataSource class to the rootmap

90710ac

Minor adjustments

113ba68

Another set of small adjustments

29f88a0

kjvbrt force-pushed the r-data-source branch from c20eb40 to 29f88a0 Compare July 24, 2024 07:58

kjvbrt added 2 commits July 25, 2024 09:30

Separating datasource into standalone library

3dc015b

Adding ON flag to all tests

c33159e

Formatting

e95d800

tmadlener reviewed Jul 25, 2024

View reviewed changes

andresailer reviewed Jul 30, 2024

View reviewed changes

include/podio/DataSource.h

};

///

/// Not used.

Copy link

Member

andresailer Jul 30, 2024

Choose a reason for hiding this comment

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

If this is not used, can you explain why it is needed to be here?

andresailer reviewed Jul 30, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Moving RDataSource closer to Podio/EDM4hep #593

Moving RDataSource closer to Podio/EDM4hep #593

kjvbrt commented May 3, 2024 •

edited

Loading

tmadlener left a comment

tmadlener May 3, 2024

tmadlener May 3, 2024

tmadlener commented May 6, 2024

Zehvogel left a comment

Zehvogel May 7, 2024

tmadlener left a comment

tmadlener Jul 8, 2024

kjvbrt Jul 24, 2024

kjvbrt commented Jul 25, 2024

tmadlener left a comment

tmadlener Jul 24, 2024

tmadlener Jul 24, 2024

tmadlener Jul 25, 2024

tmadlener Jul 25, 2024

tmadlener Jul 25, 2024

andresailer Jul 30, 2024

andresailer Jul 30, 2024

andresailer Jul 30, 2024

		template<typename T>
		std::vector<T**> GetColumnReaders(std::string_view columnName);

		# Export compile commands --- used by the tools from clang family
		set(CMAKE_EXPORT_COMPILE_COMMANDS ON)

	frame = podio::Frame(podioReader.readFrame(podio::Category::Event, 0));
	frame = podioReader.readFrame(podio::Category::Event, 0);

	m_frames.emplace_back(std::make_unique<podio::Frame>(podio::Frame{}));
	m_frames.emplace_back(std::make_unique<podio::Frame>());

Moving RDataSource closer to Podio/EDM4hep #593

Are you sure you want to change the base?

Moving RDataSource closer to Podio/EDM4hep #593

Conversation

kjvbrt commented May 3, 2024 • 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

tmadlener commented May 6, 2024

Zehvogel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

kjvbrt commented Jul 25, 2024

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kjvbrt commented May 3, 2024 •

edited

Loading