EventSetup consumes migration for MuonServiceProxy #28840
Conversation
This completes the migration of MuonServiceProxy to call consumes for the EventSetup products it gets. An earlier commit added support for this if a new constructor was called but the old constructor still existed. This commit migrates all clients to use the new constructor and deletes the old one. The main change is that a ConsumesCollector needs to be passed to the MuonServiceProxy constructor. Note some modules were constructing and updating a MuonServiceProxy object, but then never using it. Instead of migrating those, I deleted MuonServiceProxy from those modules. Note that this deletes the old constructor. This will break code outside the repository that uses MuonServiceProxy, but eventually support for that mode will be dropped by the Framework anyway.
|
The code-checks are being triggered in jenkins. |
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-28840/13583
|
|
A new Pull Request was created by @wddgit (W. David Dagenhart) for master. It involves the following packages: Alignment/OfflineValidation @perrotta, @Martin-Grunewald, @ssekmen, @lveldere, @kmaeshima, @schneiml, @civanch, @sbein, @christopheralanwest, @andrius-k, @mdhildreth, @cmsbuild, @franzoni, @jfernan2, @tlampen, @fioriNTU, @slava77, @tocheng, @fwyzard, @pohsun can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
|
please test |
|
The tests are being triggered in jenkins. |
|
+1 |
|
Comparison job queued. |
| enum class UseEventSetupIn { Run, Event, RunAndEvent }; | ||
|
|
||
| /// Constructor | ||
| MuonServiceProxy(const edm::ParameterSet&, edm::ConsumesCollector&&); | ||
| MuonServiceProxy(const edm::ParameterSet&, | ||
| edm::ConsumesCollector&&, | ||
| UseEventSetupIn useEventSetupIn = UseEventSetupIn::Event); |
There was a problem hiding this comment.
I'm not sure why these alternatives are needed.
The updates made per beginRun were likely made in attempt to optimize CPU use and perhaps also to cache some values.
Having, e.g. separate MagneticField per run and per event sounds rather questionable to me.
There should be just one (self)consistent ES product.
Or did I miss the subtlety in the framework and we have to declare separate tokens for the products which can be accessed in the BeginRun transition?
There was a problem hiding this comment.
@slava77 So there are two different questions here:
- Does the framework require separate tokens if a module needs to get an EventSetup product in the event and also in beginRun? Yes. There have to be separate tokens for objects retrieved in beginRun and in the Event. The Transition is a template parameter of the esConsumes function.
https://cmssdt.cern.ch/lxr/source/FWCore/Framework/interface/EDConsumerBase.h#0168
One might be able to handle it with templates, but the complexity of that seemed worse than having 2 tokens and also if you are retrieving in both beginRun and Event in the same job with the same module, then you have to have 2 tokens.
- Why are we getting objects in the run in some modules, in the event in some modules, and in both run and event in some modules? Partially the answer is I just maintained the existing behavior. That is what the modules were doing before this PR. I'm not an expert in this part of the code and I didn't want to spend time trying to understand this behavior and fix it. Actually, all the modules only call update in the event except for two modules.
A. FastSimulation/MuonSimHitProducer/src/MuonSimHitProducer.cc gets the products by calling update only during beginRun, then there are function calls using MuonServiceProxy in both beginRun and the Event. I don't know what they are doing and whether this can be changed. One also might consider changing all the other modules to have this behavior. If the EventSetup products never change in the middle of a run (and never will in the future), then this might improve performance slightly.
B. RecoMuon/TrackerSeedGenerator/plugins/TSGFromL2Muon.cc calls update both during the run and during the event. And after update is called it uses the MuonServiceProxy in both the Event and beginRun. I don't know exactly what it is doing. It is quite possible it would be good enough to only get the data during the run and delete the update call during the event. I don't know. I was trying to avoid things that might possibly change results.
If an expert who understands this part of the code gives me some direction and takes responsibility for it, I would be happy to modify the code to behave in a different way. If I just guess based on my past experience and intuition, then I would say it is probable the data is the same for a run and all the lumps and events in that run. But I don't know and am not an expert in this area in CMS. This could also be addressed in a future PR. I am really focused on the consumes migration so we gain the ability to run EventSetup modules concurrently.
|
Comparison is ready Comparison Summary:
|
|
I added #28843 to follow up on #28840 (comment) |
|
+1 |
|
+1 |
|
+1 |
|
Kind reminder for fastsim: @civanch @lveldere @mdhildreth @sbein @ssekmen |
|
Kind reminder for fastsim: @civanch @lveldere @mdhildreth @sbein @ssekmen |
|
urgent |
|
merge |
PR description:
This completes the migration of MuonServiceProxy to call
consumes for the EventSetup products it gets. An earlier
commit added support for this if a new constructor was
called, but the old constructor still existed and most clients
were still using the old constructor. This commit migrates all
clients to use the new constructor and deletes the old one.
The main change is that a ConsumesCollector needs to
be passed to the MuonServiceProxy constructor as an argument.
Another change is that MuonServiceProxy now allows calling
update at beginRun because a few modules were doing that
instead of calling update during the event.
Note some modules were constructing and updating a MuonServiceProxy
object, but then never using it. Instead of migrating those, I deleted
MuonServiceProxy from those modules.
Note that this deletes the old constructor. This will break code
outside the repository that uses MuonServiceProxy, but eventually
support for that mode will be dropped by the Framework anyway.
PR validation:
Relies on existing tests. Nothing should change in the output.