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

pixel DQM@HLT #18252

Merged
merged 5 commits into from May 10, 2017
Merged

pixel DQM@HLT #18252

merged 5 commits into from May 10, 2017

Conversation

mtosi
Copy link
Contributor

@mtosi mtosi commented Apr 7, 2017

--finally--
we have the pixel@HLT DQM
this PR add the pixel@HLT DQM on both online and offline workflow

@fioriNTU
while testing this w/ runTheMatrix.py --limited
I saw that SiPixelPhase1TrackCluster was not checking the validity of the input collections,
not it is fixed

I also add the lumiMonitoring to the HLT offline DQM
(even if this module makes use of the lumi measurement from the scaler)

in addition, I fixed the strip DQM in the HLT online DQM as well
and add the monitoring of the HLT_ZeroBias_FirstCollisionAfterAbortGap
which is one of the calibration triggers for the tracker DPG (used for the strip APV gain)

finally, I add the client in the online DQM,
as now for both the tracking (iterative and GSF) and pixel

@cmsbuild
Copy link
Contributor

cmsbuild commented Apr 7, 2017

A new Pull Request was created by @mtosi (mia tosi) for CMSSW_9_0_X.

It involves the following packages:

DQM/HLTEvF
DQM/Integration
DQM/SiPixelPhase1TrackClusters
DQMOffline/Trigger

@cmsbuild, @silviodonato, @dmitrijus, @Martin-Grunewald, @fwyzard, @vanbesien, @davidlange6 can you please review it and eventually sign? Thanks.
@batinkov, @hdelanno, @battibass, @threus, @jhgoh, @calderona, @HuguesBrun, @fioriNTU, @idebruyn, @trocino, @rociovilar this is something you requested to watch as well.
@davidlange6, @smuzaffar you are the release manager for this.

cms-bot commands are listed here

@threus
Copy link
Contributor

threus commented Apr 7, 2017

@mtosi @fioriNTU since it's for online DQM workflow, I added this PR to https://twiki.cern.ch/twiki/bin/view/CMS/DQMP5TagCollector
and will start testing it.

@Martin-Grunewald
Copy link
Contributor

please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Apr 7, 2017

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/18991/console Started: 2017/04/07 12:34

@Martin-Grunewald
Copy link
Contributor

Hi, you need to make a 91X PR which gets integrated first (in 91X)

@mtosi
Copy link
Contributor Author

mtosi commented Apr 7, 2017 via email

@mtosi
Copy link
Contributor Author

mtosi commented Apr 7, 2017 via email

@cmsbuild
Copy link
Contributor

cmsbuild commented Apr 7, 2017

@cmsbuild
Copy link
Contributor

cmsbuild commented Apr 7, 2017

Comparison job queued.

@cmsbuild
Copy link
Contributor

cmsbuild commented Apr 7, 2017

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-18252/18991/summary.html

Comparison Summary:

  • You potentially added 81 lines to the logs
  • ROOTFileChecks: Some differences in event products or their sizes found
  • Reco comparison results: 0 differences found in the comparisons
  • DQMHistoTests: Total files compared: 23
  • DQMHistoTests: Total histograms compared: 1868728
  • DQMHistoTests: Total failures: 15044
  • DQMHistoTests: Total nulls: 21
  • DQMHistoTests: Total successes: 1853490
  • DQMHistoTests: Total skipped: 173
  • DQMHistoTests: Total Missing objects: 0
  • Checked 94 log files, 14 edm output root files, 23 DQM output files

@Martin-Grunewald
Copy link
Contributor

+1

@mtosi
Copy link
Contributor Author

mtosi commented Apr 10, 2017

while testing this PR on the DQM playback
by running on MC phase1 sample and CRUZET data (w/o pixel),
@threus found the error [1]
not sure which input sample has been used,
but most probably it is not the DQMOutput which is generally produced by HLT
=> the needed collections are missing
and the MeasurementTrackerEventProducer is not safe w.r.t. missing input collections

I need to update this PR

[1]

Begin processing the 1st record. Run 500150, Event 1, LumiSection 1 at 10-Apr-2017 10:00:34.241 CEST
%MSG-w SiPixelPhase1TrackClusters: SiPixelPhase1TrackClusters:hltSiPixelPhase1TrackClustersAnalyzer 10-Apr-2017 10:00:34 CEST Run: 500150 Event: 1
track collection is not valid
%MSG
----- Begin Fatal Exception 10-Apr-2017 10:00:35 CEST-----------------------
An exception of category 'ProductNotFound' occurred while
[0] Processing run: 500150 lumi: 1 event: 1
[1] Running path 'hlt4vector'
[2] Calling method for module MeasurementTrackerEventProducer/'hltMeasurementTrackerEvent'
Exception Message:
Principal::getByToken: Found zero products matching all criteria
Looking for type: edmNew::DetSetVector
Looking for module label: hltSiStripRawToClustersFacility
Looking for productInstanceName:

@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-18252/19456/summary.html

Comparison Summary:

  • You potentially added 1858 lines to the logs
  • ROOTFileChecks: Some differences in event products or their sizes found
  • Reco comparison results: 0 differences found in the comparisons
  • DQMHistoTests: Total files compared: 23
  • DQMHistoTests: Total histograms compared: 1837442
  • DQMHistoTests: Total failures: 14954
  • DQMHistoTests: Total nulls: 21
  • DQMHistoTests: Total successes: 1822294
  • DQMHistoTests: Total skipped: 173
  • DQMHistoTests: Total Missing objects: 0
  • Checked 94 log files, 14 edm output root files, 23 DQM output files

@perrotta
Copy link
Contributor

perrotta commented Apr 28, 2017

@mtosi : there are indeed a handful of other differences between this PR and the one merged in the master. First of all, the ones I don't care about (just for your info):

  • DQM/Integration/python/clients/hlt_dqm_sourceclient-live_cfg.py: maxEvents not set in pixel DQM@HLT #18252
  • DQM/Integration/python/config/inputsource_cfi.py only in pixel DQM@HLT #18264
  • Differences in DQMOffline/Trigger/python/SiPixel_OfflineMonitoring_Cluster_cff.py

Then, in the following I notice that L162 was removed in #18264 and not here:

  • DQM/SiPixelPhase1Summary/src/SiPixelPhase1Summary.cc

Are you sure that it had to be removed from #18264? (It doesn't look so to me; but then, if it is correct you would like to remove that line also from here)

Finally, about what mostly refers to reco in this PR: the protections added to

  • RecoTracker/MeasurementDet/plugins/MeasurementTrackerEventProducer.cc

are all quite reasonable, but I don't see them in your 91X PR, and not even merged in the master with some other pull request. Is there any reason why they should only stay here and not in the master? (Or perhaps they are already queued with some other PR and I just did not spot them)?

@mtosi
Copy link
Contributor Author

mtosi commented Apr 28, 2017

opsi
thanks @perrotta

ok, let me check

  1. DQM/Integration/python/clients/hlt_dqm_sourceclient-live_cfg.py: maxEvents not set in pixel DQM@HLT #18252
  2. DQM/Integration/python/config/inputsource_cfi.py only in pixel DQM@HLT #18264
  3. Differences in DQMOffline/Trigger/python/SiPixel_OfflineMonitoring_Cluster_cff.py

point 1. and 2. probably should not be in any PR :(
=> they should be fixed in the master :( and I'll do a PR (sorry)

the differences in point 3. should simply reflect the differences between the 2 releases of the main SiPixelPhase1 DQM code (there are some updates / new features only available in the master)

  1. DQM/SiPixelPhase1Summary/src/SiPixelPhase1Summary.cc
    for point 4. I do not see L192 :(

  2. RecoTracker/MeasurementDet/plugins/MeasurementTrackerEventProducer.cc
    this indeed I'm afraid has been spotted only w/in the test of this PR
    and when I asked if it would have made sense to have it in the PR for 91x
    I think I did not receive any comment and then I forgot ! (sorry)

=> if you agree, I would leave this PR as it is now
and add a new PR for the master in order to address points 1,2, and 5

thanks again for your help/support (and patience !)

@perrotta
Copy link
Contributor

  1. DQM/SiPixelPhase1Summary/src/SiPixelPhase1Summary.cc
    for point 4. I do not see L192 :(

It was L162, in fact (not wearing my glasess...):

immagine_mtosi

@perrotta
Copy link
Contributor

Fine with me if you integrate in this PR the "best possible" version, and propagate it to the master with another dedicated PR.

If you keep the codes in the different releases synchronized (unless needed otherwise) it then becomes easier for you to propagate the possible fixes... and for us to review!

Ciao,
A.

@mtosi
Copy link
Contributor Author

mtosi commented Apr 28, 2017

ehehe ;)
thanks !
I prepared the branch for fixing the master
and opened PR #18520

@Martin-Grunewald
Copy link
Contributor

+1

@perrotta
Copy link
Contributor

perrotta commented May 4, 2017

+1
RecoTracker/MeasurementDet/plugins/MeasurementTrackerEventProducer.cc is now identical to what already merged in 91X
About my previous comments in #18252 (comment): - the issue with DQM/SiPixelPhase1Summary/src/SiPixelPhase1Summary.cc had been fixed in 91X (and it was already correct here)

  • the aesthetic differences between the files in the DQM area are annoying, but of no concern for reco

@Martin-Grunewald
Copy link
Contributor

@dmitrijus
Please sign this PR.
Thanks!

@dmitrijus
Copy link
Contributor

+1

@cmsbuild
Copy link
Contributor

cmsbuild commented May 9, 2017

This pull request is fully signed and it will be integrated in one of the next CMSSW_9_0_X IBs (tests are also fine) and once validation in the development release cycle CMSSW_9_2_X is complete. This pull request requires discussion in the ORP meeting before it's merged. @davidlange6, @smuzaffar

@davidlange6
Copy link
Contributor

+1

@cmsbuild cmsbuild merged commit bf1ce5d into cms-sw:CMSSW_9_0_X May 10, 2017
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

7 participants