Run GeneratorFilter modules in an external process controlled by cmsRun #29445

Dr15Jones · 2020-04-10T00:32:19Z

PR description:

ExternalGeneratorFilter is a cmsRun module which can be configured to run a module built from GeneratorFilter<> in a separate process. This separate process is controlled by ExternalGeneratorFilter and cooperatively shares the CPU with cmsRun.

PR validation:

Additional unit tests were added to compare the output of Pythia8GeneratorFilter run directly within cmsRun and run via the ExternalGeneratorFilter. With just 1 stream/thread the output is identical. With multiple streams, the GenLumiInfoHeader, edm::HepMCProduct, GenEventInfoProduct and the GenRunInfoProduct are identical. The GenLumiInfoProduct is nearly identical exception the XSec value and errors are different.

The ReadBuffers can now safely determine if the underlying buffer address was changed by a different ReadBuffer using the same shared memory.

These classes are used by the ExternalGeneratorFilter module for communicating with an external process.

Identical objects were returned failed comparisons

The added tests make sure what are supposed to be identical products are actually identical.

Also applied formatting.

The code used to merge GenLumiInfoProducts does not handle merging of the XSec information.

Can use a configuration parameter of ExternalGeneratorFilter to turn on verbose reporting from the external process.

cmsbuild · 2020-04-10T00:32:48Z

The code-checks are being triggered in jenkins.

cmsbuild · 2020-04-10T00:39:40Z

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-29445/14624

This PR adds an extra 92KB to repository

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

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

cmsbuild · 2020-04-10T01:14:28Z

The code-checks are being triggered in jenkins.

cmsbuild · 2020-04-10T01:20:35Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-29445/14625

This PR adds an extra 96KB to repository

cmsbuild · 2020-04-10T01:21:03Z

A new Pull Request was created by @Dr15Jones (Chris Jones) for master.

It involves the following packages:

FWCore/Integration
FWCore/SharedMemory
GeneratorInterface/Core
GeneratorInterface/Pythia8Interface
SimDataFormats/GeneratorProducts

@SiewYan, @smuzaffar, @efeyazgan, @Dr15Jones, @mkirsano, @makortel, @cmsbuild, @agrohsje, @alberto-sanchez, @qliphy can you please review it and eventually sign? Thanks.
@mkirsano, @makortel, @rovere, @apsallid, @agrohsje, @wddgit, @lecriste, @alberto-sanchez, @fabiocos this is something you requested to watch as well.
@silviodonato, @dpiparo you are the release manager for this.

cms-bot commands are listed here

makortel · 2020-04-10T01:34:14Z

@cmsbuild, please test

makortel · 2020-04-17T01:03:34Z

+core

makortel · 2020-04-17T01:06:55Z

How can we eventually deploy the external GeneratorFilter? I'm wondering like what steps are needed from the GEN side, and where exactly we can or have to specify that a generator would be run externally (like does it have to into the generator configuration fragments, or can we have some more general switch e.g. in cmsDriver).

(I know this PR is probably not the best place to discuss if the questions are nontrivial, should we open an issue, or how should we proceed?)

Dr15Jones · 2020-04-20T13:54:03Z

What is the plan for evaluating this? This was placed as a high priority by the L1s.

silviodonato · 2020-04-20T16:15:19Z

What is the plan for evaluating this? This was placed as a high priority by the L1s.

@alberto-sanchez @agrohsje @efeyazgan @mkirsano @qliphy @SiewYan

qliphy · 2020-04-21T06:02:05Z

+1

cmsbuild · 2020-04-21T06:02:33Z

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. @silviodonato, @dpiparo (and backports should be raised in the release meeting by the corresponding L2)

silviodonato · 2020-04-21T07:55:19Z

FWCore/SharedMemory/interface/ReadBuffer.h


 // forward declarations

 namespace edm::shared_memory {
  class ReadBuffer {
  public:
    /** iUniqueName : must be unique for all processes running on a system.
-        iBufferIndex : is a pointer to a shared_memory address where the same address needs to be shared by ReadBuffer and WriteBuffer.
+        iBufferInfo : is a pointer to a shared_memory address where the same address needs to be shared by ReadBuffer and WriteBuffer.


Have you kept the same description on purpose?

The purpose of the variable is unchanged, I just changed the name to better reflect what it was doing.

silviodonato · 2020-04-21T07:55:31Z

FWCore/SharedMemory/interface/WriteBuffer.h


 // forward declarations

 namespace edm::shared_memory {
  class WriteBuffer {
  public:
    /** iUniqueName : must be unique for all processes running on a system.
-        iBufferIndex : is a pointer to a shared_memory address where the same address needs to be shared by ReadBuffer and WriteBuffer.
+        iBufferInfo : is a pointer to a shared_memory address where the same address needs to be shared by ReadBuffer and WriteBuffer.


That was intentional.

silviodonato · 2020-04-21T08:44:12Z

+1
@Dr15Jones please have a look at my (trivial) comments above

agrohsje · 2020-04-22T11:21:37Z

Hi @Dr15Jones @smrenna @qliphy ,
I thought about this a little bit. Wouldn't it make sense to combine the cross sections taking their relative uncertainties into account. Referring to this comment from Chris:

For reviewers: the existing GenLumiInfoProduct::mergeProduct method does not attempt to combine the values for XSec. Instead, we just get the value from the first GenLumiInfoProduct. That seems less than optimal. Is there a reasonable way to combine the XSec values to get a more accurate number?

makortel · 2020-04-22T14:31:05Z

I thought about this a little bit. Wouldn't it make sense to combine the cross sections taking their relative uncertainties into account.

Hi @agrohsje, I believe @Dr15Jones already did that (see below)? Or is the logic in GenXSecAnalyzer sub-optimal?

#29445 (comment)

You may refer to GenXSecAnalyzer which combines GenLumiInfoProduct to get accurate Xsec.

I used that calculation to handle XSec improvements in call to GenLumiInfoProduct::mergeProduct.

srimanob · 2020-04-22T22:23:03Z

Should we plan to do validation on high statistics relvals for the next pre-release? Could GEN come with few workflows to use this feature before?

agrohsje · 2020-04-22T22:29:08Z

@makortel no. that is exactly what i had in mind. missed the commit from chris, sorry.

jordan-martins · 2020-04-22T23:45:50Z

I did some preliminary test for the BPH dumby test as Chris used and further pointed me and it looks really nice. Previously, this dumby one had, with whatever number of cores (evtGen does not support MP), something around 35 events/lumi. With these implementations we could reach, with 4 cores, around 196 events/lumi. In terms of time/evt, prevously we had something around 0.3s to 0.6s. Now, my test indicates something around 0.0734s (a huge gain in core efficiency).

qliphy · 2020-04-23T04:11:20Z

Should we plan to do validation on high statistics relvals for the next pre-release? Could GEN come with few workflows to use this feature before?

We have done some local tests as you can see from above what Jordan reported on a BPH request, and also a MadGraph request:
#28899 (comment)

GEN plan to have a dedicated discussions on May 4th:
https://indico.cern.ch/event/894534/
where we will make further plans on validation, and foresee further improvement with MG support with readonly gridpack option.

…un (106X) Backport from cms-sw#29445. Additional changes: - GeneratorInterface/Core/bin/externalGenerator.cc: L131: disable macro CMS_SA_ALLOW. - GeneratorInterface/Pythia8Interface/test/compare_identical_generators_cfg.py: L6-8: default way to specify process.maxEvents, process.options. - GeneratorInterface/Pythia8Interface/test/compare_external_generators_cfg.py: L6-8: same. - GeneratorInterface/Pythia8Interface/test/compare_external_generators_streams_cfg.py: L6-12: same.

…un (106X) Backport from cms-sw#29445. Additional changes: - GeneratorInterface/Core/bin/externalGenerator.cc: L131: disable macro CMS_SA_ALLOW. - GeneratorInterface/Core/plugins/ExternalGeneratorFilter.cc: L3: add header "FWCore/Framework/interface/Run.h". - GeneratorInterface/Pythia8Interface/test/compare_identical_generators_cfg.py: L6-8: default way to specify process.maxEvents, process.options. - GeneratorInterface/Pythia8Interface/test/compare_external_generators_cfg.py: L6-8: same. - GeneratorInterface/Pythia8Interface/test/compare_external_generators_streams_cfg.py: L6-12: same.

Dr15Jones added 9 commits April 8, 2020 16:02

Allow memory sharing between multiple ReadBuffers/WriteBuffers

0e9d1a6

The ReadBuffers can now safely determine if the underlying buffer address was changed by a different ReadBuffer using the same shared memory.

Added ExternalGenerator*Info classes

ae17cbf

These classes are used by the ExternalGeneratorFilter module for communicating with an external process.

First version of ExternalGeneratorFilter

ccf03f8

Fixed GenLumiInfoProduct::operator==

33e1bd4

Identical objects were returned failed comparisons

Added test to compare products from Generators

c54a096

The added tests make sure what are supposed to be identical products are actually identical.

Treat 'missing' FinalStat special when adding

9a2a821

Properly merge GenLumiInfoProduct at end LuminosityBlock

c4e2d4d

Also applied formatting.

Allow XSec measurement to be different in comparisons

cb3f0df

The code used to merge GenLumiInfoProducts does not handle merging of the XSec information.

Add verose option to external generator application

5d86ceb

Can use a configuration parameter of ExternalGeneratorFilter to turn on verbose reporting from the external process.

cmsbuild added this to the CMSSW_11_1_X milestone Apr 10, 2020

cmsbuild added code-checks-pending comparison-pending core-pending generators-pending orp-pending pending-signatures tests-pending labels Apr 10, 2020

cmsbuild added code-checks-rejected and removed code-checks-pending labels Apr 10, 2020

apply clang-tidy changes

9ff75a4

cmsbuild added code-checks-pending and removed code-checks-rejected labels Apr 10, 2020

cmsbuild added code-checks-approved and removed code-checks-pending labels Apr 10, 2020

cmsbuild added comparison-available and removed comparison-pending labels Apr 17, 2020

cmsbuild added core-approved and removed core-pending labels Apr 17, 2020

Dr15Jones mentioned this pull request Apr 20, 2020

Access edm::event from GeneratorInterface #28636

Closed

cmsbuild added fully-signed generators-approved and removed generators-pending pending-signatures labels Apr 21, 2020

silviodonato reviewed Apr 21, 2020

View reviewed changes

cmsbuild added orp-approved and removed orp-pending labels Apr 21, 2020

cmsbuild merged commit 8869156 into cms-sw:master Apr 21, 2020

Dr15Jones deleted the interProcGenerator branch April 21, 2020 14:21

colizz mentioned this pull request Jul 12, 2020

Backport ExternalGeneratorFilter and core functions it depends on to 10_6_X #30649

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Run GeneratorFilter modules in an external process controlled by cmsRun #29445

Run GeneratorFilter modules in an external process controlled by cmsRun #29445

Dr15Jones commented Apr 10, 2020

cmsbuild commented Apr 10, 2020

cmsbuild commented Apr 10, 2020

cmsbuild commented Apr 10, 2020

cmsbuild commented Apr 10, 2020

cmsbuild commented Apr 10, 2020

makortel commented Apr 10, 2020

makortel commented Apr 17, 2020

makortel commented Apr 17, 2020

Dr15Jones commented Apr 20, 2020

silviodonato commented Apr 20, 2020

qliphy commented Apr 21, 2020

cmsbuild commented Apr 21, 2020

silviodonato Apr 21, 2020

Dr15Jones Apr 21, 2020

Dr15Jones Apr 21, 2020

silviodonato Apr 21, 2020

Dr15Jones Apr 21, 2020

silviodonato commented Apr 21, 2020

agrohsje commented Apr 22, 2020

makortel commented Apr 22, 2020

srimanob commented Apr 22, 2020

agrohsje commented Apr 22, 2020

jordan-martins commented Apr 22, 2020

qliphy commented Apr 23, 2020

Run GeneratorFilter modules in an external process controlled by cmsRun #29445

Run GeneratorFilter modules in an external process controlled by cmsRun #29445

Conversation

Dr15Jones commented Apr 10, 2020

PR description:

PR validation:

cmsbuild commented Apr 10, 2020

cmsbuild commented Apr 10, 2020

cmsbuild commented Apr 10, 2020

cmsbuild commented Apr 10, 2020

cmsbuild commented Apr 10, 2020

makortel commented Apr 10, 2020

makortel commented Apr 17, 2020

makortel commented Apr 17, 2020

Dr15Jones commented Apr 20, 2020

silviodonato commented Apr 20, 2020

qliphy commented Apr 21, 2020

cmsbuild commented Apr 21, 2020

silviodonato Apr 21, 2020

Choose a reason for hiding this comment

Dr15Jones Apr 21, 2020

Choose a reason for hiding this comment

Dr15Jones Apr 21, 2020

Choose a reason for hiding this comment

silviodonato Apr 21, 2020

Choose a reason for hiding this comment

Dr15Jones Apr 21, 2020

Choose a reason for hiding this comment

silviodonato commented Apr 21, 2020

agrohsje commented Apr 22, 2020

makortel commented Apr 22, 2020

srimanob commented Apr 22, 2020

agrohsje commented Apr 22, 2020

jordan-martins commented Apr 22, 2020

qliphy commented Apr 23, 2020