[Draft] esConsumes migration #35659

hyunyong · 2021-10-13T16:22:13Z

PR description:

esConsumes migration for the muon alignment modules.

PR validation:

PR has an issue.

I try to use consumesCollector() for Alignment/MuonAlignment/plugins/MuonGeometryDBConverter.cc (https://github.com/hyunyong/cmssw/blob/76328c9d2b24b0365d41fbff38ac6208e590e249/Alignment/MuonAlignment/plugins/MuonGeometryDBConverter.cc#L93)
However, es tokens are initialized at https://github.com/hyunyong/cmssw/blob/76328c9d2b24b0365d41fbff38ac6208e590e249/Alignment/MuonAlignment/plugins/MuonGeometryDBConverter.cc#L168

I guess this is the reason why I got this issue.

A module declared it consumes an EventSetup product after its constructor.
This must be done in the constructor

cmsbuild · 2021-10-13T16:30:08Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-35659/25929

This PR adds an extra 60KB to repository

cmsbuild · 2021-10-13T16:30:34Z

A new Pull Request was created by @hyunyong for master.

It involves the following packages:

Alignment/MuonAlignment (alca)
Alignment/MuonAlignmentAlgorithms (alca)
Alignment/SurveyAnalysis (alca)

@cmsbuild, @malbouis, @tvami, @yuanchao, @francescobrivio can you please review it and eventually sign? Thanks.
@pakhotin, @adewit, @abbiendi, @jhgoh, @tocheng, @tlampen, @mmusich, @trocino this is something you requested to watch as well.
@perrotta, @dpiparo, @qliphy you are the release manager for this.

cms-bot commands are listed here

tvami · 2021-10-13T16:56:56Z

@makortel can you please help out with the error msg reported in the PR description?

tvami · 2021-10-13T17:11:28Z

@cmsbuild , please test

let's see if the unit tests also crash or not

cmsbuild · 2021-10-13T17:19:52Z

-1

Failed Tests: Build ClangBuild
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-f57575/19609/summary.html
COMMIT: 76328c9
CMSSW: CMSSW_12_1_X_2021-10-13-1100/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/35659/19609/install.sh to create a dev area with all the needed externals and cmssw changes.

Build

I found compilation error when building:

>> Building  edm plugin tmp/slc7_amd64_gcc900/src/Alignment/MuonAlignment/plugins/MuonGeometryArrange/libMuonGeometryArrange.so
/cvmfs/cms-ib.cern.ch/nweek-02702/slc7_amd64_gcc900/external/gcc/9.3.0/bin/../lib/gcc/x86_64-unknown-linux-gnu/9.3.0/../../../../x86_64-unknown-linux-gnu/bin/ld: tmp/slc7_amd64_gcc900/src/Alignment/MuonAlignment/plugins/MuonGeometryArrange/MuonGeometryArrange.cc.o: in function `MuonGeometryArrange::analyze(edm::Event const&, edm::EventSetup const&)':
MuonGeometryArrange.cc:(.text+0x5eba): undefined reference to `MuonAlignmentInputXML::MuonAlignmentInputXML(std::__cxx11::basic_string, std::allocator > const&, std::__cxx11::basic_string, std::allocator >, edm::ConsumesCollector)'
/cvmfs/cms-ib.cern.ch/nweek-02702/slc7_amd64_gcc900/external/gcc/9.3.0/bin/../lib/gcc/x86_64-unknown-linux-gnu/9.3.0/../../../../x86_64-unknown-linux-gnu/bin/ld: MuonGeometryArrange.cc:(.text+0x5f4f): undefined reference to `MuonAlignmentInputXML::MuonAlignmentInputXML(std::__cxx11::basic_string, std::allocator > const&, std::__cxx11::basic_string, std::allocator >, edm::ConsumesCollector)'
/cvmfs/cms-ib.cern.ch/nweek-02702/slc7_amd64_gcc900/external/gcc/9.3.0/bin/../lib/gcc/x86_64-unknown-linux-gnu/9.3.0/../../../../x86_64-unknown-linux-gnu/bin/ld: MuonGeometryArrange.cc:(.text+0x5fe0): undefined reference to `MuonAlignmentInputXML::MuonAlignmentInputXML(std::__cxx11::basic_string, std::allocator > const&, std::__cxx11::basic_string, std::allocator >, edm::ConsumesCollector)'
collect2: error: ld returned 1 exit status
gmake: *** [tmp/slc7_amd64_gcc900/src/Alignment/MuonAlignment/plugins/MuonGeometryArrange/libMuonGeometryArrange.so] Error 1
Leaving library rule at src/Alignment/MuonAlignment/plugins
Entering library rule at src/Alignment/MuonAlignment/plugins
>> Compiling edm plugin /data/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_12_1_X_2021-10-13-1100/src/Alignment/MuonAlignment/plugins/MuonGeometryDBConverter.cc
>> Building  edm plugin tmp/slc7_amd64_gcc900/src/Alignment/MuonAlignment/plugins/MuonGeometryDBConverter/libMuonGeometryDBConverter.so

Clang Build

I found compilation error while trying to compile with clang. Command used:

USER_CUDA_FLAGS='--expt-relaxed-constexpr' USER_CXXFLAGS='-Wno-register -fsyntax-only' scram build -k -j 32 COMPILER='llvm compile'

>> Compiling  /data/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_12_1_X_2021-10-13-1100/src/Alignment/MuonAlignment/src/MuonAlignmentInputMethod.cc
>> Compiling  /data/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_12_1_X_2021-10-13-1100/src/Alignment/MuonAlignment/src/MuonAlignmentInputSurveyDB.cc
>> Compiling  /data/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_12_1_X_2021-10-13-1100/src/Alignment/MuonAlignment/src/MuonAlignmentOutputXML.cc
>> Compiling  /data/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_12_1_X_2021-10-13-1100/src/Alignment/MuonAlignment/src/MuonAlignmentInputXML.cc
>> Compiling  /data/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_12_1_X_2021-10-13-1100/src/Alignment/MuonAlignment/src/MuonScenarioBuilder.cc
/data/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_12_1_X_2021-10-13-1100/src/Alignment/MuonAlignment/src/MuonAlignmentOutputXML.cc:42:25: error: out-of-line definition of 'MuonAlignmentOutputXML' does not match any declaration in 'MuonAlignmentOutputXML'
MuonAlignmentOutputXML::MuonAlignmentOutputXML(const edm::ParameterSet &iConfig)
                        ^~~~~~~~~~~~~~~~~~~~~~
1 error generated.
gmake: *** [tmp/slc7_amd64_gcc900/src/Alignment/MuonAlignment/src/AlignmentMuonAlignment/MuonAlignmentOutputXML.cc.o] Error 1
Entering library rule at src/Alignment/MuonAlignment/test

makortel · 2021-10-13T17:37:09Z

Alignment/MuonAlignment/interface/MuonAlignment.h

 class MuonAlignment {
 public:
-  MuonAlignment(const edm::EventSetup& iSetup);
+  MuonAlignment(const edm::EventSetup& iSetup, edm::ConsumesCollector iC);


The problem lies here. As the error message says, all consumes declarations must be done in EDModule (or ESProducer) constructor, or in functions called from the constructor. Therefore, any function that takes EventSetup (not available at module construction time) and ConsumesCollector (not usable outside module construction time) will not work.

This implies that the code needs to be organized differently. Based on a quick look on how MuonAlignment is used, I'd look into moving the construction of MuonAlignment to the constructors of the EDModules, and passing in the EventSetup in a member function call instead of the constructor.

@hyunyong any updates on this?

@hyunyong any updates on this?
I'm testing and validating updates.
I will close this PR and make a new one.

Dr15Jones · 2021-10-15T17:50:19Z

Alignment/SurveyAnalysis/plugins/DTSurveyConvert.h


  const edm::ESGetToken<DTGeometry, MuonGeometryRecord> muonGeoToken_;

+  edm::ConsumesCollector m_cc;


Never make a copy of the ConsumesCollector

Dr15Jones · 2021-10-15T17:50:43Z

Alignment/SurveyAnalysis/plugins/DTSurveyConvert.cc


-DTSurveyConvert::DTSurveyConvert(const edm::ParameterSet &iConfig) : muonGeoToken_(esConsumes()) {
+DTSurveyConvert::DTSurveyConvert(const edm::ParameterSet &iConfig)
+    : muonGeoToken_(esConsumes()), m_cc(consumesCollector()) {


Never copy a ConsumesCollector. It must be used in the constructors.

tvami · 2021-10-15T18:18:05Z

@hyunyong please also update the title to "Draft: esConsumes migration for the muon alignment modules" thanks

tvami · 2021-10-16T12:43:21Z

-alca

according to this comment [Draft] esConsumes migration #35659 (comment) this PR is to be closed and a new with a different approach will be opened soon

hyunyong · 2021-10-18T18:49:32Z

I will make a new PR.

hyunyong · 2021-10-18T19:13:36Z

I made a new PR #35719

esConsumes migration

76328c9

cmsbuild added this to the CMSSW_12_1_X milestone Oct 13, 2021

cmsbuild added alca-pending code-checks-pending orp-pending pending-signatures tests-pending labels Oct 13, 2021

cmsbuild added code-checks-approved and removed code-checks-pending labels Oct 13, 2021

cmsbuild added tests-started and removed tests-pending labels Oct 13, 2021

cmsbuild added tests-rejected and removed tests-started labels Oct 13, 2021

makortel reviewed Oct 13, 2021

View reviewed changes

Dr15Jones reviewed Oct 15, 2021

View reviewed changes

cmsbuild mentioned this pull request Oct 15, 2021

Added esConsumes calls to more Muon related code #35697

Merged

cmsbuild added alca-rejected and removed alca-pending labels Oct 16, 2021

cmsbuild mentioned this pull request Oct 17, 2021

Migrate to the new ref. based writeOneIOV() in Alignment package #35711

Closed

hyunyong closed this Oct 18, 2021


		const edm::ESGetToken<DTGeometry, MuonGeometryRecord> muonGeoToken_;

		edm::ConsumesCollector m_cc;

[Draft] esConsumes migration #35659

[Draft] esConsumes migration #35659

Uh oh!

Conversation

hyunyong commented Oct 13, 2021

PR description:

PR validation:

Uh oh!

cmsbuild commented Oct 13, 2021

Uh oh!

cmsbuild commented Oct 13, 2021

Uh oh!

tvami commented Oct 13, 2021

Uh oh!

tvami commented Oct 13, 2021

Uh oh!

cmsbuild commented Oct 13, 2021

Build

Clang Build

Uh oh!

makortel Oct 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tvami Oct 15, 2021

Choose a reason for hiding this comment

Uh oh!

hyunyong Oct 16, 2021

Choose a reason for hiding this comment

Uh oh!

Dr15Jones Oct 15, 2021

Choose a reason for hiding this comment

Uh oh!

Dr15Jones Oct 15, 2021

Choose a reason for hiding this comment

Uh oh!

tvami commented Oct 15, 2021

Uh oh!

tvami commented Oct 16, 2021

Uh oh!

hyunyong commented Oct 18, 2021

Uh oh!

hyunyong commented Oct 18, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

makortel Oct 13, 2021 •

edited

Loading