Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Obsolete tags PCaloGeometry_Castor_v1_hlt and L1CaloGeometry_CRAFT09_hlt still consumed by code #36806

Closed
malbouis opened this issue Jan 26, 2022 · 42 comments · Fixed by #36986
Closed

Comments

@malbouis
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

srimanob commented Jan 26, 2022

@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
Copy link
Contributor

tvami commented Jan 26, 2022

assign l1

@tvami
Copy link
Contributor

tvami commented Jan 26, 2022

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

@cmsbuild
Copy link
Contributor

New categories assigned: l1

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

@tvami
Copy link
Contributor

tvami commented Jan 26, 2022

assign reconstruction

@cmsbuild
Copy link
Contributor

New categories assigned: reconstruction

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

@tvami
Copy link
Contributor

tvami commented Jan 26, 2022

assign reconstruction

for the Castor part

@francescobrivio
Copy link
Contributor

assign geometry

since this is mainly a geometry topic

@cmsbuild
Copy link
Contributor

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
Copy link
Contributor

assign reconstruction

for the Castor part

EcalDQMonitorTask is DQM, no?

@tvami
Copy link
Contributor

tvami commented Jan 26, 2022

unassign reconstruction

@tvami
Copy link
Contributor

tvami commented Jan 26, 2022

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
Copy link
Contributor

tvami commented Jan 26, 2022

assign dqm

@cmsbuild
Copy link
Contributor

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
Copy link
Contributor

tvami commented Jan 26, 2022

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

@thomreis
Copy link
Contributor

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

@makortel
Copy link
Contributor

(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
Copy link
Contributor

abhih1 commented Jan 26, 2022

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

sure.

@thomreis
Copy link
Contributor

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

@tvami
Copy link
Contributor

tvami commented Jan 26, 2022

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

@tvami
Copy link
Contributor

tvami commented Jan 26, 2022

unassign dqm

@srimanob
Copy link
Contributor

srimanob commented Feb 1, 2022

Any news on L1CaloGeometry_CRAFT09_hlt?

@tvami
Copy link
Contributor

tvami commented Feb 1, 2022

@cecilecaillol ?

@tvami
Copy link
Contributor

tvami commented Feb 2, 2022

tagging also @epalencia for the L1T part

@cvuosalo
Copy link
Contributor

cvuosalo commented Feb 3, 2022

@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
Copy link
Contributor

tvami commented Feb 3, 2022

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
Copy link
Contributor

cvuosalo commented Feb 3, 2022

@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
Copy link
Contributor

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
Copy link
Contributor

tvami commented Feb 7, 2022

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
Copy link
Contributor

bundocka commented Feb 8, 2022

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

#36916

@tvami
Copy link
Contributor

tvami commented Feb 12, 2022

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
Copy link
Contributor

srimanob commented Feb 12, 2022

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

@tvami
Copy link
Contributor

tvami commented Feb 12, 2022

@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

'hlt_data_PRef' : ['cmsDriver.py RelVal -s L1REPACK:Full --data --scenario=pp -n 10 --conditions auto:run3_hlt_PRef --relval 9000,50 --datatier "RAW" --customise=HLTrigger/Configuration/CustomConfigs.L1T --era Run3 --eventcontent RAW --fileout file:RelVal_Raw_PRef_DATA.root --filein /store/data/Commissioning2021/MinimumBias1/RAW/v1/000/346/304/00000/0949cd03-66a6-4034-a630-b9fef4dde3d2.root',

@srimanob
Copy link
Contributor

srimanob commented Feb 12, 2022

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
Copy link
Contributor

tvami commented Feb 12, 2022

Thanks @srimanob !

@srimanob
Copy link
Contributor

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
Copy link
Contributor

civanch commented Feb 14, 2022

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

@tvami
Copy link
Contributor

tvami commented Feb 14, 2022

tagging @bundocka @missirol

I follow-up a bit on #36940 (comment)

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
Copy link
Contributor

missirol commented Feb 14, 2022

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.