New sio backend #130

tmadlener · 2020-09-01T13:27:13Z

BEGINRELEASENOTES

Add SIO as a second I/O backend (as alternative to ROOT) that can be enabled with ENABLE_SIO. If enabled, a separate podioSioIO library is built that allows reading and writing sio files. For serializing the different datatypes, additional code is generated to build an SioBlocks library that is loaded at runtime (if found somewhere on LD_LIBRARY_PATH). To facilitate the whole process at the cmake level, new cmake functions are provided to generate the core datamodel library PODIO_ADD_DATAMODEL_CORE_LIBRARY, to (conditionally) define the ROOT dictionary target PODIO_ADD_ROOT_IO_DICT and to (conditionally) define the Sio Blocks library target PODIO_ADD_SIO_IO_BLOCKS. The I/O backends that are supported by podio are exported via the PODIO_IO_HANDLERS list variable.
podio_generate_datamodel.py now additionally takes the I/O handlers that should be generated as arguments. This is also reflected in an additional argument to PODIO_GENERATE_DATAMODEL. To have backwards compatibility, this additional argument defaults to ROOT in both cases and downstream packages should work as usual without changes.

ENDRELEASENOTES

This is more or less a copy/paste exercise from a previous attempt from Frank with the main changes in the templates and the c++ code generator. At the moment this should be considered as a prototype version and there are still some things to do:

gaede · 2020-09-02T07:05:08Z

@tmadlener: you had commented out reading of the meta data in the first record, which is uncompressed (for whatever reason) and then reading the first event in compressed mode failed...

tmadlener · 2020-09-08T17:08:40Z

I have added the handling of the event meta data in a first version and also slightly reworked the handling of the collection IDs and the data that is necessary to reconstruct all collections afterwards. I have temporarily disabled the tests that check the collection meta data, as that still needs to be done.

Currently the changes in #133 and #134 are also present here

tmadlener · 2020-09-09T13:44:26Z

From a functionality point of view I think everything that is covered by the ROOTReader and 'ROOTWriter' is now also covered by the SIOReader and SIOWriter, with the exception of reading multiple files.

There are, however, a few things that could be improved in the current implementation:

The GenericParameters now have public methods to access their internal maps, solely for the purpose of easier I/O using sio. I don't particularly like this.
The SIOBlockFactory with its static instance, is not really ideal for multi threading. From the looks of it, it could potentially be replaced by a constant map that could be generated from the yaml file, but I am not entirely sure about that.
The SIOReader currently owns the collections, which makes the EventStore behave slightly differently, as compared with when it is used with the ROOTReader (i.e. EventStore::clear will destroy resources that are not owned by it). Ideally the two readers would be drop-in replacable.

For these reasons I am currently keeping this as [WIP], but any intermediate comments are very welcome.

tmadlener · 2020-09-14T16:18:12Z

I am currently not entirely happy with the splitting of the different libraries, especially for the tests, where I have tried to have a "core" library, as well as two separate libraries that are only concerned with either root or sio based I/O. For root I have basically followed the same path as in edm4hep and this seems to work nicely out of the box. However, for sio I am currently struggling with getting the self registering factory approach to work nicely. The problem is that all the SIOBlocks that are necessary for this are placed in a separate library and so the SIOBlockFactory instance in the tests will not know about the one that is created and filled in this library. Including one SIOBlock header in the tests fixes this for now, but this is not really a solution in my opinion.

For the tests here, it doesn't really matter, but for actual usage it might be nice to separate the core libraries from the one that deal with I/O. Is there an easy way to do this?

CMakeLists.txt

python/podio_class_generator.py

andresailer · 2020-09-15T11:59:04Z

python/podio_class_generator.py

@@ -205,6 +206,9 @@ def _process_datatype(self, name, definition):
    self._fill_templates('Obj', datatype)
    self._fill_templates('Collection', datatype)

+    if self.reader.options["createSIOHandlers"]:


Suggested change

if self.reader.options["createSIOHandlers"]:

if 'SIO' in self.reader.options["IOHandlers"]:

Or something like this, and making IOHandlers a list of IOHandlers to create (ROOT and SIO for the moment)

That is a good idea. It would definitely scale better than the current approach in the yaml file of the datamodel. I will make this change, and since ROOT doesn't need any special treatment at the moment it will basically be simply ignored. Do you think that is a reasonable approach, or should we do something more explicit for IO systems that do not need additional generated code? E.g. only list the handlers that actually need additional code generation?

After reading more of the PR.

Maybe this should be coupled to the CMake option for enabling SIO, in the base CMakeLists in the block

if(SIO_HANDLERS) LIST(APPEND PODIO_IO_HANDLERS SIO) endif()

The PODIO_GENERATE_DATAMODEL function could just set environment variables for each handler,

FOREACH(HANDLER PODIO_IO_HANDLERS) SET(EXPORT_HANDLER "${EXPORT_HANDLER} ${HANDLER}_HANDLER=1") ENDFOREACH()

This

podio/cmake/podioMacros.cmake

Line 141 in d4a0265

COMMAND python ${podio_PYTHON_DIR}/podio_class_generator.py --quiet ${YAML_FILE} ${ARG_OUTPUT_FOLDER} ${datamodel}

becomes

COMMAND ${EXPORT_HANDLER} python ${podio_PYTHON_DIR}/podio_class_generator.py --quiet ${YAML_FILE} ${ARG_OUTPUT_FOLDER} ${datamodel}

and then the check becomes

if os.environ.get('SIO_HANDLER'):

The variable PODIO_IO_HANDLERS is exported to the podioConfig, so edm4hep can pick this up as well when calling PODIO_GENERATE_DATAMODEL

Which reminds me that the dependency on SIO should also be exported in the podioConfig, if enabled.

ROOT doesn't need any special treatment

Actually, I just realized that this is not true, because the selection.xml file is only necessary for the ROOT dictionaries.

Regarding the handling of the generation of different IO systems, I have started some kind of discussion in that direction in #131. Maybe instead of setting environment variables, we could pass command line arguments to the generator from cmake? Or would that defeat the purpose of exporting PODIO_IO_HANDLERS? I, personally, find explicitly passing command line arguments a bit easier to debug and to follow the flow of the whole thing.

For exporting the dependency on SIO, it would be ideal if SIO already exports targets, right?

python/templates/SIOBlock.cc.jinja2

src/CMakeLists.txt

tests/CMakeLists.txt

src/SIOReader.cc

tmadlener · 2020-09-16T09:49:55Z

With the latest changes I have now implemented the following:

A very basic runtime library loader in form of the SIOBlockLibraryLoader. It is designed to be used as a singleton that tries to load the necessary libraries during construction. It is then simply instantiated in the constructors of the SIOReader and SIOWriter. To load libraries all directories on LD_LIBRARY_PATH are checked for files containing "SioBlocks" in their name, which are then loaded. Since this also implies a naming convention, this does the job for the moment, but maybe this should be made more stable in the future.
SIOReader now follows the same interface as ROOTReader and should now be in principle usable as a drop-in replacement. However, some differences still remain due to the different philosophies of SIO and ROOT, e.g. ROOT offers a getEntries method, while SIO signals the end of the file with an exception and a corresponding error code.
Replaced the createSIOHandlers option with an IOHandlers one for configuring which IO handler code should be generated. I would put a potential switch to command line arguments (see discussion above and in Discussion: How to best deal with the different IO backends #131) in a different PR to not pollute this one too much.
Addressed Andres cmake comments

hegner · 2020-09-16T10:15:50Z

CMakeLists.txt

@@ -68,6 +68,7 @@ endif()

 #--- Declare options -----------------------------------------------------------
 option(CREATE_DOC "Whether or not to create doxygen doc target." OFF)
+option(SIO_HANDLERS "Build the SIO block handlers and readers and writers." OFF)


naming a bit too technical. Why not "SIO_SUPPORT" or "ENABLE_SIO" ? One can use equivalent naming for every further backend.

I have changed it to ENABLE_SIO. I realized that this has to be done all over the place, so we should come up with a more "compact" way of adding backends.

yap. that was my comment in the meeting already. thanks!

I think we can broaden the discussion in #131 to cover the whole topic of how to best deal with different I/O backends.

hegner · 2020-09-16T10:25:54Z

python/podio_config_reader.py

@@ -321,6 +321,8 @@ def __init__(self, yamlfile):
        "exposePODMembers": True,
        # use subfolder when including package header files
        "includeSubfolder": False,
+        # generate the SIO block handlers


certainly comment and code are out of sync ;-)

Ah yes, most definitely... Thanks for checking

andresailer · 2020-09-16T11:02:27Z

tests/CMakeLists.txt

-file(GLOB sources_utils utilities/*.cc)
-file(GLOB headers_utils utilities/*.h)
+if (ENABLE_SIO)
+  # Filter out the sio block handlers to avoid polluting the TestDataModel lib


The same issue will appear in edm4hep.
Can we find a way to make this automated for users of PODIO_GENERATE_DATAMODEL?

Yes, we definitely should. I would suggest to discuss this in #131.

tests/CMakeLists.txt

hegner · 2020-09-16T11:55:23Z

tests/read_test.h

+    throw std::runtime_error("Couldn't read event meta data parameters 'UserEventWeight'");
+  }
+  std::stringstream ss ; ss << " event_number_" << eventNum ;
+  std::string evtName = evtMD.getStringVal( "UserEventName" ) ;


If you would have used const auto, you would have gotten const std::string& instead of a copy.

Ah yes, this is true. But these changes are currently only here because this PR starts on top of #133. I will fix it there. There are probably also a few other instances, where the usage of auto would make things a bit nicer in the tests. I haven't really checked, as I have basically just moved the already available code into a common header.

gaede · 2020-09-16T14:38:03Z

@tmadlener nice work. I have pushed an option to use the boost_filesystem library in SioIO as filesystem is not yet available on all Macs (at least on my mac running Mojave).

andresailer · 2020-09-16T14:48:44Z

@gaede Have you tried linking against c++fs with llvm, instead of bringing in boost?
See:
https://github.com/AIDASoft/DD4hep/blob/a2f3c03d74f27e24fa8b4a1b8b7478ae654d8a35/cmake/DD4hepBuild.cmake#L721
Or is dd4hep falling back on boost on your machine?

gaede · 2020-09-16T14:54:28Z

I have neither c++fs nor <experimental/filesystem> on my machine, so boost is the only reasonable option...

andresailer · 2020-09-16T15:05:56Z

I think we need something like https://github.com/AIDASoft/DD4hep/blob/a2f3c03d74f27e24fa8b4a1b8b7478ae654d8a35/cmake/DD4hepBuild.cmake#L721-L751 to support compilers older than gcc9.1

tmadlener · 2020-09-23T09:36:06Z

Hi @andresailer,
can you check whether I got the essential parts right for detecting a suitable filesystem library in 74a9486 ?

@gaede, I added a simple TOC in c624808 as we have discussed. I am not yet entirely sure whether placing a marker outside of sio blocks at the end of the file really plays nicely with everything, but the tests don't uncover anything unexpected at the moment.

andresailer

Comments for the filesystem checks

cmake/podioMacros.cmake

tmadlener · 2020-09-25T09:19:43Z

I have added some cmake functions to facilitate the downstream usage of the different I/O backends. Additionally I have added podio_IO_HANDLERS to the exported variables. The cmake functions do not provide a complete automatization of the downstream process, but should facilitate the usage. Example usage can be seen in the tests directory. I have briefly checked with edm4hep to see if things actually work downstream. With the usage of these new macros the edm4hep/CMakeLists.txt could look something like this:

# For now unconditionally generate all the code that is supported by the intalled podio
PODIO_GENERATE_DATAMODEL(edm4hep ../edm4hep.yaml headers sources IO_BACKEND_HANDLERS ${podio_IO_HANDLERS})

PODIO_ADD_DATAMODEL_CORE_LIB(edm4hep "${headers}" "${sources}")

PODIO_ADD_ROOT_IO_DICT(edm4hepDict edm4hep "${headers}" src/selection.xml)
IF(TARGET edm4hepDict)
  add_library(edm4hep::edm4hepDict ALIAS edm4hepDict )
ENDIF()

PODIO_ADD_SIO_IO_BLOCKS(edm4hep "${headers}" "${sources}")
IF(TARGET edm4hepSioBlocks)
  message(STATUS "SioBlocks are here!")
ENDIF()

# 
# The rest: Installing targets, etc..
#

Depending on whether podio actually provides capabilities to do I/O via SIO edm4hep would also build the corresponding library. It is still necessary to check whether it is actually available, but the boilerplate code to conditionally add it is hidden in the cmake function.

It is also still possible to pass different IO_BACKEND_HANDLERS to PODIO_GENERATE_DATAMODEL and the functions still work, because they only check for generated files and act depending on whether it is present or not.

tmadlener · 2020-09-25T09:58:38Z

I have updated the release notes and I think this PR is now ready for (another) review. I am keeping at as [WIP] at the moment, because it still contains the changes from #133, and I think it would be best if that is merged first. Additionally, also #141 could be merged first to see if everything still works with the udpated CI.

After that it might be necessary to rebase. In the course of that also the final review comments can be addressed and then it should be possible to merge this.

Make podio look for a compatible filesystem library itself during the cmake stage, eventually falling back to boost in case the compiler doesn't support it natively.

Marking the getEntries and endOfEvent methods virtual and adding them to the IReader interface allows to re-use the same code for the read tests independent of the reader that actually provides the data. NOTE: The current implementation of SIOReader::getEntries is just here for feature completeness, and should be replaced with a more performant and const-correct version.

VERBOSE and DEBUG become available as modes to message only in cmake 3.15.

Provide cmake functions to: - Add the core datamodel library without any I/O backend dependencies. - Conditionally add a ROOT dict target (if the corresponding code has been generated) - Conditionally add a SioBlocks library target (if the corresponding code has been generated). Example usage in tests/CMakeLists.txt The usage still requires downstream users to check whether the target has been generated if they absolutely require it, but at least a lot of the boilerplate that comes with building (or trying to do so) is now hidden in these functions.

Co-authored-by: Andre Sailer <andre.philippe.sailer@cern.ch>

tmadlener · 2020-10-07T07:13:20Z

Removed [WIP] and rebased on top of master. Barring any further review comments, this is ready to be merged from my side.

hegner · 2020-10-07T07:25:14Z

CMakeLists.txt

@@ -68,6 +68,8 @@ endif()

 #--- Declare options -----------------------------------------------------------
 option(CREATE_DOC "Whether or not to create doxygen doc target." OFF)
+option(ENABLE_SIO "Build the SIO block handlers and readers and writers." OFF)


Less technical: "Build SIO read/write backend"

or like said further below "build SIO I/O support"

hegner · 2020-10-07T07:32:25Z

Thanks @tmadlener !

I will review it during the day. It is quite a long one.

gaede

Looks very good to me.

cmake/podioConfig.cmake.in

gaede · 2020-10-14T07:54:01Z

Thanks @tmadlener !

I will review it during the day. It is quite a long one.

Hi @hegner, did you find the time to review/approve this PR ? We'd like to merge it soon...

tmadlener · 2020-10-14T11:01:50Z

Closing and opening again, to retrigger CI.

tmadlener · 2020-10-19T13:18:03Z

close + reopen to trigger CI

tmadlener mentioned this pull request Sep 2, 2020

Discussion: How to best deal with the different IO backends #131

Closed

tmadlener force-pushed the new-sio-backend branch from 029df0a to 4145b42 Compare September 8, 2020 16:11

andresailer reviewed Sep 15, 2020

View reviewed changes

CMakeLists.txt Outdated Show resolved Hide resolved

andresailer reviewed Sep 15, 2020

View reviewed changes

python/podio_class_generator.py Outdated Show resolved Hide resolved

andresailer reviewed Sep 15, 2020

View reviewed changes

src/SIOReader.cc Show resolved Hide resolved

hegner reviewed Sep 16, 2020

View reviewed changes

tmadlener force-pushed the new-sio-backend branch from 5c0b5bd to a8a23ea Compare September 16, 2020 10:46

andresailer reviewed Sep 16, 2020

View reviewed changes

tests/CMakeLists.txt Outdated Show resolved Hide resolved

hegner reviewed Sep 16, 2020

View reviewed changes

tmadlener mentioned this pull request Sep 17, 2020

reading and writing events leaks memory #139

Closed

tmadlener force-pushed the new-sio-backend branch 2 times, most recently from 36cbd12 to 2313ed9 Compare September 17, 2020 14:28

andresailer reviewed Sep 23, 2020

View reviewed changes

cmake/podioMacros.cmake Outdated Show resolved Hide resolved

cmake/podioMacros.cmake Outdated Show resolved Hide resolved

cmake/podioMacros.cmake Outdated Show resolved Hide resolved

cmake/podioMacros.cmake Outdated Show resolved Hide resolved

tmadlener mentioned this pull request Sep 23, 2020

Fix python bindings for root 6.22 and update CI #141

Merged

tmadlener and others added 7 commits October 6, 2020 22:49

Copy functionality to check for fs libraries from dd4hep

a126485

Make podio look for a compatible filesystem library itself during the cmake stage, eventually falling back to boost in case the compiler doesn't support it natively.

Implement a simple TOC Record type for holding position information

d70602c

Make fs check compatible with older cmake versions

dcd9462

VERBOSE and DEBUG become available as modes to message only in cmake 3.15.

Use correct function name also in docstring

9c4a059

Co-authored-by: Andre Sailer <andre.philippe.sailer@cern.ch>

Fix Wignored-qualifiers warning by putting const at the correct place

643fcba

tmadlener force-pushed the new-sio-backend branch from beec481 to 643fcba Compare October 6, 2020 20:50

tmadlener changed the title ~~[WIP] New sio backend~~ New sio backend Oct 7, 2020

hegner reviewed Oct 7, 2020

View reviewed changes

tmadlener added 2 commits October 8, 2020 11:15

Use correct exception type for better error message

f2df7ec

Make description of build option less technical

f7c9271

tmadlener mentioned this pull request Oct 13, 2020

Use new cmake macros from podio to build the datamodel libraries and dictionary key4hep/EDM4hep#84

Merged

gaede approved these changes Oct 13, 2020

View reviewed changes

andresailer reviewed Oct 13, 2020

View reviewed changes

cmake/podioConfig.cmake.in Outdated Show resolved Hide resolved

andresailer reviewed Oct 13, 2020

View reviewed changes

cmake/podioConfig.cmake.in Show resolved Hide resolved

Add SIO to dependencies for downstream consumers

cdf77a9

tmadlener closed this Oct 14, 2020

tmadlener reopened this Oct 14, 2020

tmadlener closed this Oct 19, 2020

tmadlener reopened this Oct 19, 2020

tmadlener mentioned this pull request Nov 3, 2020

Decouple writers and EventStore #144

Merged

gaede merged commit 839c39d into AIDASoft:master Nov 10, 2020

tmadlener mentioned this pull request Nov 11, 2020

Newest podio potentially depends on sio, but sio only lives here key4hep/key4hep-spack#104

Closed

tmadlener deleted the new-sio-backend branch January 26, 2021 10:19

	if self.reader.options["createSIOHandlers"]:
	if 'SIO' in self.reader.options["IOHandlers"]:

New sio backend #130

New sio backend #130

Conversation

tmadlener commented Sep 1, 2020 • edited Loading

gaede commented Sep 2, 2020

tmadlener commented Sep 8, 2020

tmadlener commented Sep 9, 2020

tmadlener commented Sep 14, 2020

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

tmadlener commented Sep 16, 2020

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

Choose a reason for hiding this comment

gaede commented Sep 16, 2020

andresailer commented Sep 16, 2020

gaede commented Sep 16, 2020

andresailer commented Sep 16, 2020

tmadlener commented Sep 23, 2020

andresailer left a comment

Choose a reason for hiding this comment

tmadlener commented Sep 25, 2020

tmadlener commented Sep 25, 2020

tmadlener commented Oct 7, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hegner commented Oct 7, 2020

gaede left a comment

Choose a reason for hiding this comment

gaede commented Oct 14, 2020

tmadlener commented Oct 14, 2020

tmadlener commented Oct 19, 2020

tmadlener commented Sep 1, 2020 •

edited

Loading