Fix modules which are directly calling DQMStore

[Dr15Jones]

The modules
https://github.com/cms-sw/cmssw/blob/09c3fce6626f70fd04223e7dacebf0b485f73f54/DQM/SiStripCommissioningSources/interface/SiStripCommissioningSource.h
https://github.com/cms-sw/cmssw/blob/09c3fce6626f70fd04223e7dacebf0b485f73f54/Validation/RecoTau/plugins/DQMFileLoader.h

are both directly calling thread-unsafe interfaces of DQMStore. These need to be converted to use the new IBooker interface or the modules should be removed.

[Dr15Jones]

assign dqm

[cmsbuild]

A new Issue was created by @Dr15Jones Chris Jones.

@davidlange6, @Dr15Jones, @smuzaffar can you please review it and eventually sign/assign? Thanks.

cms-bot commands are listed here

[cmsbuild]

New categories assigned: dqm

@vanbesien,@dmitrijus you have been requested to review this Pull request/Issue and eventually sign? Thanks

[Dr15Jones]

I found an additional module which is calling unsafe DQM interfaces:
https://github.com/cms-sw/cmssw/blob/master/CalibTracker/SiStripChannelGain/plugins/SiStripGainFromCalibTree.cc

This books and gets histograms without the lock being taken.

[Dr15Jones]

assign alca

[cmsbuild]

New categories assigned: alca

@mmusich,@cerminar,@arunhep,@franzoni,@ghellwig you have been requested to review this Pull request/Issue and eventually sign? Thanks

[mmusich]

These need to be converted to use the new IBooker interface or the modules should be removed.

@Dr15Jones can you clarify in which time-scale you foresee for the migration to happen?

SiStripGainFromCalibTree.cc is an essential ingredient for the SiStrip Gains Prompt Calibration Loop and looks like that to comply with thread-safety requirement some major refactoring of the code is needed. Is there in the plans to run the PCL multi-threaded?
@dimattia @boudoul you might want to follow this.

[Dr15Jones]

@Dr15Jones can you clarify in which time-scale you foresee for the migration to happen?

Using multiple threads for Run, LuminosityBlock processing (as well as allowing multiple Run and LuminosityBlocks to be running concurrently) is the next item to address on increasing our threading efficiency. The framework should be capable of doing that early to mid Summer, but could not be put into the release until we have modules like these fixed.

SiStripGainFromCalibTree.cc is an essential ingredient for the SiStrip Gains Prompt Calibration Loop and looks like that to comply with thread-safety requirement some major refactoring of the code is needed. Is there in the plans to run the PCL multi-threaded?

I'm strongly of the opinion that we should make it possible to run all workflows with multiple threads. If we don't, at some time we will forget and move a module to a new workflow or change an existing workflow which will lead to unknown consequences.

[dmitrijus]

Contacted @fioriNTU, the SiStripCommissioningSource should be fixed soon.

The RecoTau part I believe is not even used any more, will double check with RecoTau and hopefully remove this module.

[Dr15Jones]

@dmitrijus thanks for following up so quickly.

[dimattia]

@Dr15Jones

We cannot implement the thread safeness on SiStripGainFromCalibTree immediately because we are working on updating the monitoring histograms for the multi run harvesting. Such development is urgent and is expected to come within a couple of weeks.

Is there any documentation that explains how the thread model works in CMSSW? For an analyser, at which point of its life state (e.g. BeginJob, BeginRun, etc.) the thread forks happens? We need some infos in order to perform the fix at best.

Thanks.

[Dr15Jones]

It is best to assume that anytime your module is called, another thread may be running another module. We are not there yet, but ultimately we have to move that way to get the most out of our computational resources. It is important to remember that for 'stream', 'one' and legacy modules, the framework will only allow one thread to work with that module at a time. Therefore it is only resources shared by multiple modules (such as the DQMStore) which need to be protected.

As for documentation, you can find the outline for the design at
https://twiki.cern.ch/twiki/bin/view/CMSPublic/FWMultithreadedFrameworkTransitions

with full module documentation
https://twiki.cern.ch/twiki/bin/view/CMSPublic/FWMultithreadedFrameworkModuleTypes

[boudoul]

thanks @Dr15Jones - We should indeed plan to migrate this SiStripGainFromCalibTree as soon as we can
Gaelle -Tracker DPG

[tvami]

Hi, I was browsing open alca-pending issues and found this one. @cms-sw/trk-dpg-l2 has this been ever resolved and just the issue was not closed? Seems like #18363 touched on the topic. Thanks!

[mmusich]

@tvami indeed the issue with SiStripGainFromCalibTree has been fixed 4 (!) years ago via #18363...

[tvami]

@tvami indeed the issue with SiStripGainFromCalibTree has been fixed 4 (!) years ago via #18363...

Thanks for confirming this!

[tvami]

+alca

[tvami]

Hi @cms-sw/dqm-l2 as this issue was solved years ago, can you please sign so it can be closed? Thanks!

[jfernan2]

+1

[cmsbuild]

This issue is fully signed and ready to be closed.