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

Hcal DQM remove OneLumiEDAnalyzer #29544

Merged
merged 5 commits into from
Apr 30, 2020
Merged

Conversation

lwang046
Copy link
Contributor

A fix to Hcal related legacy modules raised in issues #25090.

Passed local runTheMatrix.py -l limited -i all --ibeos test without fails.

@cmsbuild
Copy link
Contributor

The code-checks are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-29544/14828

  • This PR adds an extra 80KB to repository

Code check has found code style and quality issues which could be resolved by applying following patch(s)

@cmsbuild
Copy link
Contributor

The code-checks are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-29544/14830

  • This PR adds an extra 80KB to repository

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @lwang046 for master.

It involves the following packages:

DQM/HcalCommon
DQM/HcalTasks

@andrius-k, @kmaeshima, @schneiml, @cmsbuild, @jfernan2, @fioriNTU can you please review it and eventually sign? Thanks.
@DryRun this is something you requested to watch as well.
@silviodonato, @dpiparo you are the release manager for this.

cms-bot commands are listed here

@jfernan2
Copy link
Contributor

please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Apr 23, 2020

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-run-pr-tests/5837/console Started: 2020/04/23 16:27

@cmsbuild
Copy link
Contributor

+1
Tested at: 5695eab
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-6725ce/5837/summary.html
CMSSW: CMSSW_11_1_X_2020-04-23-1100
SCRAM_ARCH: slc7_amd64_gcc820

@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-6725ce/5837/summary.html

Comparison Summary:

  • No significant changes to the logs found
  • Reco comparison results: 2 differences found in the comparisons
  • DQMHistoTests: Total files compared: 34
  • DQMHistoTests: Total histograms compared: 2696435
  • DQMHistoTests: Total failures: 218
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 2695898
  • DQMHistoTests: Total skipped: 319
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 33 files compared)
  • Checked 147 log files, 16 edm output root files, 34 DQM output files

@abdoulline
Copy link

@DryRun @lwang046
Could you, please, remind me/us whether DigiPhase1Task is now obsolete (and why)? - To possibly just exclude it.

@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-6725ce/5887/summary.html

Comparison Summary:

  • No significant changes to the logs found
  • Reco comparison results: 2 differences found in the comparisons
  • DQMHistoTests: Total files compared: 34
  • DQMHistoTests: Total histograms compared: 2695267
  • DQMHistoTests: Total failures: 30
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 2694918
  • DQMHistoTests: Total skipped: 319
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: -6778.892 KiB( 33 files compared)
  • DQMHistoSizes: changed ( 10024.0,... ): -1495.319 KiB Hcal/DigiPhase1Task
  • DQMHistoSizes: changed ( 20034.0,... ): 3891.355 KiB Hcal/DigiTask
  • DQMHistoSizes: changed ( 20034.0,... ): 395.128 KiB Hcal/DigiRunHarvesting
  • DQMHistoSizes: changed ( 20034.0,... ): 0.070 KiB Hcal/EventInfo
  • Checked 147 log files, 16 edm output root files, 34 DQM output files

@jfernan2
Copy link
Contributor

@lwang046 I understand that DigiTask contains the old/reduced DigiPhase1Task.
However I see for Phase2 in addition a new DigiRunHarvesting folder created inside Hcal folder. But I don't see to find out where it does come from, I guess it is coming from DigiTask downstream since now it is not a copyAndExclude but toModify. It contains 47 MEs, it would be good to understand them.
https://cmssdt.cern.ch/SDT/jenkins-artifacts/baseLineComparisons/CMSSW_11_1_X_2020-04-28-1100+6725ce/36042/dqm-histo-comparison-summary.html
Any idea?

@jfernan2
Copy link
Contributor

On a totally different subject: the changes affecting to Online DQM are not and cannot be tested here in github. And we (DQM) cannot test them since P5 is down at present. So, I understand you have made some independent validation that the plots including per LS/events are remaining unchanged.
Thanks

@jfernan2
Copy link
Contributor

Related to my last comment: the reportSummaryMap which contains info vs LS changes already at Offline
https://tinyurl.com/ycxjwe6a

@jfernan2
Copy link
Contributor

Another strange fact is double number of processed events in non-Phase2 workflows, eg.:

https://tinyurl.com/y9r2un8q

@lwang046
Copy link
Contributor Author

lwang046 commented Apr 28, 2020

@lwang046 I understand that DigiTask contains the old/reduced DigiPhase1Task.
However I see for Phase2 in addition a new DigiRunHarvesting folder created inside Hcal folder. But I don't see to find out where it does come from, I guess it is coming from DigiTask downstream since now it is not a copyAndExclude but toModify. It contains 47 MEs, it would be good to understand them.
https://cmssdt.cern.ch/SDT/jenkins-artifacts/baseLineComparisons/CMSSW_11_1_X_2020-04-28-1100+6725ce/36042/dqm-histo-comparison-summary.html
Any idea?

Hi @jfernan2, the DigiRunHarvesting folder comes from here: https://github.com/cms-sw/cmssw/blob/master/DQM/HcalTasks/plugins/HcalOfflineHarvesting.cc#L13-L24. Previously with the DigiPhase1Task the std::find(_summaryList.begin(), _summaryList.end(), fDigi) condition was not fulfilled so there was no such folder, but now the condition is true.

@lwang046
Copy link
Contributor Author

lwang046 commented Apr 28, 2020

Another strange fact is double number of processed events in non-Phase2 workflows, eg.:

https://tinyurl.com/y9r2un8q

Forgiving me for a stupid question, if the same ME was booked twice:

ib.setCurrentFolder(_subsystem + "/RunInfo");
meNumEvents1LS = ib.book1D("NumberOfEvents", "NumberOfEvents", 1, 0, 1);
&
ib.setCurrentFolder(_subsystem + "/RunInfo");
meNumEvents1LS = ib.book1D("NumberOfEvents", "NumberOfEvents", 1, 0, 1);
, would the previously filled one be cleared or be added? Also I thought for this workflow there were only 10 events generated in the cmdlog? If these 2 wonderments are correct, that will explain why there were 20 events before but 10 events now?

NB: I did a local test with the master branch by commenting out one of these 2 same ME filling cases and it confirmed that previously it was filled twice. I guess this was overlooked before when the DigiPhase1Task was used as an assistant module. But now the event number is correct.

@lwang046
Copy link
Contributor Author

lwang046 commented Apr 28, 2020

On a totally different subject: the changes affecting to Online DQM are not and cannot be tested here in github. And we (DQM) cannot test them since P5 is down at present. So, I understand you have made some independent validation that the plots including per LS/events are remaining unchanged.
Thanks

Related to my last comment: the reportSummaryMap which contains info vs LS changes already at Offline https://tinyurl.com/ycxjwe6a

@jfernan2 My apologies I failed to understand your comments here, may I ask which checks you were looking for? If the question was why now we have an extra DIGI on Y-axis, the reason is very similar to the DigiRunHarvesting folder issue, now that we replaced DigiPhase1Task with DigiTask, the DIGI condition (

for (auto& it_sum : _summaryList) {
if (_summarks[it_sum]) {
datatiers.insert(std::pair<std::string, int>(datatier_names[it_sum], num));
) is fulfilled thus it's plotted.

@jfernan2
Copy link
Contributor

Thanks a lot @lwang046
I understand then that, the different plot on:
https://tinyurl.com/ycxjwe6a
which is related to the tests I was proposing for Online DQM, it is fine to have the digi values in red now.
About extra Online DQM tests, let me explain: with this PR, the way the plots which have information vs time/LS are filled changes, so the plots content may be differet too. The link above is the only one which seems to change in Offline, but it is understood since Digis are now checked, assuming red color is expected.

Hence ,in Online DQM you should probably test the changes running standalone the two clients
DQM/Integration/python/clients/hcalreco_dqm_sourceclient-live_cfg.py
DQM/Integration/python/clients/hcal_dqm_sourceclient-live_cfg.py
to avoid surprises once we turn on the DQM Online Clients at P5
Thank you very much

@jfernan2
Copy link
Contributor

+1
After private succesful tests for Online DQM output

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

@abdoulline
Copy link

@jfernan2 thank you very much for your help!
@lwang046 congrats for your first major contribution to HCAL Online DQM!

@silviodonato
Copy link
Contributor

+1

@makortel
Copy link
Contributor

It seems that this PR might have caused 3 workflows to segfault in HARVESTING step, see #29605.

@schneiml
Copy link
Contributor

schneiml commented May 1, 2020

Hi @lwang046, to have this clarified:

Forgiving me for a stupid question, if the same ME was booked twice:

If you book the same name twice, you get the same object in both places. If you then call fill from two modules, you see all the data in one histogram.

That is how the "Number of Events" histograms showed double number of events. This behavior changed recently (in the new DQMStore work, merged in 11_1), and these plots were among the few changes noticed there.

@jfernan2
Copy link
Contributor

Resolves #25090
(Partially)

@lwang046 lwang046 deleted the HcalDQM-FixLegacy branch July 16, 2020 12:15
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.

9 participants