Added protection from multiple creation of geometry

[civanch]

PR description:

There are the crash discussed in #38624, which happens for alca run. The problem happens because in this run geometry was created twice. The fix is done in two places:

1. inside SiPixelLorentzAnglePCLHarvester value of the magnetic field is accessed just after access to the EventSetup and stored in local class member. Likely this change has no effect but logically is more safe.
2. inside DD4hep_VolumeBasedMagneticFieldESProducerFromDB a new class member added - pointer to geometry. Added a check, which guaranteed that the geometry is created only once.

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:

should be also in 12_3 and 12_4

[cmsbuild]

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-38640/30928

• This PR adds an extra 20KB to repository

[cmsbuild]

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

It involves the following packages:

• CalibTracker/SiPixelLorentzAngle (alca)

@malbouis, @yuanchao, @cmsbuild, @francescobrivio, @ChrisMisan, @tvami can you please review it and eventually sign? Thanks.
@tocheng, @OzAmram, @ferencek, @mmusich, @dkotlins, @tvami 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

[civanch]

please test

[mmusich]

assign trk-dpg

[cmsbuild]

New categories assigned: trk-dpg

@connorpa,@mmusich,@tsusa you have been requested to review this Pull request/Issue and eventually sign? Thanks

[mmusich]

-1

• tested with the config provided at Segfault in dd4hep geometry when changing runs #38624 and it does NOT fix.

[tvami]

@cmsbuild , please abort

[tvami]

-1

• see discussion in the 12_3_X backport

[mmusich]

-1

I mean the change per se is OK, though I would prefer to push it together with some other clean-up later (once the main issue is solved)

[tvami]

type bug-fix

[cmsbuild]

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-ff942f/26079/summary.html
COMMIT: 9d3abc4
CMSSW: CMSSW_12_5_X_2022-07-07-2300/el8_amd64_gcc10
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/38640/26079/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: 10 differences found in the comparisons
• DQMHistoTests: Total files compared: 50
• DQMHistoTests: Total histograms compared: 3654999
• DQMHistoTests: Total failures: 20
• DQMHistoTests: Total nulls: 0
• DQMHistoTests: Total successes: 3654957
• 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

[mmusich]

+1

• with the understanding that the caveats at [12.4.X] Added protection from multiple creation of geometry #38653 (comment) should be dealt with in a later PR

[tvami]

+alca

[clacaputo]

+reconstruction

[cvuosalo]

Suggested change

	cms::DDDetector* detector_{nullptr};
	std::unique_ptr<cms::DDDetector> detector_{nullptr};

unique_ptr is preferred over a bare pointer. Then the delete in the destructor is not needed.

[tvami]

@cms-sw/orp-l2 can we merge this for 1100 IB?
Obviously there is a lot of improvements to be done both suggestions from Nicola and Carl, but these are code improvements, worries for exotic cases, so I think for 12_4_2 12_4_3 we can go on with this PR (well it's backport oc), and then whenever the perfect solutions comes it comes. What do you think?

[perrotta]

@cms-sw/orp-l2 can we merge this for 1100 IB? Obviously there is a lot of improvements to be done both suggestions from Nicola and Carl, but these are code improvements, worries for exotic cases, so I think for 12_4_2 we can go on with this PR (well it's backport oc), and then whenever the perfect solutions comes it comes. What do you think?

FYI: CMSSW_12_4_2-was cut two days ago, see e.g. https://cms-talk.web.cern.ch/t/production-release-cmssw-12-4-2-now-available/12579
This may be entitled to enter 12_4_next though

[tvami]

yes, too many versions, I meant 12_4_3 or 12_4_2_patch1

[tvami]

Anyway @perrotta do didnt seem to address my comments. What's the plan given that the PR is fully signed?

[perrotta]

Anyway @perrotta do didnt seem to address my comments. What's the plan given that the PR is fully signed?

The plan must take into account that today is Saturday, ICHEP is ongoing, and we already rushed to create quite a few releases in the past days.
Besides that, I conceptually don't see possible counterindications in merging this patched protection, also in the current data taking cycles. Let me find some time to go through the whole thread and the code itself, and it will likely get merged soon.

[perrotta]

+1

• Patch that can get rid of multiple creations of geometries
• The comments in [12.4.X] Added protection from multiple creation of geometry #38653 (comment) are relevant, but as far as I understand they refere to a possible but rather exotic scenario: I've opened the github issue Make a more robust fix to deal with possible multiple creations of the MF geometries #38669 to keep track of them

[tvami]

Thanks Andrea for merging this (on a Sat and during ICHEP ) -- much appreciated!

[Dr15Jones]

This simple test seems like a bad idea. We also need to be sure this is the correct geometry to use as well.