Fixing FastTimerClient Hist Remaking Issue : 12_4_X #37402

Sam-Harper · 2022-03-29T17:14:03Z

PR description:

This PR is a bug fix doing two things

properly checking if a histogram already exists so there is no needless remaking (the path was missing before so the hist was not found)
~~removing the per lumisection making of the histograms, its just the same histograms being reset to exactly the same content each time~~
update: 2) after review has been changed to keep this behavior as default as it is needed by the online DQM and it is now controlled by a parameter which defaults to keep the old behaviour.

point 1) is absolutely necessary. 2) is just nice but can be removed if needed.

This is an important fix as we can not harvest timing results over many lumisections without it. This is because its very very slow to change a histogram labels which this does when it thinks it has created a new histogram.

It superseeds: #36313

PR validation:

The histograms are identical before and after this PR with the exception that the number of entries now accurately reflects the number of times SetBinContent should be called (before it was that number times the number of LS +1)

if this PR is a backport please specify the original PR and why you need to backport that PR:

We'll need this in 12_3_0, its causing a real problem when running on reemulated L1 samples (which have lots of LS to gain stats).

… existing hists

cmsbuild · 2022-03-29T17:22:09Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-37402/29067

This PR adds an extra 16KB to repository
There are other open Pull requests which might conflict with changes you have proposed:
- File HLTrigger/Timer/plugins/FastTimerServiceClient.cc modified in PR(s): Replace boost/regex.hpp with standard library regex #35919

cmsbuild · 2022-03-29T17:22:27Z

A new Pull Request was created by @Sam-Harper (Sam Harper) for master.

It involves the following packages:

HLTrigger/Timer (hlt)

@cmsbuild, @missirol, @Martin-Grunewald can you please review it and eventually sign? Thanks.
@Martin-Grunewald, @missirol, @silviodonato 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

missirol · 2022-03-29T17:41:50Z

please test

Doing the honours.

missirol · 2022-03-29T17:42:23Z

type bugfix

To be backported to 12_3_X to ease HLT development.

cmsbuild · 2022-03-29T21:22:10Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-6c8f0d/23510/summary.html
COMMIT: a0aa423
CMSSW: CMSSW_12_4_X_2022-03-29-1100/slc7_amd64_gcc10
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/37402/23510/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: 2 differences found in the comparisons
DQMHistoTests: Total files compared: 48
DQMHistoTests: Total histograms compared: 3585896
DQMHistoTests: Total failures: 7
DQMHistoTests: Total nulls: 1
DQMHistoTests: Total successes: 3585866
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.004 KiB( 47 files compared)
DQMHistoSizes: changed ( 312.0 ): 0.004 KiB MessageLogger/Warnings
Checked 200 log files, 45 edm output root files, 48 DQM output files
TriggerResults: no differences found

missirol · 2022-03-30T06:03:18Z

A quick look back in the history led to #5305: "Adapt the DQM clients to the 7.2.x DQMEDHarvesting interface, which is necessary to properly update histograms online, and in general when running the DQM harvesting with the "Online" convention.".

Is it possible that this update per-LS has to do with displaying the updated plots in the online DQM (or something along these lines..)?

Would it make sense to add a config parameter here to enable/disable per-LS updates? (default could be "disable")

cmsbuild · 2022-03-30T07:36:43Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-37402/29073

This PR adds an extra 16KB to repository
There are other open Pull requests which might conflict with changes you have proposed:
- File HLTrigger/Timer/plugins/FastTimerServiceClient.cc modified in PR(s): Replace boost/regex.hpp with standard library regex #35919

cmsbuild · 2022-03-30T07:37:06Z

Pull request #37402 was updated. @cmsbuild, @missirol, @Martin-Grunewald can you please check and sign again.

missirol · 2022-03-30T09:34:10Z

please test

Sam-Harper · 2022-03-30T13:38:36Z

Hi Andrea,

This module isnt run inside the HLT menu as far as I can tell

Appears to be only the service, not the client. However you are right, it may be running as part of the DQM at p5? This I dont know and dont know how to find out.

Lets just solve the issue by setting the default back to the old behaviour and people can then just set it in their harvesting jobs to the correct one.

Point 1) is the actual bug which should have zero effect on anything. Other histograms already do this.

cmsbuild · 2022-03-30T13:47:19Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-37402/29086

This PR adds an extra 16KB to repository
There are other open Pull requests which might conflict with changes you have proposed:
- File HLTrigger/Timer/plugins/FastTimerServiceClient.cc modified in PR(s): Replace boost/regex.hpp with standard library regex #35919

cmsbuild · 2022-03-30T13:47:42Z

Pull request #37402 was updated. @cmsbuild, @missirol, @Martin-Grunewald can you please check and sign again.

fwyzard · 2022-03-30T14:05:08Z

This module isnt run inside the HLT menu as far as I can tell

You are right -- being a "client" means that is should be running in the harvesting step, so only in the DQM machines, reading the histograms produces by the FastTimerService which is inside the HLT itself.

missirol · 2022-03-30T17:19:04Z

please test

cmsbuild · 2022-03-30T21:19:03Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-6c8f0d/23537/summary.html
COMMIT: 4b84728
CMSSW: CMSSW_12_4_X_2022-03-30-1100/slc7_amd64_gcc10
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/37402/23537/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: 12 differences found in the comparisons
DQMHistoTests: Total files compared: 48
DQMHistoTests: Total histograms compared: 3585896
DQMHistoTests: Total failures: 19
DQMHistoTests: Total nulls: 1
DQMHistoTests: Total successes: 3585854
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: -0.004 KiB( 47 files compared)
DQMHistoSizes: changed ( 312.0 ): -0.004 KiB MessageLogger/Warnings
Checked 200 log files, 45 edm output root files, 48 DQM output files
TriggerResults: no differences found

missirol · 2022-03-31T08:39:47Z

+hlt

bugfix + 1 new config parameter (without changes in default behaviour)
to be backported to 12_3_X

cmsbuild · 2022-03-31T08:40:08Z

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

perrotta · 2022-03-31T09:13:49Z

HLTrigger/Timer/plugins/FastTimerServiceClient.cc

@@ -488,7 +492,7 @@ void FastTimerServiceClient::fillDescriptions(edm::ConfigurationDescriptions& de
  edm::ParameterSetDescription puMEPSet;
  fillPUMePSetDescription(puMEPSet);
  desc.add<edm::ParameterSetDescription>("puME", puMEPSet);
-
+  desc.add<bool>("fillEveryLumiSection", true);


Shouldn't the default be "false", according to what you write in the comments?

I think it's just that the description does not take into account the latest changes applied to the PR.

In the description, replacing this sentence

removing the per lumisection making of the histograms

with this sentence

adding an option to disable the per-lumisection updating of the histograms (which remains enabled by default)

would better reflect the content of the PR.

@missirol in #37402 (comment) you were also suggesting to keep "disable per-LS updates" as the default, which is the same as written in the PR description.
@Sam-Harper , @missirol could you please agree on what's your actual intent here, and either fix the PR or its description? It can be merged right after.

@perrotta , I wrote that, but later comments clarified that it was safer to keep the default unchanged (per-LS update enabled by default), see #37402 (comment).

In summary, the PR description needs to be updated (it is already correct in the backport PR). @Sam-Harper could you please edit ? (I don't think I can)

sorry the intent changed. So 99% of people will want it to default to false. However changing the default behaviour will mess up online DQM and for the quiet life, I decided it was better to just keep the old behaviour (which is fine just inefficient) and have users update. Regardless the majority of users will actually be running a config we control not them so we can just disable it there.

So just to sumarise: Andrea raised the point that turning this off will effect online DQM. Therefore we have kept the old behaviour as default otherwise we would need to adapt DQM workflows and this is a late fix.

perrotta · 2022-03-31T16:01:48Z

+1

Thank you Marino and Sam

disabling remaking the plots every lumi section and fixing checks for…

a0aa423

… existing hists

cmsbuild added this to the CMSSW_12_4_X milestone Mar 29, 2022

cmsbuild added code-checks-pending hlt-pending orp-pending pending-signatures tests-pending labels Mar 29, 2022

Sam-Harper mentioned this pull request Mar 29, 2022

Speeding up FastTimerServiceClient #36313

Closed

cmsbuild added code-checks-approved and removed code-checks-pending labels Mar 29, 2022

cmsbuild added tests-started and removed tests-pending labels Mar 29, 2022

cmsbuild added the bug-fix label Mar 29, 2022

cmsbuild added tests-approved and removed tests-started labels Mar 29, 2022

cmsbuild added code-checks-pending tests-pending and removed tests-approved code-checks-approved labels Mar 30, 2022

cmsbuild added code-checks-approved and removed code-checks-pending labels Mar 30, 2022

adding option to fill each ls

4b84728

Sam-Harper force-pushed the FastTimerClientHistResetFixes_1230pre6 branch from 9a3e591 to 4b84728 Compare March 30, 2022 13:40

cmsbuild added code-checks-pending tests-pending and removed tests-approved code-checks-approved labels Mar 30, 2022

cmsbuild added code-checks-approved and removed code-checks-pending labels Mar 30, 2022

cmsbuild added tests-started and removed tests-pending labels Mar 30, 2022

cmsbuild added tests-approved and removed tests-started labels Mar 30, 2022

cmsbuild added fully-signed hlt-approved and removed hlt-pending pending-signatures labels Mar 31, 2022

Sam-Harper mentioned this pull request Mar 31, 2022

Fixing FastTimerClient Hist Remaking Issue : 12_3_X #37421

Merged

perrotta reviewed Mar 31, 2022

View reviewed changes

cmsbuild added orp-approved and removed orp-pending labels Mar 31, 2022

cmsbuild merged commit e35468f into cms-sw:master Mar 31, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixing FastTimerClient Hist Remaking Issue : 12_4_X #37402

Fixing FastTimerClient Hist Remaking Issue : 12_4_X #37402

Sam-Harper commented Mar 29, 2022 •

edited

cmsbuild commented Mar 29, 2022

cmsbuild commented Mar 29, 2022

missirol commented Mar 29, 2022

missirol commented Mar 29, 2022

cmsbuild commented Mar 29, 2022

missirol commented Mar 30, 2022

cmsbuild commented Mar 30, 2022

cmsbuild commented Mar 30, 2022

missirol commented Mar 30, 2022

Sam-Harper commented Mar 30, 2022

cmsbuild commented Mar 30, 2022

cmsbuild commented Mar 30, 2022

fwyzard commented Mar 30, 2022 •

edited

missirol commented Mar 30, 2022

cmsbuild commented Mar 30, 2022

missirol commented Mar 31, 2022 •

edited

cmsbuild commented Mar 31, 2022

perrotta Mar 31, 2022

missirol Mar 31, 2022

perrotta Mar 31, 2022

missirol Mar 31, 2022

Sam-Harper Mar 31, 2022

perrotta commented Mar 31, 2022

Fixing FastTimerClient Hist Remaking Issue : 12_4_X #37402

Fixing FastTimerClient Hist Remaking Issue : 12_4_X #37402

Conversation

Sam-Harper commented Mar 29, 2022 • edited

PR description:

PR validation:

if this PR is a backport please specify the original PR and why you need to backport that PR:

cmsbuild commented Mar 29, 2022

cmsbuild commented Mar 29, 2022

missirol commented Mar 29, 2022

missirol commented Mar 29, 2022

cmsbuild commented Mar 29, 2022

Comparison Summary

missirol commented Mar 30, 2022

cmsbuild commented Mar 30, 2022

cmsbuild commented Mar 30, 2022

missirol commented Mar 30, 2022

Sam-Harper commented Mar 30, 2022

cmsbuild commented Mar 30, 2022

cmsbuild commented Mar 30, 2022

fwyzard commented Mar 30, 2022 • edited

missirol commented Mar 30, 2022

cmsbuild commented Mar 30, 2022

Comparison Summary

missirol commented Mar 31, 2022 • edited

cmsbuild commented Mar 31, 2022

perrotta Mar 31, 2022

Choose a reason for hiding this comment

missirol Mar 31, 2022

Choose a reason for hiding this comment

perrotta Mar 31, 2022

Choose a reason for hiding this comment

missirol Mar 31, 2022

Choose a reason for hiding this comment

Sam-Harper Mar 31, 2022

Choose a reason for hiding this comment

perrotta commented Mar 31, 2022

Sam-Harper commented Mar 29, 2022 •

edited

fwyzard commented Mar 30, 2022 •

edited

missirol commented Mar 31, 2022 •

edited