fix incomplete esconsumes migration

[ptcox]

PR description:

PR validation:

[cmsbuild]

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-34631/24218

• This PR adds an extra 12KB to repository

[cmsbuild]

A new Pull Request was created by @ptcox for master.

It involves the following packages:

• SimMuon/CSCDigitizer (simulation)

@cmsbuild, @civanch, @mdhildreth can you please review it and eventually sign? Thanks.
@dildick, @slomeo, @jhgoh this is something you requested to watch as well.
@silviodonato, @dpiparo, @qliphy, @perrotta you are the release manager for this.

cms-bot commands are listed here

[civanch]

@ptcox , please, have a look into current approach, for example, https://cmssdt.cern.ch/lxr/source/SimG4Core/Application/src/OscarMTMasterThread.cc - this may be not one to one for digitizer but is checked to be correct, also have a look into header file.

[ptcox]

I'm not sure what you mean. The entire handling of ParticleDataTable should be changed? I didn't handle the original change to esConsumes so I don't understand.

[civanch]

@ptcox , you are trying to remove a correct line but left previous, which is obsolete. The idea of this migration is to substitute ESHandle by ESToken. See recent examples: #34634, #34637.

[civanch]

@ptcox , in your case, Tokens should be defined in the class constructor via consumesCollector(), access to geometry, magnetic field, and particle table - beginRun(const edm::Run&, const edm::EventSetup& es).

[ptcox]

I really don't understand. All the tokens are already in the code. I don't see why the getHandle(token) calls are not doing the correct thing. &* to convert to a pointer to pass down into the code. It seems to me the getData line I delete in this PR is the only thing that is 'wrong' - it's left over from the code before the esConsumes tokens were added.
I understand one could use getData(token) instead of getHandle(token), and then just take & of the object. But what's wrong with getHandle?

[civanch]

@ptcox , it is working now but will be deprecated in 12_1_0. It is planned design change needed for proper multi-threading.

[ptcox]

So let me get this straight. A call like
auto pdt = eventSetup.getHandle(pdt_Token);
is deprecated? It's very hard to know what one is trying to do when I cannot find any documentation of that. For example https://twiki.cern.ch/twiki/bin/view/CMSPublic/SWGuideHowToGetDataFromES#In_ED_module
uses getHandle. And even the example ECAL PR you suggested above #34637 does too.

[ptcox]

So the minimal fix seems ok
https://hypernews.cern.ch/HyperNews/CMS/get/edmFramework/3930/1.html

[civanch]

@ptcox , my main concern is to this producer structure. There are three type of data: geometry, magnetic field, and particle data. These data are unique and shared between all threads. In the constructor of this class tokens are defined correctly. There is also digitazer as a part of this producer. Normally, access to these data should be obtained inside beginRun method using the tokens and getData() method (which you are trying to remove). In the produce method you do this each time when number of hits more than zero. This is a not needed overhead.

[ptcox]

Vladimir, This is the first time you said anything about the structure of the producer. That may well need work. What I was trying to do is remove the EXTRANEOUS call to a getData(.) which does nothing. It seems to be left from the old code in 9_4_X, for example. When I have time I'll look at the the structure. However, I was under the impression the way to access EventData IS exactly like this. It does the caching, not user code. You seem to be saying it doesn't. Regards, Tim Vladimir Ivantchenko wrote on 8/3/21 13:30:

…

@ptcox <https://github.com/ptcox> , my main concern is to this producer structure. There are three type of data: geometry, magnetic field, and particle data. These data are unique and shared between all threads. In the constructor of this class tokens are defined correctly. There is also digitazer as a part of this producer. Normally, access to these data should be obtained inside beginRun method using the tokens and getData() method (which you are trying to remove). In the produce method you do this each time when number of hits more than zero. This is a not needed overhead. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#34631 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABGYLHXOCJPA2GXGGXC5ZATT27HLRANCNFSM5BAWO6YA>. Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&utm_campaign=notification-email>.

[ptcox]

In fact
https://twiki.cern.ch/twiki/bin/view/CMSPublic/SWGuideHowToGetDataFromES#Getting_data_each_time_a_Record
says
"Accessing data from the EventSetup system is very efficient, so it is normally fine to access the data again for each new Event. However, there are times when you want to do a complex calculation only when the data in a Record has changed and then cache the results of the calculation:

The current code is not making any complex calculations and is working just like this. I think this is why there is no beginRun function. It just wasn't needed.

[civanch]

+1

OK, may I was wrong, let us merge this fix.

[cmsbuild]

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

[civanch]

please test

[cmsbuild]

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-a08460/17510/summary.html
COMMIT: 3bc03d5
CMSSW: CMSSW_12_1_X_2021-08-03-2300/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/34631/17510/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: 0 differences found in the comparisons
• DQMHistoTests: Total files compared: 39
• DQMHistoTests: Total histograms compared: 2998564
• DQMHistoTests: Total failures: 1
• DQMHistoTests: Total nulls: 0
• DQMHistoTests: Total successes: 2998541
• DQMHistoTests: Total skipped: 22
• DQMHistoTests: Total Missing objects: 0
• DQMHistoSizes: Histogram memory added: 0.0 KiB( 38 files compared)
• Checked 165 log files, 37 edm output root files, 39 DQM output files
• TriggerResults: no differences found

[perrotta]

+1

• An apparently useless line is removed