SiStripMonitorClient cleanup

[knoepfel]

The DQM/SiStripMonitorClient directly uses the DQMStore interface, instead of using the IBooker and IGetter transaction classes. In addition, a pointer to the DQMStore object is cached in several plugins. This PR removes explicitly caching of the DQMStore pointer. In addition, many C++ improvements are included related to ownership semantics, updated syntax, etc.

To actually move to using the IBooker and IGetter interface will require more effort.

Clang-format has been run for this PR--those changes are included as a separate commit.

[cmsbuild]

The code-checks are being triggered in jenkins.

[cmsbuild]

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-24467/6312

[cmsbuild]

A new Pull Request was created by @knoepfel (Kyle Knoepfel) for master.

It involves the following packages:

DQM/SiStripMonitorClient

@kmaeshima, @cmsbuild, @andrius-k, @jfernan2, @schneiml can you please review it and eventually sign? Thanks.
@hdelanno, @fioriNTU, @jandrea, @idebruyn, @threus, @venturia this is something you requested to watch as well.
@davidlange6, @slava77, @fabiocos you are the release manager for this.

cms-bot commands are listed here

[andrius-k]

please test

[cmsbuild]

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/30273/console Started: 2018/09/05 15:10

[mmusich]

@suchandradutta FYI

[mmusich]

@knoepfel, thanks for the cleanup. May I ask why it was decided to do this now?
I think this will require quite some degree of validation on the Tk DPG side, as 10_3_X is a production release (that we'll use to certify 2018 PbPb data). Any reason this could not be postponed to LS2?

[knoepfel]

Hi @mmusich, good questions. @Dr15Jones, @schneiml and I have been working toward upgrading uses of DQMStore to support concurrent processing of events, lumis, etc. One of the impediments to getting there has been the direct use of the DQMStore interface. We are going through the DQM codebase trying to remove such uses. This PR is a step in that direction.

As far as when to include these changes, I do not have a strong preference. Note, however, that in a few weeks my development time on CMS will be zero due to other responsibilities. Also, none of the changes I made should have any effect on the histogram contents--hopefully there are enough PR tests that can exercise the modified code...?

[jandrea]

@knoepfel Thank you very much for this. This seems to me like a major modification of the code, which is always a little scary when operations are running. May I ask you why these modifications were urgently needed ? Has somebody made sure that the DQM plots are left unchanged ?

[knoepfel]

@jandrea, see my comments to Marco.

[jandrea]

@knoepfel sorry, our messages just crossed. I understand from your answer to Marco that there are no major issues with the current code. I also understand that it was not fully tested that the plots are left unchanged (?). So I would feel much more comfortable if we could postpone this PR to at least when these tests are performed ?

[knoepfel]

@jandrea, are you saying the integration tests that are currently being executed (as a consequence of @andrius-k typing please test) will not sufficiently test the modified code?

[jandrea]

@knoepfel I actually don't know. Your modifications potentially affects all the plots, right ? I guess one would need to run on data with the old and new codes and make sure plots are the same ? Is this kind of things automatised ?

[knoepfel]

@jandrea, many tests are run during the PR "please test" phase, but I do not know which ones actually exercise the SiStripMonitorClient code. @schneiml, @Dr15Jones are you able to look if any runTheMatrix.py PR-testing workflows contain the modules in DQM/SiStripMonitorClient/plugins?

[cmsbuild]

+1
Tested at: 7931758
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-24467/30273/summary.html

[cmsbuild]

Comparison job queued.

[knoepfel]

We took an example module (SiStripOfflineDQM), and traced the following imports:

• DQM/SiStripMonitorClient/python/SiStripClientConfig_Tier0_cff.py :
  siStripOfflineAnalyser_ = cms.EDAnalyzer("SiStripOfflineDQM", imported by
• DQMOffline/Configuration/python/DQMOffline_SecondStep_cff.p :
  from DQM.SiStripMonitorClient.SiStripClientConfig_Tier0_cff import * imported by
• Configuration/StandardSequences/python/Harvesting_cff.py :
  from DQMOffline.Configuration.DQMOffline_SecondStep_cff import * imported by
• Configuration/Applications/python/ConfigBuilder.py : self.HARVESTINGDefaultCFF="Configuration/StandardSequences/Harvesting_cff", which is used by cmsDriver.py for the harvesting step

Once the comparison job is done, we can make sure the histograms match with the reference histograms.

[Dr15Jones]

@fabiocos I'm afraid @knoepfel no longer has any time on CMS. He was gracious enough to try to quickly resolve the merge conflicts via the web interface, however there is no opportunity to rebase.

[cmsbuild]

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-24467/8539

• This PR adds an extra 76KB to repository

[cmsbuild]

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/33296/console Started: 2019/02/26 15:57

[cmsbuild]

Pull request #24467 was updated. @andrius-k, @kmaeshima, @schneiml, @cmsbuild, @jfernan2, @fioriNTU can you please check and sign again.

[arossi83]

@fabiocos I think that indeed @arossi83 has validated the changes and the PR can proceed.

I confirm, I perform some local test and everything was fine.

[cmsbuild]

+1
Tested at: d548099
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-24467/33296/summary.html

The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic:
d548099
9ae739a
fe753c4
c6d3ed5
f9e6f12
d3568c2
af644fd
You can see more details here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-24467/33296/git-log-recent-commits
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-24467/33296/git-merge-result

[cmsbuild]

Comparison job queued.

[cmsbuild]

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

Comparison Summary:

• No significant changes to the logs found
• Reco comparison results: 0 differences found in the comparisons
• DQMHistoTests: Total files compared: 32
• DQMHistoTests: Total histograms compared: 3098286
• DQMHistoTests: Total failures: 1
• DQMHistoTests: Total nulls: 0
• DQMHistoTests: Total successes: 3098088
• DQMHistoTests: Total skipped: 197
• DQMHistoTests: Total Missing objects: 0
• DQMHistoSizes: Histogram memory added: 0.0 KiB( 31 files compared)
• Checked 133 log files, 14 edm output root files, 32 DQM output files

[fabiocos]

unhold

[fabiocos]

@andrius-k @schneiml do you confirm the signature?

[mmusich]

why is this file and the corresponding header being removed entirely?

[knoepfel]

Honestly, I don't remember why I removed it. Of course, it could be added back if needed. As @Dr15Jones said above, I have no time to spend on CMS code, so I'm afraid someone else with have to restore it if that is necessary.

[Dr15Jones]

I just checked, and nothing in CMSSW refers to SiStripInformationExtractor. It is therefore easier to remove then to continue fixing.

[andrius-k]

+1

[cmsbuild]

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)

[fabiocos]

+1

tests performed by the SiStrip groups outside of the regular production workflows were positive