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

DQM: Remove dqmEndRun in DQMEDAnalyzer #28552

Merged
merged 3 commits into from Jan 9, 2020

Conversation

schneiml
Copy link
Contributor

@schneiml schneiml commented Dec 4, 2019

PR description:

Another step forward on the way towards edm::stream modules, I decided that we cannot support endRun operations in DQMEDAnalyzer. Same goes for begin/end lumi, but they are less common. I allow dqmBeginRun, since it logically makes sense and is also mostly save (It's hard to do anything illegal with DQM objects before bookHistograms).

The full API is still available in DQMOneEDAnalyzer, and the few modules having non-trivial endRun today where migrated to it. However, the majority was just debug printouts, which I simply removed.

This is separate PR (separate form the coming bigger changes, PR #28622 ), to make the changes affecting subsystem code more visible and allow a validation independent from the coming bigger PR.

PR validation:

Compiles, and no changes to file output are expected. There will be less log output in some configurations.

Some "summary" printouts where removed; these might be useful, but should not be part of DQM modules.

@cmsbuild
Copy link
Contributor

cmsbuild commented Dec 4, 2019

The code-checks are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

cmsbuild commented Dec 4, 2019

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-28552/13023

  • This PR adds an extra 988KB to repository

  • There are other open Pull requests which might conflict with changes you have proposed:

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

@cmsbuild
Copy link
Contributor

cmsbuild commented Dec 5, 2019

The code-checks are being triggered in jenkins.

@schneiml
Copy link
Contributor Author

schneiml commented Dec 5, 2019

Properly rebased to CMSSW_11_1_X_2019-12-04-2300 and fixed the code-format.

@cmsbuild
Copy link
Contributor

cmsbuild commented Dec 5, 2019

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-28552/13041

  • This PR adds an extra 564KB to repository

  • There are other open Pull requests which might conflict with changes you have proposed:

@cmsbuild
Copy link
Contributor

cmsbuild commented Dec 5, 2019

A new Pull Request was created by @schneiml (Marcel Schneider) for master.

It involves the following packages:

DQM/CTPPS
DQM/CastorMonitor
DQM/HLTEvF
DQM/L1TMonitor
DQM/Physics
DQM/PixelLumi
DQM/SiStripMonitorHardware
DQM/SiStripMonitorPedestals
DQM/TrackingMonitor
DQMOffline/CalibCalo
DQMOffline/JetMET
DQMOffline/L1Trigger
DQMOffline/Trigger
DQMServices/Core
DQMServices/Examples
HLTriggerOffline/Egamma
HLTriggerOffline/Exotica
HLTriggerOffline/Higgs
HLTriggerOffline/Muon
HLTriggerOffline/SUSYBSM
PhysicsTools/NanoAOD
SimTracker/SiPhase2Digitizer
Validation/EcalDigis
Validation/EventGenerator
Validation/HcalDigis
Validation/HcalHits
Validation/RecoEgamma
Validation/RecoMuon
Validation/RecoParticleFlow
Validation/SiTrackerPhase2V

@SiewYan, @andrius-k, @schneiml, @kpedro88, @Martin-Grunewald, @rekovic, @fioriNTU, @alberto-sanchez, @santocch, @peruzzim, @civanch, @cmsbuild, @agrohsje, @fwyzard, @efeyazgan, @mdhildreth, @jfernan2, @qliphy, @benkrikler, @mkirsano, @kmaeshima, @fgolf can you please review it and eventually sign? Thanks.
@rappoccio, @forthommel, @hatakeyamak, @argiro, @fioriNTU, @ahinzmann, @threus, @seemasharmafnal, @mmarionncern, @kreczko, @hdelanno, @battibass, @makortel, @jhgoh, @missirol, @HuguesBrun, @cericeci, @trocino, @schoef, @rociovilar, @abbiendi, @barvic, @GiacomoSguazzoni, @rovere, @VinInn, @jdamgov, @nhanvtran, @gkasieczka, @jandrea, @idebruyn, @ebrondol, @mtosi, @dgulhan, @clelange, @jdolen, @jan-kaspar, @Fedespring, @calderona, @gpetruc, @mariadalfonso, @folguera 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

@schneiml
Copy link
Contributor Author

schneiml commented Dec 5, 2019

please test

No changes expected.

@cmsbuild
Copy link
Contributor

cmsbuild commented Dec 5, 2019

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-run-pr-tests/3816/console Started: 2019/12/05 11:31

@schneiml
Copy link
Contributor Author

+1

Comparison is all clean now.

@schneiml
Copy link
Contributor Author

@fabiocos What about this one? As far as I see it is uncontroversial, just missing a lot of signatures.

Getting this in should also reduce the number of signatures on #28622.

@fabiocos
Copy link
Contributor

@schneiml the move back to DQMOneEDAnalyzer forces lack of concurrency for all the migrated classes, please correct me in case. What is the plan to move them to avoid this constraint, if any?

@schneiml
Copy link
Contributor Author

@fabiocos as of today, all DQMEDAnalyzers are edm::one modules watching lumi transitions and block concurrent lumisections (which is why we can't have them at this time).

The modules which are moved to DQMOneEDAnalyzer actually drop the lumisection watching and should be ok for concurrent lumis.

The modules moved to DQMOneLumiEDAnalyzer now explicitly request the current behavior and will continue to block concurrent lumis until manually modified.

The plan is that after #28622, we can change DQMEDAnalyzer to edm::stream, and release the "watching lumisections" constraint from ~all DQM modules, allowing concurrent lumisections in general. Of course, that is only as long as the DQMOneLumiEDAnalyzers are either not part of the relevant sequences or manually modified.

@Martin-Grunewald
Copy link
Contributor

+1

@kpedro88
Copy link
Contributor

+upgrade

@schneiml
Copy link
Contributor Author

@santocch @efeyazgan @rekovic @benkrikler @civanch @peruzzim Could you please have look and sign this PR? The changes should be rather trivial.

@santocch
Copy link

santocch commented Jan 2, 2020

+1

@schneiml
Copy link
Contributor Author

schneiml commented Jan 6, 2020

Still waiting for signatures, no real objections as far as I remember (@fabiocos ?).

@silviodonato
Copy link
Contributor

This PR seems ready to be merged but several signatures are still missing:
generators: @alberto-sanchez @SiewYan @qliphy @efeyazgan @mkirsano @agrohsje
l1: @benkrikler @rekovic
xpog: @fgolf @peruzzim
simulation: @mdhildreth @civanch
could you review and eventually sign this PR, please?

@silviodonato
Copy link
Contributor

+1

@silviodonato
Copy link
Contributor

please merge

@silviodonato
Copy link
Contributor

merge

@cmsbuild cmsbuild merged commit 1f5425d into cms-sw:master Jan 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment