Enforce EventSetup products can be obtained only with ESGetToken #35588

makortel · 2021-10-08T14:27:05Z

PR description:

This PR is intended only for 12_3_X, but is opened already now to find test code outside of framework that would fail with it so we can start migrating those.

This PR changes the deprecated EventSetupRecord::get() function to throw an exception, causing all code accessing EventSetup products without ESGetToken.

Resolves cms-sw/framework-team#96

PR validation:

Framework unit tests ~~except three (that I will discuss below)~~ succeed.

cmsbuild · 2021-10-08T14:35:16Z

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-35588/25840

This PR adds an extra 88KB to repository

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-35588/25840/code-format.patch
e.g. curl https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-35588/25840/code-format.patch | patch -p1
You can also run scram build code-format to apply code format directly

Remove all tests of the deprecated API for which tests for the new API exist, and migrate the rest.

cmsbuild · 2021-10-08T14:43:17Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-35588/25841

This PR adds an extra 88KB to repository

cmsbuild · 2021-10-08T14:50:56Z

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

It involves the following packages:

FWCore/Framework (core)

@cmsbuild, @smuzaffar, @Dr15Jones, @makortel can you please review it and eventually sign? Thanks.
@wddgit 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

makortel · 2021-10-08T14:51:27Z

test parameters:

full_cmssw = true

makortel · 2021-10-08T14:51:35Z

@cmsbuild, please test

makortel

I pointed out the checks in eventsetuprecord_t test that fail currently. I've been focusing on proxyResetTest(), and transientTest() looks somewhat similar. I have not looked to getExepTest() much yet.

makortel · 2021-10-08T14:53:56Z

FWCore/Framework/test/eventsetuprecord_t.cppunit.cc

  CPPUNIT_ASSERT(&myDummy == &(*hDummy));
  CPPUNIT_ASSERT(!workingProxy->invalidateCalled());
  CPPUNIT_ASSERT(!workingProxy->invalidateTransientCalled());

  dummyProvider->resetProxies();
  CPPUNIT_ASSERT(workingProxy->invalidateCalled());
  CPPUNIT_ASSERT(workingProxy->invalidateTransientCalled());
-  dummyRecord.get(hDummy);
+  hDummy = dummyRecord.getHandle(token);
  CPPUNIT_ASSERT(&myDummy2 == &(*hDummy));
  CPPUNIT_ASSERT(!workingProxy->invalidateCalled());


This check fails.

makortel · 2021-10-08T14:55:35Z

FWCore/Framework/test/eventsetuprecord_t.cppunit.cc


-  dummyRecord.get(hDummy);
+  hDummy = dummyRecord.getHandleImpl<edm::ESHandle>(token);
  CPPUNIT_ASSERT(&myDummy2 == &(*hDummy));
  nonConstDummyRecordImpl.resetIfTransientInProxies();
  CPPUNIT_ASSERT(workingProxy->invalidateCalled() == false);


This check fails.

makortel · 2021-10-08T14:56:39Z

FWCore/Framework/test/eventsetuprecord_t.cppunit.cc

-  dummyRecord.setImpl(&dummyRecordImpl, 0, nullptr, &eventSetupImpl_, &pc, false);
-  dummyRecord.get(dummyPtr);
+  dummyRecord.setImpl(&dummyRecordImpl, 0, getTokenIndices.data(), &eventSetupImpl_, &pc);
+  ESHandle<Dummy> dummyPtr = dummyRecord.getHandle(token);
  //CPPUNIT_ASSERT_THROW(dummyRecord.get(dummyPtr), ExceptionType);
 }


This test expects edm::eventsetup::MakeDataException to be thrown, but it isn't.

That exception originates from

cmssw/FWCore/Framework/src/DataProxy.cc

Lines 93 to 95 in d310d6e

if UNLIKELY (nullptr == cache_) {

throwMakeException(iRecord, iKey);

}

The FailingDummyProxy returns a nullptr from its functions that generate the ES product. That should definitely trigger the exception. What happens of you try to dereference the handle?

So a big difference is calling EventSetupRecord::get(edm::ESHandle<....>....) will trigger running the DataProxy, while EventSetupRecord::getHandle will not. So I think you need to explicitly trigger the DataProxy.

@Dr15Jones Is this getExepTest() actually useful given

cmssw/FWCore/Framework/test/eventsetuprecord_t.cppunit.cc

Lines 278 to 297 in 5538699

void testEventsetupRecord::getHandleTest() {

FailingDummyProxy dummyProxy;

const DataKey dummyDataKey(DataKey::makeTypeTag<FailingDummyProxy::value_type>(), "");

ESHandle<Dummy> dummyPtr;

{

DummyDataConsumer consumer{edm::ESInputTag("", "")};

SetupRecord sr{consumer, dummyRecordKey_, eventSetupImpl_, &activityRegistry, {}};

DummyRecord dummyRecord = sr.makeRecord();

CPPUNIT_ASSERT(not dummyRecord.getHandle(consumer.m_token));

dummyPtr = dummyRecord.getHandle(consumer.m_token);

CPPUNIT_ASSERT(not dummyPtr.isValid());

CPPUNIT_ASSERT(not dummyPtr);

CPPUNIT_ASSERT(dummyPtr.failedToGet());

CPPUNIT_ASSERT_THROW(*dummyPtr, NoDataExceptionType);

CPPUNIT_ASSERT_THROW(makeESValid(dummyPtr), cms::Exception);

}

makortel · 2021-10-09T00:44:29Z

@smuzaffar Does the bot run tests for draft PRs?

smuzaffar · 2021-10-09T05:06:16Z

@smuzaffar Does the bot run tests for draft PRs?

Yes it does

smuzaffar · 2021-10-09T05:07:28Z

Please test

cmsbuild · 2021-10-09T10:41:35Z

-1

Failed Tests: UnitTests RelVals AddOn
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-cea191/19511/summary.html
COMMIT: 5538699
CMSSW: CMSSW_12_1_X_2021-10-08-2300/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/35588/19511/install.sh to create a dev area with all the needed externals and cmssw changes.

Unit Tests

I found errors in the following unit tests:

---> test test_MuonGeometryDBConverter had ERRORS
---> test TestDQMOnlineClient-hcalreco_dqm_sourceclient had ERRORS
---> test TestDQMOnlineClient-l1tstage2emulator_dqm_sourceclient had ERRORS
---> test TestDQMOnlineClient-mutracking_dqm_sourceclient had ERRORS
and more ...

RelVals

136.731

----- Begin Fatal Exception 09-Oct-2021 12:19:00 CEST-----------------------
An exception of category 'MustUseESGetToken' occurred while
   [0] Processing  stream begin Run run: 274199 stream: 0
   [1] Calling method for module L1TCaloLayer1/'simCaloStage2Layer1Digis'
Exception Message:
Called EventSetupRecord::get without using a ESGetToken.
 While requesting data type:HcalTrigTowerGeometry label:''
See https://twiki.cern.ch/twiki/bin/view/CMSPublic/SWGuideHowToGetDataFromES
for instructions how to migrate the calling code
----- End Fatal Exception -------------------------------------------------

136.793

----- Begin Fatal Exception 09-Oct-2021 12:19:00 CEST-----------------------
An exception of category 'MustUseESGetToken' occurred while
   [0] Processing  stream begin Run run: 301998 stream: 0
   [1] Calling method for module L1TCaloLayer1/'simCaloStage2Layer1Digis'
Exception Message:
Called EventSetupRecord::get without using a ESGetToken.
 While requesting data type:HcalTrigTowerGeometry label:''
See https://twiki.cern.ch/twiki/bin/view/CMSPublic/SWGuideHowToGetDataFromES
for instructions how to migrate the calling code
----- End Fatal Exception -------------------------------------------------

136.874

----- Begin Fatal Exception 09-Oct-2021 12:19:00 CEST-----------------------
An exception of category 'MustUseESGetToken' occurred while
   [0] Processing  stream begin Run run: 319450 stream: 0
   [1] Calling method for module L1TCaloLayer1/'simCaloStage2Layer1Digis'
Exception Message:
Called EventSetupRecord::get without using a ESGetToken.
 While requesting data type:HcalTrigTowerGeometry label:''
See https://twiki.cern.ch/twiki/bin/view/CMSPublic/SWGuideHowToGetDataFromES
for instructions how to migrate the calling code
----- End Fatal Exception -------------------------------------------------

Expand to see more relval errors ...

AddOn Tests

fastsim

----- Begin Fatal Exception 09-Oct-2021 12:21:30 CEST-----------------------
An exception of category 'MustUseESGetToken' occurred while
   [0] Processing  stream begin Run run: 1 stream: 0
   [1] Calling method for module HiggsValidation/'hplusValidation'
Exception Message:
Called EventSetupRecord::get without using a ESGetToken.
 While requesting data type:HepPDT::ParticleDataTable label:''
See https://twiki.cern.ch/twiki/bin/view/CMSPublic/SWGuideHowToGetDataFromES
for instructions how to migrate the calling code
----- End Fatal Exception -------------------------------------------------

fastsim1

----- Begin Fatal Exception 09-Oct-2021 12:21:29 CEST-----------------------
An exception of category 'MustUseESGetToken' occurred while
   [0] Processing  stream begin Run run: 1 stream: 2
   [1] Calling method for module HiggsValidation/'hplusValidation'
Exception Message:
Called EventSetupRecord::get without using a ESGetToken.
 While requesting data type:HepPDT::ParticleDataTable label:''
See https://twiki.cern.ch/twiki/bin/view/CMSPublic/SWGuideHowToGetDataFromES
for instructions how to migrate the calling code
----- End Fatal Exception -------------------------------------------------

fastsim2

----- Begin Fatal Exception 09-Oct-2021 12:21:30 CEST-----------------------
An exception of category 'MustUseESGetToken' occurred while
   [0] Processing  stream begin Run run: 1 stream: 0
   [1] Calling method for module HiggsValidation/'hplusValidation'
Exception Message:
Called EventSetupRecord::get without using a ESGetToken.
 While requesting data type:HepPDT::ParticleDataTable label:''
See https://twiki.cern.ch/twiki/bin/view/CMSPublic/SWGuideHowToGetDataFromES
for instructions how to migrate the calling code
----- End Fatal Exception -------------------------------------------------

Expand to see more addon errors ...

makortel · 2021-10-09T23:51:39Z

I collected the EDModules causing the failures to https://docs.google.com/spreadsheets/d/1cUypk2EK4xVcEpFGtaNqXrWZEoFz_E9l4gNN3YoBQH4/edit#gid=72982552

cmsbuild · 2021-12-07T13:56:08Z

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-12-12T17:52:42Z

please test
(to refresh them before merging)

cmsbuild · 2021-12-12T23:55:06Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-cea191/21208/summary.html
COMMIT: 958abb5
CMSSW: CMSSW_12_3_X_2021-12-12-0000/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/35588/21208/install.sh to create a dev area with all the needed externals and cmssw changes.

CMS deprecated warnings: 39 CMS deprecated warnings found, see summary page for details.

Comparison Summary

Summary:

No significant changes to the logs found
Reco comparison results: 4 differences found in the comparisons
DQMHistoTests: Total files compared: 42
DQMHistoTests: Total histograms compared: 3250704
DQMHistoTests: Total failures: 11
DQMHistoTests: Total nulls: 1
DQMHistoTests: Total successes: 3250670
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.004 KiB( 41 files compared)
DQMHistoSizes: changed ( 312.0 ): 0.004 KiB MessageLogger/Warnings
Checked 177 log files, 37 edm output root files, 42 DQM output files
TriggerResults: no differences found

qliphy · 2021-12-13T01:09:26Z

enable threading
just to make sure further there will be no issue in IB tests with multithreading

qliphy · 2021-12-13T01:09:54Z

please test

cmsbuild · 2021-12-13T07:39:32Z

-1

Failed Tests: UnitTests
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-cea191/21209/summary.html
COMMIT: 958abb5
CMSSW: CMSSW_12_3_X_2021-12-12-0000/slc7_amd64_gcc900
Additional Tests: THREADING
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/35588/21209/install.sh to create a dev area with all the needed externals and cmssw changes.

CMS deprecated warnings: 39 CMS deprecated warnings found, see summary page for details.

Unit Tests

I found errors in the following unit tests:

---> test testCondToolsSiStripBuildersReaders had ERRORS

Comparison Summary

Summary:

No significant changes to the logs found
Reco comparison results: 2 differences found in the comparisons
DQMHistoTests: Total files compared: 42
DQMHistoTests: Total histograms compared: 3250704
DQMHistoTests: Total failures: 6
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3250676
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 41 files compared)
Checked 177 log files, 37 edm output root files, 42 DQM output files
TriggerResults: no differences found

qliphy · 2021-12-13T08:36:21Z

the unit test failure is not related to this PR:
#36319

perrotta · 2021-12-13T09:20:49Z

+1

Unit test error in the last tests is uncorrelated
It is probably time to finally enforce EventSetup products to be obtained only with ESGetToken, now that we are at the beginning of the 12_3_X cycle: lets do it now, and see in the coming days if there are any remaining modules to fix, in case any crashes appear in relvals or unit tests

perrotta · 2021-12-13T09:21:48Z

merge

cmsbuild added this to the CMSSW_12_1_X milestone Oct 8, 2021

cmsbuild added code-checks-pending core-pending orp-pending pending-signatures tests-pending labels Oct 8, 2021

makortel mentioned this pull request Oct 8, 2021

[12_3_X] Throw exception for EDModules calling deprecated EventSetupRecord::get() cms-sw/framework-team#96

Closed

cmsbuild added code-checks-rejected and removed code-checks-pending labels Oct 8, 2021

Throw exception from deprecated EventSetupRecord::get()

5538699

Remove all tests of the deprecated API for which tests for the new API exist, and migrate the rest.

makortel force-pushed the throwDeprecatedESRGet branch from ddf4586 to 5538699 Compare October 8, 2021 14:37

cmsbuild added code-checks-pending and removed code-checks-rejected labels Oct 8, 2021

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

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

makortel commented Oct 8, 2021

View reviewed changes

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

cmsbuild added core-approved fully-signed and removed core-pending pending-signatures labels Dec 7, 2021

cmsbuild added tests-started and removed tests-approved labels Dec 12, 2021

cmsbuild added tests-approved and removed tests-started labels Dec 12, 2021

cmsbuild added tests-started and removed tests-approved labels Dec 13, 2021

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

cmsbuild added orp-approved and removed orp-pending labels Dec 13, 2021

cmsbuild merged commit 0b3d20d into cms-sw:master Dec 13, 2021

makortel deleted the throwDeprecatedESRGet branch December 13, 2021 13:56

This was referenced Dec 13, 2021

[12.3.X] Move to root 6.24 cms-sw/cmsdist#7500

Merged

[GCC11] Fix warning "strncmp ... evaluates to nonzero" #36467

Merged

[ROOT624] Testing root 62.4 for gcc10 cms-sw/cmsdist#7505

Closed

This was referenced Dec 14, 2021

Investigate possibility for batched DB queries in PoolDBESSource #31084

Open

Look into enabling edm::ESTransientHandle memory optimization again #31085

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enforce EventSetup products can be obtained only with ESGetToken #35588

Enforce EventSetup products can be obtained only with ESGetToken #35588

makortel commented Oct 8, 2021 •

edited

cmsbuild commented Oct 8, 2021

cmsbuild commented Oct 8, 2021

cmsbuild commented Oct 8, 2021

makortel commented Oct 8, 2021

makortel commented Oct 8, 2021

makortel left a comment

makortel Oct 8, 2021

makortel Oct 8, 2021

makortel Oct 8, 2021

Dr15Jones Oct 8, 2021

Dr15Jones Oct 8, 2021

Dr15Jones Oct 8, 2021

makortel Oct 27, 2021

makortel commented Oct 9, 2021

smuzaffar commented Oct 9, 2021

smuzaffar commented Oct 9, 2021

cmsbuild commented Oct 9, 2021

makortel commented Oct 9, 2021

cmsbuild commented Dec 7, 2021

perrotta commented Dec 12, 2021

cmsbuild commented Dec 12, 2021

qliphy commented Dec 13, 2021

qliphy commented Dec 13, 2021

cmsbuild commented Dec 13, 2021

qliphy commented Dec 13, 2021

perrotta commented Dec 13, 2021

perrotta commented Dec 13, 2021

	if UNLIKELY (nullptr == cache_) {
	throwMakeException(iRecord, iKey);
	}

	void testEventsetupRecord::getHandleTest() {
	FailingDummyProxy dummyProxy;

	const DataKey dummyDataKey(DataKey::makeTypeTag<FailingDummyProxy::value_type>(), "");

	ESHandle<Dummy> dummyPtr;
	{
	DummyDataConsumer consumer{edm::ESInputTag("", "")};

	SetupRecord sr{consumer, dummyRecordKey_, eventSetupImpl_, &activityRegistry, {}};
	DummyRecord dummyRecord = sr.makeRecord();

	CPPUNIT_ASSERT(not dummyRecord.getHandle(consumer.m_token));
	dummyPtr = dummyRecord.getHandle(consumer.m_token);
	CPPUNIT_ASSERT(not dummyPtr.isValid());
	CPPUNIT_ASSERT(not dummyPtr);
	CPPUNIT_ASSERT(dummyPtr.failedToGet());
	CPPUNIT_ASSERT_THROW(*dummyPtr, NoDataExceptionType);
	CPPUNIT_ASSERT_THROW(makeESValid(dummyPtr), cms::Exception);
	}

Enforce EventSetup products can be obtained only with ESGetToken #35588

Enforce EventSetup products can be obtained only with ESGetToken #35588

Conversation

makortel commented Oct 8, 2021 • edited

PR description:

PR validation:

cmsbuild commented Oct 8, 2021

cmsbuild commented Oct 8, 2021

cmsbuild commented Oct 8, 2021

makortel commented Oct 8, 2021

makortel commented Oct 8, 2021

makortel left a comment

Choose a reason for hiding this comment

makortel Oct 8, 2021

Choose a reason for hiding this comment

makortel Oct 8, 2021

Choose a reason for hiding this comment

makortel Oct 8, 2021

Choose a reason for hiding this comment

Dr15Jones Oct 8, 2021

Choose a reason for hiding this comment

Dr15Jones Oct 8, 2021

Choose a reason for hiding this comment

Dr15Jones Oct 8, 2021

Choose a reason for hiding this comment

makortel Oct 27, 2021

Choose a reason for hiding this comment

makortel commented Oct 9, 2021

smuzaffar commented Oct 9, 2021

smuzaffar commented Oct 9, 2021

cmsbuild commented Oct 9, 2021

Unit Tests

RelVals

AddOn Tests

makortel commented Oct 9, 2021

cmsbuild commented Dec 7, 2021

perrotta commented Dec 12, 2021

cmsbuild commented Dec 12, 2021

Comparison Summary

qliphy commented Dec 13, 2021

qliphy commented Dec 13, 2021

cmsbuild commented Dec 13, 2021

Unit Tests

Comparison Summary

qliphy commented Dec 13, 2021

perrotta commented Dec 13, 2021

perrotta commented Dec 13, 2021

makortel commented Oct 8, 2021 •

edited