DQM: Use ProcessBlock in Harvesting #30698

schneiml · 2020-07-15T11:58:31Z

PR description:

This PR aims to use the new ProcessBlock feature in DQM HARVESTING to fix issue #28354.

See also my recent talk about how this works: https://indico.cern.ch/event/934933/contributions/3928462/attachments/2072544/3479621/dqm-cmssw.pdf (Page 50).

Caveats:

It does not work for now. Either the dependencies are not correctly set up at all or at least process.Tracer = cms.Service("Tracer", dumpPathsAndConsumes = cms.untracked.bool(True)) does not correctly display them. Fixed with Add to GetterOfProducts support for ProcessBlock #30737.
This only covers DQM HARVESTING. ALCAHARVEST most likely has the same hidden dependency problem. For the DQM parts of the AlCa setup this PR will do something, but I don't know which modules actually write the conditions payload at endJob and if their assumptions remain within the guarantees of edm.
I don't know if there are any hidden dependencies left. They are hidden, after all. But at least the most obvious (between any harvesting module and the DQMFileSaver should be covered and more depdendencies can be added it if turns out to be required. (Update: One more dependency around QTests in SiPixelPhase1Summary found and fixed here. There might be more like these, and they might show up randomly now and then.)
I did not change how HARVESTING/QTests work. The interesting case here is harvesting/qtest/harvesting sequences, where as of today, we do the first harvesting and the qtests in endRun and then the second harvesting in endJob (now endProcessBlock). That was the only way to do it without ProcessBlocks. Now we could do it all in endProcessBlock (a.k.a. dqmEndJob) using token dependencies and two separate harvesting modules, but I did not perform this transform. In practice, the only use case for such a setup is SiStripMonitorClient, and I am happy that it works as is and don't want to break and debug it another time.
I have no idea how to actually set up process blocks for multi-run harvesting for now. I just replaced endJob with endProcessBlock for now, which should cover all existing cases. With ProcessBlocks, it should be possible again to harvest multiple runs independently in one job (or even multi-run harvest multiple groups of runs), but there is no need or support for that right now.
endJob harvesting is now no longer supported in legacy modules. It was not really used anyways. I removed a bunch of empty endJob methods to avoid confusion. Some modules still have endJob harvesting code, but it is not used in production; it can still work in stand-alone setups that do not rely on DQMFileSaver.

PR validation:

Dependencies are displayed by the tracer, and the PR tests seem ok. This may break some stand-alone workflows (esp. Validation) if they use legacy modules, use end-job harvesting, and use DQMFileSaver. (Not sure if that combination exists anywhere).

@wddgit @makortel FYI.

cmsbuild · 2020-07-15T11:58:53Z

The code-checks are being triggered in jenkins.

cmsbuild · 2020-07-15T12:04:49Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-30698/16998

This PR adds an extra 16KB to repository

cmsbuild · 2020-07-15T12:05:11Z

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

It involves the following packages:

DQMServices/Components
DQMServices/Core

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

cms-bot commands are listed here

schneiml · 2020-07-15T14:23:40Z

please test

I don't expect anything interesting but just to be sure.

cmsbuild · 2020-07-15T14:24:05Z

The tests are being triggered in jenkins.

CMSSW_11_2_X_2020-07-14-2300/slc7_amd64_gcc820: https://cmssdt.cern.ch/jenkins/job/ib-run-pr-tests/7966/console Started: 2020/07/15 16:27

makortel · 2020-07-15T14:38:07Z

Thanks @schneiml for trying out!

It does not work for now. Either the dependencies are not correctly set up at all or at least process.Tracer = cms.Service("Tracer", dumpPathsAndConsumes = cms.untracked.bool(True)) does not correctly display them.

By quick look we are (at least) missing the ProcessBlock here

cmssw/FWCore/Framework/interface/WillGetIfMatch.h

Lines 32 to 38 in b062071

    
           if (transition == edm::InEvent) { 
        
             return module_->template consumes<T>(tag); 
        
           } else if (transition == edm::InLumi) { 
        
             return module_->template consumes<T, edm::InLumi>(tag); 
        
           } else if (transition == edm::InRun) { 
        
             return module_->template consumes<T, edm::InRun>(tag); 
        
           }

@wddgit Would you be able to take a deeper look?

I have no idea how to actually set up process blocks for multi-run harvesting for now. I just replaced endJob with ednProcessBlock for now, which should cover all single-run (and maybe even all current multi-run?) cases.

A single ProcessBlock should cover arbitrary number of runs processed by a single job. (strictly speaking as long as the output module does not change files, but our feeling is that no-one uses that feature of PoolOutputModule anyway)

wddgit · 2020-07-15T14:53:24Z

I will take a look and investigate.

cmsbuild · 2020-07-15T15:50:21Z

-1

Tested at: 150f194

CMSSW: CMSSW_11_2_X_2020-07-14-2300
SCRAM_ARCH: slc7_amd64_gcc820
You can see the results of the tests here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-ce60a8/7966/summary.html

I found follow errors while testing this PR

Failed tests: UnitTests

Unit Tests:

I found errors in the following unit tests:

---> test TestDQMServicesDemo had ERRORS

cmsbuild · 2020-07-15T15:50:23Z

Comparison job queued.

wddgit · 2020-07-15T16:02:50Z

Yes, I missed GetterOfProducts. It is not implemented for ProcessBlock. I am working on adding support for that now.

When we were discussing the ProcessBlock design, we decided to only have support for getByToken. I think the main point was that we intentionally did not want to provide support for getByLabel and getManyByType. We didn't actually discuss GetterOfProducts, but it makes sense to support that. I will work on that now. It may be as simple as adding a couple lines at the point Matti referenced above and adding a unit test. Internally it is using tokens and depends on the same things as getByToken.

cmsbuild · 2020-07-15T17:30:48Z

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

@slava77 comparisons for the following workflows were not done due to missing matrix map:

/data/cmsbld/jenkins/workspace/compare-root-files-short-matrix/data/PR-ce60a8/28234.0_TTbar_14TeV+TTbar_14TeV_TuneCP5_2026D60_GenSimHLBeamSpotFull14+DigiFullTrigger_2026D60+RecoFullGlobal_2026D60+HARVESTFullGlobal_2026D60

Comparison Summary:

No significant changes to the logs found
Reco comparison results: 4 differences found in the comparisons
DQMHistoTests: Total files compared: 35
DQMHistoTests: Total histograms compared: 2612943
DQMHistoTests: Total failures: 7
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 2612888
DQMHistoTests: Total skipped: 48
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 34 files compared)
Checked 146 log files, 17 edm output root files, 35 DQM output files

wddgit · 2020-07-15T20:57:31Z

PR #30737 adds support to GetterOfProducts for ProcessBlock (not merged yet, tests running at the moment). Thanks for reporting the problem and trying this. Let us know if you notice any other problems or have questions.

schneiml · 2020-07-16T10:30:29Z

The unit test failure is a real problem, due to legacy modules which are (for now?) still fully supported in harvesting. Since the endProcessBlock happens before any endJob, and the legacy modules do their harvesting work in endJob, their output can no longer be saved.

I see two options:

Still allow a save in endJob in DQMFileSaver for legacy setups. This makes everything more complicated and error prone, again.
Migrate all legacy modules using endJob to endProcessBlock. I have no idea how many of these we have.

Edit: The comparison looks pretty clean, so at least we don't use many such legacy modules commonly.
Edit2: According to cmsswSequenceInfo (http://cmsswsequenceinfo.cern.ch:8000/), there are 12 legacy modules used in runTheMatrix workflows. Of these, most don't do anything in endJob. Four of them have to option to run harvesting work in endJob, but are configured not to do that in the runTheMatrix setups. This is surprising to me, but makes everything easier (I think these modules are old enough to have missed the migration of harvesting from endRun to endJob, around #3281?). So, I think we can simply stop supporting endJob harvesting entirely for legacy modules, ideally ripping out all the endJob methods to avoid confusion.
Edit3: running cmsswSequenceInfo also on ALCAHARVEST brings another 3 legacy analyzers, but they also seem fine.

makortel · 2020-07-16T13:08:24Z

Migrate all legacy modules using endJob to endProcessBlock. I have no idea how many of these we have.

Unfortunately this won't work because ProcessBlock support was not extended to legacy modules (which we really should get rid of).

cmsbuild · 2020-07-23T18:19:28Z

+1
Tested at: 4df3118
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-ce60a8/8251/summary.html
CMSSW: CMSSW_11_2_X_2020-07-23-1100
SCRAM_ARCH: slc7_amd64_gcc820

cmsbuild · 2020-07-23T18:19:30Z

Comparison job queued.

cmsbuild · 2020-07-23T20:10:21Z

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

Comparison Summary:

No significant changes to the logs found
Reco comparison results: 0 differences found in the comparisons
DQMHistoTests: Total files compared: 34
DQMHistoTests: Total histograms compared: 2526188
DQMHistoTests: Total failures: 1
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 2526140
DQMHistoTests: Total skipped: 47
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 33 files compared)
Checked 144 log files, 17 edm output root files, 34 DQM output files

schneiml · 2020-07-24T09:16:43Z

+1

The comparison is as clean as it could be. The changes seem ok to me.

We should be prepared for more random misbehavior in harvesting jobs, since these changes will throw the undefined-but-pretty-reliable module ordering out of order and there may be more harvesting code that needs explicit dependencies set up. But that should be easy to fix (like in the SiPixelPhase1 case here) and there is not much we can do about it.

silviodonato · 2020-07-28T14:31:48Z

@cms-sw/alca-l2 do you have any comments?

pohsun · 2020-08-03T03:34:37Z

+1

jfernan2 · 2020-08-05T20:53:43Z

+1
Sign from @schneiml was removed somehow....

cmsbuild · 2020-08-05T20:54:08Z

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

silviodonato · 2020-08-06T08:07:49Z

+1

cmsbuild added this to the CMSSW_11_2_X milestone Jul 15, 2020

cmsbuild added code-checks-pending comparison-pending dqm-pending orp-pending pending-signatures tests-pending labels Jul 15, 2020

schneiml mentioned this pull request Jul 15, 2020

EndJob module ordering #28354

Closed

cmsbuild added code-checks-approved and removed code-checks-pending labels Jul 15, 2020

cmsbuild added tests-started and removed tests-pending labels Jul 15, 2020

cmsbuild added tests-rejected and removed tests-started labels Jul 15, 2020

cmsbuild added comparison-available and removed comparison-pending labels Jul 15, 2020

cmsbuild added tests-started and removed tests-pending labels Jul 23, 2020

cmsbuild added tests-approved and removed tests-started labels Jul 23, 2020

cmsbuild added comparison-available and removed comparison-pending labels Jul 23, 2020

cmsbuild added dqm-approved and removed dqm-pending labels Jul 24, 2020

cmsbuild mentioned this pull request Jul 29, 2020

BS online for 11 2 x #30591

Closed

cmsbuild added alca-approved dqm-pending and removed alca-pending dqm-approved labels Aug 3, 2020

cmsbuild added dqm-approved fully-signed and removed dqm-pending pending-signatures labels Aug 5, 2020

cmsbuild added orp-approved and removed orp-pending labels Aug 6, 2020

cmsbuild merged commit 1502929 into cms-sw:master Aug 6, 2020

mmusich mentioned this pull request Sep 7, 2020

ALCAHARVEST:SiPixelAli is broken #31381

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DQM: Use ProcessBlock in Harvesting #30698

DQM: Use ProcessBlock in Harvesting #30698

schneiml commented Jul 15, 2020 •

edited

cmsbuild commented Jul 15, 2020

cmsbuild commented Jul 15, 2020

cmsbuild commented Jul 15, 2020

schneiml commented Jul 15, 2020

cmsbuild commented Jul 15, 2020 •

edited

makortel commented Jul 15, 2020

wddgit commented Jul 15, 2020

cmsbuild commented Jul 15, 2020

cmsbuild commented Jul 15, 2020

wddgit commented Jul 15, 2020

cmsbuild commented Jul 15, 2020

wddgit commented Jul 15, 2020

schneiml commented Jul 16, 2020 •

edited

makortel commented Jul 16, 2020

cmsbuild commented Jul 23, 2020

cmsbuild commented Jul 23, 2020

cmsbuild commented Jul 23, 2020

schneiml commented Jul 24, 2020

silviodonato commented Jul 28, 2020

pohsun commented Aug 3, 2020

jfernan2 commented Aug 5, 2020

cmsbuild commented Aug 5, 2020

silviodonato commented Aug 6, 2020

DQM: Use ProcessBlock in Harvesting #30698

DQM: Use ProcessBlock in Harvesting #30698

Conversation

schneiml commented Jul 15, 2020 • edited

PR description:

PR validation:

cmsbuild commented Jul 15, 2020

cmsbuild commented Jul 15, 2020

cmsbuild commented Jul 15, 2020

schneiml commented Jul 15, 2020

cmsbuild commented Jul 15, 2020 • edited

makortel commented Jul 15, 2020

wddgit commented Jul 15, 2020

cmsbuild commented Jul 15, 2020

cmsbuild commented Jul 15, 2020

wddgit commented Jul 15, 2020

cmsbuild commented Jul 15, 2020

wddgit commented Jul 15, 2020

schneiml commented Jul 16, 2020 • edited

makortel commented Jul 16, 2020

cmsbuild commented Jul 23, 2020

cmsbuild commented Jul 23, 2020

cmsbuild commented Jul 23, 2020

schneiml commented Jul 24, 2020

silviodonato commented Jul 28, 2020

pohsun commented Aug 3, 2020

jfernan2 commented Aug 5, 2020

cmsbuild commented Aug 5, 2020

silviodonato commented Aug 6, 2020

schneiml commented Jul 15, 2020 •

edited

cmsbuild commented Jul 15, 2020 •

edited

schneiml commented Jul 16, 2020 •

edited