[12.4.X] Added protection from multiple creation of geometry

[civanch]

PR description:

This is the backport of #38640, which fix issue #38624.

PR validation:

If this PR is a backport please specify the original PR and why you need to backport that PR. If this PR will be backported please specify to which release cycle the backport is meant for:

[cmsbuild]

A new Pull Request was created by @civanch (Vladimir Ivantchenko) for CMSSW_12_4_X.

It involves the following packages:

• CalibTracker/SiPixelLorentzAngle (alca)
• MagneticField/GeomBuilder (reconstruction)

@malbouis, @yuanchao, @clacaputo, @cmsbuild, @jpata, @tvami, @ChrisMisan, @francescobrivio can you please review it and eventually sign? Thanks.
@namapane, @tocheng, @OzAmram, @dkotlins, @ferencek, @mmusich, @tvami this is something you requested to watch as well.
@perrotta, @dpiparo, @qliphy, @rappoccio you are the release manager for this.

cms-bot commands are listed here

• Backported from Added protection from multiple creation of geometry #38640

[civanch]

please test

[mmusich]

@civanch please change the title to something more meaningful.
Also please put in the title that it's for [12.4.X] otherwise it will be confusing.

[tvami]

backport of #38640

[tvami]

type bugfix

[tvami]

urgent

• needed for AlCa operations

[namapane]

I understand this is an urgent fix and I don't want to interfere with its deployment, however I am a bit worried by a couple of things that I think will have to be addressed on a longer timescale:

1. The fix consists in caching the DDDetector as a pointer in DD4hep_VolumeBasedMagneticFieldESProducerFromDB::produce. I am not sure if I understand correctly, that the issue is the fact that the MF geometry is re-created among different runs, and you want to prevent this, right?
   One thing that is possibly not self-evident, but that should be kept in mind, is that if a job spans different runs where the magnet current changes, then the MF geometry must be re-made, since a different one is used for different nominal values (becasue data tables for eg 2T maps exist only for a simplified MF geometry). The current fix, as far as I can tell, will prevent this and I expect it to crash in such a case. It is indeed a very exotic case, since we likely do not use anything other than the 3.8T nominal current. Still I think it would be a good idea to figure out a proper solution. That should be: check that the geometry is still valid for the new configuration (based on its geometryVersion); if so keep the existing one; otherwise... we are in trouble, since the existing one must be discarded and a new one must be created.
   In any case I would recommend to add a protection at: https://github.com/cms-sw/cmssw/pull/38653/files#diff-693b9a5f442f0cc09b587f18ad986f1d2f216002d1b44548532d07cf6a222e75R195
   to check that geometryVersion matches the previous' geometry version (which should be also cached) and throw otherwise.
   At least if an exotic case like the one I'm discussing happens we will have to debug a throw instead of some mysterious crash elsewhere due to a mismatch of MF tables and geometry.

2. Unrelated to the crash, but pixel people beware: using magField->nominalValue() as a physical value as in https://github.com/cms-sw/cmssw/pull/38653/files#diff-cd4b91b1150cb8ea3ac6cdb24d49a6bd99f77a8320a5d4a60eeab3000ff42eb1R141 is not a good idea. nominalValue() is intended to be used as a label, and is truncated to int, with the corresponding loss of accuracy.
   The method inverseBzAtOriginInGeV() was added to the interface for the benefit of pixel code (Move "inverse of Bz at origin in GeV" from PixelRecoUtilities to MagneticField #35081), for example.

3. MF in SiPixelLorentzAnglePCLHarvester.cc is just used to get Bz at (0,0,0). Building the full MF geometry is not necessary for this purpose. As discussed for example here, [RFC] Optimize MagGeoBuilder with TBB #37936 (comment), I think we should figure out a way to provide Bz at (0,0,0) without building the full map for these kind of purposes. It does not make a difference for processes where the full map is required anyhow, but it will make a lot in cases like the one mentioned above.
   I am not sure how this can be technically achieved; if there is interest in this we can maybe open a discussion in an appropriate issue.

[cmsbuild]

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-48ac5f/26080/summary.html
COMMIT: 18ad6f9
CMSSW: CMSSW_12_4_X_2022-07-08-1100/el8_amd64_gcc10
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/38653/26080/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: 4 differences found in the comparisons
• DQMHistoTests: Total files compared: 50
• DQMHistoTests: Total histograms compared: 3676076
• DQMHistoTests: Total failures: 8
• DQMHistoTests: Total nulls: 0
• DQMHistoTests: Total successes: 3676046
• DQMHistoTests: Total skipped: 22
• DQMHistoTests: Total Missing objects: 0
• DQMHistoSizes: Histogram memory added: 0.0 KiB( 49 files compared)
• Checked 208 log files, 45 edm output root files, 50 DQM output files
• TriggerResults: no differences found

[tvami]

Thanks @namapane for your insights! I think those should be indeed taken care of in a next PR

[tvami]

+alca

• test pass
• future todo explained in [12.4.X] Added protection from multiple creation of geometry #38653 (comment)

[clacaputo]

+reconstruction

[cmsbuild]

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

[tvami]

@cms-sw/orp-l2 the master PR is in the 1100 IB and works fine, please merge this

[qliphy]

+1
further improvement tracked with a github issue #38669