search

cms-sw / cmssw

CMS Offline Software
http://cms-sw.github.io/
Apache License 2.0
1.08k stars 4.3k forks source link

Obsolete tags `PCaloGeometry_Castor_v1_hlt` and `L1CaloGeometry_CRAFT09_hlt` still consumed by code #36806

Closed malbouis closed 2 years ago

malbouis commented 2 years ago

AlCaDB has started the migration of the DD4hep Geometry for the data Global Tags. While doing so, we noticed a couple of very old DDD tags that are included in the online GTs and apparently still consumed.

The obsolete tags are below:

PCaloGeometry_Castor_v1_hlt, from the Castor detector that has been decommissioned. L1CaloGeometry_CRAFT09_hlt, that has not been updated since 2008.

We tried removing them and running on Run-3 data but we observed an unexpected crash because apparently they are still consumed and the code was not updated.

There was a candidate GT prepared with the dd4hep tags and the removal of the tag PCaloGeometry_Castor_v1_hlt. This is the diff wrt to the 122X_dataRun3_Prompt_v2 GT where we can see that the Castor tag was removed: https://cms-conddb.cern.ch/cmsDbBrowser/diff/Prod/gts/122X_dataRun3_Prompt_v2/122X_dataRun3_Prompt_dd4hep_2022_01_25_19_31_04

When running the relval workflow 138.4 (Prompt GT on 2021 PilotBeam data) with the above Candidate GT, we observe the error message below:

----- Begin Fatal Exception 25-Jan-2022 20:48:49 CET-----------------------
An exception of category 'NoRecord' occurred while
   [0] Processing global begin Run run: 346512
   [1] Prefetching for module EcalDQMonitorTask/'ecalMonitorTask'
   [2] Prefetching for EventSetup module CaloGeometryBuilder/''
   [3] Calling method for EventSetup module CastorGeometryFromDBEP/''
   [4] While getting dependent Record from Record CastorGeometryRecord
Exception Message:
No "PCastorRcd" record found in the EventSetup.

 Please add an ESSource or ESProducer that delivers such a record.
----- End Fatal Exception -------------------------------------------------

A similar thing happens when running on the same workflow with another Candidate GT where the tag L1CaloGeometry_CRAFT09_hlt was removed this time: https://cms-conddb.cern.ch/cmsDbBrowser/diff/Prod/gts/122X_dataRun3_Prompt_v2/122X_dataRun3_Prompt_dd4hep_Candidate_2022_01_25_19_57_39

The error message observed with the above Candidate GT is:

----- Begin Fatal Exception 25-Jan-2022 21:06:18 CET-----------------------
An exception of category 'NoRecord' occurred while
   [0] Processing  Event run: 346512 lumi: 250 event: 243042266 stream: 0
   [1] Running path 'L1Reco_step'
   [2] Calling method for module L1ExtraParticlesProd/'l1extraParticles'
Exception Message:
No "L1CaloGeometryRecord" record found in the EventSetup.

 Please add an ESSource or ESProducer that delivers such a record.
----- End Fatal Exception -------------------------------------------------

Both error messages indicate that the tags L1CaloGeometry_CRAFT09_hlt and PCaloGeometry_Castor_v1_hlt are still consumed in the code.

FYI, @tvami @francescobrivio @srimanob

cmsbuild commented 2 years ago

A new Issue was created by @malbouis .

@Dr15Jones, @perrotta, @dpiparo, @makortel, @smuzaffar, @qliphy can you please review it and eventually sign/assign? Thanks.

cms-bot commands are listed here

srimanob commented 2 years ago

@cms-sw/l1-l2 Could you please comment on l1extraparticles that consume old tag? I am not sure what still need or why it is still OK to pick the old geometry record. Do we run L1Reco in Run-3?

Thanks.

tvami commented 2 years ago

assign l1

tvami commented 2 years ago

FYI @cms-sw/hcal-dpg-l2 @bsunanda

cmsbuild commented 2 years ago

New categories assigned: l1

@epalencia,@rekovic,@cecilecaillol you have been requested to review this Pull request/Issue and eventually sign? Thanks

tvami commented 2 years ago

assign reconstruction

cmsbuild commented 2 years ago

New categories assigned: reconstruction

@slava77,@jpata,@clacaputo you have been requested to review this Pull request/Issue and eventually sign? Thanks

tvami commented 2 years ago

assign reconstruction

for the Castor part

francescobrivio commented 2 years ago

assign geometry

since this is mainly a geometry topic

cmsbuild commented 2 years ago

New categories assigned: geometry

@cvuosalo,@mdhildreth,@ianna,@Dr15Jones,@makortel,@civanch you have been requested to review this Pull request/Issue and eventually sign? Thanks

makortel commented 2 years ago

assign reconstruction

for the Castor part

EcalDQMonitorTask is DQM, no?

tvami commented 2 years ago

unassign reconstruction

tvami commented 2 years ago

assign reconstruction

for the Castor part

EcalDQMonitorTask is DQM, no?

yes, you are right! I'm also a bit confused, I thought Castor stuff is connected to HCAL and not ECAL

tvami commented 2 years ago

assign dqm

cmsbuild commented 2 years ago

New categories assigned: dqm

@jfernan2,@ahmad3213,@rvenditti,@emanueleusai,@pbo0,@pmandrik you have been requested to review this Pull request/Issue and eventually sign? Thanks

tvami commented 2 years ago

and then I guess FYI @cms-sw/ecal-dpg-l2 :)

thomreis commented 2 years ago

@alejands, @abhih1 could you take a look at the EcalDQMonitorTask issue please?

makortel commented 2 years ago

(sorry for adding noise before) The problem with Castor stems from the configuration of CaloGeometryBuilder (so for @cms-sw/geometry-l2), that in 138.4 step2 is

process.CaloGeometryBuilder = cms.ESProducer("CaloGeometryBuilder",
    SelectedCalos = cms.vstring(
        'HCAL',
        'ZDC',
        'CASTOR',
        'EcalBarrel',
        'EcalEndcap',
        'EcalPreshower',
        'TOWER'
    )
)

which then leads to anything consuming the CaloGeometry leading to consumption of Castor geometry.

abhih1 commented 2 years ago

@alejands, @abhih1 could you take a look at the EcalDQMonitorTask issue please?

sure.

thomreis commented 2 years ago

@abhih1 according to the comment of @makortel this seems to be a geometry issue and not directly cause by ECAL DQM.

tvami commented 2 years ago

yes, indeed, let me remove dqm from the issue, thanks @thomreis

tvami commented 2 years ago

unassign dqm

cvuosalo commented 2 years ago

PR #36814 should remove the need for the CastorGeometryRecord.

cvuosalo commented 2 years ago

+1

srimanob commented 2 years ago

Any news on L1CaloGeometry_CRAFT09_hlt?

tvami commented 2 years ago

@cecilecaillol ?

tvami commented 2 years ago

tagging also @epalencia for the L1T part

cvuosalo commented 2 years ago

@cecilecaillol @epalencia @tvami @civanch It appears that L1CaloGeometry_CRAFT09_hlt may still be in use. It is used by L1ExtraParticlesProd to construct "calo particles":

https://cmssdt.cern.ch/lxr/source/L1Trigger/L1ExtraFromDigis/src/L1ExtraParticlesProd.cc?v=CMSSW_12_3_X_2022-02-01-2300#0103

L1ExtraParticlesProd is called by HLT_Fake_cff.py:

https://cmssdt.cern.ch/lxr/source/HLTrigger/Configuration/python/HLT_Fake_cff.py?v=CMSSW_12_3_X_2022-02-01-2300

which is called by CustomConfigs.py:

https://cmssdt.cern.ch/lxr/source/HLTrigger/Configuration/python/CustomConfigs.py?v=CMSSW_12_3_X_2022-02-01-2300

There are other calls to these producers that should be checked.

L1 experts need to evaluate whether the L1CaloGeometry is actually needed.

@tvami It might be better to just include the L1CaloGeometry_CRAFT09_hlt tag for now until the L1 group can assess whether it is being used.

tvami commented 2 years ago

Hi Carl, thanks for looking at this. Does this mean you would produce a DD4HEP version of L1CaloGeometry? Btw, @bundocka told me they started to have a look at this if it's needed or not.

cvuosalo commented 2 years ago

@tvami The L1CaloGeometry is created by this config, as far as I can tell:

https://cmssdt.cern.ch/lxr/source/L1TriggerConfig/L1GeometryProducers/python/l1CaloGeometry_cfi.py?v=CMSSW_12_3_X_2022-02-01-2300

Its creation does not use DDD or DD4hep. I think its creation and uploading is the responsibility of the L1 group. They have to decide if they need it, and if so, if it needs to revised and how to do so.

panoskatsoulis commented 2 years ago

Hi, where has this been run? can somebody post a workflow or a link to this failed 138.4 step2 attempt so we can find the wf command?

tvami commented 2 years ago

Hi @panoskatsoulis , I've been in touch with @bundocka , he (I think) is looking into this. This is the command I gave him to reproduce the issue

runTheMatrix.py --what standard -l 138.4 --command "--conditions 122X_dataRun3_Prompt_dd4hep_2022_01_25_19_31_04 -n 1000" -t 8

(in the latest CMSSW IBs)

bundocka commented 2 years ago

Hi. I have made a PR here to master including the fix:

https://github.com/cms-sw/cmssw/pull/36916

tvami commented 2 years ago

Running

cmsDriver.py HLT -s L1REPACK,HLT:GRun --processName HLT2 --data --scenario pp --datatier FEVTDEBUGHLT,DQM  --conditions 122X_dataRun3_HLTNew_HCAL_w6_2022_v1 --python_filename NEWCONDITIONS0.py --filein 'filelist:step1_files.txt' --fileout 'file:step2.root' --no_exec -n 100 --eventcontent FEVTDEBUGHLT,DQM --era Run3 --lumiToProcess 'step1_lumi_ranges.txt' --inputCommands "keep *,drop *_hlt*_*_HLT,drop *_TriggerResults_*_HLT,drop *_*_*_RECO" --procModifiers siPixelQualityRawToDigi --customise "Configuration/DataProcessing/RecoTLR.customisePostEra_Run3,RecoLocalCalo/Configuration/customiseHBHEreco.hbheUseM0FullRangePhase1" 

cmsDriver.py HLT -s RAW2DIGI,L1Reco,RECO,PAT,DQM:DQMOffline+offlineValidationHLTSource --processName reRECO --data --scenario pp --datatier RECO,DQMIO --eventcontent RECO,DQM --conditions 122X_dataRun3_HLTNew_HCAL_w6_2022_v1 --hltProcess HLT2 --filein=file:step2.root --fileout=file:step3.root --python_filename recodqm_newco.py --no_exec -n 100 --era Run3 --procModifiers siPixelQualityRawToDigi --customise "Configuration/DataProcessing/RecoTLR.customisePostEra_Run3,RecoLocalCalo/Configuration/customiseHBHEreco.hbheUseM0FullRangePhase1"

in our full track validation for HLT conditions https://cmssdt.cern.ch/cms-jenkins/blue/organizations/jenkins/AlCaDBValidation/detail/master/36/pipeline/#step-106-log-1407

We still get

----- Begin Fatal Exception 12-Feb-2022 15:36:04 CET-----------------------
An exception of category 'NoRecord' occurred while
   [0] Processing  Event run: 346512 lumi: 343 event: 333745401 stream: 0
   [1] Running path 'HLTAnalyzerEndpath'
   [2] Prefetching for module L1TRawToDigi/'hltGtStage2Digis'
   [3] Prefetching for module RawDataCollectorByLabel/'rawDataCollector'
   [4] Prefetching for module L1GTDigiToRaw/'l1GtPack'
   [5] Calling method for module L1GlobalTrigger/'simGtDigis'

Exception Message:
No "L1CaloGeometryRecord" record found in the EventSetup.

@silviodonato do you see anything obviously wrong in what we do here? Is it that the HLT menu would need a change?

srimanob commented 2 years ago

@tvami Is not that the issue of L1Repack? For example, you can run through with HLT step only.

tvami commented 2 years ago

@tvami Is not that the issue of L1Repack? For example, you can run through with HLT step only.

I was thinking about that, but the add-on tets use the L1Repack too, see e.g. here https://github.com/cms-sw/cmssw/blob/eee164032884afc912307611be61213ff74b7ad3/Configuration/HLT/python/addOnTestsHLT.py#L43

srimanob commented 2 years ago

Your job will run with L1REPACK:Full,HLT:GRun as I test locally. :) L1REPACK:Full is an obsolete sequence, if I understand correctly.

cmsDriver.py HLT -s L1REPACK:Full,HLT:GRun --processName HLT2 --data --scenario pp --datatier FEVTDEBUGHLT,DQM --conditions 122X_dataRun3_HLTNew_HCAL_w6_2022_v1 --python_filename NEWCONDITIONS0_dump_L1REPACKFULL.py --filein 'filelist:step1_files.txt' --fileout 'file:step2.root' --no_exec -n 100 --eventcontent FEVTDEBUGHLT,DQM --era Run3 --lumiToProcess 'step1_lumi_ranges.txt' --inputCommands "keep *,drop *_hlt*_*_HLT,drop *_TriggerResults_*_HLT,drop *_*_*_RECO" --procModifiers siPixelQualityRawToDigi --customise "Configuration/DataProcessing/RecoTLR.customisePostEra_Run3,RecoLocalCalo/Configuration/customiseHBHEreco.hbheUseM0FullRangePhase1" --dump_python

tvami commented 2 years ago

Thanks @srimanob !

srimanob commented 2 years ago

Do we want to continue this issue to follow up question on L1CaloGeometry with L1T, or do we open a new issue to focus on the topic?

civanch commented 2 years ago

@srimanob , it seems that this issue in a good part focused on L1, so an extra issue is not needed.

tvami commented 2 years ago

tagging @bundocka @missirol

I follow-up a bit on https://github.com/cms-sw/cmssw/pull/36940#issuecomment-1038805021

So I hoped one could just move from 

git diff RecoEgamma/EgammaHLTProducers/plugins/HLTRecHitInAllL1RegionsProducer.cc

 void L1RegionData<l1extra::L1JetParticleCollection>::getEtaPhiRegions(const edm::Event& event,
                                                                       std::vector<RectangularEtaPhiRegion>& regions,
-                                                                      const L1CaloGeometry& l1CaloGeom) const {
+                                                                      const CaloGeometry& caloGeom) const {
   edm::Handle<l1extra::L1JetParticleCollection> l1Cands;
   event.getByToken(token_, l1Cands);

@@ -306,11 +299,11 @@ void L1RegionData<l1extra::L1JetParticleCollection>::getEtaPhiRegions(const edm:
       int etaIndex = l1Cand.gctJetCand()->etaIndex();
       int phiIndex = l1Cand.gctJetCand()->phiIndex();

-      // Use the L1CaloGeometry to find the eta, phi bin boundaries.
-      double etaLow = l1CaloGeom.etaBinLowEdge(etaIndex);
-      double etaHigh = l1CaloGeom.etaBinHighEdge(etaIndex);
-      double phiLow = l1CaloGeom.emJetPhiBinLowEdge(phiIndex);
-      double phiHigh = l1CaloGeom.emJetPhiBinHighEdge(phiIndex);
+      // Use the CaloGeometry to find the eta, phi bin boundaries.
+      double etaLow = caloGeom.etaBinLowEdge(etaIndex);
+      double etaHigh = caloGeom.etaBinHighEdge(etaIndex);
+      double phiLow = caloGeom.emJetPhiBinLowEdge(phiIndex);
+      double phiHigh = caloGeom.emJetPhiBinHighEdge(phiIndex);

but apparently CaloGeometry doesnt have the needed functions, so that didnt work out.

But this makes me wonder that there is information taken out from this tag and then it's just all wrong about the L1T geometry in terms of the eta bins, no?

missirol commented 2 years ago

Just to summarise my current understanding

Assuming I'm not missing something, the HLT producer could/should be improved to avoid consuming the record in question unnecessarily. I personally would not rush this update for pre5 ; in my opinion it can be done by pre6. Until then, I see no alternative to reinstating the L1CaloGeometry tag.