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

Migrate DQMEDAnalyze to producers #22319

Merged

Conversation

dmitrijus
Copy link
Contributor

@dmitrijus dmitrijus commented Feb 23, 2018

Also fixes MEtoEDMConverter.

Supersedes #22307

@cmsbuild
Copy link
Contributor

The code-checks are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @dmitrijus (Dmitrijus) for master.

It involves the following packages:

DQMServices/Components
DQMServices/Core
DataFormats/Histograms
Validation/RecoTau

@smuzaffar, @Dr15Jones, @vazzolini, @kmaeshima, @dmitrijus, @cmsbuild, @jfernan2, @vanbesien can you please review it and eventually sign? Thanks.
@barvic, @rovere 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

@davidlange6
Copy link
Contributor

please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 23, 2018

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

@cmsbuild
Copy link
Contributor

-1

Tested at: c528197

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

I found follow errors while testing this PR

Failed tests: Build

  • Build:

I found an error when building:

                 from /build/cmsbld/jenkins/workspace/ib-any-integration/CMSSW_10_1_X_2018-02-23-1100/src/DQM/EcalPreshowerMonitorClient/src/EcalPreshowerMonitorClient.cc:1:
/cvmfs/cms-ib.cern.ch/nweek-02512/slc6_amd64_gcc630/external/gcc/6.3.0-cms/include/c++/6.3.0/bits/unique_ptr.h: In instantiation of 'typename std::_MakeUniq<_Tp>::__single_object std::make_unique(_Args&& ...) [with _Tp = EcalPreshowerMonitorClient; _Args = {const edm::ParameterSet&}; typename std::_MakeUniq<_Tp>::__single_object = std::unique_ptr >]':
/cvmfs/cms-ib.cern.ch/nweek-02512/slc6_amd64_gcc630/cms/cmssw-patch/CMSSW_10_1_X_2018-02-23-1100/src/FWCore/Framework/src/MakeModuleHelper.h:41:40:   required from 'static std::unique_ptr<_Tp> edm::MakeModuleHelper::makeModule(const edm::ParameterSet&) [with T = EcalPreshowerMonitorClient; Base = edm::one::EDProducerBase]'
/cvmfs/cms-ib.cern.ch/nweek-02512/slc6_amd64_gcc630/cms/cmssw-patch/CMSSW_10_1_X_2018-02-23-1100/src/FWCore/Framework/src/WorkerMaker.h:85:147:   required from 'std::shared_ptr edm::WorkerMaker::makeModule(const edm::ParameterSet&) const [with T = EcalPreshowerMonitorClient]'
/build/cmsbld/jenkins/workspace/ib-any-integration/CMSSW_10_1_X_2018-02-23-1100/src/DQM/EcalPreshowerMonitorClient/src/EcalPreshowerMonitorClient.cc:103:46:   required from here
/cvmfs/cms-ib.cern.ch/nweek-02512/slc6_amd64_gcc630/external/gcc/6.3.0-cms/include/c++/6.3.0/bits/unique_ptr.h:791:30: error: invalid new-expression of abstract class type 'EcalPreshowerMonitorClient'
     { return unique_ptr<_Tp>(new _Tp(std::forward<_Args>(__args)...)); }
                              ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from /build/cmsbld/jenkins/workspace/ib-any-integration/CMSSW_10_1_X_2018-02-23-1100/src/DQM/EcalPreshowerMonitorClient/src/EcalPreshowerMonitorClient.cc:1:0:
/build/cmsbld/jenkins/workspace/ib-any-integration/CMSSW_10_1_X_2018-02-23-1100/src/DQM/EcalPreshowerMonitorClient/interface/EcalPreshowerMonitorClient.h:8:7: note:   because the following virtual functions are pure within 'EcalPreshowerMonitorClient':
 class EcalPreshowerMonitorClient : public DQMEDHarvester {


@cmsbuild
Copy link
Contributor

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

@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-22319/26331/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-22319/136.8311_RunJetHT2017F_reminiaod+RunJetHT2017F_reminiaod+REMINIAOD_data2017+HARVEST2017_REMINIAOD_data2017

Comparison Summary:

  • You potentially added 1020 lines to the logs
  • Reco comparison results: 54 differences found in the comparisons
  • DQMHistoTests: Total files compared: 29
  • DQMHistoTests: Total histograms compared: 2479896
  • DQMHistoTests: Total failures: 75929
  • DQMHistoTests: Total nulls: 2
  • DQMHistoTests: Total successes: 2403789
  • DQMHistoTests: Total skipped: 176
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.120000000003 KiB( 6 files compared)
  • Checked 118 log files, 9 edm output root files, 29 DQM output files

@dmitrijus
Copy link
Contributor Author

+1

@Dr15Jones
Copy link
Contributor

+1
removal of blank link in DataFormats/Histograms

@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)

@knoepfel
Copy link
Contributor

@dmitrijus: congrats on much cleaner code--it is much clearer in terms of intent.

I'm working on the concurrent booking of histograms in DQMStore, so I'd like to avoid collisions with you. So far, so good, I think. However, to support concurrent booking, the std::set<MonitorElement> data member will need to be replaced with something else that does not support the concept of lower_bound. I'm not particularly concerned with that, but I'm giving you a heads-up.

@fwyzard
Copy link
Contributor

fwyzard commented Feb 27, 2018

Ha ha ha

@fwyzard
Copy link
Contributor

fwyzard commented Feb 27, 2018

(try not to break the DQM GUI, too)

@Dr15Jones
Copy link
Contributor

@fwyzard see #18574

@fwyzard
Copy link
Contributor

fwyzard commented Feb 27, 2018

I would say that

  • we should not have two (groups of) people try to migrate the same code base at the same time
  • however the migration is done, it should be made sure that it does not break the use of the DQM code base outside of CMSSW

@dmitrijus and @schneiml should be working on a test to outline the set of necessary changes, possible steps, and required features.
I would suggest that @knoepfel discuss the possible changes with them beforehand ...

@schneiml
Copy link
Contributor

@knoepfel yes, we should for sure have a talk at some point. We have a few plans on how to continue development -- and we can use any help we get.

@knoepfel
Copy link
Contributor

@fwyzard: agreed--let's not collide/duplicate efforts.

@schneiml, @dmitrijus: I can meet by Zoom, Skype, or whatever. I can be available as early as 9 am (CST) tomorrow morning (2/28). Otherwise, it will likely need to wait until next week.

@fabiocos
Copy link
Contributor

+1

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

8 participants