Migrate Geometry from setConsumes() to type-deducing consumes() #31289

makortel · 2020-08-29T01:37:56Z

PR description:

To try out the simpler consumes from #31223.

PR validation:

Code compiles.

cmsbuild · 2020-08-29T01:38:15Z

The code-checks are being triggered in jenkins.

cmsbuild · 2020-08-29T01:43:06Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-31289/18005

This PR adds an extra 20KB to repository

cmsbuild · 2020-08-29T01:43:30Z

A new Pull Request was created by @makortel (Matti Kortelainen) for master.

It involves the following packages:

Geometry/CSCGeometryBuilder
Geometry/GEMGeometryBuilder
Geometry/MuonNumbering
Geometry/RPCGeometryBuilder
Geometry/TrackerGeometryBuilder

@civanch, @Dr15Jones, @makortel, @cvuosalo, @ianna, @mdhildreth, @cmsbuild, @kpedro88 can you please review it and eventually sign? Thanks.
@JanFSchulte, @watson-ij, @VinInn, @jshlee, @ptcox, @dildick, @ebrondol, @mtosi, @fabiocos, @slomeo, @venturia this is something you requested to watch as well.
@silviodonato, @dpiparo, @qliphy you are the release manager for this.

cms-bot commands are listed here

makortel · 2020-08-29T01:46:29Z

@cmsbuild, please test

makortel · 2020-08-29T01:46:34Z

FYI @Dr15Jones

cmsbuild · 2020-08-29T01:46:46Z

The tests are being triggered in jenkins.

makortel · 2020-08-29T01:47:04Z

please abort

cmsbuild · 2020-08-29T01:47:09Z

Jenkins tests are aborted.

cmsbuild · 2020-08-29T01:51:28Z

The code-checks are being triggered in jenkins.

cmsbuild · 2020-08-29T01:57:23Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-31289/18006

This PR adds an extra 20KB to repository

cmsbuild · 2020-08-29T01:57:50Z

Pull request #31289 was updated. @civanch, @Dr15Jones, @makortel, @cvuosalo, @ianna, @mdhildreth, @cmsbuild, @kpedro88 can you please check and sign again.

makortel · 2020-08-29T01:58:03Z

@cmsbuild, please test

cmsbuild · 2020-08-29T01:58:09Z

The tests are being triggered in jenkins.

CMSSW_11_2_X_2020-08-28-2300/slc7_amd64_gcc820: https://cmssdt.cern.ch/jenkins/job/ib-run-pr-tests/8998/console Started: 2020/08/29 07:58

cmsbuild · 2020-08-29T08:38:04Z

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

Comparison Summary:

No significant changes to the logs found
Reco comparison results: 0 differences found in the comparisons
DQMHistoTests: Total files compared: 35
DQMHistoTests: Total histograms compared: 2609667
DQMHistoTests: Total failures: 1
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 2609644
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 34 files compared)
Checked 149 log files, 22 edm output root files, 35 DQM output files

Dr15Jones · 2020-08-29T12:16:06Z

Geometry/MuonNumbering/plugins/MuonNumberingInitialization.cc

-  setWhatProduced(this).setConsumes(geomToken_);
-}
+MuonNumberingInitialization::MuonNumberingInitialization(const edm::ParameterSet&)
+    : geomToken_{setWhatProduced(this).consumes()} {}


I believe

Suggested change

: geomToken_{setWhatProduced(this).consumes()} {}

: geomToken_{setWhatProduced(this)} {}

also works. Not sure I like it or not.

Actually it doesn't work. I tested and it leads to compilation errors

In file included from .../src/FWCore/Framework/interface/es_impl/MayConsumeChooser.h:28, from .../src/FWCore/Framework/interface/ESConsumesCollector.h:33, from .../src/FWCore/Framework/interface/ESProducer.h:77, from .../Geometry/MuonNumbering/plugins/MuonNumberingInitialization.cc:22: .../FWCore/Utilities/interface/ESGetToken.h: In instantiation of 'constexpr edm::ESGetToken<ESProduct, ESRecord>::ESGetToken(ADAPTER&&) [with ADAPTER = edm::ESConsumesCollectorT<Mu onNumberingRecord>; ESProduct = DDCompactView; ESRecord = IdealGeometryRecord]': .../Geometry/MuonNumbering/plugins/MuonNumberingInitialization.cc:41:39: required from here .../FWCore/Utilities/interface/ESGetToken.h:52:117: error: no matching function for call to 'edm::ESConsumesCollectorT<MuonNumberingRecord>::consumes<DDCompactView, IdealGeometryRe cord>()' constexpr explicit ESGetToken(ADAPTER&& iAdapter) : ESGetToken(iAdapter.template consumes<ESProduct, ESRecord>()) {} In file included from .../FWCore/Framework/interface/es_impl/MayConsumeChooser.h:28, from .../FWCore/Framework/interface/ESConsumesCollector.h:33, from .../FWCore/Framework/interface/ESProducer.h:77, from .../Geometry/MuonNumbering/plugins/MuonNumberingInitialization.cc:22: .../FWCore/Utilities/interface/ESGetToken.h: In instantiation of 'constexpr edm::ESGetToken<ESProduct, ESRecord>::ESGetToken(ADAPTER&&) [with ADAPTER = edm::ESConsumesCollectorT<Mu onNumberingRecord>; ESProduct = DDCompactView; ESRecord = IdealGeometryRecord]': .../Geometry/MuonNumbering/plugins/MuonNumberingInitialization.cc:41:39: required from here .../FWCore/Utilities/interface/ESGetToken.h:52:117: error: no matching function for call to 'edm::ESConsumesCollectorT<MuonNumberingRecord>::consumes<DDCompactView, IdealGeometryRe cord>()' constexpr explicit ESGetToken(ADAPTER&& iAdapter) : ESGetToken(iAdapter.template consumes<ESProduct, ESRecord>()) {}

I think this happens because setWhatConsumes() returns ESConsumesCollector<T>, whose consumes() takes only one template argument, while the ESGetToken constructor calls consumes() with two template arguments.

Anyway, I think the explicit call to consumes() makes the call slightly easier to understand.

cvuosalo · 2020-08-31T20:59:44Z

+1

kpedro88 · 2020-09-03T14:45:26Z

+upgrade

cmsbuild · 2020-09-03T14:45:47Z

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-09-03T15:06:32Z

assign core
do you actually want to migrate to the type-deducing consumes()?
My understanding after the Core software meeting was that there is a slight preference to avoid migrating to the new consumes(). In this way we avoid to complicate (a little bit) the possible future backports.
I'm asking just for a clarification, I don't have a strong opinion about this.

cmsbuild · 2020-09-03T15:06:54Z

New categories assigned: core

@Dr15Jones,@smuzaffar,@makortel you have been requested to review this Pull request/Issue and eventually sign? Thanks

makortel · 2020-09-03T15:15:10Z

do you actually want to migrate to the type-deducing consumes()?

From setConsumes() yes. I'd like to avoid maintaining two type-deducing APIs for consumes (in ESProducers).

My understanding after the Core software meeting was that there is a slight preference to avoid migrating to the new consumes(). In this way we avoid to complicate (a little bit) the possible future backports.

Correct, but in that discussion I meant from the consumes()/esConsumes() with the types specified explicitly. I think I mentioned my plan to migrate all setConsumes() to type-deducing consumes().

makortel · 2020-09-03T15:15:15Z

+core

cmsbuild · 2020-09-03T15:15:36Z

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-09-03T15:16:38Z

+1

cmsbuild added this to the CMSSW_11_2_X milestone Aug 29, 2020

cmsbuild added code-checks-pending comparison-pending geometry-pending orp-pending pending-signatures tests-pending upgrade-pending labels Aug 29, 2020

cmsbuild added code-checks-approved and removed code-checks-pending labels Aug 29, 2020

Migrate Geometry from setConsumes() to type-deducing consumes()

18b4c4b

makortel force-pushed the consumesGeometry branch from cddf889 to 18b4c4b Compare August 29, 2020 01:51

cmsbuild added code-checks-pending and removed code-checks-approved labels Aug 29, 2020

cmsbuild added code-checks-approved and removed code-checks-pending labels Aug 29, 2020

cmsbuild removed the tests-pending label Aug 29, 2020

cmsbuild added comparison-available and removed comparison-pending labels Aug 29, 2020

Dr15Jones reviewed Aug 29, 2020

View reviewed changes

cmsbuild added geometry-approved and removed geometry-pending labels Aug 31, 2020

cmsbuild added fully-signed upgrade-approved and removed pending-signatures upgrade-pending labels Sep 3, 2020

cmsbuild added core-pending pending-signatures and removed fully-signed labels Sep 3, 2020

cmsbuild added core-approved fully-signed and removed core-pending pending-signatures labels Sep 3, 2020

cmsbuild added orp-approved and removed orp-pending labels Sep 3, 2020

cmsbuild merged commit af81ead into cms-sw:master Sep 3, 2020

makortel deleted the consumesGeometry branch September 4, 2020 01:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate Geometry from setConsumes() to type-deducing consumes() #31289

Migrate Geometry from setConsumes() to type-deducing consumes() #31289

makortel commented Aug 29, 2020

cmsbuild commented Aug 29, 2020

cmsbuild commented Aug 29, 2020

cmsbuild commented Aug 29, 2020

makortel commented Aug 29, 2020

makortel commented Aug 29, 2020

cmsbuild commented Aug 29, 2020

makortel commented Aug 29, 2020

cmsbuild commented Aug 29, 2020

cmsbuild commented Aug 29, 2020

cmsbuild commented Aug 29, 2020

cmsbuild commented Aug 29, 2020

makortel commented Aug 29, 2020

cmsbuild commented Aug 29, 2020 •

edited

Loading

cmsbuild commented Aug 29, 2020

Dr15Jones Aug 29, 2020

makortel Aug 31, 2020

cvuosalo commented Aug 31, 2020

kpedro88 commented Sep 3, 2020

cmsbuild commented Sep 3, 2020

silviodonato commented Sep 3, 2020

cmsbuild commented Sep 3, 2020

makortel commented Sep 3, 2020

makortel commented Sep 3, 2020

cmsbuild commented Sep 3, 2020

silviodonato commented Sep 3, 2020

	: geomToken_{setWhatProduced(this).consumes()} {}
	: geomToken_{setWhatProduced(this)} {}

Migrate Geometry from setConsumes() to type-deducing consumes() #31289

Migrate Geometry from setConsumes() to type-deducing consumes() #31289

Conversation

makortel commented Aug 29, 2020

PR description:

PR validation:

cmsbuild commented Aug 29, 2020

cmsbuild commented Aug 29, 2020

cmsbuild commented Aug 29, 2020

makortel commented Aug 29, 2020

makortel commented Aug 29, 2020

cmsbuild commented Aug 29, 2020

makortel commented Aug 29, 2020

cmsbuild commented Aug 29, 2020

cmsbuild commented Aug 29, 2020

cmsbuild commented Aug 29, 2020

cmsbuild commented Aug 29, 2020

makortel commented Aug 29, 2020

cmsbuild commented Aug 29, 2020 • edited Loading

cmsbuild commented Aug 29, 2020

Dr15Jones Aug 29, 2020

Choose a reason for hiding this comment

makortel Aug 31, 2020

Choose a reason for hiding this comment

cvuosalo commented Aug 31, 2020

kpedro88 commented Sep 3, 2020

cmsbuild commented Sep 3, 2020

silviodonato commented Sep 3, 2020

cmsbuild commented Sep 3, 2020

makortel commented Sep 3, 2020

makortel commented Sep 3, 2020

cmsbuild commented Sep 3, 2020

silviodonato commented Sep 3, 2020

cmsbuild commented Aug 29, 2020 •

edited

Loading