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

73X - consumes migration XI #5944

Merged
merged 15 commits into from
Oct 29, 2014
Merged

Conversation

vadler
Copy link

@vadler vadler commented Oct 22, 2014

After the integration of the new reco::JetCorrector with #5141 to resolve the issue of edm::ConsumesCollectors to be passed to an ES object, this PR makes use of the new ED product and its producer in the scope of AT.
Beside some necessary code changes, it mostly consists of configuration changes/additions, since the new approach requires to add EDProducers, where the JetCorrectionService has been called before.
No regressions are expected. Some example comparisons showing identity of before/after are attached.

73x-consumesmigrationxi_run_default

73x-consumesmigrationxi_pattuple_standard

73x-consumesmigrationxi_pattuple_pf2pat

73x-consumesmigrationxi_pattuple_addjets

73x-consumesmigrationxi_pattuple_metuncertainties

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @vadler (Volker Adler) for CMSSW_7_3_X.

73X - consumes migration XI

It involves the following packages:

CommonTools/ParticleFlow
DQMOffline/Trigger
ElectroWeakAnalysis/WENu
JetMETCorrections/Configuration
JetMETCorrections/Modules
JetMETCorrections/TauJet
JetMETCorrections/Type1MET
PhysicsTools/PatAlgos
PhysicsTools/PatUtils
Validation/RecoMET

@nclopezo, @StoyanStoynev, @danduggan, @rovere, @monttj, @cmsbuild, @deguio, @slava77, @vadler, @ojeda can you please review it and eventually sign? Thanks.
@TaiSakuma, @imarches, @pvmulder, @acaudron, @abbiendi, @mmarionncern, @nhanvtran, @schoef, @ferencek, @battibass, @mariadalfonso, @trocino, @rociovilar this is something you requested to watch as well.
You can sign-off by replying to this message having '+1' in the first line of your reply.
You can reject by replying to this message having '-1' in the first line of your reply.
@nclopezo, @ktf you are the release manager for this.
You can merge this pull request by typing 'merge' in the first line of your comment.

@vadler
Copy link
Author

vadler commented Oct 22, 2014

@deguio : I also migrated some small pieces of DQM code/configs, because they were very "close" to the touched JetMETCorrection configs.

@vadler
Copy link
Author

vadler commented Oct 22, 2014

@rappoccio : Could you also take a look, please?

@@ -523,19 +528,19 @@ def toolCode(self, process):
setattr(process,jetCorrections[0]+_labelCorrName+'pfCandsNotInJetsPtrForMetCorr'+postfix,pfCandsNotInJetsPtrForMetCorr.clone(topCollection = jetCorrections[0]+_labelCorrName+'pfJetsPtrForMetCorr'+postfix))
setattr(process,jetCorrections[0]+_labelCorrName+'pfCandsNotInJetsForMetCorr'+postfix,pfCandsNotInJetsForMetCorr.clone(src = jetCorrections[0]+_labelCorrName+'pfCandsNotInJetsPtrForMetCorr'+postfix))
setattr(process,jetCorrections[0]+_labelCorrName+'CandMETcorr'+postfix, pfCandMETcorr.clone(src = cms.InputTag(jetCorrections[0]+_labelCorrName+'pfCandsNotInJetsForMetCorr'+postfix)))
setattr(process,jetCorrections[0]+_labelCorrName+'JetMETcorr'+postfix, corrPfMetType1.clone(src = jetSource))
setattr(process,jetCorrections[0]+_labelCorrName+'JetMETcorr'+postfix, corrPfMetType1.clone(src = jetSource, jetCorrLabel = cms.InputTag(jetCorrections[0]+'CombinedCorrector'))) # FIXME: Originally w/o jet corrections?
Copy link
Author

Choose a reason for hiding this comment

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

@mmarionncern : Is this fix correct?

@rappoccio
Copy link
Contributor

Overall looks good to me, but perhaps I have one question : does this maintain the "FWLite" method that takes in text files? This was a popular method of access in Run 1 (i.e. talking to the DB, pulling in the constants, writing a text file, and using the text file in FWlite).

@vadler
Copy link
Author

vadler commented Oct 22, 2014

@rappoccio : FWLite is not affected by the consumes-migration (but the consumes-migration is affected by FWLite). The original services and producers are still available. The "talking to DB" part" should in principle be migrated also there, but I assume this is mostly about private code, not official production WFs, right?

@rappoccio
Copy link
Contributor

@vadler Correct, this should be okay then. Thanks!

@vadler
Copy link
Author

vadler commented Oct 22, 2014

@rappoccio : I have also one question:
JetMETCorrections/Configuration/python/CorrectedJetProducers_cff.py is essentially a derivative of JetMETCorrections/Configuration/python/JetCorrectionProducers_cff.py (and the same for JetMETCorrections/Configuration/python/JetCorrectors_cff.py and JetMETCorrections/Configuration/python/JetCorrectionServices_cff.py). I found some missing pieces, which I added to the new files in vadler@b961b4f . Were they missing intentionally, or should they also be added to the original files?

@rappoccio
Copy link
Contributor

@vadler : if the services are missing, usually it means they don't exist so we take them out to avoid confusion. That way the user gets descriptive errors at compile time rather than opaque errors at run time.

@vadler
Copy link
Author

vadler commented Oct 22, 2014

@rappoccio : I see. Then the changes in vadler@b961b4f#diff-1 and their correspondences in vadler@b961b4f#diff-0 should be reverted, right?

@rappoccio
Copy link
Contributor

@vadler : Yes, probably that is for the best.

@slava77
Copy link
Contributor

slava77 commented Oct 29, 2014

+1

for #5944 878506b
no changes in reco

the issue in JetMETCorrections/Type1MET/interface/JetCorrExtractorT.h should be followed up, I guess.

@cmsbuild
Copy link
Contributor

This pull request is fully signed and it will be integrated in one of the next CMSSW_7_3_X IBs unless changes (tests are also fine). This pull request will be automatically merged.

@vadler
Copy link
Author

vadler commented Oct 29, 2014

@vadler
Copy link
Author

vadler commented Oct 29, 2014

It looks like it was my mistake, overlooking the obvious somehow :-(
I will provide a fix right away...

@vadler
Copy link
Author

vadler commented Oct 29, 2014

I think, I slipped into the wrong diff (CaloJetMETcorrInputProducerT).

vadler pushed a commit to vadler/cmssw that referenced this pull request Oct 29, 2014
@vadler vadler mentioned this pull request Oct 29, 2014
vadler pushed a commit to vadler/cmssw that referenced this pull request Oct 30, 2014
cmsbuild added a commit that referenced this pull request Oct 30, 2014
@slava77
Copy link
Contributor

slava77 commented Oct 30, 2014

@vadler
Volker,
apparently this one broke 140.51:

----- Begin Fatal Exception 30-Oct-2014 14:01:33 CET-----------------------
An exception of category 'ProductNotFound' occurred while
   [0] Processing run: 152698 lumi: 17 event: 49592
   [1] Running path 'dqmoffline_step'
   [2] Calling event method for module L1FastjetCorrectorProducer/'ak4CaloL1FastjetCorrector'
Exception Message:
Principal::getByToken: Found zero products matching all criteria
Looking for type: double
Looking for module label: fixedGridRhoFastjetAllCalo
Looking for productInstanceName: 


@vadler
Copy link
Author

vadler commented Oct 30, 2014

@slava77 : Hmm, the old ESProducer requests the same data:
https://github.com/cms-sw/cmssw/blob/CMSSW_7_3_X/JetMETCorrections/Configuration/python/JetCorrectionServices_cff.py#L43
Looking into it...

@slava77
Copy link
Contributor

slava77 commented Oct 30, 2014

it could as well be that the ESProducer's products were never requested, the EDProducer now runs, since this is a scheduled mode

@vadler
Copy link
Author

vadler commented Oct 30, 2014

@slava77 , @deguio : Looking closer to it, I rather claim an inconsistency between RECO and DQM in the failing WFs (300.0, 301.0, 302.0, 140.0, 140.51, 140.52, 140.53). The module in question is the JetMETHLTOfflineSource, which silently bails out in case of missing jet collections. So far, the jet correction services have never been called due to that in these WFs w/o standard RECO, but now we have the JetCorrector sequence, where a standard RECO element is needed before the JetMETHLTOfflineSource is run and has the chance to die silently. In fact, #5944 (comment) describes it exactly.
IMHO, the sequence jetMETHLTOfflineAnalyzer should not appear at all in these WFs; the Matrix should be fixed.
The symptoms in WFs 10025.0, 11325.0 are different. I will check them tomorrow.

@vadler
Copy link
Author

vadler commented Oct 31, 2014

Just for "bookkeeping": Discussion is continued in
https://hypernews.cern.ch/HyperNews/CMS/get/swReleases/4128.html ff.

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.

6 participants