Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move premixing worker plugins to packages with corresponding MixingModule plugins #22953

Merged
merged 2 commits into from Apr 19, 2018

Conversation

makortel
Copy link
Contributor

This PR makes two technical updates to the PreMixingModule introduced in #22416 (both to make the plugin code closer to MixingModule plugins)

  • Use PileUpEventPrincipal from MixingModule
    • Worker plugins can use the usual edm::Handle+getByLabel() to retrieve data products instead of std::shared_ptr+getProductByTag()
  • Move premixing worker plugins to the same packages as corresponding MixingModule plugins are
    • I believe this move makes the premixing code less hidden from digitizer maintainers
    • In addition, e.g. for phase2 tracker the digitizer code that the premixing plugin would have to call is in plugins, and in my opinion it is better to place the premixing plugins there as well rather than moving the phase2 tracker digitizer code to interface+src.

Tested in CMSSW_10_2_X_2018-04-12-2300, no changes expected.

@kpedro88 @mdhildreth

Using it makes the worker interface closer to MixingModule
DigiMixAccumulatorMixMod interface, and allows to use edm::Handle to
retrieve data products from pileup events.
@cmsbuild
Copy link
Contributor

The code-checks are being triggered in jenkins.

@makortel makortel changed the title Move premixing workers Move premixing worker plugins to packages with corresponding MixingModule plugins Apr 13, 2018
@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @makortel (Matti Kortelainen) for master.

It involves the following packages:

SimCalorimetry/EcalSimProducers
SimCalorimetry/HcalSimProducers
SimGeneral/MixingModule
SimGeneral/PreMixingModule
SimGeneral/TrackingAnalysis
SimMuon/CSCDigitizer
SimMuon/DTDigitizer
SimMuon/RPCDigitizer
SimTracker/SiPixelDigitizer
SimTracker/SiStripDigitizer

The following packages do not have a category, yet:

SimGeneral/PreMixingModule
Please create a PR for https://github.com/cms-sw/cms-bot/blob/master/categories_map.py to assign category

@cmsbuild, @civanch, @mdhildreth can you please review it and eventually sign? Thanks.
@echabert, @abbiendi, @argiro, @threus, @mmusich, @battibass, @jhgoh, @prolay, @HuguesBrun, @drkovalskyi, @ptcox, @trocino, @GiacomoSguazzoni, @rovere, @VinInn, @dildick, @ebrondol, @dgulhan, @calderona, @dkotlins, @mariadalfonso, @folguera this is something you requested to watch as well.
@davidlange6, @slava77, @fabiocos you are the release manager for this.

cms-bot commands are listed here

@makortel
Copy link
Contributor Author

@cmsbuild, please test workflow 250202.0,250202.17,250202.18,250402.0

@cmsbuild
Copy link
Contributor

cmsbuild commented Apr 13, 2018

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/27473/console Started: 2018/04/13 13:10

@cmsbuild
Copy link
Contributor

-1

Tested at: 3321534

You can see the results of the tests here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-22953/27473/summary.html

I found follow errors while testing this PR

Failed tests: RelVals

  • RelVals:

When I ran the RelVals I found an error in the following worklfows:
1000.0 step1

DAS Error
1001.0 step1
DAS Error

@cmsbuild
Copy link
Contributor

Comparison not run due to runTheMatrix errors (RelVals and Igprof tests were also skipped)

@makortel
Copy link
Contributor Author

@cmsbuild, please test workflow 250202.0,250202.17,250202.18,250402.0

@cmsbuild
Copy link
Contributor

cmsbuild commented Apr 16, 2018

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/27501/console Started: 2018/04/16 09:02

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

Comparison job queued.

@cmsbuild
Copy link
Contributor

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-22953/27501/summary.html

@slava77 comparisons for the following workflows were not done due to missing matrix map:

  • /build/cmsbld/jenkins/workspace/compare-root-files-short-matrix/results/JR-comparison/PR-22953/250202.18_TTbar_13UP18+TTbar_13UP18+DIGIPRMXUP18_PU25+RECOPRMXUP18_PU25+HARVESTUP18_PU25
  • /build/cmsbld/jenkins/workspace/compare-root-files-short-matrix/results/JR-comparison/PR-22953/250402.0_TTbar_13+FS_TTbar_13_PRMXUP15_PU25+HARVESTUP15FS+MINIAODMCUP15FS

Comparison Summary:

  • No significant changes to the logs found
  • Reco comparison results: 0 differences found in the comparisons
  • DQMHistoTests: Total files compared: 29
  • DQMHistoTests: Total histograms compared: 2492536
  • DQMHistoTests: Total failures: 1
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 2492359
  • DQMHistoTests: Total skipped: 176
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 23 files compared)
  • Checked 119 log files, 9 edm output root files, 29 DQM output files

@civanch
Copy link
Contributor

civanch commented Apr 16, 2018

+1

@cmsbuild
Copy link
Contributor

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

@fabiocos
Copy link
Contributor

+1

good move

@cmsbuild cmsbuild merged commit 8b21175 into cms-sw:master Apr 19, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants