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 Validation/DTRecHits DQM sources to concurrent DQM (10.1.x) #22082

Merged
merged 8 commits into from Feb 10, 2018

Conversation

fwyzard
Copy link
Contributor

@fwyzard fwyzard commented Feb 2, 2018

No description provided.

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 2, 2018

The code-checks are being triggered in jenkins.

@fwyzard
Copy link
Contributor Author

fwyzard commented Feb 2, 2018

type bugfix

@fwyzard
Copy link
Contributor Author

fwyzard commented Feb 2, 2018

@cmsbuild, please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 2, 2018

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 2, 2018

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/25860/console Started: 2018/02/02 10:17

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 2, 2018

A new Pull Request was created by @fwyzard (Andrea Bocci) for master.

It involves the following packages:

Validation/DTRecHits

@vazzolini, @kmaeshima, @dmitrijus, @cmsbuild, @jfernan2, @vanbesien can you please review it and eventually sign? Thanks.
@davidlange6, @slava77, @fabiocos you are the release manager for this.

cms-bot commands are listed here

@fwyzard
Copy link
Contributor Author

fwyzard commented Feb 2, 2018

@battibass, this should take care of the migration to concurrent DQM; I did not try to address the part of the code that should be moved to the harvesting step.

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 2, 2018

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 2, 2018

Comparison job queued.

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 2, 2018

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-22082/25860/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-22082/1325.7_TTbar_13_94XNanoAODINPUT+TTbar_13_94XNanoAODINPUT+NANOEDMMC2017

Comparison Summary:

  • No significant changes to the logs found
  • Reco comparison results: 2 differences found in the comparisons
  • DQMHistoTests: Total files compared: 26
  • DQMHistoTests: Total histograms compared: 2464203
  • DQMHistoTests: Total failures: 1
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 2464033
  • DQMHistoTests: Total skipped: 169
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.529999999999 KiB( 9 files compared)
  • Checked 110 log files, 9 edm output root files, 26 DQM output files


class DTRecHitQuality : public edm::EDAnalyzer {
class DTRecHitQuality : public DQMGlobalEDAnalyzer<Histograms> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do these need to be concurrent EDAnalyzer?
The usual DQM migration should have been enough...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because a DQMGlobalEDAnalyzer takes N times less memory than a DQMEDAnalyzer (where N is the number of concurrent streams in the job), makes the endRun() transitions faster, and has no downsides that I know of.

void DT2DSegmentClients::endLuminosityBlock(edm::LuminosityBlock const& lumi,
edm::EventSetup const& setup)
{
DQMStore* dbe = Service<DQMStore>().operator->();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DT2DSegmentClients should become DQMEDHarvester. DQMStore should really never be accessed.

The analyze is empty and harvester does gives you the dqmEndLuminosityBlock call.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I wrote 4 days ago

this should take care of the migration to concurrent DQM; I did not try to address the part of the code that should be moved to the harvesting step.

The DT or DQM groups can take care of this further migration in a separate PR.

@fwyzard
Copy link
Contributor Author

fwyzard commented Feb 7, 2018

@vazzolini, @kmaeshima, @dmitrijus, @jfernan2
can you please sign this, or at least comment ?

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 8, 2018

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 8, 2018

Pull request #22082 was updated. @vazzolini, @kmaeshima, @dmitrijus, @cmsbuild, @jfernan2, @vanbesien can you please check and sign again.

@fwyzard
Copy link
Contributor Author

fwyzard commented Feb 8, 2018

@fwyzard
Copy link
Contributor Author

fwyzard commented Feb 8, 2018

@cmsbuild please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 8, 2018

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

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 8, 2018

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 8, 2018

Comparison job queued.

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 8, 2018

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-22082/25971/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-22082/1325.7_TTbar_13_94XNanoAODINPUT+TTbar_13_94XNanoAODINPUT+NANOEDMMC2017+HARVESTNANOAODMC2017

Comparison Summary:

  • No significant changes to the logs found
  • Reco comparison results: 4 differences found in the comparisons
  • DQMHistoTests: Total files compared: 27
  • DQMHistoTests: Total histograms compared: 2464162
  • DQMHistoTests: Total failures: 1
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 2463992
  • DQMHistoTests: Total skipped: 169
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.879999999888 KiB( 22 files compared)
  • Checked 111 log files, 9 edm output root files, 27 DQM output files

@dmitrijus
Copy link
Contributor

+1

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 8, 2018

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)

@fwyzard
Copy link
Contributor Author

fwyzard commented Feb 9, 2018

please merge ?

@fwyzard
Copy link
Contributor Author

fwyzard commented Feb 9, 2018

@fabiocos , what do I need to do to get the same preferential treatment as the framework changes, and have my PRs merged within 12 hours instead of 12 days ?

@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

4 participants