Refactor PathsAndConsumesOfModules #47937

Dr15Jones · 2025-04-23T19:32:12Z

PR description:

moved complex logic for creating the internal data structures to the class itself
can now ask an ESProducer what is produces
provide a mechanism to get minimal information about what ES products are being consumed

PR validation:

Framework unit tests pass.

- moved complex logic for creating the internal data structures to the class itself - can no ask an ESProducer what is produces - provide a mechanism to get minimal information about what ES products are being consumed

cmsbuild · 2025-04-23T19:32:40Z

cms-bot internal usage

cmsbuild · 2025-04-23T19:33:57Z

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-47937/44595

ERROR: Build errors found during clang-tidy run.

  350 |                                module,
--
src/FWCore/Framework/interface/EDConsumerBase.h:349:13: error: invalid use of incomplete type 'ModuleConsumesInfo' [clang-diagnostic-error]
  349 |       iFunc(ModuleConsumesInfo(itInfo->m_type,
      |             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  350 |                                module,
--
src/FWCore/Framework/interface/EDConsumerBase.h:349:13: error: invalid use of incomplete type 'ModuleConsumesInfo' [clang-diagnostic-error]
  349 |       iFunc(ModuleConsumesInfo(itInfo->m_type,
      |             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  350 |                                module,
--
src/FWCore/Framework/interface/EDConsumerBase.h:349:13: error: invalid use of incomplete type 'ModuleConsumesInfo' [clang-diagnostic-error]
  349 |       iFunc(ModuleConsumesInfo(itInfo->m_type,
      |             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  350 |                                module,
--
src/FWCore/Framework/interface/EDConsumerBase.h:349:13: error: invalid use of incomplete type 'ModuleConsumesInfo' [clang-diagnostic-error]
  349 |       iFunc(ModuleConsumesInfo(itInfo->m_type,
      |             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  350 |                                module,
--
gmake: *** [config/SCRAM/GMake/Makefile.coderules:129: code-checks] Error 2
gmake: *** [There are compilation/build errors. Please see the detail log above.] Error 2

cmsbuild · 2025-04-23T19:49:08Z

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-47937/44596

There are other open Pull requests which might conflict with changes you have proposed:
- File CondCore/CondHDF5ESSource/plugins/CondHDF5ESSource.cc modified in PR(s): [RFC] descriptions.addDefault -> descriptions.addWithDefaultLabel #44372
- File FWCore/Integration/plugins/ESTestProducers.cc modified in PR(s): [RFC] descriptions.addDefault -> descriptions.addWithDefaultLabel #44372
- File FWCore/Integration/plugins/TestESConcurrentSource.cc modified in PR(s): [RFC] descriptions.addDefault -> descriptions.addWithDefaultLabel #44372
- File FWCore/Integration/plugins/TestESSource.cc modified in PR(s): [RFC] descriptions.addDefault -> descriptions.addWithDefaultLabel #44372

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

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

cmsbuild · 2025-04-23T20:03:06Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-47937/44597

There are other open Pull requests which might conflict with changes you have proposed:
- File CondCore/CondHDF5ESSource/plugins/CondHDF5ESSource.cc modified in PR(s): [RFC] descriptions.addDefault -> descriptions.addWithDefaultLabel #44372
- File FWCore/Integration/plugins/ESTestProducers.cc modified in PR(s): [RFC] descriptions.addDefault -> descriptions.addWithDefaultLabel #44372
- File FWCore/Integration/plugins/TestESConcurrentSource.cc modified in PR(s): [RFC] descriptions.addDefault -> descriptions.addWithDefaultLabel #44372
- File FWCore/Integration/plugins/TestESSource.cc modified in PR(s): [RFC] descriptions.addDefault -> descriptions.addWithDefaultLabel #44372

cmsbuild · 2025-04-23T20:03:33Z

A new Pull Request was created by @Dr15Jones for master.

It involves the following packages:

CondCore/CondHDF5ESSource (db)
CondCore/ESSources (db, alca)
FWCore/Framework (core)
FWCore/Integration (core)
FWCore/Services (core)
FWCore/TestProcessor (core)
PhysicsTools/CondLiteIO (analysis)

@Dr15Jones, @atpathak, @cmsbuild, @consuegs, @francescobrivio, @makortel, @perrotta, @smuzaffar, @tvami can you please review it and eventually sign? Thanks.
@JanChyczynski, @PonIlya, @fwyzard, @makortel, @missirol, @mmusich, @rsreds, @tocheng, @wddgit, @yuanchao this is something you requested to watch as well.
@antoniovilela, @mandrenguyen, @rappoccio, @sextonkennedy you are the release manager for this.

cms-bot commands are listed here

Dr15Jones · 2025-04-23T20:04:08Z

please test

Dr15Jones · 2025-04-23T20:07:08Z

Additional changes to PathsAndConsumesOfModules , probably for another PR

delay constructing many of the data structures until they are requested
further break the long functions into smaller ones, especially if the logic can be shared

makortel · 2025-04-23T20:12:32Z

CondCore/CondHDF5ESSource/plugins/CondHDF5ESSource.cc

@@ -236,4 +238,20 @@ CondHDF5ESSource::KeyedResolversVector CondHDF5ESSource::registerResolvers(Event
  return returnValue;
 }

+std::vector<edm::eventsetup::ESModuleProducesInfo> CondHDF5ESSource::producesInfo() const {
+  std::vector<edm::eventsetup::ESModuleProducesInfo> returnValue;


Would returnValue.reserve() be worth it? E.g. with records_.size()? (unlikely to be exact, but maybe would avoid a number of small reallocations)

makortel · 2025-04-23T20:13:04Z

CondCore/ESSources/plugins/CondDBESSource.cc

@@ -648,6 +649,22 @@ edm::eventsetup::ESProductResolverProvider::KeyedResolversVector CondDBESSource:
  return keyedResolversVector;
 }

+std::vector<edm::eventsetup::ESModuleProducesInfo> CondDBESSource::producesInfo() const {
+  std::vector<edm::eventsetup::ESModuleProducesInfo> returnValue;


How about

returnValue.reserve(m_resolvers.size());

?

makortel · 2025-04-23T20:22:05Z

FWCore/Framework/interface/EDConsumerBase.h

+    ///This can only be called before the end of beginJob (after that the underlying data has been deleted)
+    std::vector<ModuleConsumesMinimalESInfo> moduleConsumesMinimalESInfos() const;


A comment here if the returned ModuleConsumesMinimalESInfo can be used beyond the end of beginJob would be helpful (given that ModuleConsumesMinimalESInfo holds string_view).

makortel · 2025-04-23T20:25:36Z

FWCore/Framework/src/PathsAndConsumesOfModules.cc

+          assert(*consumedModuleLabel.data() !=
+                 '\0');  // consumesMany used to create empty labels before we removed consumesMany


Suggested change

assert(*consumedModuleLabel.data() !=

'\0'); // consumesMany used to create empty labels before we removed consumesMany

// consumesMany used to create empty labels before we removed consumesMany

assert(*consumedModuleLabel.data() != '\0');

would look better

makortel · 2025-04-23T20:26:48Z

FWCore/Framework/src/PathsAndConsumesOfModules.cc

-      eventsetup::EventSetupProvider const& eventSetupProvider) {
-    esRecordsToProductResolverIndices_ = std::move(esRecordsToProductResolverIndices);
-    schedule_->fillESModuleAndConsumesInfo(esModulesWhoseProductsAreConsumedBy_, esRecordsToProductResolverIndices_);
+  namespace {}  // namespace


This can probably be removed?

Suggested change

namespace {} // namespace

cmsbuild · 2025-04-23T21:52:34Z

-1

Failed Tests: ClangBuild
Size: This PR adds an extra 184KB to repository
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-40a674/45690/summary.html
COMMIT: f1d9dc9
CMSSW: CMSSW_15_1_X_2025-04-23-1100/el8_amd64_gcc12
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/47937/45690/install.sh to create a dev area with all the needed externals and cmssw changes.

The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic:

You can see more details here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-40a674/45690/git-recent-commits.json
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-40a674/45690/git-merge-result

Clang Build

I found compilation warning while trying to compile with clang. Command used:

USER_CUDA_FLAGS='--expt-relaxed-constexpr' USER_CXXFLAGS='-Wno-register -fsyntax-only' /usr/bin/time -v scram build -k -j 32 COMPILER='llvm compile'

See details on the summary page.

Dr15Jones · 2025-04-24T14:02:23Z

Thinking more about this change, I'm not certain it would properly handle the case where multiple ES modules want to deliver the same data product.

- The EventSetupProvider structures already handle such cases.

cmsbuild · 2025-04-24T19:05:32Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-47937/44608

There are other open Pull requests which might conflict with changes you have proposed:
- File CondCore/CondHDF5ESSource/plugins/CondHDF5ESSource.cc modified in PR(s): [RFC] descriptions.addDefault -> descriptions.addWithDefaultLabel #44372
- File FWCore/Integration/plugins/ESTestProducers.cc modified in PR(s): [RFC] descriptions.addDefault -> descriptions.addWithDefaultLabel #44372
- File FWCore/Integration/plugins/TestESConcurrentSource.cc modified in PR(s): [RFC] descriptions.addDefault -> descriptions.addWithDefaultLabel #44372
- File FWCore/Integration/plugins/TestESSource.cc modified in PR(s): [RFC] descriptions.addDefault -> descriptions.addWithDefaultLabel #44372

cmsbuild · 2025-04-24T19:05:57Z

Pull request #47937 was updated. @Dr15Jones, @atpathak, @cmsbuild, @consuegs, @francescobrivio, @makortel, @perrotta, @smuzaffar, @tvami can you please check and sign again.

Dr15Jones · 2025-04-24T19:41:27Z

please test

cmsbuild · 2025-04-24T19:43:15Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-47937/44609

There are other open Pull requests which might conflict with changes you have proposed:
- File CondCore/CondHDF5ESSource/plugins/CondHDF5ESSource.cc modified in PR(s): [RFC] descriptions.addDefault -> descriptions.addWithDefaultLabel #44372
- File FWCore/Integration/plugins/ESTestProducers.cc modified in PR(s): [RFC] descriptions.addDefault -> descriptions.addWithDefaultLabel #44372
- File FWCore/Integration/plugins/TestESConcurrentSource.cc modified in PR(s): [RFC] descriptions.addDefault -> descriptions.addWithDefaultLabel #44372
- File FWCore/Integration/plugins/TestESSource.cc modified in PR(s): [RFC] descriptions.addDefault -> descriptions.addWithDefaultLabel #44372

cmsbuild · 2025-04-24T19:43:43Z

Pull request #47937 was updated. @Dr15Jones, @atpathak, @consuegs, @francescobrivio, @makortel, @perrotta, @smuzaffar, @tvami can you please check and sign again.

cmsbuild · 2025-04-24T23:31:42Z

+1

Size: This PR adds an extra 100KB to repository
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-40a674/45707/summary.html
COMMIT: fd2c767
CMSSW: CMSSW_15_1_X_2025-04-24-1100/el8_amd64_gcc12
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/47937/45707/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

You potentially added 2 lines to the logs
Reco comparison results: 8 differences found in the comparisons
DQMHistoTests: Total files compared: 50
DQMHistoTests: Total histograms compared: 3913297
DQMHistoTests: Total failures: 17
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3913260
DQMHistoTests: Total skipped: 20
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 49 files compared)
Checked 215 log files, 184 edm output root files, 50 DQM output files
TriggerResults: no differences found

Dr15Jones added 2 commits April 23, 2025 14:29

Refactor consumes info for PathsAndConsumesOfModules

d834701

- moved complex logic for creating the internal data structures to the class itself - can no ask an ESProducer what is produces - provide a mechanism to get minimal information about what ES products are being consumed

Updated to new ProductResolverProvider interface

bcf17d7

cmsbuild added this to the CMSSW_15_1_X milestone Apr 23, 2025

cmsbuild added core-pending analysis-pending alca-pending db-pending pending-signatures tests-pending orp-pending code-checks-pending labels Apr 23, 2025

cmsbuild added code-checks-rejected code-checks-pending and removed code-checks-pending code-checks-rejected labels Apr 23, 2025

cmsbuild added code-checks-rejected and removed code-checks-pending labels Apr 23, 2025

clang-tidy changes

f1d9dc9

Dr15Jones force-pushed the refactorPathsAndConsumesOfModules branch from 632df35 to f1d9dc9 Compare April 23, 2025 20:00

cmsbuild added code-checks-pending and removed code-checks-rejected labels Apr 23, 2025

cmsbuild added code-checks-approved and removed code-checks-pending labels Apr 23, 2025

cmsbuild removed the tests-pending label Apr 23, 2025

makortel reviewed Apr 23, 2025

View reviewed changes

cmsbuild added tests-rejected and removed tests-started labels Apr 23, 2025

Dr15Jones added 3 commits April 24, 2025 13:56

fix clang compilation issues

cbba1ae

Return registered DataKeys by const ref

c63aa5b

Handle case where DataKey can come from many ES modules

f488fec

- The EventSetupProvider structures already handle such cases.

cmsbuild added tests-pending code-checks-pending and removed tests-rejected code-checks-approved labels Apr 24, 2025

cmsbuild added code-checks-approved and removed code-checks-pending labels Apr 24, 2025

Improvements based on review feedback

fd2c767

cmsbuild added code-checks-pending and removed code-checks-approved labels Apr 24, 2025

cmsbuild added tests-started and removed tests-pending labels Apr 24, 2025

cmsbuild added code-checks-approved and removed code-checks-pending labels Apr 24, 2025

cmsbuild added tests-approved and removed tests-started labels Apr 24, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor PathsAndConsumesOfModules #47937

Refactor PathsAndConsumesOfModules #47937

Dr15Jones commented Apr 23, 2025 •

edited

Loading

cmsbuild commented Apr 23, 2025 •

edited

Loading

cmsbuild commented Apr 23, 2025

cmsbuild commented Apr 23, 2025

cmsbuild commented Apr 23, 2025

cmsbuild commented Apr 23, 2025

Dr15Jones commented Apr 23, 2025

Dr15Jones commented Apr 23, 2025

makortel Apr 23, 2025

makortel Apr 23, 2025

makortel Apr 23, 2025

makortel Apr 23, 2025

makortel Apr 23, 2025

cmsbuild commented Apr 23, 2025

Dr15Jones commented Apr 24, 2025

cmsbuild commented Apr 24, 2025

cmsbuild commented Apr 24, 2025

Dr15Jones commented Apr 24, 2025

cmsbuild commented Apr 24, 2025

cmsbuild commented Apr 24, 2025

cmsbuild commented Apr 24, 2025

		///This can only be called before the end of beginJob (after that the underlying data has been deleted)
		std::vector<ModuleConsumesMinimalESInfo> moduleConsumesMinimalESInfos() const;

		assert(*consumedModuleLabel.data() !=
		'\0'); // consumesMany used to create empty labels before we removed consumesMany

Refactor PathsAndConsumesOfModules #47937

Are you sure you want to change the base?

Refactor PathsAndConsumesOfModules #47937

Conversation

Dr15Jones commented Apr 23, 2025 • edited Loading

PR description:

PR validation:

cmsbuild commented Apr 23, 2025 • edited Loading

cmsbuild commented Apr 23, 2025

cmsbuild commented Apr 23, 2025

cmsbuild commented Apr 23, 2025

cmsbuild commented Apr 23, 2025

Dr15Jones commented Apr 23, 2025

Dr15Jones commented Apr 23, 2025

makortel Apr 23, 2025

Choose a reason for hiding this comment

makortel Apr 23, 2025

Choose a reason for hiding this comment

makortel Apr 23, 2025

Choose a reason for hiding this comment

makortel Apr 23, 2025

Choose a reason for hiding this comment

makortel Apr 23, 2025

Choose a reason for hiding this comment

cmsbuild commented Apr 23, 2025

Clang Build

Dr15Jones commented Apr 24, 2025

cmsbuild commented Apr 24, 2025

cmsbuild commented Apr 24, 2025

Dr15Jones commented Apr 24, 2025

cmsbuild commented Apr 24, 2025

cmsbuild commented Apr 24, 2025

cmsbuild commented Apr 24, 2025

Comparison Summary

Dr15Jones commented Apr 23, 2025 •

edited

Loading

cmsbuild commented Apr 23, 2025 •

edited

Loading