Improve ExternalGeneratorFilter to handle a stream skipping a lumi #43783

wddgit · 2024-01-24T22:02:14Z

PR description:

Improve ExternalGeneratorFilter to handle a stream skipping a lumi. Allowing a stream to skip a lumi is a proposed new Framework behavior. See PR #43522, which is not merged yet. This PR is part of an effort to improve code outside the Framework so all modules will be able properly handle the new Framework behavior.

The previous version of the code in this module selected a particular stream to use to run beginLumiProduce in globalBeginLuminosityBlockProduce. For other streams, beginLumiProduce is called in streamBeginLuminosityBlock.

On the selected stream, the pointer to the stream cache is set to null in streamBeginLuminosityBlock. If the stream skips the lumi, then that never happens. Then the compare_exchange_strong in streamEndLuminosityBlockSummary never executes because it is expecting the value to be null and it is not. The same stream cache would be used for the next globalBeginLuminosityBlock. That stream might not be done yet with the preceding lumi. And at that point we would be in a bad state.

There is a second pre-existing problem unrelated to a stream skipping a lumi. This problem probably happens very rarely or never. After a global begin lumi transition occurs and before the selected stream runs streamBeginLuminosityBlock and sets the stream cache pointer to null, a different stream could finish and try to move to the next lumi. The next global begin lumi will wait. That wait depends on the stream cache pointer being set to null before you get there. If that pointer has not been set null yet, then the next lumi could start early and then we would also be in a bad state. This problem could occur even without a stream skipping a lumi.

The fixes in this PR address both issues.

There was also a compiler warning about a std::move call that accomplishes nothing and I fixed that also (one line).

PR validation:

There are some nice and thorough unit tests of this module in a different package, GeneratorInterface/Pythia8Interface. I checked that package out also and ran those tests and they passed. I also temporarily edited the code in my working area to force it to use the new code path in globalEndLuminosityBlockProduce and ran the same unit tests again and they still pass.

cmsbuild · 2024-01-24T22:02:41Z

cms-bot internal usage

cmsbuild · 2024-01-24T22:08:56Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-43783/38536

This PR adds an extra 16KB to repository

cmsbuild · 2024-01-24T22:09:17Z

A new Pull Request was created by @wddgit (W. David Dagenhart) for master.

It involves the following packages:

GeneratorInterface/Core (generators)

@SiewYan, @mkirsano, @cmsbuild, @menglu21, @bbilin, @GurpreetSinghChahal, @alberto-sanchez can you please review it and eventually sign? Thanks.
@alberto-sanchez, @mkirsano this is something you requested to watch as well.
@sextonkennedy, @antoniovilela, @rappoccio you are the release manager for this.

cms-bot commands are listed here

wddgit · 2024-01-24T22:10:53Z

please test

FYI @makortel @Dr15Jones

wddgit · 2024-01-24T22:22:30Z

Some extra explanation of how this fix works. The new code in the global end lumi transition is only used when the selected stream skips the lumi, which is probably a somewhat rare thing. In that case it takes care of the actions normally handled in streamEndLuminosityBlockSummary.

makortel · 2024-01-25T00:02:57Z

GeneratorInterface/Core/plugins/ExternalGeneratorFilter.cc

@@ -287,13 +287,14 @@ void ExternalGeneratorFilter::streamEndRun(edm::StreamID iID, edm::Run const& iR
  }
 }
 void ExternalGeneratorFilter::globalEndRunProduce(edm::Run& iRun, edm::EventSetup const&) const {
-  iRun.emplace(runInfoToken_, std::move(runCache(iRun.index())->runInfo_));
+  iRun.emplace(runInfoToken_, runCache(iRun.index())->runInfo_);


For anyone else who was wondering the removal of the std::move, the GenRunInfoProduct consists of 7 doubles, so moving brings no benefits over copying.

cmsbuild · 2024-01-25T00:51:33Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-12008b/37028/summary.html
COMMIT: 65b37cf
CMSSW: CMSSW_14_0_X_2024-01-24-1100/el8_amd64_gcc12
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/43783/37028/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

You potentially removed 15 lines from the logs
Reco comparison results: 58 differences found in the comparisons
DQMHistoTests: Total files compared: 48
DQMHistoTests: Total histograms compared: 3247482
DQMHistoTests: Total failures: 6
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3247454
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 47 files compared)
Checked 200 log files, 161 edm output root files, 48 DQM output files
TriggerResults: no differences found

Dr15Jones · 2024-01-29T20:01:08Z

GeneratorInterface/Core/plugins/ExternalGeneratorFilter.cc

@@ -155,6 +155,11 @@ namespace externalgen {
    //Only stream 0 sets this at stream end Lumi and it is read at global end Lumi


Is this comment still correct?

Good question. The comment is definitely wrong and I will delete it. I'm still looking at this. I think I can make the data member that follows const or constexpr...

I deleted the comment and changed randomState_ in the LumiCache from CMS_THREAD_SAFE mutable to const.

Dr15Jones · 2024-01-29T20:08:16Z

GeneratorInterface/Core/plugins/ExternalGeneratorFilter.cc

  }
+  state_.store(State::kNotReadyForNextGlobalBeginLumi);


This should probably be a compare_exchange_strong just incase there is a race for 2 different Lumis calling globalBeginLuminosityBlockProduce. (maybe one Lumi is empty and the next has something and the empty Lumi call was delayed by the OS?)

All the globalBeginLuminosityBlockProduce function calls for a LuminosityBlock (and the other functions in the global begin lumi transition) must complete before the EventProcessor can move to the next lumi. The EventProcessor does not even look to see what is next before the entire transition for a lumi is complete. We have to get to the call to handleNextEventForStreamAsync for another lumi to start. See:

https://cmssdt.cern.ch/dxr/CMSSW/source/FWCore/Framework/src/EventProcessor.cc#1742

I think this is OK.

Dr15Jones · 2024-01-29T20:16:49Z

GeneratorInterface/Core/plugins/ExternalGeneratorFilter.cc

@@ -341,16 +340,25 @@ void ExternalGeneratorFilter::streamEndLuminosityBlockSummary(edm::StreamID iID,
                                                              GenLumiInfoProduct* iProduct) const {
  iProduct->mergeProduct(*streamCache(iID)->endLumiProduce(iLuminosityBlock.run()));

-  if (lastLumiIndex_ == iLuminosityBlock.index()) {
+  if (!luminosityBlockCache(iLuminosityBlock.index())->selectedStreamTransitionsCompleted_) {


This is a race condition. Two streams could be calling streamEndLuminosityBlockSummary at the same time, one could be trying to read this value at the same time as one is on line 346 changing the value.

According to our documentation (https://twiki.cern.ch/twiki/bin/view/CMSPublic/FWMultithreadedFrameworkGlobalModuleInterface#edm_LuminosityBlockSummaryCacheT)

The framework guarantees that only one streamEndLuminosityBlockSummary is called at a time

I agree with Matti. There is a mutex lock around those function calls:

https://cmssdt.cern.ch/dxr/CMSSW/source/FWCore/Framework/interface/global/implementors.h#267

Since it is in a lumi cache, different lumis will use different instances and different memory locations. No conflict there. And also globalEndLuminosityBlock will execute after these functions are called. This variable isn't accessed or modified anywhere else so it should be OK.

Dr15Jones · 2024-01-29T20:24:12Z

GeneratorInterface/Core/plugins/ExternalGeneratorFilter.cc

  }
 }

 void ExternalGeneratorFilter::globalEndLuminosityBlockProduce(edm::LuminosityBlock& iLuminosityBlock,
                                                              edm::EventSetup const&,
                                                              GenLumiInfoProduct const* iProduct) const {
+  if (!luminosityBlockCache(iLuminosityBlock.index())->selectedStreamTransitionsCompleted_) {


I believe a comment explaining why this is needed would be helpful.

I added a comment. I think that helps. Thanks.

cmsbuild · 2024-01-30T18:33:37Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-12008b/37112/summary.html
COMMIT: c894f45
CMSSW: CMSSW_14_0_X_2024-01-30-1100/el8_amd64_gcc12
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/43783/37112/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

You potentially added 14 lines to the logs
Reco comparison results: 60 differences found in the comparisons
DQMHistoTests: Total files compared: 48
DQMHistoTests: Total histograms compared: 3248614
DQMHistoTests: Total failures: 1189
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3247403
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 47 files compared)
Checked 200 log files, 161 edm output root files, 48 DQM output files
TriggerResults: no differences found

cmsbuild · 2024-02-06T10:11:06Z

Milestone for this pull request has been moved to CMSSW_14_1_X. Please open a backport if it should also go in to CMSSW_14_0_X.

makortel · 2024-02-08T19:36:40Z

From core point of view this PR looks good now. @cms-sw/generators-l2 could you review and sign? Thanks!

makortel · 2024-02-15T21:37:41Z

@cms-sw/generators-l2 Could you please review and sign? Thanks!

makortel · 2024-02-19T14:43:29Z

@cms-sw/generators-l2 ping

makortel · 2024-02-22T22:09:47Z

@cms-sw/generators-l2 ping

makortel · 2024-02-23T21:07:28Z

@cms-sw/generators-l2 ping

makortel · 2024-02-26T15:11:02Z

@cms-sw/generators-l2 ping

menglu21 · 2024-02-27T11:30:18Z

+1

cmsbuild · 2024-02-27T11:30:44Z

This pull request is fully signed and it will be integrated in one of the next master IBs (tests are also fine). This pull request will now be reviewed by the release team before it's merged. @rappoccio, @sextonkennedy, @antoniovilela (and backports should be raised in the release meeting by the corresponding L2)

antoniovilela · 2024-02-29T11:12:42Z

+1

cmsbuild added this to the CMSSW_14_0_X milestone Jan 24, 2024

cmsbuild added generators-pending pending-signatures tests-pending orp-pending code-checks-pending labels Jan 24, 2024

cmsbuild added code-checks-approved and removed code-checks-pending labels Jan 24, 2024

cmsbuild added tests-started and removed tests-pending labels Jan 24, 2024

makortel mentioned this pull request Jan 24, 2024

Improve ExternalGeneratorFilter cms-sw/framework-team#799

Closed

makortel reviewed Jan 25, 2024

View reviewed changes

cmsbuild added tests-approved and removed tests-started labels Jan 25, 2024

Dr15Jones reviewed Jan 29, 2024

View reviewed changes

Improve ExternalGeneratorFilter to handle a stream skipping a lumi

c894f45

wddgit force-pushed the improveExternalGeneratorFilterSkippingALumi branch from 65b37cf to c894f45 Compare January 30, 2024 15:46

cmsbuild added tests-pending code-checks-pending and removed tests-approved code-checks-approved labels Jan 30, 2024

cmsbuild added tests-approved and removed tests-started labels Jan 30, 2024

cmsbuild modified the milestones: CMSSW_14_0_X, CMSSW_14_1_X Feb 6, 2024

cmsbuild modified the milestones: CMSSW_14_0_X, CMSSW_14_1_X Feb 8, 2024

cmsbuild added generators-approved fully-signed and removed generators-pending pending-signatures labels Feb 27, 2024

cmsbuild added orp-approved and removed orp-pending labels Feb 29, 2024

cmsbuild merged commit 25a0225 into cms-sw:master Feb 29, 2024
11 checks passed

This was referenced Feb 29, 2024

Fix some minor RooFit user errors #44069

Merged

Reduce complexity of erasing elements from vectors in PATTauHybridProducer #44266

Merged

Small fix in HeterogeneousCore/AlpakaCore/README.md #44279

Merged

wddgit deleted the improveExternalGeneratorFilterSkippingALumi branch March 7, 2024 19:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve ExternalGeneratorFilter to handle a stream skipping a lumi #43783

Improve ExternalGeneratorFilter to handle a stream skipping a lumi #43783

wddgit commented Jan 24, 2024

cmsbuild commented Jan 24, 2024 •

edited

Loading

cmsbuild commented Jan 24, 2024

cmsbuild commented Jan 24, 2024

wddgit commented Jan 24, 2024

wddgit commented Jan 24, 2024

makortel Jan 25, 2024

cmsbuild commented Jan 25, 2024

Dr15Jones Jan 29, 2024

wddgit Jan 29, 2024

wddgit Jan 30, 2024

Dr15Jones Jan 29, 2024

wddgit Jan 29, 2024

Dr15Jones Jan 29, 2024

makortel Jan 29, 2024

wddgit Jan 29, 2024

Dr15Jones Jan 29, 2024

wddgit Jan 29, 2024

cmsbuild commented Jan 30, 2024

cmsbuild commented Feb 6, 2024

makortel commented Feb 8, 2024

makortel commented Feb 15, 2024

makortel commented Feb 19, 2024

makortel commented Feb 22, 2024

makortel commented Feb 23, 2024

makortel commented Feb 26, 2024

menglu21 commented Feb 27, 2024

cmsbuild commented Feb 27, 2024

antoniovilela commented Feb 29, 2024

		@@ -155,6 +155,11 @@ namespace externalgen {
		//Only stream 0 sets this at stream end Lumi and it is read at global end Lumi

Improve ExternalGeneratorFilter to handle a stream skipping a lumi #43783

Improve ExternalGeneratorFilter to handle a stream skipping a lumi #43783

Conversation

wddgit commented Jan 24, 2024

PR description:

PR validation:

cmsbuild commented Jan 24, 2024 • edited Loading

cmsbuild commented Jan 24, 2024

cmsbuild commented Jan 24, 2024

wddgit commented Jan 24, 2024

wddgit commented Jan 24, 2024

Choose a reason for hiding this comment

cmsbuild commented Jan 25, 2024

Comparison Summary

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmsbuild commented Jan 30, 2024

Comparison Summary

cmsbuild commented Feb 6, 2024

makortel commented Feb 8, 2024

makortel commented Feb 15, 2024

makortel commented Feb 19, 2024

makortel commented Feb 22, 2024

makortel commented Feb 23, 2024

makortel commented Feb 26, 2024

menglu21 commented Feb 27, 2024

cmsbuild commented Feb 27, 2024

antoniovilela commented Feb 29, 2024

cmsbuild commented Jan 24, 2024 •

edited

Loading