Make "always read EventHeader" collection more robust #161
Conversation
tmadlener
force-pushed
the
robust-event-header
branch
from
abe573a to
3330439
Compare
| } | ||
|
|
||
| void PodioInput::operator()() const { | ||
| if (m_podioDataSvc->hasCollection(edm4hep::EventHeaderName)) { | ||
| m_readers[edm4hep::EventHeaderCollection::typeName](edm4hep::EventHeaderName); |
There was a problem hiding this comment.
I thought about doing it this way initially, what happens if the EventHeader collection is also in the list of collections to read? Will it be read twice?
There was a problem hiding this comment.
It would query the underlying Frame again for a second time. However, the second time around that effectively is a map lookup and return. Technically, hasCollection does all the "heavy lifting" of prompting the Frame to actually read the data and populate it's internal map. Most importantly you will always get the same collection.
I quickly checked what happens if I explicitly make the test read the EventHeader collection and nothing seems to have broken, but it looks like we would be putting two DataHandles pointing to the same collection into the EDS at the moment. I have to check why that seems to work without breaking.
There was a problem hiding this comment.
We indeed do get two DataHandles (and corresponding objects in the EDS) if EventHeader is explicitly passed in. However, since the collection is owned by the Frame and the DataSvc defers the cleanup to that, there is at least no double free.
I will add a simple filter to clean out duplicate collection names as well as "EventHeader". Additionally, I will adapt the PodioDataSvc::readCollection to check for existence in the EDS before populating it twice.
There was a problem hiding this comment.
Didn't do the filtering in the end, because it is effectively unnecessary with the guard against duplicate entries in the EDS.
|
Unless there are any complaints until this evening I would merge this and then tag k4FWCore |
| @@ -95,6 +95,7 @@ add_test_with_env(TestDataHandleUniquePtr options/TestDataHandleUniquePtr.py) | |||
| add_test_with_env(TestUniqueIDGenSvc options/TestUniqueIDGenSvc.py) | |||
| add_test_with_env(CreateLegacyExampleEventData options/createLegacyExampleEventData.py) | |||
| add_test_with_env(TestEventHeaderFiller options/createEventHeader.py) | |||
| add_test_with_env(EventHeaderCheck options/runEventHeaderCheck.py PROPERTIES DEPENDS TestEventHeaderFiller) | |||
There was a problem hiding this comment.
Look at how easy it is to add one test now...
test/k4FWCoreTest/src/components/ExampleEventHeaderConsumer.cpp
Outdated
Show resolved
Hide resolved
test/k4FWCoreTest/src/components/ExampleEventHeaderConsumer.cpp
Outdated
Show resolved
Hide resolved
k4FWCore/components/PodioInput.cpp
Outdated
| } | ||
|
|
||
| void PodioInput::operator()() const { | ||
| if (m_podioDataSvc->hasCollection(edm4hep::EventHeaderName)) { |
There was a problem hiding this comment.
Since it only happens one the hasCollection function can be removed, I think this should work:
| if (m_podioDataSvc->hasCollection(edm4hep::EventHeaderName)) { | |
| if (m_podioDataSvc->getEventFrame().get(edm4hep::EventHeaderName)) { |
|
I don't have any more comments; I think many parts need to be reworked anyway to be able to use the |
BEGINRELEASENOTES
EventHeaderFiller(introduced in Add a Gaudi Algorithm filling EventHeader eventNumber and runNumber #133) are correct when reading back.PodioDataSvconly reads and registers collections to the EDS once even in case there are duplicates.ENDRELEASENOTES
#160 introduced a bug that went undetected in tests, because all the tests apparently override the list of collections that is set in the
PodioInputconstructor. Using an empty (default) list and an input file that doesn't contain theEventHeadercollection leads to Gaudi exiting early because it cannot retrieve the collection from the event store.This PR adds a simple consumer that makes sure that things run, even if the input file does not have an EventHeader collection.
Overall there is a discussion to be head whether making the
EventHeadera datatype like all the rest is really the best design, as it is technically rather part of the "metadata" that describe an Event rather than content of it.