Move EventSetup module PSet validation to be called from ComponentMaker #39556

makortel · 2022-09-30T20:14:22Z

PR description:

The #39391 missed the detail that in the EventSetup the PSet validation is done via a separate plugin factory.

This PR attempts to address that deficiency by moving the PSet validation to be called from the ComponentMaker, just before the ES module is constructed. A benefit of this approach is that it makes the EventSetup side a little bit more consistent with the Event side. A downside is that more code gets mutable access to the PSet (there could be more I didn't think of).

An alternative would be to extend the module type resolving logic in some way to edm::eventsetup::validateEventSetupParameters(). In a later PR I'll change the

cmssw/FWCore/Framework/interface/resolveMaker.h

Line 16 in 8e73e83

    
           auto resolveMaker(std::string const& moduleType, ModuleTypeResolverBase const* resolver) {

to handle the insertion of the found maker into the caches in Factory/ComponentFactory (in order to cache based on the resolved module type rather than the requested module type), and while handling this non-cached plugin can be added there with some complexity, I felt the approach of this PR to be simpler in the bigger picture.

PR validation:

Framework unit tests pass.

The python configuration system always injects @module_label.

…DescriptionFiller

makortel

@Dr15Jones @wddgit What do you think?

FWCore/Framework/src/EventSetupsController.cc

makortel · 2022-09-30T20:16:14Z

FWCore/ParameterSet/interface/ParameterSetDescriptionFillerBase.h

@@ -78,12 +78,13 @@ namespace edm {

    // ---------- member functions ---------------------------

-  protected:
-    static const std::string kEmpty;
-    static const std::string kBaseForService;
    static const std::string kBaseForESSource;
    static const std::string kBaseForESProducer;
    static const std::string kBaseForEDLooper;


Maybe there is a better way than making these public? Although maybe it isn't too bad either?

(in the Event system the EDModule base classes define the corresponding strings)

Is it possible to use constexpr std::string_view for this to avoid allocation when the shared library is loaded? [Although many implementations of std::string have a small string optimization]

Sounds plausible, but I'd leave that to a subsequent PR because of being somewhat orthogonal.

cmsbuild · 2022-09-30T20:22:02Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-39556/32345

This PR adds an extra 64KB to repository
There are other open Pull requests which might conflict with changes you have proposed:
- File FWCore/Framework/interface/EventSetupsController.h modified in PR(s): Implement support for concurrent runs in the Framework #38801
- File FWCore/Framework/src/EventProcessor.cc modified in PR(s): Implement support for concurrent runs in the Framework #38801
- File FWCore/Framework/src/EventSetupsController.cc modified in PR(s): Implement support for concurrent runs in the Framework #38801

cmsbuild · 2022-09-30T20:22:28Z

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

It involves the following packages:

FWCore/Framework (core)
FWCore/ParameterSet (core)

@cmsbuild, @smuzaffar, @Dr15Jones, @makortel can you please review it and eventually sign? Thanks.
@missirol, @wddgit this is something you requested to watch as well.
@perrotta, @dpiparo, @rappoccio you are the release manager for this.

cms-bot commands are listed here

makortel · 2022-09-30T20:37:45Z

@cmsbuild, please test

wddgit · 2022-09-30T21:47:25Z

I took a quick look. It all looks reasonable to me.

cmsbuild · 2022-10-01T05:58:39Z

-1

Failed Tests: UnitTests
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-db8483/27888/summary.html
COMMIT: b17d5d8
CMSSW: CMSSW_12_6_X_2022-09-30-1100/el8_amd64_gcc10
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/39556/27888/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 createDBObjecs had ERRORS
---> test GeometryTrackerGeometryBuilderTestDriver had ERRORS
---> test testPhase2SkimmedGeometry had ERRORS
---> test testPhase2PixelNtuple had ERRORS
and more ...

Comparison Summary

@slava77 comparisons for the following workflows were not done due to missing matrix map:

/data/cmsbld/jenkins/workspace/compare-root-files-short-matrix/data/PR-db8483/41834.0_TTbar_14TeV+2026D94+TTbar_14TeV_TuneCP5_GenSimHLBeamSpot14+DigiTrigger+RecoGlobal+HARVESTGlobal

Summary:

No significant changes to the logs found
Reco comparison results: 9 differences found in the comparisons
DQMHistoTests: Total files compared: 49
DQMHistoTests: Total histograms compared: 3432650
DQMHistoTests: Total failures: 9
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3432619
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 48 files compared)
Checked 204 log files, 49 edm output root files, 49 DQM output files
TriggerResults: no differences found

FWCore/Framework/interface/ComponentMaker.h

FWCore/Framework/src/EventSetupProvider.cc

Dr15Jones · 2022-10-03T20:04:44Z

done with comments

This change is motivated by ModuleTypeResolver functionality, which, on the EventSetup side, currently misses the PSet validation plugin factory. On the Event side the PSet validation is done as part of the module maker, and this particular change makes the module creation on EventSetup and Event sides a little bit more consistent.

…ProducerPSet()

cmsbuild · 2022-10-04T00:04:38Z

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

makortel · 2022-10-04T00:31:14Z

@cmsbuild, please test

cmsbuild · 2022-10-04T08:18:45Z

-1

Failed Tests: UnitTests RelVals-INPUT
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-db8483/27962/summary.html
COMMIT: 7a302e0
CMSSW: CMSSW_12_6_X_2022-10-03-2300/el8_amd64_gcc10
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/39556/27962/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 createDBObjecs had ERRORS
---> test materialBudgetHGCalPlots had ERRORS

RelVals-INPUT

The relvals timed out after 4 hours.

Comparison Summary

@slava77 comparisons for the following workflows were not done due to missing matrix map:

/data/cmsbld/jenkins/workspace/compare-root-files-short-matrix/data/PR-db8483/41834.0_TTbar_14TeV+2026D94+TTbar_14TeV_TuneCP5_GenSimHLBeamSpot14+DigiTrigger+RecoGlobal+HARVESTGlobal

Summary:

No significant changes to the logs found
Reco comparison results: 1 differences found in the comparisons
DQMHistoTests: Total files compared: 49
DQMHistoTests: Total histograms compared: 3432650
DQMHistoTests: Total failures: 3
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3432625
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 48 files compared)
Checked 204 log files, 49 edm output root files, 49 DQM output files
TriggerResults: no differences found

makortel · 2022-10-05T15:09:10Z

@cmsbuild, please test

aandvalenzuela · 2022-10-06T14:51:31Z

please test

Tests need to be restarted due to an issue with singularity when publishing to cvmfs. Sorry for the inconvenience!

cmsbuild · 2022-10-06T22:59:27Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-db8483/28076/summary.html
COMMIT: 7a302e0
CMSSW: CMSSW_12_6_X_2022-10-05-2300/el8_amd64_gcc10
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/39556/28076/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

@slava77 comparisons for the following workflows were not done due to missing matrix map:

/data/cmsbld/jenkins/workspace/compare-root-files-short-matrix/data/PR-db8483/41834.0_TTbar_14TeV+2026D94+TTbar_14TeV_TuneCP5_GenSimHLBeamSpot14+DigiTrigger+RecoGlobal+HARVESTGlobal

Summary:

No significant changes to the logs found
Reco comparison results: 4 differences found in the comparisons
DQMHistoTests: Total files compared: 49
DQMHistoTests: Total histograms compared: 3391103
DQMHistoTests: Total failures: 3
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3391078
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 48 files compared)
Checked 204 log files, 49 edm output root files, 49 DQM output files
TriggerResults: no differences found

makortel · 2022-10-07T13:14:46Z

+1

cmsbuild · 2022-10-07T13:15:07Z

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

perrotta · 2022-10-09T13:49:45Z

+1

makortel added 2 commits September 30, 2022 20:15

Remove handling of unlabeled ESModule as obsolete

e595797

The python configuration system always injects @module_label.

Simplify fillDescriptions() and prevalidate() calling in ParameterSet…

2749c23

…DescriptionFiller

cmsbuild added this to the CMSSW_12_6_X milestone Sep 30, 2022

cmsbuild added code-checks-pending core-pending orp-pending pending-signatures tests-pending labels Sep 30, 2022

makortel commented Sep 30, 2022

View reviewed changes

cmsbuild added code-checks-approved and removed code-checks-pending labels Sep 30, 2022

cmsbuild added tests-started and removed tests-pending labels Sep 30, 2022

makortel mentioned this pull request Sep 30, 2022

Make EventSetup PSet validation to work with ModuleTypeResolver cms-sw/framework-team#444

Closed

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

cmsbuild mentioned this pull request Oct 2, 2022

HGCAL: Change ToA to local time #39447

Closed

Dr15Jones reviewed Oct 3, 2022

View reviewed changes

FWCore/Framework/interface/ComponentMaker.h Outdated Show resolved Hide resolved

Dr15Jones reviewed Oct 3, 2022

View reviewed changes

FWCore/Framework/src/EventSetupProvider.cc Outdated Show resolved Hide resolved

makortel added 2 commits October 4, 2022 01:56

Return reference instead of pointer from EventSetupsController::getES…

7a302e0

…ProducerPSet()

makortel force-pushed the psetValidationComponentMaker branch from b17d5d8 to 7a302e0 Compare October 3, 2022 23:57

cmsbuild removed tests-rejected code-checks-approved labels Oct 3, 2022

cmsbuild added the code-checks-approved label Oct 4, 2022

cmsbuild added tests-started and removed tests-pending labels Oct 4, 2022

cmsbuild added tests-rejected and removed tests-started labels Oct 4, 2022

cmsbuild added tests-started and removed tests-rejected labels Oct 5, 2022

This was referenced Oct 5, 2022

Revert "Implement support for concurrent runs in the Framework" #39642

Merged

Bug fix, start Services in runOrQueueEventSetupForInstanceAsync #39644

Closed

HLT Tau DQM from MiniAOD #39647

Closed

cmsbuild added tests-approved and removed tests-started labels Oct 6, 2022

cmsbuild added core-approved fully-signed and removed core-pending pending-signatures labels Oct 7, 2022

cmsbuild mentioned this pull request Oct 7, 2022

Implement support for concurrent runs in the Framework #39673

Merged

cmsbuild added orp-approved and removed orp-pending labels Oct 9, 2022

cmsbuild merged commit 2a0f9ce into cms-sw:master Oct 9, 2022

makortel deleted the psetValidationComponentMaker branch October 10, 2022 14:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move EventSetup module PSet validation to be called from ComponentMaker #39556

Move EventSetup module PSet validation to be called from ComponentMaker #39556

makortel commented Sep 30, 2022

makortel left a comment

makortel Sep 30, 2022

Dr15Jones Oct 3, 2022

makortel Oct 3, 2022

cmsbuild commented Sep 30, 2022

cmsbuild commented Sep 30, 2022

makortel commented Sep 30, 2022

wddgit commented Sep 30, 2022

cmsbuild commented Oct 1, 2022

Dr15Jones commented Oct 3, 2022

cmsbuild commented Oct 4, 2022

makortel commented Oct 4, 2022

cmsbuild commented Oct 4, 2022

makortel commented Oct 5, 2022

aandvalenzuela commented Oct 6, 2022

cmsbuild commented Oct 6, 2022

makortel commented Oct 7, 2022

cmsbuild commented Oct 7, 2022

perrotta commented Oct 9, 2022

Move EventSetup module PSet validation to be called from ComponentMaker #39556

Move EventSetup module PSet validation to be called from ComponentMaker #39556

Conversation

makortel commented Sep 30, 2022

PR description:

PR validation:

makortel left a comment

Choose a reason for hiding this comment

makortel Sep 30, 2022

Choose a reason for hiding this comment

Dr15Jones Oct 3, 2022

Choose a reason for hiding this comment

makortel Oct 3, 2022

Choose a reason for hiding this comment

cmsbuild commented Sep 30, 2022

cmsbuild commented Sep 30, 2022

makortel commented Sep 30, 2022

wddgit commented Sep 30, 2022

cmsbuild commented Oct 1, 2022

Unit Tests

Comparison Summary

Dr15Jones commented Oct 3, 2022

cmsbuild commented Oct 4, 2022

makortel commented Oct 4, 2022

cmsbuild commented Oct 4, 2022

Unit Tests

RelVals-INPUT

Comparison Summary

makortel commented Oct 5, 2022

aandvalenzuela commented Oct 6, 2022

cmsbuild commented Oct 6, 2022

Comparison Summary

makortel commented Oct 7, 2022

cmsbuild commented Oct 7, 2022

perrotta commented Oct 9, 2022