Herwig lhe matching fix (including multithreading fixes) #42673

Dominic-Stafford · 2023-08-28T16:22:27Z

This PR reintroduces the HadronizerFilter to Herwig from #40939, designed to address a long-standing issue of the wrong lhe events being saved when using matching in Herwig. This was reverted in #41237 (see #41230) due to issues when running on multiple cores: firstly, one line of regex in the script which merges LHEs from different thrreads incorrectly caught the new lhe tag, and for the Herwig workflows the event number wasn't being properly propagated between the threads, as it needed to be passed to the LHEEventProduct. Both of these issues are now resolved, so it should be possible to re-add the reverted changes.

Should be tested with cms-sw/cmsdist#8670.

cmsbuild · 2023-08-28T16:29:07Z

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-42673/36748

This PR adds an extra 32KB to repository
There are other open Pull requests which might conflict with changes you have proposed:
- File SimDataFormats/GeneratorProducts/interface/LHEEventProduct.h modified in PR(s): Refactoring gen weight storage in EDM + Nano integration #32167

Code check has found code style and quality issues which could be resolved by applying following patch(s)

code-format:
https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-42673/36748/code-format.patch
e.g. curl -k https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-42673/36748/code-format.patch | patch -p1
You can also run scram build code-format to apply code format directly

…g events Read lhe event numbers in, if present Add HadroniserFilter to Herwig which correctly matches LHE numbers between CMSSW and herwig Add option to Herwig input fragements to use LHE numbering Change test examples to use new HadroniserFilter+ LHE numbering code style code style Call addLHEnumbers from mergeLHE.py if asked to number events and not using the DefaultLHEMerger Number events before merge (so numbers are also available to LHEReader) Include evtnum in LHEEventProduct (necesary for multithreading)

cmsbuild · 2023-08-28T16:56:00Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-42673/36750

This PR adds an extra 32KB to repository
There are other open Pull requests which might conflict with changes you have proposed:
- File SimDataFormats/GeneratorProducts/interface/LHEEventProduct.h modified in PR(s): Refactoring gen weight storage in EDM + Nano integration #32167
- File SimDataFormats/GeneratorProducts/src/classes_def.xml modified in PR(s): Refactoring gen weight storage in EDM + Nano integration #32167, Adding HepMC3 products to EDMProduct #38033

cmsbuild · 2023-08-28T16:56:22Z

A new Pull Request was created by @Dominic-Stafford for master.

It involves the following packages:

Configuration/Generator (generators)
GeneratorInterface/Herwig7Interface (generators)
GeneratorInterface/LHEInterface (generators)
SimDataFormats/GeneratorProducts (generators)

@SiewYan, @mkirsano, @Saptaparna, @cmsbuild, @alberto-sanchez, @menglu21, @GurpreetSinghChahal can you please review it and eventually sign? Thanks.
@youyingli, @mkirsano, @missirol, @rovere, @Martin-Grunewald, @apsallid, @bsunanda, @alberto-sanchez, @fabiocos this is something you requested to watch as well.
@perrotta, @dpiparo, @antoniovilela, @rappoccio you are the release manager for this.

cms-bot commands are listed here

smuzaffar · 2023-08-29T05:10:04Z

test parameters:

pull_request = Re-add patches to propagate lhe numbering through herwig cmsdist#8670
relvals_threading = 512.0
enable = threading

smuzaffar · 2023-08-29T05:12:23Z

please test

cmsbuild · 2023-08-29T08:57:22Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-94bbc7/34511/summary.html
COMMIT: c7d31e6
CMSSW: CMSSW_13_3_X_2023-08-28-2300/el8_amd64_gcc11
Additional Tests: THREADING
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/42673/34511/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

You potentially added 8 lines to the logs
Reco comparison results: 4 differences found in the comparisons
DQMHistoTests: Total files compared: 48
DQMHistoTests: Total histograms compared: 3153095
DQMHistoTests: Total failures: 3
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3153070
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 47 files compared)
Checked 207 log files, 159 edm output root files, 48 DQM output files
TriggerResults: no differences found

menglu21 · 2023-09-05T09:30:05Z

Configuration/Generator/python/DYToLL012Jets_5FS_TuneCH3_13TeV_amcatnloFxFx_herwig7_cff.py

@@ -40,7 +40,7 @@
    outputFile = cms.string('cmsgrid_final.lhe'),
    scriptName = cms.FileInPath('GeneratorInterface/LHEInterface/data/run_generic_tarball_cvmfs.sh'),
    generateConcurrently = cms.untracked.bool(True),
-    postGenerationCommand = cms.untracked.vstring('mergeLHE.py', '-i', 'thread*/cmsgrid_final.lhe', '-o', 'cmsgrid_final.lhe')
+    postGenerationCommand = cms.untracked.vstring('mergeLHE.py', '-n', '-i', 'thread*/cmsgrid_final.lhe', '-o', 'cmsgrid_final.lhe')


since we dropped the concurrency for MG some time ago, you can remove line 42 and 43

menglu21 · 2023-09-05T09:30:15Z

Configuration/Generator/python/DYToll01234Jets_5f_LO_MLM_Madgraph_LHE_13TeV_cff.py

@@ -7,5 +7,5 @@
  args = cms.vstring('/cvmfs/cms.cern.ch/phys_generator/gridpacks/UL/13TeV/madgraph/V5_2.6.5/dyellell01234j_5f_LO_MLM_v2/DYJets_HT-incl_slc6_amd64_gcc630_CMSSW_9_3_16_tarball.tar.xz','false','slc6_amd64_gcc630','CMSSW_9_3_16'),
  nEvents = cms.untracked.uint32(10),
  generateConcurrently = cms.untracked.bool(True),
-  postGenerationCommand = cms.untracked.vstring('mergeLHE.py', '-i', 'thread*/cmsgrid_final.lhe', '-o', 'cmsgrid_final.lhe'),
+  postGenerationCommand = cms.untracked.vstring('mergeLHE.py', '-n', '-i', 'thread*/cmsgrid_final.lhe', '-o', 'cmsgrid_final.lhe'),


same comment here

menglu21 · 2023-09-05T09:30:29Z

Configuration/Generator/python/TTbar_Pow_LHE_13TeV_cff.py

@@ -7,7 +7,7 @@
    outputFile = cms.string('cmsgrid_final.lhe'),
    scriptName = cms.FileInPath('GeneratorInterface/LHEInterface/data/run_generic_tarball_cvmfs.sh'),
    generateConcurrently = cms.untracked.bool(True),
-    postGenerationCommand = cms.untracked.vstring('mergeLHE.py', '-i', 'thread*/cmsgrid_final.lhe', '-o', 'cmsgrid_final.lhe'),
+    postGenerationCommand = cms.untracked.vstring('mergeLHE.py', '-n', '-i', 'thread*/cmsgrid_final.lhe', '-o', 'cmsgrid_final.lhe'),


same comment here

Dominic-Stafford · 2023-09-05T09:45:37Z

Hi @menglu21, what is the plan for madgraph concurrency longer term, will it be reintroduced once a satisfactory read-only gridpack solution is found? If so I think it would be better to leave the unit tests as they are so we can catch multithreading bugs rather than having to resolve all of them when we move back to multi-threading. Also have we also dropped multithreading for POWHEG?

menglu21 · 2023-09-05T10:58:22Z

Hi @menglu21, what is the plan for madgraph concurrency longer term, will it be reintroduced once a satisfactory read-only gridpack solution is found? If so I think it would be better to leave the unit tests as they are so we can catch multithreading bugs rather than having to resolve all of them when we move back to multi-threading. Also have we also dropped multithreading for POWHEG?

it should depend on the readonly status and behavior eventually, we are still using multithread for powheg, we can keep the code as you submit

menglu21 · 2023-09-05T10:58:40Z

+1

cmsbuild · 2023-09-05T10:58: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, @antoniovilela, @sextonkennedy (and backports should be raised in the release meeting by the corresponding L2)

antoniovilela · 2023-09-05T16:02:52Z

+1

iarspider · 2023-09-06T11:33:53Z

@Dominic-Stafford @menglu21 Relvals are failing after this PR was merged:

----- Begin Fatal Exception 06-Sep-2023 05:53:57 CEST-----------------------
An exception of category 'ModulesSynchingOnLumis' occurred while
   [0] Calling beginJob
Exception Message:
The framework is configured to use at least two streams, but the following modules
require synchronizing on LuminosityBlock boundaries:
  Herwig7HadronizerFilter generator

The situation can be fixed by either
 * modifying the modules to support concurrent LuminosityBlocks (preferred), or
 * setting 'process.options.numberOfConcurrentLuminosityBlocks = 1' in the configuration file
----- End Fatal Exception -------------------------------------------------

Dominic-Stafford · 2023-09-06T12:17:58Z

Hi, I'm not seeing this in my tests, which are with CMSSW_10_6_30. In the relval you linked there's an earlier error:
Error: The interface 'LesHouchesHandler:EventNumbering' was not found.
which is due to cms-sw/cmsdist#8670 not being merged, is the later error potentially spurious and due to this? Otherwise I can add process.options.numberOfConcurrentLuminosityBlocks = 1 to all the cards (we don't currently have the facility to run Herwig on multiple threads), but I don't understand why this wasn't an issue before with multithreaded externalLheProducer + single threaded Herwig7GeneratorFilter

iarspider · 2023-09-06T12:22:27Z

Hi, I'm not seeing this in my tests, which are with CMSSW_10_6_30. In the relval you linked there's an earlier error: Error: The interface 'LesHouchesHandler:EventNumbering' was not found. which is due to cms-sw/cmsdist#8670 not being merged, is the later error potentially spurious and due to this? Otherwise I can add process.options.numberOfConcurrentLuminosityBlocks = 1 to all the cards (we don't currently have the facility to run Herwig on multiple threads), but I don't understand why this wasn't an issue before with multithreaded externalLheProducer + single threaded Herwig7GeneratorFilter

It is quite possible, thanks!

makortel · 2023-09-06T13:50:48Z

The proper workaround would be to add Herwig7HadronizerFilter to

cmssw/Configuration/Generator/python/concurrentLumisDisable.py

Lines 1 to 19 in 6303322

    
           # list of generator EDModules (C++ type) that do not support concurrentLuminosityBlocks 
        
           noConcurrentLumiGenerators = [ 
        
               "AMPTGeneratorFilter", 
        
               "BeamHaloProducer", 
        
               "CosMuoGenProducer", 
        
               "ExhumeGeneratorFilter", 
        
               "Herwig7GeneratorFilter", 
        
               "HydjetGeneratorFilter", 
        
               "Hydjet2GeneratorFilter", 
        
               "PyquenGeneratorFilter", 
        
               "Pythia6GeneratorFilter", 
        
               "Pythia8EGun", 
        
               "Pythia8GeneratorFilter", 
        
               "Pythia8HadronizerFilter", 
        
               "Pythia8PtAndDxyGun", 
        
               "Pythia8PtGun", 
        
               "ReggeGribovPartonMCGeneratorFilter", 
        
               "SherpaGeneratorFilter", 
        
           ]

Then cmsDriver.py will automatically set numberOfConcurrentLuminosityBlocks = 1 for all jobs that use Herwig7HadronizerFilter.

This error was not visible in 10_6_X, because concurrent lumis was enabled by default in 12_0_X.

Dominic-Stafford · 2023-09-06T15:32:34Z

Ah, thank you @makortel for explaining, I wasn't aware of this, I'll make another PR to add this there

cmsbuild added this to the CMSSW_13_3_X milestone Aug 28, 2023

cmsbuild added generators-pending pending-signatures tests-pending orp-pending code-checks-pending labels Aug 28, 2023

cmsbuild added code-checks-rejected and removed code-checks-pending labels Aug 28, 2023

Dominic-Stafford force-pushed the herwig_lhe_matching_fix branch from 2494ca5 to c7d31e6 Compare August 28, 2023 16:48

cmsbuild added code-checks-pending and removed code-checks-rejected labels Aug 28, 2023

cmsbuild added code-checks-approved and removed code-checks-pending labels Aug 28, 2023

cmsbuild added the requires-external label Aug 29, 2023

cmsbuild added tests-started and removed tests-pending labels Aug 29, 2023

smuzaffar mentioned this pull request Aug 29, 2023

Re-add patches to propagate lhe numbering through herwig cms-sw/cmsdist#8670

Merged

cmsbuild added tests-approved and removed tests-started labels Aug 29, 2023

menglu21 reviewed Sep 5, 2023

View reviewed changes

cmsbuild added generators-approved fully-signed and removed generators-pending pending-signatures labels Sep 5, 2023

cmsbuild added orp-approved and removed orp-pending labels Sep 5, 2023

cmsbuild merged commit 6303322 into cms-sw:master Sep 5, 2023

cmsbuild mentioned this pull request Sep 6, 2023

Llvm16 cms-sw/root#185

Closed

Dominic-Stafford mentioned this pull request Sep 6, 2023

Add Herwig7HadronizerFilter to list of Filters with concurrent lumis disabled #42732

Merged

This was referenced Sep 28, 2023

Herwig lhe matching fix 12_4 #42905

Closed

Herwig lhe matching fix 12_4 #42906

Merged

Herwig lhe matching fix 10_6 #42907

Merged

Dominic-Stafford mentioned this pull request Jun 17, 2024

Herwig LHE matching fix and Matchbox relval for 13_0 #45241

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Herwig lhe matching fix (including multithreading fixes) #42673

Herwig lhe matching fix (including multithreading fixes) #42673

Dominic-Stafford commented Aug 28, 2023

cmsbuild commented Aug 28, 2023

cmsbuild commented Aug 28, 2023

cmsbuild commented Aug 28, 2023

smuzaffar commented Aug 29, 2023 •

edited

Loading

smuzaffar commented Aug 29, 2023

cmsbuild commented Aug 29, 2023

menglu21 Sep 5, 2023

menglu21 Sep 5, 2023

menglu21 Sep 5, 2023

Dominic-Stafford commented Sep 5, 2023

menglu21 commented Sep 5, 2023

menglu21 commented Sep 5, 2023

cmsbuild commented Sep 5, 2023

antoniovilela commented Sep 5, 2023

iarspider commented Sep 6, 2023

Dominic-Stafford commented Sep 6, 2023

iarspider commented Sep 6, 2023

makortel commented Sep 6, 2023

Dominic-Stafford commented Sep 6, 2023

Herwig lhe matching fix (including multithreading fixes) #42673

Herwig lhe matching fix (including multithreading fixes) #42673

Conversation

Dominic-Stafford commented Aug 28, 2023

cmsbuild commented Aug 28, 2023

cmsbuild commented Aug 28, 2023

cmsbuild commented Aug 28, 2023

smuzaffar commented Aug 29, 2023 • edited Loading

smuzaffar commented Aug 29, 2023

cmsbuild commented Aug 29, 2023

Comparison Summary

menglu21 Sep 5, 2023

Choose a reason for hiding this comment

menglu21 Sep 5, 2023

Choose a reason for hiding this comment

menglu21 Sep 5, 2023

Choose a reason for hiding this comment

Dominic-Stafford commented Sep 5, 2023

menglu21 commented Sep 5, 2023

menglu21 commented Sep 5, 2023

cmsbuild commented Sep 5, 2023

antoniovilela commented Sep 5, 2023

iarspider commented Sep 6, 2023

Dominic-Stafford commented Sep 6, 2023

iarspider commented Sep 6, 2023

makortel commented Sep 6, 2023

Dominic-Stafford commented Sep 6, 2023

smuzaffar commented Aug 29, 2023 •

edited

Loading