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

introduce auto:phase1_2021_dd4hep and use it in RelVals #35464

Merged
merged 1 commit into from
Oct 2, 2021

Conversation

mmusich
Copy link
Contributor

@mmusich mmusich commented Sep 29, 2021

PR description:

Implement proposal described in #35436 (comment) by creating a symbolic GlobalTag assembled on-the-fly (auto:phase1_2021_dd4hep) and putting it on par with the current auto:phase1_2021_realistic excepted the dd4hep geometry tags.
This guarantees the new key will stay up-to-date with the main 2021 realistic GT in automatic way.

PR validation:

Run successfully runTheMatrix.py -l 11634.912 -t 4 -j 8.

NB: Changes are expected since the current diff between the main 2021 Global Tag and the one used in RelVals is:

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

and these two differences:

EcalPFRecHitThresholdsRcd  -         EcalPFRecHitThresholds_34sigma_TL235     EcalPFRecHitThresholds_UL_2018_2e3sig        
HcalRespCorrsRcd           -         HcalRespCorrs_2021_v3.0_mc               HcalRespCorrs_2021_v2.0_mc                      

are re-absorbed here.

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

N/A
cc:
@cvuosalo

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-35464/25619

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @mmusich (Marco Musich) for master.

It involves the following packages:

  • Configuration/AlCa (alca)
  • Configuration/PyReleaseValidation (pdmv, upgrade)

@malbouis, @yuanchao, @jordan-martins, @bbilin, @wajidalikhan, @cmsbuild, @AdrianoDee, @srimanob, @kskovpen, @francescobrivio, @tvami can you please review it and eventually sign? Thanks.
@makortel, @kpedro88, @Martin-Grunewald, @missirol, @tocheng, @mmusich, @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

@kskovpen
Copy link
Contributor

+1

@francescobrivio
Copy link
Contributor

Thanks @mmusich this is a nice suggestion, but I'm not sure this is the way we want to go now (we should have done actually a couple of months ago as it would have been easier to maintain during the validation 😅 ). But if we now manage to switch to DD4hep in 12_1 then we could implement something like in the opposite way, i.e. add a customization to go back to DDD if needed.
(Nothing is decided yet but with are discussing with Geometry experts how to properly do it in these days.)

@mmusich
Copy link
Contributor Author

mmusich commented Sep 29, 2021

@francescobrivio what's the timeline to do the switch? 12_1_0_pre4 or 12_1_0_pre5 ?
Anyway you can recycle the infrastructure to do the reverse :)

@francescobrivio
Copy link
Contributor

@francescobrivio what's the timeline to do the switch? 12_1_0_pre4 or 12_1_0_pre5 ?

That's what we are discussing :)

Anyway you can recycle the infrastructure to do the reverse :)

Indeed! thanks a lot for the suggestion, really appreciated!

@mmusich
Copy link
Contributor Author

mmusich commented Sep 29, 2021

That's what we are discussing :)

good. If it's for pre5 this can go in ~now and you can do the switch-over later.
Incidentally this also leaves the possibility to @cvuosalo to change tags directly without bothering you at every step :)

@mmusich
Copy link
Contributor Author

mmusich commented Sep 29, 2021

assign geometry

@cmsbuild
Copy link
Contributor

New categories assigned: geometry

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

@cmsbuild
Copy link
Contributor

+1

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

The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic:

You can see more details here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-724263/19221/git-recent-commits.json
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-724263/19221/git-merge-result

Comparison Summary

Summary:

  • No significant changes to the logs found
  • Reco comparison results: 2103 differences found in the comparisons
  • DQMHistoTests: Total files compared: 40
  • DQMHistoTests: Total histograms compared: 3211080
  • DQMHistoTests: Total failures: 9560
  • DQMHistoTests: Total nulls: 1
  • DQMHistoTests: Total successes: 3201497
  • 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

@civanch
Copy link
Contributor

civanch commented Sep 29, 2021

The plan is to test DD4Hep on top of pre4 and switch to it for pre5.

@tvami
Copy link
Contributor

tvami commented Sep 29, 2021

The plan is to test DD4Hep on top of pre4 and switch to it for pre5.

@civanch but this is only true fo Run-3 MCs right? Data and Phase-2 will come later, no? What's the ETA for data change? After the 12_1_0 comes out and got validated?

@srimanob
Copy link
Contributor

As I see @tvami signs, I wonder what is the status of this PR? It is PR to merge, or it is a draft as now?
Sorry, I am confused about the status of the PR. Thanks.

@mmusich mmusich marked this pull request as ready for review September 30, 2021 05:59
@mmusich
Copy link
Contributor Author

mmusich commented Sep 30, 2021

It is PR to merge, or it is a draft as now?
Sorry, I am confused about the status of the PR. Thanks.

Hi @srimanob, it used to be a draft because I wanted to collect feedback first. Since AlCa seems to agree, it's now open for review.

@mmusich mmusich changed the title [RFC] introduce auto:phase1_2021_dd4hep and use it in RelVals introduce auto:phase1_2021_dd4hep and use it in RelVals Sep 30, 2021
@mmusich
Copy link
Contributor Author

mmusich commented Sep 30, 2021

P.s.: let me know if you wish also a backport

@francescobrivio
Copy link
Contributor

P.s.: let me know if you wish also a backport

Thanks @mmusich I don't think we need a backport for this.

@civanch
Copy link
Contributor

civanch commented Sep 30, 2021

Once more about the plan for 12_1_0_pre4:

  • for the baseline we need DDD GT for Run-3, which includes all modifications in geometry
  • we need an optional GT for Run-3 with DD4Hep

In this way, we can validate DDD geometry as the baseline versus previous versions and additionally validate DD4Hep versus DDD. The goal is to switch to DD4Hep in pre5 if the validation will converge.

@civanch
Copy link
Contributor

civanch commented Sep 30, 2021

+1

@srimanob
Copy link
Contributor

srimanob commented Oct 1, 2021

Does this PR need before another PR? Can we discuss on the plan on Monday Alca meeting before merging?

@mmusich
Copy link
Contributor Author

mmusich commented Oct 1, 2021

@srimanob this PR doesn't need any other PR before.
Also I will not be able to attend to any discussion.

@tvami
Copy link
Contributor

tvami commented Oct 1, 2021

@srimanob on the other hand there is a PR planned after this, so please sign it asap, so we can go ahead with the PR that includes the correct DD4HEP tags

@srimanob
Copy link
Contributor

srimanob commented Oct 1, 2021

Hi @tvami

What I would like to clarify is that if we switch to DD4hep in pre5, what is the migration plan? For example, how will we treat GT in near future, i.e. 12_2 with dd4hep tag as default in GT?

This PR can be merged if you would like, I am OK to sign, but then it will be another piece to change in future. If this come earlier during the update on dd4hep, I will not be slow on signing it. This is why I would like to discuss the future of this feature. I assume for GT on pre4, you can have real GT for DD4hep.

@mmusich
Copy link
Contributor Author

mmusich commented Oct 1, 2021

@srimanob If dd4hep becomes default, all the physical GlobalTags in autoCond will have to be changed and this modifier will do nothing (e.g. will reapply changes that are already there).
Nonetheless the infastructure can be re-used to create a DDD-version of the GloblaTags once dd4hep is default.

I assume for GT on pre4, you can have real GT for DD4hep.

why? This can be used as "optional GT for Run-3 with DD4Hep" for the pre4 relvals.

@tvami
Copy link
Contributor

tvami commented Oct 1, 2021

yes, @srimanob the plan is what Marco wrote. We will rename the modifier in this PR so that it takes the DDD geom after the DD4HEP becomes the default (as so will be in the default GTs)

Again as Marco said, we do not plan to have a dedicated GT for the pre4 relvals, we plan to have the correct DD4HEP tags in the follow-up of this PR and then use auto:phase1_2021_dd4hep as a GT

@tvami
Copy link
Contributor

tvami commented Oct 1, 2021

Again as Marco said, we do not plan to have a dedicated GT for the pre4 relvals, we plan to have the correct DD4HEP tags in the follow-up of this PR and then use auto:phase1_2021_dd4hep as a GT

Please note that this is actually a less error prone approach as well, this way the differences between the DD4HEP GT and the standard GT will really just be the geometry tags, and other changes will be factored out (like what happened in the past with HCAL response and ECAL PF thresholds)

And since we wanted to have the changes in two steps, merging this is urgent, so the new PR can include the tags that are indeed should be included in the pre4 release on Tuesday

@srimanob
Copy link
Contributor

srimanob commented Oct 1, 2021

+upgrade

This PR introduces the way to make dd4hep GT on-the-fly from auto:phase1_2021_realistic Alca plans to use this way in dd4hep GT of pre4. This method will be reviewed when dd4hep will become available by default, to remove or to remake for DDD instead.

@cmsbuild
Copy link
Contributor

cmsbuild commented Oct 1, 2021

This pull request is fully signed and it will be integrated in one of the next master IBs (tests are also fine). This pull request will now be reviewed by the release team before it's merged. @perrotta, @dpiparo, @qliphy (and backports should be raised in the release meeting by the corresponding L2)

@perrotta
Copy link
Contributor

perrotta commented Oct 2, 2021

+1

  • It allows switching easily between DD4Hep and DDD geometries
  • Observed changes are only in the DD4Hep workflows, as expected
  • Please, once this merged proceed with the update of the tags so that the DD4hep workflows can use the new 121DD4hepV1 tags already in pre4, for testing: @mmusich @cvuosalo @tvami ...

@cmsbuild cmsbuild merged commit c9e62f9 into cms-sw:master Oct 2, 2021
@mmusich mmusich deleted the ddh4epGlobalTagInModifiers branch October 2, 2021 06:04
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.

None yet

10 participants