add new frame based podio interface

[hegner]

BEGINRELEASENOTES

• Add new frame based PODIO interface
• Temporarily add legacy components to support previous PODIO interface
• Drop feature of automatically storing simple non-PODIO types automatically in ROOT
• Temporarily remove feature for access to collection metadata in preparation of redesign

ENDRELEASENOTES

[hegner]

This PR requires the latest PODIO v00-16-03 for proper linking. There has been a problem with non-inlined functions in previous versions,

[tmadlener]

I have had a first look and assuming that all the Legacy things are simple copy & pastes of the previous versions focused on the non legacy ones. I think this is in a pretty good state already, there are just a few smaller issues to be sorted out potentially, but nothing that changes the architecture overall. Some of my comments could also be pushed to follow up PRs.

For storing non podio types we could potentially use the Frame parameters under the hood (for the supported types, and for single elements per event). But we can also leave this for now, and see if there is a need for this.

[tmadlener]

Actually there is on sort of architecture/design question that I would have. Currently the PodioDataSvc still holds a reader and a Frame. Would it be possible to shift the reader from DataSvc to the PodioInput? Because then we could probably also get rid of the unintuitive pattern of having to specify the input file at the DataSvc, but the collections we want to read in the input:

k4FWCore/test/k4FWCoreTest/options/readExampleEventData.py

Lines 3 to 9 in 22af6ca

	from Configurables import k4DataSvc
	podioevent = k4DataSvc("EventDataSvc")
	podioevent.input = "output_k4test_exampledata.root"
	from Configurables import PodioInput
	inp = PodioInput()
	inp.collections = ["MCParticles", "SimTrackerHits", "TrackerHits", "Tracks"]

[hegner]

Thanks Thomas for having a look. I think we can get most of the things resolved quickly.

I would as well very much like DataSvc and Input to be split in a better way. Then however we need to change the interface of the Frames a bit so that a frame can be updated rather than created in the reading chain. So we could make it an issue in both Podio and here. And then we take it from there.

[tmadlener]

Then however we need to change the interface of the Frames a bit so that a frame can be updated rather than created in the reading chain.

Updating would in this case basically mean to (re)set the FrameData at the beginning of the event, right? That should be fairly straight forward to podio

[hegner]

no. we would start with an empty Frame. And then insert things via the FrameReader / new FrameData

[tmadlener]

I again mainly have minor comments and I think one bug. Shall we do the collection metadata handling still in this one, or do we do that in a separate PR?

[hegner]

OK. Addressed all points. the metadata comes in a separate PR as we may need a few iterations.

[tmadlener]

OK. I think this is good as it is. Just to make sure that we are not missing anything obvious due to the failing CI, could we also make the minimum version for podio explicit in the CMakeLists, here?

k4FWCore/k4FWCore/CMakeLists.txt

Line 5 in 22af6ca

find_package(podio)

Than CI would fail already at that stage and not during linking or when trying to compile, and it would be more obvious why it fails and whether 0.16.3 is really already enough.

[kjvbrt]

Are #include "TFile.h" and #include "rootUtils.h" still needed?

[tmadlener]

Good catch, they should be obsolete now.

[kjvbrt]

Hello all,

Running one of our tests from k4RecCalorimeter: link

I get the following error:

out                 DEBUG Property update for OutputLevel : new value = 2
out                 DEBUG Initialize base class GaudiCommon<Algorithm>
out                 DEBUG could not locate CounterSummarySvc, no counter summary will be made
out                 DEBUG List of ALL properties of PodioOutput/out  #properties = 31
out                 DEBUG Property ['Name': Value] =  'remoteFilename':''
out                 DEBUG Property ['Name': Value] =  'outputCommands':[ 'keep *' ]
out                 DEBUG Property ['Name': Value] =  'filename':'output_fullCalo_SimAndDigi.root'
out                 DEBUG Property ['Name': Value] =  'RequireObjects':[  ]
out                 DEBUG Property ['Name': Value] =  'VetoObjects':[  ]
out                 DEBUG Property ['Name': Value] =  'CounterList':[ '.*' ]
out                 DEBUG Property ['Name': Value] =  'Context':''
out                 DEBUG Property ['Name': Value] =  'TypePrint':True
out                 DEBUG Property ['Name': Value] =  'PropertiesPrint':False
out                 DEBUG Property ['Name': Value] =  'ErrorsPrint':True
out                 DEBUG Property ['Name': Value] =  'RootInTES':''
out                 DEBUG Property ['Name': Value] =  'FilterCircularDependencies':True
out                 DEBUG Property ['Name': Value] =  'Blocking':False
out                 DEBUG Property ['Name': Value] =  'NeededResources':[  ]
out                 DEBUG Property ['Name': Value] =  'Cardinality':1
out                 DEBUG Property ['Name': Value] =  'RegisterForContextService':True
out                 DEBUG Property ['Name': Value] =  'MonitorService':'MonitorSvc'
out                 DEBUG Property ['Name': Value] =  'Timeline':False
out                 DEBUG Property ['Name': Value] =  'AuditStop':False
out                 DEBUG Property ['Name': Value] =  'AuditStart':False
out                 DEBUG Property ['Name': Value] =  'AuditFinalize':False
out                 DEBUG Property ['Name': Value] =  'AuditExecute':True
out                 DEBUG Property ['Name': Value] =  'AuditRestart':False
out                 DEBUG Property ['Name': Value] =  'AuditReinitialize':False
out                 DEBUG Property ['Name': Value] =  'AuditInitialize':False
out                 DEBUG Property ['Name': Value] =  'AuditAlgorithms':False
out                 DEBUG Property ['Name': Value] =  'ErrorMax':1
out                 DEBUG Property ['Name': Value] =  'Enable':True
out                 DEBUG Property ['Name': Value] =  'OutputLevel':2
out                 DEBUG Property ['Name': Value] =  'ExtraOutputs':[]
out                 DEBUG Property ['Name': Value] =  'ExtraInputs':[]
out                 ERROR Could not get DataSvc!
out                 DEBUG input handles: 0
out                 DEBUG output handles: 0
EventLoopMgr        ERROR Unable to initialize Algorithm: out
ServiceManager      ERROR Unable to initialize Service: EventLoopMgr
ApplicationMgr      ERROR Application Manager Terminated with error code 1

The test gets the input data from particle gun.

[tmadlener]

Thanks @kjvbrt for trying to run this already. I think for that to work you have to use the PodioDataSvc from this PR. Currently it uses an FCCDataSvc instead:
https://github.com/HEP-FCC/k4RecCalorimeter/blob/f8d3ad8f65344b0b9a0abb0eae1806d564803a7b/RecFCCeeCalorimeter/tests/options/runCaloSim.py#L16

[kjvbrt]

Well, one needs to replace FCCDataSvc with k4DataSvc in order to even run.
Using PodioDataSvc I get:

ImportError: cannot import name 'PodioDataSvc' from 'Gaudi.Configurables' (unknown location)

[tmadlener]

Ah alright. It looks like we are missing an actual gaudi component to get the PodioDataSvc into action somehow. We wrap the PodioLegacyDataSvc into a k4LegacyDataSvc, and then do a DECLARE_COMPONENT for that, but we do not expose the new PodioDataSvc yet this way.

@hegner this should be fairly straight forward to add, but looking at the code I am not entirely sure we need the k4DataSvc wrapper. But potentially also something that we can make work for now (would also be fairly transparent this way for existing option files) and then change later if necessary.

[hegner]

As in the previous incarnation it is still known as k4DataSvc. You would just need to do

from Configurables import k4DataSvc
podioevent = k4DataSvc("EventDataSvc")

as done in the tests. Maybe an install step is missing?

[tmadlener]

Ah, I completely missed that the k4DataSvc remains completely untouched by this, because it simply inherits from PodioDataSvc. Is there a reason why to not expose the PodioDataSvc as component, instead of introducing another abstraction here?

[hegner]

IMHO we can simply remove the indirection. We however should discuss that in a meeting. Since that has an impact on all configs

[tmadlener]

I have done some further tests with this, including trying to read the output of ddsim and there is still some issue. The current implementation seems to skip every other event when reading. This is also reproducible with a simpler test case that uses the output of the example event data creation (which is never really checked for correctness at the moment).

I have put together a simple test case that exhibits the problem, shall I just push it to this branch, so that it will also be directly included in the test suite in the future?

[hegner]

yes. please add the test and I will investigate

[hegner]

Found the reason for the event skipping. The clearStore(), which forwards to the next event is called actually twice. First via a direct call
https://gitlab.cern.ch/gaudi/Gaudi/-/blob/master/GaudiCoreSvc/src/ApplicationMgr/EventLoopMgr.cpp#L312
and then as an indirect one via the internals of DataSvc::setRoot
https://gitlab.cern.ch/gaudi/Gaudi/-/blob/master/GaudiCoreSvc/src/ApplicationMgr/EventLoopMgr.cpp#L326

Need to put some functionality into i_setroot instead of clearStore, which is actually the better design anyhow. Augmenting this:
https://gitlab.cern.ch/gaudi/Gaudi/-/blob/master/GaudiKernel/src/Lib/DataSvc.cpp#L175

(please note: i_setroot is virtual while setRoot is not)

[tmadlener]

Maybe we should enclose this inside a k4FWCore namespace to avoid (unlikely) collisions?

[hegner]

@jmcarcell - can you have another look? thanks

[jmcarcell]

I don't think I have any more comments other than style ones (if (nullptr == thing) could just be if(!nullptr)) but we can leave that to another time (it's certainly not worth it to go over them and change them manually)