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

Fix all DQM paths to properly handle TriggerResults #15810

Merged

Conversation

Dr15Jones
Copy link
Contributor

@Dr15Jones Dr15Jones commented Sep 11, 2016

This pull request fixes all problems from issue #15769 and includes the changes in #15809. The most important additional change on top #15809 changes ConfigBuilder.py and therefore could have a larger impact on the evaluation of this pull request as opposed to #15809.

The ConfigBuilder.py change always makes the dqmoffline*_step path be an EndPath. Previously, based on which processing step is being configured the dqmoffline*_step might sometimes be a Path and other times be an EndPath. The reason appeared to be because sometimes the TriggerResults was being requested by some modules using an empty processName and the hope was one would pick up the HLT results. This change has all modules explicitly state what process name to use when getting the TriggerResults.

This is needed since the next threaded framework change requires that the consumes of a module and its place on a Path or EndPath guarantee that it can get all the data it requests (or will fail since there is no source or module producing the data it wants). Asking for a TriggerResults for the present process from a Path is therefore not allowed (and was not a good idea before either).

Instead of asking for the default TriggerResults and assuming it is the HLT one,
explicitly ask for the HLT results.
When running with fastsim, these modules need to ignore the HLT
TriggerResults. Previously this was done by moving the Sequence they
were on from an EndPath to the Path. But with an upcoming framework
change this will no longer be allowed. Instead we now explicitly
turn off that functionality when running under fastsim.
Fix a segmentation fault which can occur if the miniAOD filters are
checked when not diong miniAOD MET.
This fix allows METAnalyzer to be properly used on EndPaths.
Many of the modules on the different dqmoffline*_step paths require
access to the TriggerResults. Previously those paths were sometimes
made Paths and other times made EndPaths in an attempt to only pick
up the TriggerResults that were actually needed. Now all the modules
have been explicitly changed to ask for the TriggerResults using the
process name so it is always safe to use an EndPath. In addition, an
upcoming framework change will enforce that all data requested must be
accessible to a module. Therefore all modules asking for the TriggerResults
without specifying a process name (or specifying the present process name)
must be on EndPaths.
@cmsbuild
Copy link
Contributor

A new Pull Request was created by @Dr15Jones (Chris Jones) for CMSSW_8_1_X.

It involves the following packages:

Configuration/Applications
DQMOffline/JetMET
DQMOffline/Trigger
Validation/RecoTau

@cmsbuild, @dmitrijus, @franzoni, @vanbesien, @davidlange6 can you please review it and eventually sign? Thanks.
@ghellwig, @mmarionncern, @battibass, @ahinzmann, @rappoccio, @TaiSakuma, @jhgoh, @Martin-Grunewald, @jdolen, @nhanvtran, @HuguesBrun, @gkasieczka, @schoef, @mariadalfonso, @trocino, @rociovilar this is something you requested to watch as well.
@slava77, @smuzaffar you are the release manager for this.

cms-bot commands are list here #13028

@Dr15Jones
Copy link
Contributor Author

please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Sep 11, 2016

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/15085/console

@cmsbuild
Copy link
Contributor

-1

Tested at: 965e6d1

You can see the results of the tests here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-15810/15085/summary.html

I found follow errors while testing this PR

Failed tests: RelVals

  • RelVals:

When I ran the RelVals I found an error in the following worklfows:
140.53 step2

runTheMatrix-results/140.53_RunHI2011+RunHI2011+RECOHID11+HARVESTDHI/step2_RunHI2011+RunHI2011+RECOHID11+HARVESTDHI.log

With dqmoffline_step now always being an EndPath, we need to be sure
that modifications to Paths are also applied to EndPaths.
@cmsbuild
Copy link
Contributor

Pull request #15810 was updated. @cmsbuild, @dmitrijus, @franzoni, @vanbesien, @davidlange6 can you please check and sign again.

@Dr15Jones
Copy link
Contributor Author

please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Sep 12, 2016

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/15087/console

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

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

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

  • 136.731_RunSinglePh2016B+RunSinglePh2016B+HLTDR2_2016+RECODR2_2016reHLT_skimSinglePh_HIPM+HARVESTDR2

@Dr15Jones
Copy link
Contributor Author

hold
I did the full runTheMatrix.py and a few workflows appear to have problems presumably from this change. I'm investigating.

@cmsbuild
Copy link
Contributor

Pull request has been put on hold by @Dr15Jones
They need to issue an unhold command to remove the hold state or L1 can unhold it for all

@cmsbuild cmsbuild added the hold label Sep 13, 2016
@Dr15Jones
Copy link
Contributor Author

unhold
It was a false alarm. My work area for some reason didn't have everything checked out so the harvesting step on a few workflows failed because a plugin couldn't be found.

@cmsbuild cmsbuild removed the hold label Sep 13, 2016
@Dr15Jones
Copy link
Contributor Author

Ran the full runTheMatrix.py and all workflows passed.

@Dr15Jones
Copy link
Contributor Author

This fix is needed to allow further work on the threaded framework.

@Dr15Jones
Copy link
Contributor Author

ping

1 similar comment
@Dr15Jones
Copy link
Contributor Author

ping

@Dr15Jones
Copy link
Contributor Author

ping
This Ian holding up framework development

@dmitrijus
Copy link
Contributor

+1

@davidlange6 davidlange6 merged commit 851461b into cms-sw:CMSSW_8_1_X Sep 16, 2016
@Dr15Jones Dr15Jones deleted the fixAllQMSequenceForTriggerResults branch September 19, 2016 15:30
else:
# schedule DQM as a standard Path
setattr(self.process,pathName, cms.Path( getattr(self.process, sequence) ) )
self.schedule.append(getattr(self.process,pathName))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suspect this removal caused loss of MiniAOD validation histograms in 810pre12, see
https://hypernews.cern.ch/HyperNews/CMS/get/relval/6213/46/2.html

I have a proposal for a fix in #16265.

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

5 participants