cmssw icon indicating copy to clipboard operation
cmssw copied to clipboard

[RFC] Add Transformer ability to modules

Open Dr15Jones opened this issue 3 years ago • 27 comments

PR description:

Added ability to register tranform methods to modules. This allows a module to have a method which takes a data product already put into the Event by the same module and then create a new data product from it which is also put into the Event.

The plan is to use this ability to implement an automatic conversion from GPU to CPU data products.

PR validation:

Code compiles and new unit test passes.

Dr15Jones avatar Jun 21 '22 20:06 Dr15Jones

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-38454/30666

  • This PR adds an extra 200KB to repository

cmsbuild avatar Jun 21 '22 20:06 cmsbuild

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

It involves the following packages:

  • DataFormats/Provenance (core)
  • FWCore/Framework (core)
  • FWCore/Integration (core)

@cmsbuild, @smuzaffar, @Dr15Jones, @makortel can you please review it and eventually sign? Thanks. @makortel, @wddgit, @rovere this is something you requested to watch as well. @perrotta, @dpiparo, @qliphy you are the release manager for this.

cms-bot commands are listed here

cmsbuild avatar Jun 21 '22 20:06 cmsbuild

please test

Dr15Jones avatar Jun 21 '22 21:06 Dr15Jones

-1

Failed Tests: Build ClangBuild Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-49175f/25673/summary.html COMMIT: 73463055968d7500803837dd1b80a7cd0ccca89d CMSSW: CMSSW_12_5_X_2022-06-21-1100/el8_amd64_gcc10 User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/38454/25673/install.sh to create a dev area with all the needed externals and cmssw changes.

The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic:

  • @fabiocos cms-sw/cmssw#38419
  • @missirol cms-sw/cmssw#38418
  • @cms-sw cms-sw/cmssw#38451
  • @jeongeun cms-sw/cmssw#38374
  • @trackreco cms-sw/cmssw#38437

You can see more details here: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-49175f/25673/git-recent-commits.json https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-49175f/25673/git-merge-result

Build

I found compilation error when building:

>> Leaving Package AnalysisAlgos/SiStripClusterInfoProducer
>> Package AnalysisAlgos/SiStripClusterInfoProducer built
Entering library rule at src/AnalysisAlgos/SiStripClusterInfoProducer/plugins
>> Compiling edm plugin /data/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_12_5_X_2022-06-21-1100/src/AnalysisAlgos/SiStripClusterInfoProducer/plugins/SiStripProcessedRawDigiProducer.cc
/data/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_12_5_X_2022-06-21-1100/src/AnalysisAlgos/SiStripClusterInfoProducer/plugins/SiStripProcessedRawDigiProducer.cc: In member function 'void SiStripProcessedRawDigiProducer::pr_process(const edm::DetSetVector&, edm::DetSetVector&, const SiStripGain&)':
/data/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_12_5_X_2022-06-21-1100/src/AnalysisAlgos/SiStripClusterInfoProducer/plugins/SiStripProcessedRawDigiProducer.cc:126:5: error: 'virtual void edm::stream::EDProducerBase::transform(std::size_t, edm::EventForTransformer&) const' is private within this context
  126 |     transform(
      |     ^~~~~~~~~
In file included from /data/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_12_5_X_2022-06-21-1100/src/FWCore/Framework/interface/stream/implementors.h:31,
                 from /data/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_12_5_X_2022-06-21-1100/src/FWCore/Framework/interface/stream/AbilityToImplementor.h:25,
                 from /data/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_12_5_X_2022-06-21-1100/src/FWCore/Framework/interface/stream/EDProducer.h:22,

Clang Build

I found compilation error while trying to compile with clang. Command used:

USER_CUDA_FLAGS='--expt-relaxed-constexpr' USER_CXXFLAGS='-Wno-register -fsyntax-only' scram build -k -j 32 COMPILER='llvm compile'

>> Entering Package RecoMuon/MuonSeedGenerator
>> Entering Package RecoMuon/StandAloneMuonProducer
>> Entering Package SimG4Core/Application
>> Entering Package SimGeneral/MixingModule
>> Compile sequence completed for CMSSW CMSSW_12_5_X_2022-06-21-1100
gmake: *** [There are compilation/build errors. Please see the detail log above.] Error 1
+ eval scram build outputlog '&&' '(python3' /data/cmsbld/jenkins/workspace/ib-run-pr-tests/cms-bot/buildLogAnalyzer.py --ignoreWarning=Wdeprecated-declarations --logDir /data/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_12_5_X_2022-06-21-1100/tmp/el8_amd64_gcc10/cache/log/src '||' 'true)'
++ scram build outputlog
>> Entering Package DataFormats/Provenance
Entering library rule at DataFormats/Provenance
>> Compiling  /data/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_12_5_X_2022-06-21-1100/src/DataFormats/Provenance/src/BranchChildren.cc

cmsbuild avatar Jun 22 '22 00:06 cmsbuild

please test

Dr15Jones avatar Jun 22 '22 14:06 Dr15Jones

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-38454/30682

  • This PR adds an extra 20KB to repository

cmsbuild avatar Jun 22 '22 14:06 cmsbuild

Pull request #38454 was updated. @makortel, @smuzaffar, @Dr15Jones can you please check and sign again.

cmsbuild avatar Jun 22 '22 14:06 cmsbuild

-1

Failed Tests: Build Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-49175f/25701/summary.html COMMIT: 998a6287246d7c7e28c4e748024961d48931ef7c CMSSW: CMSSW_12_5_X_2022-06-22-1100/el8_amd64_gcc10 User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/38454/25701/install.sh to create a dev area with all the needed externals and cmssw changes.

Build

I found compilation error when building:

>> Leaving Package AnalysisAlgos/SiStripClusterInfoProducer
>> Package AnalysisAlgos/SiStripClusterInfoProducer built
Entering library rule at src/AnalysisAlgos/SiStripClusterInfoProducer/plugins
>> Compiling edm plugin /data/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_12_5_X_2022-06-22-1100/src/AnalysisAlgos/SiStripClusterInfoProducer/plugins/SiStripProcessedRawDigiProducer.cc
/data/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_12_5_X_2022-06-22-1100/src/AnalysisAlgos/SiStripClusterInfoProducer/plugins/SiStripProcessedRawDigiProducer.cc: In member function 'void SiStripProcessedRawDigiProducer::pr_process(const edm::DetSetVector&, edm::DetSetVector&, const SiStripGain&)':
/data/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_12_5_X_2022-06-22-1100/src/AnalysisAlgos/SiStripClusterInfoProducer/plugins/SiStripProcessedRawDigiProducer.cc:126:5: error: 'virtual void edm::stream::EDProducerBase::transform(std::size_t, edm::EventForTransformer&) const' is private within this context
  126 |     transform(
      |     ^~~~~~~~~
In file included from /data/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_12_5_X_2022-06-22-1100/src/FWCore/Framework/interface/stream/implementors.h:31,
                 from /data/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_12_5_X_2022-06-22-1100/src/FWCore/Framework/interface/stream/AbilityToImplementor.h:25,
                 from /data/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_12_5_X_2022-06-22-1100/src/FWCore/Framework/interface/stream/EDProducer.h:22,

cmsbuild avatar Jun 22 '22 17:06 cmsbuild

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-38454/30690

  • This PR adds an extra 48KB to repository

cmsbuild avatar Jun 22 '22 19:06 cmsbuild

Pull request #38454 was updated. @cmsbuild, @smuzaffar, @Dr15Jones, @makortel can you please check and sign again.

cmsbuild avatar Jun 22 '22 19:06 cmsbuild

please test

Dr15Jones avatar Jun 22 '22 19:06 Dr15Jones

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-49175f/25708/summary.html COMMIT: ba50de5fbaf2033bd54dcbe8277d89d607a168ed CMSSW: CMSSW_12_5_X_2022-06-22-1100/el8_amd64_gcc10 User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/38454/25708/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

  • No significant changes to the logs found
  • Reco comparison results: 0 differences found in the comparisons
  • DQMHistoTests: Total files compared: 50
  • DQMHistoTests: Total histograms compared: 3659099
  • DQMHistoTests: Total failures: 2
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3659075
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 49 files compared)
  • Checked 208 log files, 45 edm output root files, 50 DQM output files
  • TriggerResults: no differences found

cmsbuild avatar Jun 23 '22 05:06 cmsbuild

One additional thought (that I have been forgetting): the first use case for this feature would be device-to-host transfers, that cab be done asynchronously. With the generalized SoA approach I'd expect vast majority of cases to be along

  • construct SoA in (pinned) host memory
  • launch the asynchronous copy
  • return the host-side SoA

i.e. the data product does not need to be changed after the asynchronous copy has finished. But I can't exclude a corner case where such functionality would be useful or needed (although that part can not be fully automated).

I can quickly think of a few options for that (there are probably more):

  1. keep current registerTransform() function, but add edm::WaitingTaskWithArenaHolder parameter. Workers depending on the transformed product would become eligible to run after the WaitingTaskWithArenaHolder::doneWaiting() has been called (i.e. similar to acquire()phase ofExternalWork`)
  2. same as 1 but rename the function to registerTransformAsync() or something
  3. keep the current registerTransform() functionality for synchronous transformation, and add separate registerTransformAsync() with edm::WaitingTaskArenaHolder parameter etc
  4. same as 3, and add a third function that adds possibility for running code after the asynchronous work has finished (similar to produce() phase of `ExternalWork)

I would not necessarily aim for the most general solution now (as long as there is a path to extend towards that later).

makortel avatar Jul 21 '22 18:07 makortel

please test

Dr15Jones avatar Jul 22 '22 14:07 Dr15Jones

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-38454/31211

  • This PR adds an extra 156KB to repository

  • There are other open Pull requests which might conflict with changes you have proposed:

    • File DataFormats/Provenance/interface/BranchDescription.h modified in PR(s): #38806
    • File FWCore/Framework/test/global_filter_t.cppunit.cc modified in PR(s): #38801
    • File FWCore/Framework/test/global_producer_t.cppunit.cc modified in PR(s): #38801
    • File FWCore/Framework/test/limited_filter_t.cppunit.cc modified in PR(s): #38801
    • File FWCore/Framework/test/limited_producer_t.cppunit.cc modified in PR(s): #38801

cmsbuild avatar Jul 22 '22 14:07 cmsbuild

Pull request #38454 was updated. @makortel, @smuzaffar, @Dr15Jones can you please check and sign again.

cmsbuild avatar Jul 22 '22 14:07 cmsbuild

-1

Failed Tests: ClangBuild Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-49175f/26397/summary.html COMMIT: 18ea41a80a77bb5f76bf0effa64ae2512c2da95f CMSSW: CMSSW_12_5_X_2022-07-22-1100/el8_amd64_gcc10 User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/38454/26397/install.sh to create a dev area with all the needed externals and cmssw changes.

Clang Build

I found compilation error while trying to compile with clang. Command used:

USER_CUDA_FLAGS='--expt-relaxed-constexpr' USER_CXXFLAGS='-Wno-register -fsyntax-only' scram build -k -j 32 COMPILER='llvm compile'

>> Creating project symlinks
>> Entering Package DataFormats/Provenance
>> Entering Package FWCore/Framework
>> Entering Package FWCore/Integration
>> Compile sequence completed for CMSSW CMSSW_12_5_X_2022-07-22-1100
gmake: *** [There are compilation/build errors. Please see the detail log above.] Error 1
+ eval scram build outputlog '&&' '(python3' /data/cmsbld/jenkins/workspace/ib-run-pr-tests/cms-bot/buildLogAnalyzer.py --ignoreWarning=Wdeprecated-declarations --logDir /data/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_12_5_X_2022-07-22-1100/tmp/el8_amd64_gcc10/cache/log/src '||' 'true)'
++ scram build outputlog
>> Entering Package DataFormats/Provenance
Entering library rule at DataFormats/Provenance
>> Compiling  /data/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_12_5_X_2022-07-22-1100/src/DataFormats/Provenance/src/BranchChildren.cc

cmsbuild avatar Jul 22 '22 16:07 cmsbuild

please test

Dr15Jones avatar Jul 22 '22 18:07 Dr15Jones

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-38454/31215

  • This PR adds an extra 156KB to repository

  • There are other open Pull requests which might conflict with changes you have proposed:

    • File DataFormats/Provenance/interface/BranchDescription.h modified in PR(s): #38806
    • File FWCore/Framework/test/global_filter_t.cppunit.cc modified in PR(s): #38801
    • File FWCore/Framework/test/global_producer_t.cppunit.cc modified in PR(s): #38801
    • File FWCore/Framework/test/limited_filter_t.cppunit.cc modified in PR(s): #38801
    • File FWCore/Framework/test/limited_producer_t.cppunit.cc modified in PR(s): #38801

cmsbuild avatar Jul 22 '22 18:07 cmsbuild

Pull request #38454 was updated. @makortel, @smuzaffar, @Dr15Jones can you please check and sign again.

cmsbuild avatar Jul 22 '22 18:07 cmsbuild

An asynchronous transform ability with two parts could look like

transformAsync( m_fooPutToken, 
                [](Foo const& iFoo, auto iTask) { return setupWorkAndReturnResultsPtr(iFoo, iTask);},
                [](auto iHolder) { return *iHolder; });

Where the item returned from the first lambda is moved and then moved back to the second lambda once the task is complete.

Dr15Jones avatar Jul 22 '22 19:07 Dr15Jones

An asynchronous transform ability with two parts could look like

transformAsync( m_fooPutToken, 
                [](Foo const& iFoo, auto iTask) { return setupWorkAndReturnResultsPtr(iFoo, iTask);},
                [](auto iHolder) { return *iHolder; });

Where the item returned from the first lambda is moved and then moved back to the second lambda once the task is complete.

Is the type of of iTask edm::WaitingTaskwithArenaHolder and the type of iHolder whatever the setupWorkAndReturnResultsPtr() returns?

makortel avatar Jul 22 '22 19:07 makortel

Is the type of of iTask edm::WaitingTaskwithArenaHolder

Yes

and the type of iHolder whatever the setupWorkAndReturnResultsPtr() returns?

Yes

Dr15Jones avatar Jul 22 '22 19:07 Dr15Jones

The interface looks reasonable to me. I only wonder if the task structure can be simple-enough for just moving the intermediate data around (in contrast to ExternalWork #24186).

makortel avatar Jul 22 '22 19:07 makortel

-1

Failed Tests: RelVals-INPUT Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-49175f/26400/summary.html COMMIT: 771df138c64c8e36d749ed1314ce2fb6fcbea563 CMSSW: CMSSW_12_5_X_2022-07-22-1100/el8_amd64_gcc10 User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/38454/26400/install.sh to create a dev area with all the needed externals and cmssw changes.

RelVals-INPUT

The relvals timed out after 4 hours.

Comparison Summary

Summary:

  • No significant changes to the logs found
  • Reco comparison results: 2 differences found in the comparisons
  • DQMHistoTests: Total files compared: 51
  • DQMHistoTests: Total histograms compared: 3706484
  • DQMHistoTests: Total failures: 8
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3706454
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 50 files compared)
  • Checked 210 log files, 47 edm output root files, 51 DQM output files
  • TriggerResults: no differences found

cmsbuild avatar Jul 23 '22 03:07 cmsbuild

please test

Dr15Jones avatar Jul 25 '22 13:07 Dr15Jones

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-49175f/26445/summary.html COMMIT: 771df138c64c8e36d749ed1314ce2fb6fcbea563 CMSSW: CMSSW_12_5_X_2022-07-25-1100/el8_amd64_gcc10 User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/38454/26445/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

  • No significant changes to the logs found
  • Reco comparison results: 4 differences found in the comparisons
  • DQMHistoTests: Total files compared: 51
  • DQMHistoTests: Total histograms compared: 3667670
  • DQMHistoTests: Total failures: 2
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3667646
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 50 files compared)
  • Checked 210 log files, 47 edm output root files, 51 DQM output files
  • TriggerResults: no differences found

cmsbuild avatar Jul 25 '22 20:07 cmsbuild

Hi, @Dr15Jones , how are we doing on this one? Looks like it missed the 12_5 milestone but does it need a backport there now?

rappoccio avatar Aug 31 '22 13:08 rappoccio

Hi, @Dr15Jones , how are we doing on this one? Looks like it missed the 12_5 milestone but does it need a backport there now?

@makortel what do you want to do?

Dr15Jones avatar Aug 31 '22 13:08 Dr15Jones

Hi, @Dr15Jones , how are we doing on this one? Looks like it missed the 12_5 milestone but does it need a backport there now?

@makortel what do you want to do?

I think 12_6_X is sufficient.

makortel avatar Aug 31 '22 13:08 makortel