Throw GetByLabelWithoutRegistration exception also when no module in a job registered consumption of a given branch #38875

makortel · 2022-07-27T19:07:05Z

PR description:

#38864 reported a case where an EDAnalyzer using Event::getByLabel() without declaring the consumed data products got a ProductNotFound exception instead of more explanatory GetByLabelWithoutRegistration exception. I traced this down to ProductResolverIndexHelper not being populated (from ProductRegistry with ~~non-consumed branches~~ branches that have product type that is not consumed in the job (either via the same or different branch)

cmssw/DataFormats/Provenance/src/ProductRegistry.cc

Lines 419 to 422 in db4ff9a

    
           // The main type is not consumed, and either 
        
           // there is no contained type, or if there is, 
        
           // neither it nor any of its base classes are consumed. 
        
           // Don't insert anything in the lookup tables.

leading to ProductResolverIndexHelper::index() returning a ProductResolverIndexInvalid and Principal::findProductByLabel() returning nullptr, which is interpreted as ProductNotFound by calling code.

This PR proposes to change the Principal::findProductByLabel() to check the branch existence directly from ProductRegistry after getting ProductResolverIndexInvalid, and throw GetByLabelWithoutRegistration exception if the branch in question is present. This adds some cost, but only in case

code calls consumes but uses (deprecated) getByLabel() to get the data, and
product in question does not exist, and
code checks the Handle validity and is able to ignore the missing product

I would expect this case to be rare, and therefore the cost to be acceptable. I also think we should make an effort to address the remaining getByLabel() calls in the near-to-medium-future.

PR validation:

Checked that the test configuration in #38864 now throws GetByLabelWithoutRegistration exception.

cmsbuild · 2022-07-27T19:12:33Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-38875/31299

This PR adds an extra 24KB to repository
There are other open Pull requests which might conflict with changes you have proposed:
- File FWCore/Framework/src/Principal.cc modified in PR(s): Add Transformer ability to modules #38454

cmsbuild · 2022-07-27T19:12:57Z

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, @rappoccio you are the release manager for this.

cms-bot commands are listed here

Dr15Jones · 2022-07-27T19:21:50Z

I don't think this is the proper solution. It seems to miss the case where

module A calls getByLabel without doing the consumes
module B just happens to consumes that exact same data

This would cause a race condition as to whether A gets the data if A does not also happen to always be scheduled to run after B.

Dr15Jones · 2022-07-27T19:24:15Z

My point being that getByLabel should throw an exception for not calling comsumes for that data product even if the data product happens to be accessible in the Event at the time of the call.

makortel · 2022-07-27T19:27:59Z

If B consumes the exact same data, this code (referring now to code in master)

cmssw/FWCore/Framework/src/Principal.cc

Lines 805 to 806 in db4ff9a

    
           ProductResolverIndex index = 
        
               productLookup().index(kindOfType, typeID, label.c_str(), instance.c_str(), process.c_str());

should return a valid index. In that case the code I added in this PR will not get run, and the existing check

cmssw/FWCore/Framework/src/Principal.cc

Lines 814 to 816 in db4ff9a

    
           if (UNLIKELY(consumer and (not consumer->registeredToConsume(index, false, branchType())))) { 
        
             failedToRegisterConsumes(kindOfType, typeID, label, instance, process); 
        
           }

should check if this module declared the consumption of the data product, and throw the GetByLabelWithoutRegistration exception if consumes() was not called.

I think this case was already covered, and I don't see how changes in this PR would impact that.

Dr15Jones · 2022-07-27T19:33:07Z

What made it not possible to re-use the logic on line 814?

Dr15Jones · 2022-07-27T19:44:27Z

How about adding a unit test option for this case?

makortel · 2022-07-27T19:44:54Z

What made it not possible to re-use the logic on line 814?

The index in line 814 would be ProductResolverIndexInvalid in this case, and

cmssw/FWCore/Framework/src/EDConsumerBase.cc

Lines 348 to 358 in db4ff9a

    
           bool EDConsumerBase::registeredToConsume(ProductResolverIndex iIndex, 
        
                                                    bool skipCurrentProcess, 
        
                                                    BranchType iBranch) const { 
        
             for (auto it = m_tokenInfo.begin<kLookupInfo>(), itEnd = m_tokenInfo.end<kLookupInfo>(); it != itEnd; ++it) { 
        
               if (it->m_index.productResolverIndex() == iIndex and it->m_index.skipCurrentProcess() == skipCurrentProcess and 
        
                   it->m_branchType == iBranch) { 
        
                 return true; 
        
               } 
        
             } 
        
             return false; 
        
           }

would then return false always.

makortel · 2022-07-27T19:45:45Z

How about adding a unit test option for this case?

I was hoping to get away by getByLabel() being deprecated etc, but since you suggested it, I'll craft one.

…nsumes() is not called

…a job registered consumption of a given branch

makortel · 2022-07-28T17:44:15Z

I added a test that (in the first commit) demonstrates what happens when a data product is obtained via getByLabel() with and without consumes() calls when

data product comes from the input file, no other consumers of the same product type (the problematic case of this PR)
data product of the same type is produced+consumed by other modules
data product of different type is produced+consumed by other modules

The changes in the test in the second commit demonstrate the effect of the changes.

cmsbuild · 2022-07-28T17:46:26Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-38875/31317

This PR adds an extra 28KB to repository
There are other open Pull requests which might conflict with changes you have proposed:
- File FWCore/Framework/src/Principal.cc modified in PR(s): Add Transformer ability to modules #38454
- File FWCore/Integration/test/BuildFile.xml modified in PR(s): Add Transformer ability to modules #38454

cmsbuild · 2022-07-28T17:46:46Z

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

makortel · 2022-07-28T17:50:00Z

@cmsbuild, please test

cmsbuild · 2022-07-28T22:16:05Z

-1

Failed Tests: RelVals-INPUT
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-8f9c39/26514/summary.html
COMMIT: 7b2d9a7
CMSSW: CMSSW_12_5_X_2022-07-28-1100/el8_amd64_gcc10
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/38875/26514/install.sh to create a dev area with all the needed externals and cmssw changes.

RelVals-INPUT

The relvals timed out after 4 hours.

Comparison Summary

Summary:

No significant changes to the logs found
Reco comparison results: 2 differences found in the comparisons
DQMHistoTests: Total files compared: 51
DQMHistoTests: Total histograms compared: 3668050
DQMHistoTests: Total failures: 8
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3668020
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 50 files compared)
Checked 210 log files, 47 edm output root files, 51 DQM output files
TriggerResults: no differences found

makortel · 2022-08-01T15:07:09Z

@Dr15Jones Does the added test address your concern?

Dr15Jones · 2022-08-01T15:10:01Z

I'm good with the change once the build system decides to finish its tests :).

makortel · 2022-08-01T15:12:44Z

@cmsbuild, please test

cmsbuild · 2022-08-01T19:11:49Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-8f9c39/26569/summary.html
COMMIT: 7b2d9a7
CMSSW: CMSSW_12_5_X_2022-08-01-1100/el8_amd64_gcc10
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/38875/26569/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: 4 differences found in the comparisons
DQMHistoTests: Total files compared: 51
DQMHistoTests: Total histograms compared: 3669004
DQMHistoTests: Total failures: 7
DQMHistoTests: Total nulls: 1
DQMHistoTests: Total successes: 3668974
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: -0.004 KiB( 50 files compared)
DQMHistoSizes: changed ( 312.0 ): -0.004 KiB MessageLogger/Warnings
Checked 210 log files, 47 edm output root files, 51 DQM output files
TriggerResults: no differences found

makortel · 2022-08-01T20:38:20Z

+1

cmsbuild · 2022-08-01T20:38:37Z

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

qliphy · 2022-08-02T00:53:42Z

+1

cmsbuild added this to the CMSSW_12_5_X milestone Jul 27, 2022

cmsbuild added code-checks-pending core-pending orp-pending pending-signatures tests-pending labels Jul 27, 2022

cmsbuild added code-checks-approved and removed code-checks-pending labels Jul 27, 2022

makortel added 2 commits July 28, 2022 19:30

Add a test demonstrating the current behavior of getByLabel() when co…

abae3b4

…nsumes() is not called

Throw GetByLabelWithoutRegistration exception also when no module in …

7b2d9a7

…a job registered consumption of a given branch

makortel force-pushed the failedToRegisterConsumes branch from a8fad0e to 7b2d9a7 Compare July 28, 2022 17:39

cmsbuild added code-checks-pending and removed code-checks-approved labels Jul 28, 2022

cmsbuild added code-checks-approved and removed code-checks-pending labels Jul 28, 2022

cmsbuild added tests-started and removed tests-pending labels Jul 28, 2022

cmsbuild added tests-rejected and removed tests-started labels Jul 28, 2022

cmsbuild added tests-started and removed tests-rejected labels Aug 1, 2022

cmsbuild added tests-approved and removed tests-started labels Aug 1, 2022

cmsbuild added core-approved fully-signed and removed core-pending pending-signatures labels Aug 1, 2022

makortel mentioned this pull request Aug 1, 2022

Try to fix GeometryProducer and G4e examples #38864

Merged

cmsbuild added orp-approved and removed orp-pending labels Aug 2, 2022

cmsbuild merged commit 79e7839 into cms-sw:master Aug 2, 2022

cmsbuild mentioned this pull request Aug 2, 2022

Update run for DQM beamhlt client unitTest #38925

Merged

makortel deleted the failedToRegisterConsumes branch August 2, 2022 13:12

cmsbuild mentioned this pull request Aug 3, 2022

[ROOT626] Root update v6 26 00 patches cms-sw/cmsdist#8010

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Throw GetByLabelWithoutRegistration exception also when no module in a job registered consumption of a given branch #38875

Throw GetByLabelWithoutRegistration exception also when no module in a job registered consumption of a given branch #38875

makortel commented Jul 27, 2022 •

edited

Loading

cmsbuild commented Jul 27, 2022

cmsbuild commented Jul 27, 2022

Dr15Jones commented Jul 27, 2022

Dr15Jones commented Jul 27, 2022

makortel commented Jul 27, 2022 •

edited

Loading

Dr15Jones commented Jul 27, 2022

Dr15Jones commented Jul 27, 2022

makortel commented Jul 27, 2022

makortel commented Jul 27, 2022

makortel commented Jul 28, 2022

cmsbuild commented Jul 28, 2022

cmsbuild commented Jul 28, 2022

makortel commented Jul 28, 2022

cmsbuild commented Jul 28, 2022

makortel commented Aug 1, 2022

Dr15Jones commented Aug 1, 2022

makortel commented Aug 1, 2022

cmsbuild commented Aug 1, 2022

makortel commented Aug 1, 2022

cmsbuild commented Aug 1, 2022

qliphy commented Aug 2, 2022

	// The main type is not consumed, and either
	// there is no contained type, or if there is,
	// neither it nor any of its base classes are consumed.
	// Don't insert anything in the lookup tables.

Throw GetByLabelWithoutRegistration exception also when no module in a job registered consumption of a given branch #38875

Throw GetByLabelWithoutRegistration exception also when no module in a job registered consumption of a given branch #38875

Conversation

makortel commented Jul 27, 2022 • edited Loading

PR description:

PR validation:

cmsbuild commented Jul 27, 2022

cmsbuild commented Jul 27, 2022

Dr15Jones commented Jul 27, 2022

Dr15Jones commented Jul 27, 2022

makortel commented Jul 27, 2022 • edited Loading

Dr15Jones commented Jul 27, 2022

Dr15Jones commented Jul 27, 2022

makortel commented Jul 27, 2022

makortel commented Jul 27, 2022

makortel commented Jul 28, 2022

cmsbuild commented Jul 28, 2022

cmsbuild commented Jul 28, 2022

makortel commented Jul 28, 2022

cmsbuild commented Jul 28, 2022

RelVals-INPUT

Comparison Summary

makortel commented Aug 1, 2022

Dr15Jones commented Aug 1, 2022

makortel commented Aug 1, 2022

cmsbuild commented Aug 1, 2022

Comparison Summary

makortel commented Aug 1, 2022

cmsbuild commented Aug 1, 2022

qliphy commented Aug 2, 2022

makortel commented Jul 27, 2022 •

edited

Loading

makortel commented Jul 27, 2022 •

edited

Loading