Issues in concurrent Lumi/IOV safety in DQMOneEDAnalyzer<LuminosityBlockCache<...>> #37163

makortel · 2022-03-08T00:43:08Z

This issue follows up my comment in #25090 (comment). Below is a list of DQMOneEDAnalyzer modules that make use of edm::LuminosityCache

On quick look some of them do not seem to be safe wrt concurrent lumis or IOVs. In the comments I'm going to write some notes about them.

The text was updated successfully, but these errors were encountered:

makortel · 2022-03-08T00:43:15Z

assign core,dqm

cmsbuild · 2022-03-08T00:43:24Z

New categories assigned: core,dqm

@jfernan2,@ahmad3213,@rvenditti,@Dr15Jones,@smuzaffar,@emanueleusai,@makortel,@pbo0,@pmandrik you have been requested to review this Pull request/Issue and eventually sign? Thanks

cmsbuild · 2022-03-08T00:43:26Z

A new Issue was created by @makortel Matti Kortelainen.

@Dr15Jones, @perrotta, @dpiparo, @makortel, @smuzaffar, @qliphy can you please review it and eventually sign/assign? Thanks.

cms-bot commands are listed here

makortel · 2022-03-08T01:06:07Z

In DQM/BeamMonitor/plugins/AlcaBeamMonitor.h the problem is that per-LuminosityBlock data is stored in class member data instead of the LuminosityBlockCache (which actually is empty!). For example, the globalBeginLuminosityBlock()

cmssw/DQM/BeamMonitor/plugins/AlcaBeamMonitor.cc

Lines 227 to 232 in cbca23d

    
           std::shared_ptr<alcabeammonitor::NoCache> AlcaBeamMonitor::globalBeginLuminosityBlock(const LuminosityBlock& iLumi, 
        
                                                                                                 const EventSetup& iSetup) const { 
        
             // Always create a beamspot group for each lumi weather we have results or not! Each Beamspot will be of unknown type! 
        
             vertices_.clear(); 
        
             beamSpotsMap_.clear();

clears an std::vector<reco::VertexCollection> vertices_ and BeamSpotContainer beamSpotsMap_ (which is std::map<std::string, reco::BeamSpot> ).

In short, all lumi-dependent data should be moved into the LuminosityBlockCache.

The vertices_ is filled in analyze()

cmssw/DQM/BeamMonitor/plugins/AlcaBeamMonitor.cc

Lines 297 to 300 in cbca23d

    
           Handle<VertexCollection> PVCollection; 
        
           if (iEvent.getByToken(primaryVertexLabel_, PVCollection)) { 
        
             vertices_.push_back(*PVCollection.product()); 
        
           }

and used in globalEndLuminosityBlock()

cmssw/DQM/BeamMonitor/plugins/AlcaBeamMonitor.cc

Line 363 in cbca23d

    
           for (vector<VertexCollection>::iterator itPV = vertices_.begin(); itPV != vertices_.end(); itPV++) {

The problem with this code is perhaps easiest demonstrated with e.g. a following sequence

beginLumi 1
Event 1:1
Event 1:2
beginLumi 2 # resets the `vertices_`
Event 2:1 
Event 2:2
Event 1:3
endLumi 1  # vertices_ contains now data from Lumis 1 and 2
beginLumi 3
...

The beamspotsMap_ is filled in globalBeginLuminosityBlock(), analyze(), and in globalEndLuminosityBlock(), and used in analyze() and globalEndLuminosityBlock() such that the value filled in globalBeginLuminosityBlock() is used in analyze(). The following sequence would lead to incorrect results

beginLumi 1
Event 1:1
Event 1:2
beginLumi 2, IOV of BeamSpotObjectsRcd changes
Event 1:3 # beamspotsMap_["DB"] has BeamSpot from Lumi 2
Event 2:1
endLumi 1 # beamspotsMap_["SC"] contains BeamSpot from Lumi 2; theBeamFitter and thePVFitter contain data from both Lumis(?)

Not related, but what is the aim of this fragment?

cmssw/DQM/BeamMonitor/plugins/AlcaBeamMonitor.cc

Lines 304 to 310 in cbca23d

    
           Handle<BeamSpot> recoBeamSpotHandle; 
        
           try { 
        
             iEvent.getByToken(scalerLabel_, recoBeamSpotHandle); 
        
           } catch (cms::Exception& exception) { 
        
             LogInfo("AlcaBeamMonitor") << exception.what(); 
        
             return; 
        
           }

Event::getByToken() does not throw any exception that would make sense for user code to catch. If the aim is to check if the input product exists, that could be done e.g. along

if (Handle<BeamSpot> recoBeamSpotHandle = iEvent.getHandle(scalerLabel_)) {
  // product exists
} else {
  // product doesn't exist
}

makortel · 2022-03-08T01:15:21Z

In DQM/BeamMonitor/plugins/OnlineBeamMonitor.h the issue is only with beamSpotsMap_, and is similar to the issue with the similarly named member variable in #37163 (comment).

In addition, about the other mutable member variables

cmssw/DQM/BeamMonitor/plugins/OnlineBeamMonitor.h

Lines 60 to 61 in 2105cce

    
           mutable int numberOfProcessedLumis_; 
        
           mutable std::vector<int> processedLumis_;

the mutable could be removed from processedLumis_ by filling it in globalEndLuminosityBlock(), and numberOfProcessedLumis_ appears to be unused.

gennai · 2023-05-15T11:06:16Z

Dear @makortel @francescobrivio @mmusich , I am working now on this issue (sorry to be taken so long).
I have followed the suggestion by @makortel about the lumiCache and I have prepared a new branch with the updated code:
https://github.com/MilanoBicocca-pix/cmssw/tree/lumiCacheForBeamSpot
I hope to test it asap. I will then also fix the other things discussed in the issue

makortel · 2023-05-16T18:02:49Z

I should probably resume reviewing the modules listed in the issue description...

makortel · 2023-06-05T12:35:43Z

The DQM/CTPPS/plugins/CTPPSDiamondDQMSource.cc stores some counters in member data in PotPlots

cmssw/DQM/CTPPS/plugins/CTPPSDiamondDQMSource.cc

Line 153 in b210401

    
           unsigned int HitCounter, MHCounter, LeadingOnlyCounter, TrailingOnlyCounter, CompleteCounter;

and ChannelPlots

cmssw/DQM/CTPPS/plugins/CTPPSDiamondDQMSource.cc

Line 206 in b210401

    
           unsigned int HitCounter, MHCounter, LeadingOnlyCounter, TrailingOnlyCounter, CompleteCounter;

nested structs. These counters are incremented in the analyze()

cmssw/DQM/CTPPS/plugins/CTPPSDiamondDQMSource.cc

Lines 822 to 839 in b210401

    
           if (digi.leadingEdge() != 0 || digi.trailingEdge() != 0) { 
        
             ++potPlots_[detId_pot].HitCounter; 
        
             if (digi.leadingEdge() != 0) { 
        
               potPlots_[detId_pot].leadingEdgeCumulative_all->Fill(HPTDC_BIN_WIDTH_NS * digi.leadingEdge()); 
        
             } 
        
             if (digi.leadingEdge() != 0 && digi.trailingEdge() == 0) { 
        
               ++potPlots_[detId_pot].LeadingOnlyCounter; 
        
               potPlots_[detId_pot].leadingEdgeCumulative_le->Fill(HPTDC_BIN_WIDTH_NS * digi.leadingEdge()); 
        
             } 
        
             if (digi.leadingEdge() == 0 && digi.trailingEdge() != 0) { 
        
               ++potPlots_[detId_pot].TrailingOnlyCounter; 
        
               potPlots_[detId_pot].trailingEdgeCumulative_te->Fill(HPTDC_BIN_WIDTH_NS * digi.trailingEdge()); 
        
             } 
        
             if (digi.leadingEdge() != 0 && digi.trailingEdge() != 0) { 
        
               ++potPlots_[detId_pot].CompleteCounter; 
        
               // potPlots_[detId_pot].leadingTrailingCorrelationPot->Fill(HPTDC_BIN_WIDTH_NS * digi.leadingEdge(), 
        
               //                                                          HPTDC_BIN_WIDTH_NS * digi.trailingEdge()); 
        
             }

cmssw/DQM/CTPPS/plugins/CTPPSDiamondDQMSource.cc

Lines 1210 to 1223 in b210401

    
           if (digi.leadingEdge() != 0 || digi.trailingEdge() != 0) { 
        
             ++channelPlots_[detId].HitCounter; 
        
             if (digi.trailingEdge() == 0) { 
        
               ++channelPlots_[detId].LeadingOnlyCounter; 
        
               channelPlots_[detId].leadingEdgeCumulative_le->Fill(HPTDC_BIN_WIDTH_NS * digi.leadingEdge()); 
        
             } else if (digi.leadingEdge() == 0) { 
        
               ++channelPlots_[detId].TrailingOnlyCounter; 
        
               channelPlots_[detId].trailingEdgeCumulative_te->Fill(HPTDC_BIN_WIDTH_NS * digi.trailingEdge()); 
        
             } else { 
        
               ++channelPlots_[detId].CompleteCounter; 
        
               // channelPlots_[detId].LeadingTrailingCorrelationPerChannel->Fill(HPTDC_BIN_WIDTH_NS * digi.leadingEdge(), 
        
               //                                                                 HPTDC_BIN_WIDTH_NS * digi.trailingEdge()); 
        
             } 
        
           }

and are then used in globalEndLuminosityBlock() to fill some histograms

cmssw/DQM/CTPPS/plugins/CTPPSDiamondDQMSource.cc

Lines 1295 to 1301 in b210401

    
           double HundredOverHitCounter = .0; 
        
           if (plot.second.HitCounter != 0) 
        
             HundredOverHitCounter = 100. / plot.second.HitCounter; 
        
           plot.second.HPTDCErrorFlags->setBinContent(16, HundredOverHitCounter * plot.second.MHCounter); 
        
           plot.second.leadingWithoutTrailing->setBinContent(1, HundredOverHitCounter * plot.second.LeadingOnlyCounter); 
        
           plot.second.leadingWithoutTrailing->setBinContent(2, HundredOverHitCounter * plot.second.TrailingOnlyCounter); 
        
           plot.second.leadingWithoutTrailing->setBinContent(3, HundredOverHitCounter * plot.second.CompleteCounter);

cmssw/DQM/CTPPS/plugins/CTPPSDiamondDQMSource.cc

Lines 1305 to 1313 in b210401

    
           double HundredOverHitCounterPot = 0.; 
        
           if (plot.second.HitCounter != 0) 
        
             HundredOverHitCounterPot = 100. / plot.second.HitCounter; 
        
           plot.second.leadingWithoutTrailingCumulativePot->setBinContent( 
        
               1, HundredOverHitCounterPot * plot.second.LeadingOnlyCounter); 
        
           plot.second.leadingWithoutTrailingCumulativePot->setBinContent( 
        
               2, HundredOverHitCounterPot * plot.second.TrailingOnlyCounter); 
        
           plot.second.leadingWithoutTrailingCumulativePot->setBinContent( 
        
               3, HundredOverHitCounterPot * plot.second.CompleteCounter);

In the presence of concurrent lumis these counters may have counts from other lumis than the one the globalEndLuminosityBlock() is called for. Assuming this is important, the counters should be moved to the LuminosityBlockCache. Digging in history it looks like these counters were missed in #25188.

makortel · 2023-06-05T13:12:48Z

DQM/DTMonitorModule/interface/DTDataIntegrityTask.h had an issue that I felt was easier to just fix, see #41874

makortel · 2023-06-06T09:00:00Z

The DQM/HcalTasks/interface/RecHitTask.h class has

cmssw/DQM/HcalTasks/interface/RecHitTask.h

Line 120 in 57ceb23

bool _unknownIdsPresent;

which is set to false in bookHistograms()

cmssw/DQM/HcalTasks/plugins/RecHitTask.cc

Line 411 in 57ceb23

_unknownIdsPresent = false;

and

cmssw/DQM/HcalTasks/plugins/RecHitTask.cc

Lines 414 to 417 in 57ceb23

    
           /* virtual */ void RecHitTask::_resetMonitors(hcaldqm::UpdateFreq uf) { 
        
             switch (uf) { 
        
               case hcaldqm::f1LS: 
        
                 _unknownIdsPresent = false;

(AFAICT both are effectively at beginRun)

The variable is set to true in _process() (i.e. in event processing) in

cmssw/DQM/HcalTasks/plugins/RecHitTask.cc

Lines 596 to 600 in 57ceb23

    
           if (rawid == 0) { 
        
             meUnknownIds1LS->Fill(1); 
        
             _unknownIdsPresent = true; 
        
             continue; 
        
           }

and

cmssw/DQM/HcalTasks/plugins/RecHitTask.cc

Lines 685 to 689 in 57ceb23

    
           if (rawid == 0) { 
        
             meUnknownIds1LS->Fill(1); 
        
             _unknownIdsPresent = true; 
        
             continue; 
        
           }

It is then used in globalEndLuminosityBlock()

cmssw/DQM/HcalTasks/plugins/RecHitTask.cc

Lines 842 to 845 in 57ceb23

    
           if (_unknownIdsPresent) 
        
             _vflags[fUnknownIds]._state = flag::fBAD; 
        
           else 
        
             _vflags[fUnknownIds]._state = flag::fGOOD;

With concurrent lumis it can happen that for a lumi 1 the condition to set _unknownIdsPresent=true does not happen, but in the following lumi 2 it happens, and the _unknownIdsPresent is set to true before the globalEndLuminosityBlock() is called for lumi 1, leading to flag::fBAD being reported for lumi 1.

cmsbuild added core-pending dqm-pending pending-signatures labels Mar 8, 2022

francescobrivio mentioned this issue May 16, 2023

Fix LumiCache in AlcaBeamMonitor and OnlineBeamMonitor #41700

Merged

makortel mentioned this issue Jun 5, 2023

Move events/LS counter to LuminosityBlockCache in DTDataIntegrityTask #41874

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issues in concurrent Lumi/IOV safety in DQMOneEDAnalyzer<LuminosityBlockCache<...>> #37163

Issues in concurrent Lumi/IOV safety in DQMOneEDAnalyzer<LuminosityBlockCache<...>> #37163

makortel commented Mar 8, 2022 •

edited

makortel commented Mar 8, 2022

cmsbuild commented Mar 8, 2022

cmsbuild commented Mar 8, 2022

makortel commented Mar 8, 2022

makortel commented Mar 8, 2022

gennai commented May 15, 2023 •

edited

makortel commented May 16, 2023

makortel commented Jun 5, 2023

makortel commented Jun 5, 2023

makortel commented Jun 6, 2023

Issues in concurrent Lumi/IOV safety in DQMOneEDAnalyzer<LuminosityBlockCache<...>> #37163

Issues in concurrent Lumi/IOV safety in DQMOneEDAnalyzer<LuminosityBlockCache<...>> #37163

Comments

makortel commented Mar 8, 2022 • edited

makortel commented Mar 8, 2022

cmsbuild commented Mar 8, 2022

cmsbuild commented Mar 8, 2022

makortel commented Mar 8, 2022

makortel commented Mar 8, 2022

gennai commented May 15, 2023 • edited

makortel commented May 16, 2023

makortel commented Jun 5, 2023

makortel commented Jun 5, 2023

makortel commented Jun 6, 2023

makortel commented Mar 8, 2022 •

edited

gennai commented May 15, 2023 •

edited