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

Migrate away from consumesMany() #40167

Closed
9 tasks done
makortel opened this issue Nov 28, 2022 · 18 comments
Closed
9 tasks done

Migrate away from consumesMany() #40167

makortel opened this issue Nov 28, 2022 · 18 comments

Comments

@makortel
Copy link
Contributor

makortel commented Nov 28, 2022

In #40136 (comment) I noticed the consumes information from consumesMany() calls was not propagated to the dump by Tracer service. The consumesMany() implementation includes some trickery, and could be replaced with the edm::GetterOfProducts() (or directly with the callWhenNewProductsRegistered()). The purpose of this issue is to list the remaining cases of consumesMany() and track the progress of migration.

Note that while we expect to do the migration centrally by the core team (on low priority), any help from domain experts is welcome.

Based on a simple git grep consumesMany below are files that call (or refer to in comments) the file, in decreasing order of priority

Production code (or potential such)

Validation code

.

Used in DQM core code to guarantee proper ordering

DQMServices/Components/plugins/MEtoEDMConverter.cc
DQMServices/FileIO/plugins/DQMFileSaverBase.cc
DQMServices/FwkIO/plugins/DQMRootOutputModule.cc

Non-framework test code

.

FWCore/Framework/interface/ConsumesCollector.h
FWCore/Framework/interface/EDConsumerBase.h
FWCore/Framework/src/EDConsumerBase.cc
FWCore/Framework/src/Principal.cc
FWCore/Framework/src/StreamSchedule.cc
FWCore/Framework/test/edconsumerbase_t.cppunit.cc
FWCore/Framework/test/stubs/TestFilterModule.cc
FWCore/Framework/test/stubs/TestTriggerNames.cc
FWCore/Framework/test/stubs/ToyIntProducers.cc
FWCore/ServiceRegistry/interface/ConsumesInfo.h
FWCore/ServiceRegistry/interface/PathsAndConsumesOfModulesBase.h
@cmsbuild
Copy link
Contributor

A new Issue was created by @makortel Matti Kortelainen.

@Dr15Jones, @perrotta, @dpiparo, @rappoccio, @makortel, @smuzaffar can you please review it and eventually sign/assign? Thanks.

cms-bot commands are listed here

@makortel
Copy link
Contributor Author

makortel commented Nov 28, 2022

assign core,reconstruction,xpog,dqm,l1

(although others than core are more FYI)

@cmsbuild
Copy link
Contributor

New categories assigned: core,l1,xpog,reconstruction,dqm

@jfernan2,@epalencia,@ahmad3213,@micsucmed,@rvenditti,@mandrenguyen,@Dr15Jones,@smuzaffar,@emanueleusai,@syuvivida,@swertz,@clacaputo,@rekovic,@makortel,@cecilecaillol,@vlimant,@pmandrik you have been requested to review this Pull request/Issue and eventually sign? Thanks

@makortel
Copy link
Contributor Author

@cms-sw/l1-l2 @dildick

I see the file

L1Trigger/CSCTriggerPrimitives/test/deprecated/CSCTriggerPrimitivesReader.cc

was moved from plugins as part of deprecation work in #33582. Is this file something that could perhaps be removed now?

@vlimant
Copy link
Contributor

vlimant commented Nov 28, 2022

for what concerns NanoAODDQM, all the FlatTable products are retrieved and later post-selected based on FlatTable::name() which is different that the what the InputTag would be.

As an example the producer of the "Muon" table is https://github.com/cms-sw/cmssw/blob/master/PhysicsTools/NanoAOD/python/muons_cff.py muonTable but the NanoAODDQM module configuration only refers to "Muon" (which might actually refer to multiple EDProducer producing a FlatTable label "Muon" ... )

It looks impossible to have a proper consumes statement from the module configuration.

@wddgit
Copy link
Contributor

wddgit commented Nov 28, 2022

I have not looked at this case in detail but my recollection from 2012 is that GetterOfProducts can do anything that getManyByType can do, but is more general. It always selects products by type. Optionally, it can also select on any information in the BranchDescription using a predicate (or only select by type). TWIKI documentation is here:

https://twiki.cern.ch/twiki/bin/view/CMSPublic/SWGuideEDMGetDataFromEvent#GetterOfProducts

It uses callWhenNewProductsRegistered(). It internally makes the proper individual consumes calls and it does not use consumesMany. Client code does not need to directly call consumes at all. We only kept getManyByType for backward compatibility to keep existing code working because we didn't have time to migrate all uses. Also we were worried that some code outside the repository might be broken.

All of these are items are on my to do list now. If no one else gets to it first, I'll give this one a try. Possibly you are right and I will be surprised and find some roadblock that makes it impossible when I look at the details. If someone else wants to try this one, then please let me know and I'll not spend time on it.

@swertz
Copy link
Contributor

swertz commented Nov 29, 2022

Thanks for the pointers. I tried implementing GetterOfProducts in the NanoAODDQM as I was working on something else there and it seems to work. I can make a PR soon.

@vlimant
Copy link
Contributor

vlimant commented Dec 16, 2022

+xpog

@cecilecaillol
Copy link
Contributor

L1T file removed in #40388

@cecilecaillol
Copy link
Contributor

+l1

@emanueleusai
Copy link
Member

+1

@makortel
Copy link
Contributor Author

+1

@emanueleusai Umm, as far as I can tell, all the uses in DQM and validation code are still there. Am I missing something?

@missirol
Copy link
Contributor

missirol commented Feb 1, 2023

assign hlt

Because of RecoEgamma/EgammaHLTProducers. This is being addressed in #40629.

Production code (or potential such)

I don't think EgammaHLTExtraProducer.cc is used anywhere in production, so 'potential such'.

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 1, 2023

New categories assigned: hlt

@missirol,@Martin-Grunewald you have been requested to review this Pull request/Issue and eventually sign? Thanks

@missirol
Copy link
Contributor

missirol commented Feb 3, 2023

+hlt

HLT part done in #40629 (thanks @jeongeun).

@wddgit
Copy link
Contributor

wddgit commented Mar 17, 2023

I'm going to start working on HcalDigiDump.cc unless someone else is already working on it.

@wddgit
Copy link
Contributor

wddgit commented Mar 20, 2023

HcalDigiDump.cc is done in PR #41109, which I just submitted. (I would edit the checklist at the top, but I don't seem to have permissions for that).

I will work on DQMServices/FwkIO/plugins/DQMRootOutputModule.cc next. If someone else is already working on that one or intends to in the near future, please let me know and I'll work on other things.

@makortel
Copy link
Contributor Author

makortel commented Aug 4, 2023

+core

consumesMany() functionality was removed in #42058

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

8 participants