Fix esConsumes() for an EDModule that consumes EDProduct with @currentProcess #32217

makortel · 2020-11-20T15:05:25Z

PR description:

ES tokens use an "empty string optimization", i.e. re-using the \0 as the first element of EDConsumerBase::m_tokenLabels for empty strings. However, when an EDProduct is consumed with @currentProcess process name, EDConsumerBase::convertCurrentProcessAlias() overwrites m_tokenLabels to have something else as the first element, leading to NoProxyException being thrown when an ESHandle obtained with ESGetToken is dereferenced.

This PR fixed convertCurrentProcessAlias() to leave \0 as the first element of m_tokenLabels.

This problem was observed in #31962.

PR validation:

Framework unit tests run. In addition, with #31962 11634.0 step2 runs (that fails without this PR).

cmsbuild · 2020-11-20T15:12:47Z

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-32217/19961

This PR adds an extra 40KB to repository
There are other open Pull requests which might conflict with changes you have proposed:
- File FWCore/Framework/interface/EDConsumerBase.h modified in PR(s): Make EventSetup get to throw if called without a token when EDModule consumed any ES product #31746, Delete unscheduled modules that are not consumed by any scheduled module #29553
- File FWCore/Framework/src/EDConsumerBase.cc modified in PR(s): Delete unscheduled modules that are not consumed by any scheduled module #29553

Code check has found code style and quality issues which could be resolved by applying following patch(s)

code-format:
https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-32217/19961/code-format.patch
e.g. curl https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-32217/19961/code-format.patch | patch -p1
You can also run scram build code-format to apply code format directly

cmsbuild · 2020-11-20T15:20:52Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-32217/19962

This PR adds an extra 40KB to repository
There are other open Pull requests which might conflict with changes you have proposed:
- File FWCore/Framework/interface/EDConsumerBase.h modified in PR(s): Make EventSetup get to throw if called without a token when EDModule consumed any ES product #31746, Delete unscheduled modules that are not consumed by any scheduled module #29553
- File FWCore/Framework/src/EDConsumerBase.cc modified in PR(s): Delete unscheduled modules that are not consumed by any scheduled module #29553

cmsbuild · 2020-11-20T15:21:14Z

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

It involves the following packages:

FWCore/Framework
FWCore/Integration

@makortel, @smuzaffar, @cmsbuild, @Dr15Jones can you please review it and eventually sign? Thanks.
@wddgit 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-11-20T15:23:34Z

@cmsbuild, please test

makortel · 2020-11-20T15:23:42Z

@Dr15Jones please review

cmsbuild · 2020-11-20T15:24:02Z

The tests are being triggered in jenkins.

CMSSW_11_2_X_2020-11-19-2300/slc7_amd64_gcc820: https://cmssdt.cern.ch/jenkins/job/ib-run-pr-tests/10904/console Started: 2020/11/20 16:26

FWCore/Integration/test/ESTestAnalyzers.cc

Dr15Jones · 2020-11-20T18:15:19Z

FWCore/Integration/test/ESTestAnalyzers.cc

+  };
+
+  void ESTestAnalyzerL::analyze(edm::Event const& ev, edm::EventSetup const& es) {
+    auto const& dataJ = es.getData(esToken_);


The idea is to test on existence not on content?

Right, the de-reference of the ESHandle is the important part. I change the following printout to Info or Debug since it is not really important (mostly a leftover from copy-paste from ESTestAnalyzerJ).

cmsbuild · 2020-11-20T19:01:39Z

+1
Tested at: 3158047
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-4a06ef/10904/summary.html
CMSSW: CMSSW_11_2_X_2020-11-19-2300
SCRAM_ARCH: slc7_amd64_gcc820

cmsbuild · 2020-11-20T19:01:40Z

Comparison job queued.

cmsbuild · 2020-11-20T20:21:59Z

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-4a06ef/10904/summary.html

Comparison Summary:

No significant changes to the logs found
Reco comparison results: 4 differences found in the comparisons
DQMHistoTests: Total files compared: 37
DQMHistoTests: Total histograms compared: 2962300
DQMHistoTests: Total failures: 7
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 2962271
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 36 files compared)
Checked 158 log files, 34 edm output root files, 37 DQM output files

cmsbuild · 2020-11-23T19:28:39Z

Pull request #32217 was updated. @makortel, @smuzaffar, @cmsbuild, @Dr15Jones can you please check and sign again.

Dr15Jones · 2020-11-23T19:36:52Z

please test

cmsbuild · 2020-11-23T19:37:11Z

The tests are being triggered in jenkins.

CMSSW_11_2_X_2020-11-23-1100/slc7_amd64_gcc820: https://cmssdt.cern.ch/jenkins/job/ib-run-pr-tests/10961/console Started: 2020/11/23 20:39

cmsbuild · 2020-11-23T22:58:48Z

+1
Tested at: c2c4015
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-4a06ef/10961/summary.html
CMSSW: CMSSW_11_2_X_2020-11-23-1100
SCRAM_ARCH: slc7_amd64_gcc820

cmsbuild · 2020-11-23T22:58:51Z

Comparison job queued.

cmsbuild · 2020-11-24T00:25:13Z

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-4a06ef/10961/summary.html

Comparison Summary:

No significant changes to the logs found
Reco comparison results: 0 differences found in the comparisons
DQMHistoTests: Total files compared: 37
DQMHistoTests: Total histograms compared: 2961011
DQMHistoTests: Total failures: 5
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 2960984
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 36 files compared)
Checked 158 log files, 34 edm output root files, 37 DQM output files

makortel · 2020-11-24T00:38:50Z

+1

cmsbuild · 2020-11-24T00:39:12Z

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)

qliphy · 2020-11-24T01:38:21Z

+1

changing to esConsumes after fix in #32217

cmsbuild added this to the CMSSW_11_2_X milestone Nov 20, 2020

cmsbuild added code-checks-pending comparison-pending core-pending orp-pending pending-signatures tests-pending labels Nov 20, 2020

cmsbuild added code-checks-rejected and removed code-checks-pending labels Nov 20, 2020

makortel force-pushed the fixEsConsumesCurrentProcess branch from 26a0f35 to 3158047 Compare November 20, 2020 15:15

cmsbuild added code-checks-pending and removed code-checks-rejected labels Nov 20, 2020

cmsbuild added code-checks-approved and removed code-checks-pending labels Nov 20, 2020

makortel mentioned this pull request Nov 20, 2020

Change track muon dataformats scouting #31962

Merged

cmsbuild added tests-started and removed tests-pending labels Nov 20, 2020

Dr15Jones suggested changes Nov 20, 2020

View reviewed changes

cmsbuild added tests-approved and removed tests-started labels Nov 20, 2020

cmsbuild removed the comparison-pending label Nov 20, 2020

cmsbuild added code-checks-approved and removed code-checks-pending labels Nov 23, 2020

cmsbuild added tests-started and removed tests-pending labels Nov 23, 2020

cmsbuild added tests-approved and removed tests-started labels Nov 23, 2020

cmsbuild added comparison-available and removed comparison-pending labels Nov 24, 2020

cmsbuild added core-approved fully-signed and removed core-pending pending-signatures labels Nov 24, 2020

makortel mentioned this pull request Nov 24, 2020

Random DQM differences in PR tests #31644

Closed

cmsbuild added orp-approved and removed orp-pending labels Nov 24, 2020

cmsbuild merged commit c608f97 into cms-sw:master Nov 24, 2020

cmsbuild mentioned this pull request Nov 24, 2020

ECAL SC parameters to EventSetup #32066

Merged

makortel deleted the fixEsConsumesCurrentProcess branch November 24, 2020 21:49

jsalfeld added a commit to jsalfeld/cmssw that referenced this pull request Nov 27, 2020

changing to esConsumes after fix in cms-sw#32217

7120df7

jsalfeld mentioned this pull request Nov 27, 2020

changing to esConsumes after fix in #32217 #32311

Merged

cmsbuild added a commit that referenced this pull request Dec 2, 2020

Merge pull request #32311 from jsalfeld/adjustToEsConsumes

6f77d78

changing to esConsumes after fix in #32217

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix esConsumes() for an EDModule that consumes EDProduct with @currentProcess #32217

Fix esConsumes() for an EDModule that consumes EDProduct with @currentProcess #32217

makortel commented Nov 20, 2020 •

edited

cmsbuild commented Nov 20, 2020

cmsbuild commented Nov 20, 2020

cmsbuild commented Nov 20, 2020

makortel commented Nov 20, 2020

makortel commented Nov 20, 2020

cmsbuild commented Nov 20, 2020 •

edited

Dr15Jones Nov 20, 2020

makortel Nov 20, 2020

cmsbuild commented Nov 20, 2020

cmsbuild commented Nov 20, 2020

cmsbuild commented Nov 20, 2020

cmsbuild commented Nov 23, 2020

Dr15Jones commented Nov 23, 2020

cmsbuild commented Nov 23, 2020 •

edited

cmsbuild commented Nov 23, 2020

cmsbuild commented Nov 23, 2020

cmsbuild commented Nov 24, 2020

makortel commented Nov 24, 2020

cmsbuild commented Nov 24, 2020

qliphy commented Nov 24, 2020

Fix esConsumes() for an EDModule that consumes EDProduct with @currentProcess #32217

Fix esConsumes() for an EDModule that consumes EDProduct with @currentProcess #32217

Conversation

makortel commented Nov 20, 2020 • edited

PR description:

PR validation:

cmsbuild commented Nov 20, 2020

cmsbuild commented Nov 20, 2020

cmsbuild commented Nov 20, 2020

makortel commented Nov 20, 2020

makortel commented Nov 20, 2020

cmsbuild commented Nov 20, 2020 • edited

Dr15Jones Nov 20, 2020

Choose a reason for hiding this comment

makortel Nov 20, 2020

Choose a reason for hiding this comment

cmsbuild commented Nov 20, 2020

cmsbuild commented Nov 20, 2020

cmsbuild commented Nov 20, 2020

cmsbuild commented Nov 23, 2020

Dr15Jones commented Nov 23, 2020

cmsbuild commented Nov 23, 2020 • edited

cmsbuild commented Nov 23, 2020

cmsbuild commented Nov 23, 2020

cmsbuild commented Nov 24, 2020

makortel commented Nov 24, 2020

cmsbuild commented Nov 24, 2020

qliphy commented Nov 24, 2020

makortel commented Nov 20, 2020 •

edited

cmsbuild commented Nov 20, 2020 •

edited

cmsbuild commented Nov 23, 2020 •

edited