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

Update DD4HEP GT with latest developments #35436

Closed
wants to merge 1 commit into from

Conversation

tvami
Copy link
Contributor

@tvami tvami commented Sep 28, 2021

PR description:

Following the comment #35373 (comment)
Bringing the DD4HEP GT up to date + adding the BeamSpot tags discussed in [1].
The diff wrt to the previous DD4HEP GT [2] contains

  • the cleaning of unused tags (Pixel intermediate B-field templates, superseded magnetic field geometries, unused DT reco uncertainties)
  • for Run3 MC the two records BeamSpotOnlineHLTObjectsRcd and BeamSpotOnlineLegacyObjectsRcd are supplied to Run 3 Simulation GlobalTags via the newly created tag: BeamSpotOnlineObjects_Realistic25ns_13TeVCollisions_RoundOpticsLowSigmaZ_RunBased_v1_mc (same on both records) with the same parameters as the regular BeamSpotObjectsRcd tag (BeamSpotObjects_Realistic25ns_13TeVCollisions_RoundOpticsLowSigmaZ_RunBased_v1_mc)

[1] https://hypernews.cern.ch/HyperNews/CMS/get/calibrations/4474.html

[2] https://cms-conddb.cern.ch/cmsDbBrowser/diff/Prod/gts/121X_mcRun3_2021_realistic_dd4hep_v1/120X_mcRun3_2021_realistic_dd4hep_v1

if this PR is a backport please specify the original PR and why you need to backport that PR:

This is not a backport and no backport is needed

resolves cms-AlCaDB/AlCaTools#38

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-35436/25572

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @tvami (Tamas Vami) for master.

It involves the following packages:

  • Configuration/PyReleaseValidation (pdmv, upgrade)

@jordan-martins, @bbilin, @wajidalikhan, @cmsbuild, @AdrianoDee, @srimanob, @kskovpen can you please review it and eventually sign? Thanks.
@makortel, @kpedro88, @Martin-Grunewald, @missirol, @fabiocos, @slomeo this is something you requested to watch as well.
@perrotta, @dpiparo, @qliphy you are the release manager for this.

cms-bot commands are listed here

@tvami
Copy link
Contributor Author

tvami commented Sep 28, 2021

@cmsbuild , please test

@mmusich
Copy link
Contributor

mmusich commented Sep 28, 2021

@tvami is there a 12_0_X backport?

@cmsbuild
Copy link
Contributor

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-b038b7/19163/summary.html
COMMIT: c8682e4
CMSSW: CMSSW_12_1_X_2021-09-27-2300/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/35436/19163/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

  • No significant changes to the logs found
  • Reco comparison results: 299 differences found in the comparisons
  • DQMHistoTests: Total files compared: 40
  • DQMHistoTests: Total histograms compared: 3211080
  • DQMHistoTests: Total failures: 169
  • DQMHistoTests: Total nulls: 1
  • DQMHistoTests: Total successes: 3210888
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: -0.004 KiB( 39 files compared)
  • DQMHistoSizes: changed ( 312.0 ): -0.004 KiB MessageLogger/Warnings
  • Checked 169 log files, 37 edm output root files, 40 DQM output files
  • TriggerResults: found differences in 1 / 39 workflows

@srimanob
Copy link
Contributor

Hi @tvami
The failure in comparison happens in DD4hep DB workflow. I assume it is expected from the update of BS in GT. Could you please confirm? Thx.

@@ -1104,7 +1104,7 @@ def condition(self, fragment, stepList, key, hasHarvest):
class UpgradeWorkflow_DD4hepDB(UpgradeWorkflow):
def setup_(self, step, stepName, stepDict, k, properties):
if 'Run3' in stepDict[step][k]['--era']:
stepDict[stepName][k] = merge([{'--conditions': '120X_mcRun3_2021_realistic_dd4hep_v1', '--geometry': 'DB:Extended', '--procModifiers': 'dd4hep'}, stepDict[step][k]])
stepDict[stepName][k] = merge([{'--conditions': '121X_mcRun3_2021_realistic_dd4hep_v1', '--geometry': 'DB:Extended', '--procModifiers': 'dd4hep'}, stepDict[step][k]])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tvami @cms-sw/alca-l2 as far as I can see this GlobalTag differs from the regular one (121X_mcRun3_2021_realistic_v8 as proposed at #35373) not only in terms of the geometry tags, but there is also the pixel dynamic inefficiency

$ conddb diff 121X_mcRun3_2021_realistic_dd4hep_v1 121X_mcRun3_2021_realistic_v8
[2021-09-28 09:37:11,986] INFO: Connecting to pro [frontier://PromptProd/cms_conditions]
Record                         Label     pro::121X_mcRun3_2021_realistic_dd4hep_v1 Tag     pro::121X_mcRun3_2021_realistic_v8 Tag    
-----------------------------  --------  ------------------------------------------------  ---------------------------------------   
CSCRecoDigiParametersRcd       -         CSCRECODIGI_Geometry_120DD4hepV1                  CSCRECODIGI_Geometry_112YV2               
CSCRecoGeometryRcd             -         CSCRECO_Geometry_120DD4hepV1                      CSCRECO_Geometry_112YV2                   
DTRecoGeometryRcd              -         DTRECO_Geometry_120DD4hepV1                       DTRECO_Geometry_112YV2                    
EcalPFRecHitThresholdsRcd      -         EcalPFRecHitThresholds_UL_2018_2e3sig             EcalPFRecHitThresholds_34sigma_TL235      
GEMRecoGeometryRcd             -         GEMRECO_Geometry_120DD4hepV1                      GEMRECO_Geometry_113YV4                   
GeometryFileRcd                Extended  XMLFILE_Geometry_120DD4hepV3_Extended2021_mc      XMLFILE_Geometry_120YV2_Extended2021_mc   
HcalParametersRcd              -         HCALParameters_Geometry_120DD4hepV1               HCALParameters_Geometry_112YV2            
HcalRespCorrsRcd               -         HcalRespCorrs_2021_v2.0_mc                        HcalRespCorrs_2021_v3.0_mc                
IdealGeometryRecord            -         TKRECO_Geometry_120DD4hepV1                       TKRECO_Geometry_120YV2                    
PCaloTowerRcd                  -         CTRECO_Geometry_120DD4hepV1                       CTRECO_Geometry_112YV2                    
PEcalBarrelRcd                 -         EBRECO_Geometry_120DD4hepV1                       EBRECO_Geometry_112YV2                    
PEcalEndcapRcd                 -         EERECO_Geometry_120DD4hepV1                       EERECO_Geometry_112YV2                    
PEcalPreshowerRcd              -         EPRECO_Geometry_120DD4hepV1                       EPRECO_Geometry_112YV2                    
PHcalRcd                       -         HCALRECO_Geometry_120DD4hepV1                     HCALRECO_Geometry_112YV2                  
PTrackerParametersRcd          -         TKParameters_Geometry_120DD4hepV1                 TKParameters_Geometry_112YV2              
PZdcRcd                        -         ZDCRECO_Geometry_120DD4hepV1                      ZDCRECO_Geometry_112YV2                   
RPCRecoGeometryRcd             -         RPCRECO_Geometry_120DD4hepV1                      RPCRECO_Geometry_112YV2                   
SiPixelDynamicInefficiencyRcd  -         SiPixelDynamicInefficiency_PhaseI_Run3Studies_v2  SiPixelDynamicInefficiency_PhaseI_v9      

is this intended?
Also given that presumably you want to keep it continuously on par with the regular one excepted the geometry tags, can you consider the autoCondModifiers mechanism, see:

##
## Append for 0T conditions
##
from Configuration.StandardSequences.CondDBESSource_cff import GlobalTag as essource
connectionString = essource.connect.value()
# method called in autoCond
def autoCond0T(autoCond):
ConditionsFor0T = ','.join( ['RunInfo_0T_v1_mc', "RunInfoRcd", connectionString, "", "2020-07-01 12:00:00.000"] )
GlobalTags0T = {}
for key,val in autoCond.items():
if "phase" in key: # restrict to phase1 upgrade GTs
GlobalTags0T[key+"_0T"] = (autoCond[key], ConditionsFor0T)
autoCond.update(GlobalTags0T)
return autoCond
def autoCondHLTHI(autoCond):
GlobalTagsHLTHI = {}
# emulate hybrid ZeroSuppression on the VirginRaw data of 2015
FullPedestalsForHLTHI = ','.join( ['SiStripFullPedestals_GR10_v1_hlt', "SiStripPedestalsRcd", connectionString, "", "2021-03-11 12:00:00.000"] )
MenuForHLTHI = ','.join( ['L1Menu_CollisionsHeavyIons2015_v5_uGT_xml', "L1TUtmTriggerMenuRcd", connectionString, "", "2021-03-11 12:00:00.000"] )
for key,val in autoCond.items():
if key == 'run2_hlt_relval': # modification of HLT relval GT
GlobalTagsHLTHI['run2_hlt_hi'] = (autoCond[key], FullPedestalsForHLTHI, MenuForHLTHI)
autoCond.update(GlobalTagsHLTHI)
return autoCond

@mmusich
Copy link
Contributor

mmusich commented Sep 28, 2021

@srimanob

The failure in comparison happens in DD4hep DB workflow. I assume it is expected from the update of BS in GT. Could you please confirm? Thx.

Given the diff of the two GlobalTags is:

]$ conddb diff 121X_mcRun3_2021_realistic_dd4hep_v1 120X_mcRun3_2021_realistic_dd4hep_v1
[2021-09-28 09:43:58,277] INFO: Connecting to pro [frontier://PromptProd/cms_conditions]
Record                          Label   pro::121X_mcRun3_2021_realistic_dd4hep_v1 Tag                                            pro::120X_mcRun3_2021_realistic_dd4hep_v1 Tag   
------------------------------  ------  ---------------------------------------------------------------------------------------  ---------------------------------------------   
BeamSpotOnlineHLTObjectsRcd     -       BeamSpotOnlineObjects_Realistic25ns_13TeVCollisions_RoundOpticsLowSigmaZ_RunBased_v1_mc  -                                               
BeamSpotOnlineLegacyObjectsRcd  -       BeamSpotOnlineObjects_Realistic25ns_13TeVCollisions_RoundOpticsLowSigmaZ_RunBased_v1_mc  -                                               
DTRecoUncertaintiesRcd          -       -                                                                                        DTRecoUncertainties_v1_mc                       
MFGeometryFileRcd               120812  -                                                                                        MFGeometry_120812                               
MFGeometryFileRcd               130503  -                                                                                        MFGeometry_130503                               
SiPixelDynamicInefficiencyRcd   -       SiPixelDynamicInefficiency_PhaseI_Run3Studies_v2                                         SiPixelDynamicInefficiency_PhaseI_v9            
SiPixelDynamicInefficiencyRcd   50ns    -                                                                                        SiPixelDynamicInefficiency_13TeV_50ns_v2_mc     
SiPixelTemplateDBObjectRcd      2T      -                                                                                        SiPixelTemplateDBObject_2T_v3_mc                
SiPixelTemplateDBObjectRcd      35T     -                                                                                        SiPixelTemplateDBObject_35T_v3_mc               
SiPixelTemplateDBObjectRcd      3T      -                                                                                        SiPixelTemplateDBObject_3T_v3_mc                
SiPixelTemplateDBObjectRcd      4T      -                                                                                        SiPixelTemplateDBObject_4T_v3_mc       

excepted the SiPixelDynamicInefficiencyRcd all the other changes are technical and should not affect physics.
I think that's the source of the change, which should not be there, considering the tag proposed for Run3 from the pixel group is
SiPixelDynamicInefficiency_PhaseI_v9

cf https://indico.cern.ch/event/1056952/contributions/4443596/attachments/2279635/3874179/20210712_AlCaDB_Run3CondAndCRUZET.pdf#page=6

image

@mmusich
Copy link
Contributor

mmusich commented Sep 28, 2021

-1 (fwiw)

@francescobrivio
Copy link
Contributor

-alca

@srimanob
Copy link
Contributor

@cms-sw/alca-l2 @mmusich
Thanks for suggestion and taking care of this.
I think before that we should try to understand the timeline of dd4hep and plan. I think the current plan is to make it as default for 12_1_0. If that is the case, the modifier should take care of DDD instead. But this may also include the way we handle dd4hep workflow also (i.e. use --procmodifier to trigger the change).

@mmusich
Copy link
Contributor

mmusich commented Sep 28, 2021

@srimanob whatever is decided it needs to happen fast. #35373 is by now extremely urgent so that the backport could be used for the beam test (otherwise Express will be junk).

@srimanob
Copy link
Contributor

Hi @mmusich
I think what I proposed is for longer-term. To handle the #35373 I think we have to agree on the short term, i.e. are we OK with the failure of DD4hep in the test, or temporary PR with proper DD4hep tag is introduced first. It will be frozen soon, then whatever will be introduced is after that.

@mmusich
Copy link
Contributor

mmusich commented Sep 28, 2021

the change proposed here is just a regression, so either @cms-sw/alca-l2 prepares a new physical GlobalTag now fixing the problems this one has or some ad-hoc autoCondModifiers-based symbolic GT is created. For the longer term I'll let @cms-sw/alca-l2 decide how to tackle the switch to dd4hep.

@francescobrivio
Copy link
Contributor

francescobrivio commented Sep 28, 2021

the change proposed here is just a regression, so either @cms-sw/alca-l2 prepares a new physical GlobalTag now fixing the problems this one has or some ad-hoc autoCondModifiers-based symbolic GT is created. For the longer term I'll let @cms-sw/alca-l2 decide how to tackle the switch to dd4hep.

I have created 121X_mcRun3_2021_realistic_dd4hep_v2 which is now up to date (except the Geometry tags of course) with 121X_mcRun3_2021_realistic_v8 ( diff ) and differs from the old 121X_mcRun3_2021_realistic_dd4hep_v1 only for the EcalPFRecHitThresholdsRcd and HcalRespCorrsRcd (which were not brought up to date) and restores the SiPixelDynamicInefficiencyRcd tag (SiPixelDynamicInefficiency_PhaseI_v9) see diff here.

I will open a new PR ~now with the correct GT, so I think this can be closed.

EDIT: I will handle the 120X case after that.

@francescobrivio
Copy link
Contributor

@tvami is there a 12_0_X backport?

@mmusich I have created also 120X_mcRun3_2021_realistic_dd4hep_v3 which is now in line with 120X_mcRun3_2021_realistic_v8 except the geometry tags (diff).
It only differs wrt the old 120X_mcRun3_2021_realistic_dd4hep_v1 by the BeamSpotOnline MC tag, the EcalPFRecHitThresholdsRcd and HcalRespCorrsRcd (it already contains the correct SiPixelDynamicInefficiency_PhaseI_v9 tag)

FYI @srimanob @cvuosalo: please note that there was and intermediate v2 (120X_mcRun3_2021_realistic_dd4hep_v2) which had: XMLFILE_Geometry_120DD4hepV4_Extended2021_mc to replace XMLFILE_Geometry_120DD4hepV3_Extended2021_mc.
This GT was requested in https://hypernews.cern.ch/HyperNews/CMS/get/calibrations/4446.html and provided the next day (see https://hypernews.cern.ch/HyperNews/CMS/get/calibrations/4446/1.html), but as far as I can tell it was never used in CMSSW (CMSSW search result).

Since a new extended geometry is now available (https://hypernews.cern.ch/HyperNews/CMS/get/calibrations/4475.html) and the update requested by Marco is very urgent the "XMLFILE_Geometry_120DD4hepV4_Extended2021_mc" tag was not used in the new dd4ehp GT (120X_mcRun3_2021_realistic_dd4hep_v3) and we will deal with its update at a later moment.

@mmusich
Copy link
Contributor

mmusich commented Sep 28, 2021

I have created 121X_mcRun3_2021_realistic_dd4hep_v2 which is now up to date (except the Geometry tags of course) with 121X_mcRun3_2021_realistic_v8 ( diff ) and differs from the old 121X_mcRun3_2021_realistic_dd4hep_v1 only for the EcalPFRecHitThresholdsRcd and HcalRespCorrsRcd (which were not brought up to date) and restores the SiPixelDynamicInefficiencyRcd tag (SiPixelDynamicInefficiency_PhaseI_v9) see diff here.

@francescobrivio, but in that case if we test PR #35373 with this Global Tag, there would be spurious differences coming from the ECal and HCal conditions, wouldn't it?
Wouldn't it be possible to factorize the problem and update only the conditions which are strictly necessary for #35373 to run correctly wf. 11634.912 and leave the rest of the overdue updates for later?

@francescobrivio
Copy link
Contributor

I have created 121X_mcRun3_2021_realistic_dd4hep_v2 which is now up to date (except the Geometry tags of course) with 121X_mcRun3_2021_realistic_v8 ( diff ) and differs from the old 121X_mcRun3_2021_realistic_dd4hep_v1 only for the EcalPFRecHitThresholdsRcd and HcalRespCorrsRcd (which were not brought up to date) and restores the SiPixelDynamicInefficiencyRcd tag (SiPixelDynamicInefficiency_PhaseI_v9) see diff here.

@francescobrivio, but in that case if we test PR #35373 with this Global Tag, there would be spurious differences coming from the ECal and HCal conditions, wouldn't it? Wouldn't it be possible to factorize the problem and update only the conditions which are strictly necessary for #35373 to run correctly wf. 11634.912 and leave the rest of the overdue updates for later?

You are right. I have created new GTs:

120X new GT is 120X_mcRun3_2021_realistic_dd4hep_v4 which has only the new BeamSpotOnline MC tag and the fixed SiPixelDynEff tag. All the rest is the same as v1 (see diff here), meaning that Ecal and Hcal tags have been reverted.

121X new GT is 121X_mcRun3_2021_realistic_dd4hep_v3 same description as above, see difference wrt v3 here.

@tvami
Copy link
Contributor Author

tvami commented Sep 28, 2021

@francescobrivio thanks! Should I update this PR with the new 12_1_X GT?

@mmusich
Copy link
Contributor

mmusich commented Sep 28, 2021

@tvami I am picking them up in my PR. You can close this one.

@tvami tvami closed this Sep 28, 2021
@mmusich
Copy link
Contributor

mmusich commented Sep 28, 2021

@tvami I am picking them up in my PR. You can close this one.

Done:

@tvami
Copy link
Contributor Author

tvami commented Sep 28, 2021

@tvami I am picking them up in my PR. You can close this one.

Done:

Thanks Marco!

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

Successfully merging this pull request may close these issues.

Include BeamSpot mc tag in DD4HEP GT for RelVals
5 participants