Add support for running with IOSvc #216
Conversation
jmcarcell
force-pushed
the
iosvc-support
branch
from
cb3c7a3 to
5967d0b
Compare
| for (const auto& pidCollMeta : pidCollections) { | ||
| const auto algoId = attachParticleIDMetaData(lcio_event, edmEvent, pidCollMeta); | ||
| auto algoId = attachParticleIDMetaData(lcio_event, edmEvent.value(), pidCollMeta); |
There was a problem hiding this comment.
Maybe this also needs a similar treatment as convertObjectParameters in k4EDM4hep2LcioConv? That would make it possible to simplify the code below a bit, but I am not sure if it's worth the effort.
| @@ -64,6 +67,9 @@ class EDM4hep2LcioTool : public AlgTool, virtual public IEDMConverter { | |||
|
|
|||
| PodioDataSvc* m_podioDataSvc; | |||
There was a problem hiding this comment.
Could we have a one line description for each service under what circumstances which service is used?
There was a problem hiding this comment.
Like the ones I added?
| if (!root) { | ||
| thisClass->error() << "Failed to retrieve root object /Event" << endmsg; | ||
| } | ||
|
|
||
| auto pObj = root->registry(); | ||
| if (!pObj) { | ||
| thisClass->error() << "Failed to retrieve the root registry object" << endmsg; | ||
| } |
There was a problem hiding this comment.
Should we exit early here, or throw an exception to avoid running into a seg-fault? Or does error() already do that in this case?
There was a problem hiding this comment.
No, error() doesn't, however I don't know if there are any circumstances under which error() will run. The same code can be found in the Writer from k4FWCore and I have never seen it happening. I can add one throw if you really want, but then all the checks in that function should also have a throw.
There was a problem hiding this comment.
Does a fatal throw or abort execution? The main potential issue I see here is that if these checks fail we will have a nullptr access and a segfault, and depending on if / when stdout / stderr has last been flushed we may or may not see the error message. If there are no visible error messages in case things fail, these checks are almost useless, I think. Obviously, if the messages are guaranteed to be there even in case of a seg fault, then there is at least some information where things went wrong.
There was a problem hiding this comment.
fatal doesn't abort. endmsg writes the message so there should always be some information and it would happen right before the crash. As I said, I've never seen this fail because I think if it's in a state where that would fail something else will fail much earlier, like not being able to find /Event in the store. I'll just add the 3 or 4 throws since it's easier than continuing this conversation.
|
I just realised I made a mistake and key4hep/k4EDM4hep2LcioConv#102 is not needed, so it can be reverted. key4hep/k4EDM4hep2LcioConv#102 was needed before because I was incorrectly writing the event parameters to the metadata frame. I discovered a limitation (added in a warning) of the current implementation: when running Marlin processors with |
This is mainly (only?) relevant for the LCIO to EDM4hep direction, right? As far as I can tell, the tests are unchanged apart from the additional infrastructure to switch between IOSvc and PodioDataSvc. In that case I would say this is OK for now and we check again in the future if someone runs into problems with this. |
BEGINRELEASENOTES
IOSvcwith a few tests:IOSvcglobal_converter_mapswithIOSvc, where a functional algorithm produces some output that is used by a Marlin processor and the output of the two is used by aGaudi::Algorithm.test_link_conversion_edm4hepwithIOSvc, where links produced by functional algorithms are checked by Marlin processors.ENDRELEASENOTES
edm4hep::utils::ParticleIDMetavia theIMetadataSvck4FWCore#273Fix #202