add unit test for HLT online-DQM plugins #40334

missirol · 2022-12-15T17:46:57Z

PR description:

This PR aims to add a unit test involving most of the DQM modules (and services) running in the HLT and providing inputs to the online DQM. The test includes both DQM and harvesting steps, and it only requires that both run without errors (the unit test does not check the outputs).

Small changes are also made in the cpp implementation of a few plugins:

the parameter FolderName of PSMonitor is renamed folderName as done for LumiMonitor in give OnlineLuminosityRecord info to HLT's LumiMonitor plugin #39859;
default values are added for the parameters of LumiMonitor that don't currently have them;
a parameter fillEveryLumiSection is added to ThroughputServiceClient in analogy with FastTimeServiceClient.

There are two changes that require feedback (I'm not sure they are correct):

I needed to add a call in ThroughputService to change the 'scope of the DQM outputs' to RUN, otherwise I would not see the HLT/Throughput folder in the harvesting output of the unit test; I copied this scope change from FastTimerService, and I figure that's what was needed. I'm not sure it is the correct change; clearly the ThroughputService outputs were already produced correctly in the online DQM (maybe in that case the default scope is somehow set differently compared to this unit test);
I removed the PSMonitorClient; it does not produce outputs, it can only issue a warning, and maybe this is not so useful, but also here feedback would be needed.

This PR requires #40325.

Merely technical. No changes expected.

PR validation:

Manual tests with new unit test.

If this PR is a backport, please specify the original PR and why you need to backport that PR. If this PR will be backported, please specify to which release cycle the backport is meant for:

N/A

missirol · 2022-12-15T17:50:22Z

HLTrigger/Configuration/python/customizeHLTforCMSSW.py

@@ -258,5 +269,6 @@ def customizeHLTforCMSSW(process, menuType="GRun"):

    process = customizeHLTfor38761(process)
    process = customizeHLTfor40264(process)
+    process = customizeHLTfor40334(process)


This customisation is not necessary for the menus in CMSSW. It can become necessary for user menus extracted from ConfDB, if those somehow contain a PSMonitor.

missirol · 2022-12-15T17:51:58Z

HLTrigger/Timer/plugins/ThroughputService.cc

@@ -85,6 +85,7 @@ void ThroughputService::preGlobalBeginRun(edm::GlobalContext const& gc) {

    // define a callback that can book the histograms
    auto bookTransactionCallback = [&, this](DQMStore::IBooker& booker, DQMStore::IGetter&) {
+      auto scope = dqm::reco::DQMStore::IBooker::UseRunScope(booker);


This is copied from

cmssw/HLTrigger/Timer/plugins/FastTimerService.cc

Line 921 in c0d007c

auto scope = dqm::reco::DQMStore::IBooker::UseRunScope(booker);

Without this call, the harvesting output of the unit test does not contain a Throughtput folder, and I'm not really sure why that is.

Short version: I think this change is okay. More info below.

I had a look at https://github.com/cms-sw/cmssw/blob/master/DQMServices/Core/README.md, but that didn't really clarify things for me.

If I understand the interface, the DQM scope in ThroughputService before the call to

auto scope = dqm::reco::DQMStore::IBooker::UseRunScope(booker);

corresponds to scope.oldscope. The latter returns 1, which corresponds to JOB (and after the call, the scope becomes RUN). This seems to match the default set here, i.e. JOB.

The call to use the RUN scope in FastTimerService was introduced in #28622 by DQM (maybe ThoughputService was simply overlooked in that PR). I don't see a reason why FastTimerService and ThroughputService should differ in this respect.

With this PR, I managed to produce a ROOT output file with the client hlt_dqm_clientPB-live_cfg.py reading .pb files produced by re-running a recent HLT menu on 2022 data; in that ROOT output file, I see the HLT/Throughput folder, and the plots look as expected. This suggests that this PR does not break the workflow to produce these plots online (which somehow was already working).

Based on the above, I would conclude that this change is okay, even though I don't fully understand it; in particular, I don't know why the HLT/Throughput plots were already being produced correctly in the online DQM without this PR.

@cms-sw/dqm-l2 , do you have insight on this?

missirol · 2022-12-15T17:53:36Z

HLTrigger/Timer/plugins/ThroughputService.cc

@@ -95,7 +96,7 @@ void ThroughputService::preGlobalBeginRun(edm::GlobalContext const& gc) {
    };

    // book MonitorElement's for this run
-    edm::Service<DQMStore>()->meBookerGetter(bookTransactionCallback);
+    edm::Service<dqm::legacy::DQMStore>()->meBookerGetter(bookTransactionCallback);


Maybe this change is unimportant; again it follows what is done in the FastTimerService

cmssw/HLTrigger/Timer/plugins/FastTimerService.cc

Line 940 in c0d007c

edm::Service<dqm::legacy::DQMStore>()->meBookerGetter(bookTransactionCallback);

In ThroughputService, the following is used

cmssw/HLTrigger/Timer/plugins/ThroughputService.h

Line 32 in c0d007c

typedef dqm::reco::DQMStore DQMStore;

what difference does it make to use dqm::legacy instead of dqm::reco ?

answering to myself: none, they are typedef one to the other.

Thanks for the info. Since there is no difference, I will remove the extra dqm::legacy:: from here.

Edit : done in fda3c14.

missirol · 2022-12-15T17:55:13Z

test parameters:

pull_requests = initialise pointers in subclasses of FastTimerService #40325

cmsbuild · 2022-12-15T17:56:15Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-40334/33420

This PR adds an extra 32KB to repository

cmsbuild · 2022-12-15T17:56:40Z

A new Pull Request was created by @missirol (Marino Missiroli) for master.

It involves the following packages:

DQM/HLTEvF (dqm, hlt)
DQM/Integration (dqm)
HLTrigger/Configuration (hlt)
HLTrigger/Timer (hlt)

@Martin-Grunewald, @emanueleusai, @ahmad3213, @cmsbuild, @missirol, @jfernan2, @syuvivida, @pmandrik, @micsucmed, @rvenditti can you please review it and eventually sign? Thanks.
@batinkov, @battibass, @silviodonato, @mtosi, @Martin-Grunewald, @fwyzard, @threus, @francescobrivio this is something you requested to watch as well.
@perrotta, @dpiparo, @rappoccio you are the release manager for this.

cms-bot commands are listed here

missirol · 2022-12-15T17:59:58Z

@fwyzard , it would be very useful to have your feedback on this PR.

missirol · 2022-12-15T22:42:25Z

please test

cmsbuild · 2022-12-16T02:29:31Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-c89ae2/29647/summary.html
COMMIT: cb81125
CMSSW: CMSSW_13_0_X_2022-12-15-1100/el8_amd64_gcc11
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/40334/29647/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: 14 differences found in the comparisons
DQMHistoTests: Total files compared: 49
DQMHistoTests: Total histograms compared: 3557521
DQMHistoTests: Total failures: 163
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3557336
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 48 files compared)
Checked 211 log files, 162 edm output root files, 49 DQM output files
TriggerResults: no differences found

missirol · 2022-12-20T11:46:03Z

test parameters:

missirol · 2022-12-20T11:50:10Z

please test

Rebased on IB which includes #40325, and updated following the discussion in #40334 (comment).

missirol · 2023-01-02T21:08:15Z

please test

cmsbuild · 2023-01-03T00:58:43Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-c89ae2/29783/summary.html
COMMIT: 444109c
CMSSW: CMSSW_13_0_X_2023-01-02-1100/el8_amd64_gcc11
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/40334/29783/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: 32 differences found in the comparisons
DQMHistoTests: Total files compared: 49
DQMHistoTests: Total histograms compared: 3555748
DQMHistoTests: Total failures: 1214
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3554512
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 48 files compared)
Checked 211 log files, 162 edm output root files, 49 DQM output files
TriggerResults: no differences found

missirol · 2023-01-09T10:15:53Z

+hlt

adds a unit test involving plugins for online-DQM that are part of online HLT menus (and usually not included in offline HLT menus)
to the best of my knowledge, these changes are okay, but https://github.com/cms-sw/cmssw/pull/40334/files#r1053516164 requires feedback from @cms-sw/dqm-l2

missirol · 2023-01-10T17:52:35Z

@cms-sw/dqm-l2, could you please review this PR? Thanks!

emanueleusai · 2023-01-13T04:45:24Z

Concerning the removal of the PSMonitorClient, I agree with you, It look like the process just printed a warning if plots are not present. That's odd.

Concerning the ThroughputService, your logic is correct, and I believe what is done in the FastTimerService is correct and can be safely copied to ThroughputService, but the fact that it worked already correctly online makes me suspicious. We could in principle test this online in playback to be extra safe, but we would need a backport...
What do you think?

missirol · 2023-01-13T09:24:48Z

Concerning the ThroughputService, your logic is correct, and I believe what is done in the FastTimerService is correct and can be safely copied to ThroughputService, but the fact that it worked already correctly online makes me suspicious. We could in principle test this online in playback to be extra safe, but we would need a backport...
What do you think?

Opening a backport PR to do this test is probably easy to do (for which cycle ? 12_4_X ?); I assume the idea is just to open the backport PR to do the test, and then it would not be merged.

On the other hand, I don't think the playback test would help checking the change in ThroughputService. If I understand what that test does, it takes existing streamer files and runs the online-DQM clients, so it would not exercise the ThroughputService (which runs 'inside' the HLT, when the streamer files are created).

To try and convince myself that this change is okay, I produced streamer files by rerunning the HLT step with this PR, and then manually ran one of the HLT online-DQM clients on those streamer files [1]. In this 'online-like' test, one gets the expected histograms in the Throughput folder (both with and without the scope change in ThroughputService); in the case of the unit test (which uses DQMIO, not .pb files), the Throughput folder only appears using the 'RUN' scope in ThroughputService (i.e. the change in this PR). So, things work with this PR, but I'm not fully clear on what this 'dqm scope' does, and if it somehow acts differently for DQMIO output vs .pb output (I take the liberty to tag the author of #28622, @schneiml, just in case he can chime in).

If you think the playback test is useful anyway, I'll open the backport PR to the relevant release cycle.

[1] Tested in CMSSW_13_0_X_2023-01-12-2300.

#!/bin/bash

# scram project CMSSW CMSSW_13_0_X_2023-01-12-2300
# cd CMSSW_13_0_X_2023-01-12-2300/src
# eval `scram runtime -sh`
# git cms-merge-topic cms-sw:40334
# scram build

INPUTFILE=root://eoscms.cern.ch//eos/cms/store/group/dpg_trigger/comm_trigger/TriggerStudiesGroup/STORM/RAW/Run2022F_EphemeralHLTPhysics0_run361468/26ce1488-8c46-436b-becd-6b41535dda79.root

HLTMENU=/users/missirol/test/dev/CMSSW_13_0_0/tmp/test01/cmssw40334/HLT/V3

[ -d run361468 ] || (convertToRaw -f 100 -l 100 -r 361468:172 -o . -- "${INPUTFILE}")

if [ ! -f hlt.py ]; then
  tmpfile=$(mktemp)
  hltConfigFromDB --configName "${HLTMENU}" > "${tmpfile}"
  cat <<@EOF >> "${tmpfile}"

process.load('run361468_cff')

process.hltOnlineBeamSpotESProducer.timeThreshold = int(1e6)

from HLTrigger.Configuration.common import producers_by_type
for producer in producers_by_type(process, 'PSMonitor'):
  if hasattr(producer, 'FolderName'):
    if not hasattr(producer, 'folderName'):
      producer.folderName = producer.FolderName
    del producer.FolderName
@EOF

  edmConfigDump "${tmpfile}" > hlt.py
fi

cmsRun hlt.py &> hlt.log

cmsRun DQM/Integration/python/clients/hlt_dqm_clientPB-live_cfg.py \
  runInputDir=. runNumber=361468 runkey=pp_run \
  scanOnce=True datafnPosition=4

# output file: ./upload/DQM_V0001_HLTpb_R000361468.root

emanueleusai · 2023-01-16T07:45:47Z

@missirol thank you very much for the detailed explanation. I now understand better your private test, and I agree this is sufficient for the PR to be approved.
I do not know well enough the underlying structure of the "scopes" to answer your question about the behavior of scopes between DQMIO output vs .pb. So any input from the original developers of the infrastructure is welcome, although I believe @schneiml is not with CMS anymore.
Generally speaking, the way I understand it, the "scope" separates MEs that are filled per-lumi, per-run, or per-job. So if you fill your MEs in the endRun the scope should be set to RUN and so forth.

emanueleusai · 2023-01-16T07:49:10Z

+1

private test works as expected, online test not necessary
spurious differences in DQM comparisons
understanding of the underlying structure of scopes can continue separately from this PR imho

cmsbuild · 2023-01-16T07:49:34Z

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

perrotta · 2023-01-16T08:38:30Z

+1

cmsbuild added this to the CMSSW_13_0_X milestone Dec 15, 2022

cmsbuild added code-checks-pending dqm-pending hlt-pending orp-pending pending-signatures tests-pending labels Dec 15, 2022

missirol force-pushed the devel_testTriggerMonitors branch from f68f532 to cb81125 Compare December 15, 2022 17:49

missirol commented Dec 15, 2022

View reviewed changes

cmsbuild added the requires-external label Dec 15, 2022

cmsbuild added code-checks-approved and removed code-checks-pending labels Dec 15, 2022

cmsbuild added tests-started and removed tests-pending labels Dec 15, 2022

cmsbuild added tests-approved and removed tests-started labels Dec 16, 2022

missirol force-pushed the devel_testTriggerMonitors branch from cb81125 to 9ff78e4 Compare December 20, 2022 11:45

cmsbuild added code-checks-pending tests-pending and removed tests-approved code-checks-approved labels Dec 20, 2022

cmsbuild removed the requires-external label Dec 20, 2022

cmsbuild added tests-rejected and removed tests-started labels Jan 2, 2023

cmsbuild added tests-started and removed tests-rejected labels Jan 2, 2023

cmsbuild added tests-approved and removed tests-started labels Jan 3, 2023

missirol mentioned this pull request Jan 6, 2023

guard TriggerRatesMonitor against unavailable trigger paths #40439

Merged

cmsbuild added hlt-approved and removed hlt-pending labels Jan 9, 2023

cmsbuild mentioned this pull request Jan 10, 2023

Portable Data Formats for Pixel Track Reconstruction #40465

Merged

cmsbuild mentioned this pull request Jan 12, 2023

Remove Phase-2 IT bricked design #40443

Merged

cmsbuild added dqm-approved fully-signed and removed dqm-pending pending-signatures labels Jan 16, 2023

cmsbuild added orp-approved and removed orp-pending labels Jan 16, 2023

cmsbuild merged commit c628e55 into cms-sw:master Jan 16, 2023

missirol deleted the devel_testTriggerMonitors branch February 3, 2023 17:36

missirol mentioned this pull request Mar 2, 2023

[AARCH64] Unit test DQM/HLTEvF/testTriggerMonitors fails with FileOpenError #40904

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add unit test for HLT online-DQM plugins #40334

add unit test for HLT online-DQM plugins #40334

missirol commented Dec 15, 2022

missirol Dec 15, 2022

missirol Dec 15, 2022

missirol Dec 20, 2022

missirol Dec 15, 2022

fwyzard Dec 20, 2022

fwyzard Dec 20, 2022

missirol Dec 20, 2022 •

edited

missirol commented Dec 15, 2022

cmsbuild commented Dec 15, 2022

cmsbuild commented Dec 15, 2022

missirol commented Dec 15, 2022

missirol commented Dec 15, 2022

cmsbuild commented Dec 16, 2022

missirol commented Dec 20, 2022

missirol commented Dec 20, 2022

missirol commented Jan 2, 2023

cmsbuild commented Jan 3, 2023

missirol commented Jan 9, 2023

missirol commented Jan 10, 2023

emanueleusai commented Jan 13, 2023

missirol commented Jan 13, 2023

emanueleusai commented Jan 16, 2023 •

edited

emanueleusai commented Jan 16, 2023

cmsbuild commented Jan 16, 2023

perrotta commented Jan 16, 2023

add unit test for HLT online-DQM plugins #40334

add unit test for HLT online-DQM plugins #40334

Conversation

missirol commented Dec 15, 2022

PR description:

PR validation:

If this PR is a backport, please specify the original PR and why you need to backport that PR. If this PR will be backported, please specify to which release cycle the backport is meant for:

missirol Dec 15, 2022

Choose a reason for hiding this comment

missirol Dec 15, 2022

Choose a reason for hiding this comment

missirol Dec 20, 2022

Choose a reason for hiding this comment

missirol Dec 15, 2022

Choose a reason for hiding this comment

fwyzard Dec 20, 2022

Choose a reason for hiding this comment

fwyzard Dec 20, 2022

Choose a reason for hiding this comment

missirol Dec 20, 2022 • edited

Choose a reason for hiding this comment

missirol commented Dec 15, 2022

cmsbuild commented Dec 15, 2022

cmsbuild commented Dec 15, 2022

missirol commented Dec 15, 2022

missirol commented Dec 15, 2022

cmsbuild commented Dec 16, 2022

Comparison Summary

missirol commented Dec 20, 2022

missirol commented Dec 20, 2022

missirol commented Jan 2, 2023

cmsbuild commented Jan 3, 2023

Comparison Summary

missirol commented Jan 9, 2023

missirol commented Jan 10, 2023

emanueleusai commented Jan 13, 2023

missirol commented Jan 13, 2023

emanueleusai commented Jan 16, 2023 • edited

emanueleusai commented Jan 16, 2023

cmsbuild commented Jan 16, 2023

perrotta commented Jan 16, 2023

missirol Dec 20, 2022 •

edited

emanueleusai commented Jan 16, 2023 •

edited