UTReadoutTool - Fix for DD4HEP

added 1 deleted label

assigned to @xuyuan

added to epic &1

added dd4hep label

The error seems to have evolved slightly

https://lhcb-nightlies.web.cern.ch/nightly/lhcb-dd4hep/1131/Moore/x86_64_v2-centos7-gcc11%2Bdd4hep-opt/tests#Hlt1Conf.performance.pp_comparison

199 - PrStorePrUTHits.UTReadoutTool.sy...   ERROR  	 PrStorePrUTHits.UTReadoutTool:: get():: No valid data at '/world/BeforeMagnetRegion/UT:DetElement-Info-IOV'	 StatusCode=FAILURE
200 - PrStorePrUTHits.UTReadoutTool.sy...   ERROR Exception stack trace
253 - ToolSvc                               ERROR Error initializing tool 'PrStorePrUTHits.UTReadoutTool'
255 - HiveDataBrokerSvc                     ERROR PrStorePrUTHits 	 Failed to retrieve tool UTReadoutTool/UTReadoutTool	 StatusCode=FAILURE

changed title from PrStorePrUTHits - Fix for DD4HEP to UTReadoutTool - Fix for DD4HEP

mentioned in issue Rec#336 (closed)

Hi all, may I confirm that in this test all DeUT should be from Detector/UT instead of those from DDDB? If it is true, then I have to say currently the DeUT is not totally ready for the DD4Hep -- it can be compiled but most of the functions are still empty.

Access to anything from the depreciated DetDesc DBs, DDDB, SIMDCOND etc. is now completely blocked in the dd4hep builds. Everything has to come from the dd4hep detector elements and the new yaml based DB it uses.

Note though, the issue above is I think something different. The problem is with dd4hep and the new Detector elements, access to them during initialize() is not really allowed, and this appears to be what UTReadoutTool is attempting to do, which is failing, even though its using the correct dd4hep path. @clemenci can probably comment more here, but access to the detector info has to be migrated to be performed via the functional algorithm API, and if needed to use derived conditions.

Note this issue is currently the one blocking a lot of Moore tests in the dd4hep builds

https://lhcb-nightlies.web.cern.ch/nightly/lhcb-dd4hep/1131/Moore/x86_64_v2-centos7-gcc11%2Bdd4hep-opt/tests

We really need to progress past this soon so we can see what issue is next up...

Indeed conditions (or detectors) cannot be accessed during initialize. I'll try to have a look today.

Thanks. Note this is currently also blocking even the 'NoUT' tests. This is discussed in Rec#336 (closed) and @decianm is going to look into it.

The elements defined in DD4hep are based on the new Geo/structure as in lhcb-conddb/DDDB!79 (merged) and lhcb-conddb/SIMCOND!143 (merged), which means we have to develop new Decodes for UT. It is on my schedule and I am working on this now.

There are two parts in this issue: move to the new Geo/structure and make the code agnostic of the detector description backend.

I can help with the second, for example replacing the calls like getDet<DeUTDetector> using data members like ConditionAccessor<DeUTDetector>.

I started looking into it, but what stopped me is that UTReadoutTool is used to generate XML valid for DetDesc. I find it surprising that IIUC the tool reads a condition from the detector transient store, modifies it in place and generates the XML file containing the new data. This kind of workflow cannot work with DD4hep, so we have to do something different (that can work with both DetDesc and DD4hep).

Hi @clemenci I start working on the De-codes for new Geo in the frame of Detector today. I have to mention that all my codes developing base on related MRs(!2995 (merged)), in which hits are encoded/decoded using new Geo/ID, and that would also effect the tracks reconstructing method.

For the second issue, I would be glad that you can help us.

@clemenci I'm not the expert of course, @xuyuan should comment, but it looks like the XML writer part of the tool is just there as a convenience for generating condition updates, not used in general. I would say it can be either just ignore, or if need be commented out until it can be adapted to dd4hep/yaml when needed.

You are probably right... I didn't dive too deep in the callbacks passed to the UpdateManagerSvc. I finish checking something in Boole, then I try to modernize UTReadoutTool.

I started the work to modernize UTReadoutTool and I found a couple of anti-patterns that require some deeper refactoring.

The normal change that is required is that the caching of geometry/condition info has to move from tool data members to a dedicated derived condition, so I started that refactoring, but I stumbled on a part of this cache that is not in data members but in some global storage (very bad for multithreading). Apart from UTReadoutTool that fills it, the global states from UTBoardMapping.h are used only in a couple of places and only for debugging/error messages.

The other anti-pattern is that UTReadoutTool is used sometimes as a private tool and sometimes as a public one. I have the feeling that for this use case we should switch from using a tool to a derived condition.

This brings me to the final finding of yesterday: one of the uses UTReadoutTool is in PrStoreUTHit to fill the derived condition UTGeomCache. I believe it would be more practical to merge the UTReadoutTool cache and LHCb::Pr::UT::UTGeomCache into one shared derived condition.

The changes needed for a proper modernization are design decision I cannot make for code I'm not responsible for.

I have two options:

the quick way: drop uses of UTBoardMapping (alternative ways can be found in a second moment) and wrap up the derived condition I started implementing
the long way: merge UTReadoutTool and LHCb::Pr::UT::UTGeomCache into one shared derived condition (still dropping UTBoardMapping)

Given the amount of time I can spend on the task I prefer to go the quick way. Let me know what do you think.

Thanks @clemenci

Personally, given we aren't going to be running UT this year anyway, and the UTReadoutTool issue is the one currently blocking most Moore tests in the dd4hep builds, I have no problems with the quick way. The lost debugging info can be added back at a later date.

For the record I found what I consider a bug in the current UTReadoutTool: !2631 (comment 5577101)

mentioned in merge request Moore!1567 (merged)

assigned to @clemenci and unassigned @xuyuan

mentioned in merge request !3586 (merged)

@clemenci It seems the fixes here are not enough, as we are now hitting issues at the point the derived condition objects should be instanciated. See the discussion in Rec#345 (closed).

closed with merge request !3586 (merged)

mentioned in commit cef5e38e

UTReadoutTool - Fix for DD4HEP

Child items ...

Activity