Consumes migration of Alignment/ReferenceTrajectories #35259

bbilin · 2021-09-14T08:28:43Z

PR description:

This PR is about the consumes migration of Alignment/ReferenceTrajectories and its dependencies.

PR validation:

Ran successfully wf 1001.0

1001.0_RunMinBias2011A+RunMinBias2011A+TIER0EXP+ALCAEXP+ALCAHARVDSIPIXELCALRUN1+ALCAHARVD1+ALCAHARVD2+ALCAHARVD3+ALCAHARVD4+ALCAHARVD5 Step0-PASSED Step1-PASSED Step2-PASSED Step3-PASSED Step4-PASSED Step5-PASSED Step6-PASSED Step7-PASSED Step8-PASSED - time date Tue Sep 28 10:38:23 2021-date Tue Sep 28 10:27:48 2021; exit: 0 0 0 0 0 0 0 0 0
1 1 1 1 1 1 1 1 1 tests passed, 0 0 0 0 0 0 0 0 0 failed

if this PR is a backport please specify the original PR and why you need to backport that PR:

This is not a backport and no backport is needed.

cmsbuild · 2021-09-14T08:36:34Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-35259/25244

This PR adds an extra 112KB to repository

cmsbuild · 2021-09-14T08:36:57Z

A new Pull Request was created by @bbilin (Bugra Bilin) for master.

It involves the following packages:

Alignment/CommonAlignmentAlgorithm (alca)
Alignment/CommonAlignmentProducer (alca)
Alignment/HIPAlignmentAlgorithm (alca)
Alignment/MillePedeAlignmentAlgorithm (alca)
Alignment/MuonAlignmentAlgorithms (alca)
Alignment/ReferenceTrajectories (alca)
Alignment/SurveyAnalysis (alca)

@yuanchao, @malbouis, @cmsbuild, @tvami, @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

mmusich · 2021-09-14T08:45:11Z

@cmsbuild, please test

cmsbuild · 2021-09-14T10:40:27Z

-1

Failed Tests: RelVals
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-695a5a/18581/summary.html
COMMIT: 52d9ac8
CMSSW: CMSSW_12_1_X_2021-09-13-2300/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/35259/18581/install.sh to create a dev area with all the needed externals and cmssw changes.

RelVals

1001.0

----- Begin Fatal Exception 14-Sep-2021 11:43:19 CEST-----------------------
An exception of category 'LogicError' occurred while
   [0] Processing global begin Run run: 165121
   [1] Calling method for module AlignmentProducerAsAnalyzer/'SiPixelAliMilleAlignmentProducer'
Exception Message:
A module declared it consumes an EventSetup product after its constructor.
This must be done in the contructor
The product type was: MagneticField in record IdealMagneticFieldRecord
and ESInputTag was Module label:  Data label: 
----- End Fatal Exception -------------------------------------------------

cmsbuild · 2021-09-14T10:40:46Z

-1

Failed Tests: RelVals
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-695a5a/18581/summary.html
COMMIT: 52d9ac8
CMSSW: CMSSW_12_1_X_2021-09-13-2300/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/35259/18581/install.sh to create a dev area with all the needed externals and cmssw changes.

RelVals

1001.0

----- Begin Fatal Exception 14-Sep-2021 11:43:19 CEST-----------------------
An exception of category 'LogicError' occurred while
   [0] Processing global begin Run run: 165121
   [1] Calling method for module AlignmentProducerAsAnalyzer/'SiPixelAliMilleAlignmentProducer'
Exception Message:
A module declared it consumes an EventSetup product after its constructor.
This must be done in the contructor
The product type was: MagneticField in record IdealMagneticFieldRecord
and ESInputTag was Module label:  Data label: 
----- End Fatal Exception -------------------------------------------------

mmusich · 2021-09-14T17:30:02Z

@makortel @Dr15Jones do you have suggestions about #35259 (comment)?

makortel · 2021-09-14T17:40:53Z

From just browsing the code in this PR it is not immediately clear to me which of the helper classes is constructed outside of AlignmentProducerAsAnalyzer constructor (or how the ConsumesCollector gets delivered there). I'm going to continue looking, but I'd like to note already now that adding ConsumesCollector arguments to functions that take Event or EventSetup like here

cmssw/Alignment/CommonAlignmentProducer/interface/AlignmentProducerBase.h

Line 90 in 52d9ac8

    
           bool processEvent(const edm::Event&, const edm::EventSetup&, edm::ConsumesCollector);

is not only unnecessary (declaring consumes information there would be way too late), those make reasoning the code by just browsing more difficult.

makortel · 2021-09-14T17:51:47Z

Ok, now I found the incorrect calls

cmssw/Alignment/CommonAlignmentProducer/plugins/AlignmentProducer.cc

Line 40 in 52d9ac8

initAlignmentAlgorithm(iSetup, consumesCollector());

cmssw/Alignment/CommonAlignmentProducer/plugins/AlignmentProducer.cc

Lines 85 to 86 in 52d9ac8

    
           edm::EDLooper::Status AlignmentProducer::duringLoop(const edm::Event &event, const edm::EventSetup &setup) { 
        
             if (processEvent(event, setup, consumesCollector()))

cmssw/Alignment/CommonAlignmentProducer/plugins/AlignmentProducer.cc

Line 94 in 52d9ac8

beginRunImpl(run, setup, consumesCollector());

The consumesCollector() may be called only in the constructor of an EDModule, as is already done in

cmssw/Alignment/CommonAlignmentProducer/plugins/AlignmentProducer.cc

Lines 13 to 14 in 52d9ac8

    
           AlignmentProducer::AlignmentProducer(const edm::ParameterSet &config) 
        
               : AlignmentProducerBase(config, consumesCollector()),

The helper classes need to be reworked such that either they take the ConsumesCollector in their constructor, or the class that uses the helpers declares the consumes information, gets the ESProducts, and passes them to the helper classes. Which of these is easier depends. In rare cases it is possible that neither of these really work (like in #35269), in which case we can discuss more.

tvami · 2021-09-17T13:50:31Z

@bbilin do you maybe have an update on this?

bbilin · 2021-09-19T18:58:20Z

Hi @tvami unfortunately I dont have any updates yet.

@makortel what I tried was to change, I am stuck as below. So any help, example similar codes or PRs would be useful for me to draw the path.

What I tried, for example, to replace

cmssw/Alignment/CommonAlignmentProducer/plugins/AlignmentProducer.cc
Lines 85 to 86

to

edm::EDLooper::Status AlignmentProducer::duringLoop(const edm::Event &event, const edm::EventSetup &setup,edm::ConsumesCollector iC) {
if (processEvent(event, setup, iC))

this

Status duringLoop(const edm::Event&, const edm::EventSetup&, edm::ConsumesCollector) override;
is coming from FWCore/Framework/interface/EDLooper.h.

So I am stuck here, this is clearly not the path to take. Any help, suggestion is welcome.

makortel · 2021-09-20T01:08:18Z

@bbilin I'm not sure if I understood your question correctly, so please elaborate if I don't answer to it. The problem on lines 85-86

cmssw/Alignment/CommonAlignmentProducer/plugins/AlignmentProducer.cc

Lines 85 to 86 in 52d9ac8

    
           edm::EDLooper::Status AlignmentProducer::duringLoop(const edm::Event &event, const edm::EventSetup &setup) { 
        
             if (processEvent(event, setup, consumesCollector()))

is the call to consumesCollector() on line 86. That function can be called only in the AlignmentProducer constructor. Trying to get an edm::ConsumesCollector in any other way will not work.

The AlignmentAlgorithmBase::alignmentAlgo_ already gets the ConsumesCollector in its constructor

cmssw/Alignment/CommonAlignmentProducer/src/AlignmentProducerBase.cc

Lines 281 to 289 in 52d9ac8

    
           void AlignmentProducerBase::createAlignmentAlgorithm(edm::ConsumesCollector& iC) { 
        
             auto algoConfig = config_.getParameter<edm::ParameterSet>("algoConfig"); 
        
             algoConfig.addUntrackedParameter("RunRangeSelection", config_.getParameter<edm::VParameterSet>("RunRangeSelection")); 
        
             algoConfig.addUntrackedParameter<align::RunNumber>("firstIOV", runAtPCL_ ? 1 : uniqueRunRanges_.front().first); 
        
             algoConfig.addUntrackedParameter("enableAlignableUpdates", enableAlignableUpdates_); 
        
             const auto& algoName = algoConfig.getParameter<std::string>("algoName"); 
        
             alignmentAlgo_ = AlignmentAlgorithmPluginFactory::get()->create(algoName, algoConfig, iC); 
        
           }

I guess you added the ConsumesCollector to the initialize() function because the TrajectoryFactoryPlugin::get()->create() is being called there, e.g. in

cmssw/Alignment/MillePedeAlignmentAlgorithm/plugins/MillePedeAlignmentAlgorithm.cc

Line 279 in 52d9ac8

    
           theTrajectoryFactory = TrajectoryFactoryPlugin::get()->create(fctName, fctCfg, iC);

Can those create() calls be moved to the constructors of AlignmentAlgorithmBase-derived classes?

bbilin · 2021-09-20T08:28:55Z

Hi @makortel
It is clearer now, thanks. So, please forgive the syntax for the moment, what you suggest is something like the following around line 87 of cmssw/Alignment/MillePedeAlignmentAlgorithm/plugins/MillePedeAlignmentAlgorithm.cc

MillePedeAlignmentAlgorithm::MillePedeAlignmentAlgorithm(const edm::ParameterSet &cfg, edm::ConsumesCollector &iC)
:
theTrajectoryFactory = TrajectoryFactoryPlugin::get()->create(fctCfg.getParameterstd::string("TrajectoryFactoryName"), theConfig.getParameteredm::ParameterSet("TrajectoryFactory"), iC), ...

If true I will try to rework like this.

Many thanks,
B.

makortel · 2021-09-20T14:13:25Z

@bbilin Yes, along those lines (I presume the call to create() should be done only if this->isMode(myMilleBit) as currently in the initialize() function).

bbilin · 2021-09-23T06:18:59Z

All clear, sorry, did not have any time until today. Will modify accordingly and commit. Thanks!

bbilin · 2021-09-27T13:28:10Z

Hi @makortel I played around a bit- the changes simplify the dependencies a lot, but what I get is an ld link issue during compilation. I pushed to a separate branch and can merge to the branch of the PR once fixed.

https://github.com/bbilin/cmssw/tree/ConsumesMigRefTraj2

Any ideas are welcome. Many thanks,

B.

makortel · 2021-09-27T13:38:11Z

@bbilin Could you post the link failure message?

bbilin · 2021-09-27T14:02:00Z

@makortel here it is:

Building edm plugin tmp/slc7_amd64_gcc900/src/Alignment/MillePedeAlignmentAlgorithm/plugins/AlignmentMillePedeAlignmentAlgorithmAuto/libAlignmentMillePedeAlignmentAlgorithmAuto.so
/cvmfs/cms-ib.cern.ch/nweek-02699/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/MillePedeAlignmentAlgorithm/plugins/AlignmentMillePedeAlignmentAlgorithmAuto/MillePedeAlignmentAlgorithm.cc.o: in function _GLOBAL__sub_I_MillePedeAlignmentAlgorithm.cc': MillePedeAlignmentAlgorithm.cc:(.text.startup+0x1a4): undefined reference to edmplugin::PluginFactory<AlignmentAlgorithmBase* (edm::ParameterSet const&, edm::ConsumesCollector&, edm::EventSetup&, AlignmentAlgorithmBase::EventInfo&)>::get()'
collect2: error: ld returned 1 exit status
gmake: *** [config/SCRAM/GMake/Makefile.rules:1700: tmp/slc7_amd64_gcc900/src/Alignment/MillePedeAlignmentAlgorithm/plugins/AlignmentMillePedeAlignmentAlgorithmAuto/libAlignmentMillePedeAlignmentAlgorithmAuto.so] Error 1
gmake: *** [There are compilation/build errors. Please see the detail log above.] Error 2

cmsbuild · 2021-09-28T11:14:20Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-695a5a/19169/summary.html
COMMIT: 4114f23
CMSSW: CMSSW_12_1_X_2021-09-27-2300/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/35259/19169/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

No significant changes to the logs found
Reco comparison results: 0 differences found in the comparisons
DQMHistoTests: Total files compared: 40
DQMHistoTests: Total histograms compared: 3211080
DQMHistoTests: Total failures: 0
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3211058
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 39 files compared)
Checked 169 log files, 37 edm output root files, 40 DQM output files
TriggerResults: no differences found

mmusich · 2021-09-28T11:15:46Z

@bbilin can you transform the PR from draft to "open for review"?

bbilin · 2021-09-28T12:08:34Z

@mmusich done. Thanks.

makortel · 2021-10-06T13:02:53Z

waiting for explicit green-light from the Tracker Alignment group #35259 (comment)

Any news from the tests yet?

tvami · 2021-10-06T13:05:18Z

waiting for explicit green-light from the Tracker Alignment group #35259 (comment)

Any news from the tests yet?

I just asked them privately yesterday: we expect some results by tomorrow.

mmusich · 2021-10-11T10:40:35Z

unhold

mmusich · 2021-10-11T10:40:44Z

unassign trk-dpg

mmusich · 2021-10-13T14:07:16Z

waiting for explicit green-light from the Tracker Alignment group

for the record, the results of validating this PR from the alignment point of view were presented here.

tvami · 2021-10-13T15:46:01Z

waiting for explicit green-light from the Tracker Alignment group

for the record, the results of validating this PR from the alignment point of view were presented here.

Ok so TEC changes still need to be investigated. Let's hope by Tuesday things will get cleared and we can get this in for pre5

tvami · 2021-10-19T13:19:53Z

@vbotta do you maybe have an update on the validation of this? pre5 is supposed to close today...

vbotta · 2021-10-19T15:34:51Z

@vbotta do you maybe have an update on the validation of this? pre5 is supposed to close today...

@tvami unfortunately not.

tvami · 2021-10-20T12:36:57Z

unfortunately not.

pre5 is now delayed, do you think this could converge on a week's timescale? Thanks @vbotta !

antoniovagnerini · 2021-10-20T22:50:46Z

We tracked down the origin of this small discrepancy and it is not arising from this specific esConsume migration, therefore you have green light to proceed with the merge

tvami · 2021-10-21T07:05:08Z

Great, thanks @antoniovagnerini !

tvami · 2021-10-21T07:06:03Z

+alca

jenkins ran fine
besides that the TkAlignment team ran a detailed validation which came back positive as well, see comment Consumes migration of Alignment/ReferenceTrajectories #35259 (comment)

cmsbuild · 2021-10-21T07:06:25Z

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. @perrotta, @dpiparo, @qliphy (and backports should be raised in the release meeting by the corresponding L2)

perrotta · 2021-10-21T13:43:12Z

+1

Let have this tested in the IBs as early as possible before the final pre

cmsbuild added this to the CMSSW_12_1_X milestone Sep 14, 2021

cmsbuild added alca-pending code-checks-pending orp-pending pending-signatures tests-pending labels Sep 14, 2021

cmsbuild added code-checks-approved and removed code-checks-pending labels Sep 14, 2021

cmsbuild added tests-started and removed tests-pending labels Sep 14, 2021

bbilin changed the title ~~Consumes mig ref traj~~ Consumes migration of Alignment/ReferenceTrajectories Sep 14, 2021

cmsbuild added tests-rejected and removed tests-started labels Sep 14, 2021

cmsbuild added tests-approved and removed tests-started labels Sep 28, 2021

bbilin marked this pull request as ready for review September 28, 2021 12:06

cmsbuild removed hold trk-dpg-pending labels Oct 11, 2021

cmsbuild added alca-approved fully-signed and removed alca-pending pending-signatures labels Oct 21, 2021

cmsbuild added orp-approved and removed orp-pending labels Oct 21, 2021

cmsbuild merged commit 532d278 into cms-sw:master Oct 21, 2021

cmsbuild mentioned this pull request Oct 21, 2021

switch loop index to uint from uint16 in FlagsCleanerECAL #35768

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Consumes migration of Alignment/ReferenceTrajectories #35259

Consumes migration of Alignment/ReferenceTrajectories #35259

bbilin commented Sep 14, 2021 •

edited

cmsbuild commented Sep 14, 2021

cmsbuild commented Sep 14, 2021

mmusich commented Sep 14, 2021

cmsbuild commented Sep 14, 2021

cmsbuild commented Sep 14, 2021

mmusich commented Sep 14, 2021

makortel commented Sep 14, 2021 •

edited

makortel commented Sep 14, 2021 •

edited

tvami commented Sep 17, 2021

bbilin commented Sep 19, 2021

makortel commented Sep 20, 2021

bbilin commented Sep 20, 2021 •

edited

makortel commented Sep 20, 2021

bbilin commented Sep 23, 2021

bbilin commented Sep 27, 2021

makortel commented Sep 27, 2021

bbilin commented Sep 27, 2021 •

edited

cmsbuild commented Sep 28, 2021

mmusich commented Sep 28, 2021

bbilin commented Sep 28, 2021

makortel commented Oct 6, 2021

tvami commented Oct 6, 2021

mmusich commented Oct 11, 2021

mmusich commented Oct 11, 2021

mmusich commented Oct 13, 2021

tvami commented Oct 13, 2021

tvami commented Oct 19, 2021

vbotta commented Oct 19, 2021 •

edited

tvami commented Oct 20, 2021

antoniovagnerini commented Oct 20, 2021

tvami commented Oct 21, 2021

tvami commented Oct 21, 2021

cmsbuild commented Oct 21, 2021

perrotta commented Oct 21, 2021

Consumes migration of Alignment/ReferenceTrajectories #35259

Consumes migration of Alignment/ReferenceTrajectories #35259

Conversation

bbilin commented Sep 14, 2021 • edited

PR description:

PR validation:

if this PR is a backport please specify the original PR and why you need to backport that PR:

cmsbuild commented Sep 14, 2021

cmsbuild commented Sep 14, 2021

mmusich commented Sep 14, 2021

cmsbuild commented Sep 14, 2021

RelVals

cmsbuild commented Sep 14, 2021

RelVals

mmusich commented Sep 14, 2021

makortel commented Sep 14, 2021 • edited

makortel commented Sep 14, 2021 • edited

tvami commented Sep 17, 2021

bbilin commented Sep 19, 2021

makortel commented Sep 20, 2021

bbilin commented Sep 20, 2021 • edited

makortel commented Sep 20, 2021

bbilin commented Sep 23, 2021

bbilin commented Sep 27, 2021

makortel commented Sep 27, 2021

bbilin commented Sep 27, 2021 • edited

cmsbuild commented Sep 28, 2021

Comparison Summary

mmusich commented Sep 28, 2021

bbilin commented Sep 28, 2021

makortel commented Oct 6, 2021

tvami commented Oct 6, 2021

mmusich commented Oct 11, 2021

mmusich commented Oct 11, 2021

mmusich commented Oct 13, 2021

tvami commented Oct 13, 2021

tvami commented Oct 19, 2021

vbotta commented Oct 19, 2021 • edited

tvami commented Oct 20, 2021

antoniovagnerini commented Oct 20, 2021

tvami commented Oct 21, 2021

tvami commented Oct 21, 2021

cmsbuild commented Oct 21, 2021

perrotta commented Oct 21, 2021

bbilin commented Sep 14, 2021 •

edited

makortel commented Sep 14, 2021 •

edited

makortel commented Sep 14, 2021 •

edited

bbilin commented Sep 20, 2021 •

edited

bbilin commented Sep 27, 2021 •

edited

vbotta commented Oct 19, 2021 •

edited