Removed use of edm::EDFilter & edm::EDProducer #39710

Dr15Jones · 2022-10-12T14:57:36Z

PR description:

This finished the migration away from the deprecated class.

PR validation:

Doing a 'git grep --cached interface/EDFilter.h' turns up no hits.
The core framework packages compile.

cmsbuild · 2022-10-12T15:05:21Z

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-39710/32542

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/WorkerT.cc modified in PR(s): Added transformAsync ability to modules #39557

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

cmsbuild · 2022-10-12T15:13:17Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-39710/32543

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/WorkerT.cc modified in PR(s): Added transformAsync ability to modules #39557

cmsbuild · 2022-10-12T15:13:44Z

A new Pull Request was created by @Dr15Jones (Chris Jones) 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.
@makortel, @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-10-12T15:35:18Z

@cmsbuild, please test

makortel · 2022-10-12T15:37:29Z

hold

This is more for 13_0_X.

cmsbuild · 2022-10-12T15:37:49Z

Pull request has been put on hold by @makortel
They need to issue an unhold command to remove the hold state or L1 can unhold it for all

Dr15Jones · 2022-10-12T21:20:49Z

The 'header inconsistency' error is because the test is loading FWCore/Framework/interface/EDFilter.h and sees the #error. So this is actually a success, not a failure.

cmsbuild · 2022-10-12T22:23:57Z

-1

Failed Tests: HeaderConsistency
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-4364ad/28200/summary.html
COMMIT: c529675
CMSSW: CMSSW_12_6_X_2022-10-12-1100/el8_amd64_gcc10
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/39710/28200/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-4364ad/28200/git-recent-commits.json
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-4364ad/28200/git-merge-result

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-4364ad/20834.0_TTbar_14TeV+2026D88+TTbar_14TeV_TuneCP5_GenSimHLBeamSpot14+DigiTrigger+RecoGlobal+HARVESTGlobal
/data/cmsbld/jenkins/workspace/compare-root-files-short-matrix/data/PR-4364ad/20834.75_TTbar_14TeV+2026D88_HLT75e33+TTbar_14TeV_TuneCP5_GenSimHLBeamSpot14+DigiTrigger+RecoGlobal+HLT75e33+HARVESTGlobal
/data/cmsbld/jenkins/workspace/compare-root-files-short-matrix/data/PR-4364ad/20896.0_CloseByPGun_CE_E_Front_120um+2026D88+CE_E_Front_120um_GenSimHLBeamSpotHGCALCloseBy+DigiTrigger+RecoGlobal+HARVESTGlobal
/data/cmsbld/jenkins/workspace/compare-root-files-short-matrix/data/PR-4364ad/20900.0_CloseByPGun_CE_H_Coarse_Scint+2026D88+CE_H_Coarse_Scint_GenSimHLBeamSpotHGCALCloseBy+DigiTrigger+RecoGlobal+HARVESTGlobal
/data/cmsbld/jenkins/workspace/compare-root-files-short-matrix/data/PR-4364ad/21034.999_TTbar_14TeV+2026D88PU_PMXS1S2PR+TTbar_14TeV_TuneCP5_GenSimHLBeamSpot14+PREMIX_PremixHLBeamSpot14PU+DigiTriggerPU+RecoGlobalPU+HARVESTGlobalPU
/data/cmsbld/jenkins/workspace/compare-root-files-short-matrix/data/PR-4364ad/23234.0_TTbar_14TeV+2026D94+TTbar_14TeV_TuneCP5_GenSimHLBeamSpot14+DigiTrigger+RecoGlobal+HARVESTGlobal

Summary:

No significant changes to the logs found
Reco comparison results: 8 differences found in the comparisons
DQMHistoTests: Total files compared: 49
DQMHistoTests: Total histograms compared: 3392309
DQMHistoTests: Total failures: 6
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3392281
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

This finished the migration away from the deprecated class.

Dr15Jones · 2023-01-10T14:23:02Z

The 'header inconsistency' is a false alarm. It comes from the #error in the deprecated headers

src/FWCore/Framework/interface/EDFilter.h:4:2: error: #error "The use of edm::EDFilter has been removed. Please stop use of this header and switch module to a thread-friendly type."
    4 | #error \
      |  ^~~~~
gmake: *** [config/SCRAM/GMake/Makefile.rules:1722: tmp/el8_amd64_gcc11/src/FWCore/Framework/src/FWCoreFramework/EDFilter.h.chk_header] Error 1

src/FWCore/Framework/interface/EDProducer.h:4:2: error: #error "The use of edm::EDProducer has been removed. Please stop use of this header and switch module to a thread-friendly type."
    4 | #error \
      |  ^~~~~

makortel · 2023-01-10T14:56:15Z

FWCore/ParameterSet/interface/ParameterSetDescriptionFillerBase.h

@@ -106,7 +105,6 @@ namespace edm {

    static const std::string& extendedBaseType(EDAnalyzer const*) { return kExtendedBaseForEDAnalyzer; }
    static const std::string& extendedBaseType(EDProducer const*) { return kExtendedBaseForEDProducer; }


I think this PR should have removed the EDProducer overload as well. On the other hand, I suspect #40466 could already be in (mild, touching adjacent lines) conflict with this PR, so maybe it is easier to leave the cleanup to #40466 or later PR.

makortel · 2023-01-10T16:57:10Z

+1

Header consistency check failure is intentional, and further cleanup will be done in later PR(s).

cmsbuild · 2023-01-10T16:57:31Z

This pull request is fully signed and it will be integrated in one of the next master IBs (but tests are reportedly failing). 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 · 2023-01-10T17:16:40Z

The 'header inconsistency' is a false alarm. It comes from the #error in the deprecated headers

@Dr15Jones , does it mean that we must live with it in the IBs?

makortel · 2023-01-10T17:21:50Z

The 'header inconsistency' is a false alarm. It comes from the #error in the deprecated headers

does it mean that we must live with it in the IBs?

@smuzaffar Would it be easy to explicitly ignore the header consistency check for specific headers?

An alternative would be to remove the headers, whose main consequence is that a user forward-porting their (legacy) code would get a (much) less clear error message from the compiler.

smuzaffar · 2023-01-10T17:29:50Z

@makortel , the failed header are only used by [a]. So why not just move these header in to FWCore/Framework/src ?

[a]

FWCore/Framework/interface/EDProducer.h: FWCore/Framework/src/EDProducer.cc FWCore/Framework/src/WorkerT.cc 
FWCore/Framework/interface/EDFilter.h: FWCore/Framework/src/EDFilter.cc FWCore/Framework/src/WorkerT.cc

smuzaffar · 2023-01-10T17:33:01Z

ah its forward porting code .... for those they are going to get an error any way so I would suggest to just delete these headers

Dr15Jones · 2023-01-10T17:34:03Z

@smuzaffar

the failed header are only used by [a]. So why not just move these header in to FWCore/Framework/src ?

The point isn't that CMSSW depends on those headers (it shouldn't any more) it is that some people who have their own copy repositories of CMSSW still depend on that header and if they try to rebase their code onto a more recent CMSSW release we'd like them to get a better error message (than just 'header file missing').

makortel · 2023-01-10T17:34:31Z

@smuzaffar This PR removes those #includes, i.e. with this PR nothing in CMSSW #includes the FWCore/Framework/interface/EDProducer.h and FWCore/Framework/interface/EDFilter.h. But quite likely there exists user code that (despite of the compilation warnings) in 12_[46]_X is kept using the legacy EDModule base classes, that will fail to compile with 13_0_X and later. Chris made an attempt to be nice for such cases and provide an explanatory error message.

smuzaffar · 2023-01-10T17:58:24Z

Currently all files under package/interface directory are checked for header consistency and there is no way to ignore some selected file. I will add support for it e.g. a flag like

<flags IGNORE_HEADER_CHECK="header files"/>

in package/BuildFile.xml should instruct to not run header check on these files. Does this sound reasonable?

perrotta · 2023-01-10T18:01:08Z

Thank you all.
I think that an explanatory message, as the one designed by Chris, is welcome because of the user code that may still depend on them, as already pointed out.
My (naive) question was simply: will the IB's get broken by this PR, or will we get filled by several of those header consistency errors that can hide real ones?
If that is not the case, I think the PR can go even as it is.

smuzaffar · 2023-01-10T21:44:06Z

@perrotta , IB itself should not be broken (no one in cmssw master is using these headers). Only the IB header consistency check will show errors about these files. I am preparing a cmssw-config PR to ignore header checks for these.

smuzaffar · 2023-01-10T22:02:09Z

cms-sw/cmsdist#8242 should allow to ignore selected files for header consistency checks

perrotta · 2023-01-10T22:26:40Z

+1

perrotta · 2023-01-10T22:27:37Z

merge

smuzaffar · 2023-01-11T13:39:45Z

#40474 should fix the header consistency test

perrotta · 2023-01-11T13:45:45Z

#40474 should fix the header consistency test

Thank you @smuzaffar !

cmsbuild added this to the CMSSW_12_6_X milestone Oct 12, 2022

cmsbuild added code-checks-pending core-pending orp-pending pending-signatures tests-pending labels Oct 12, 2022

cmsbuild added code-checks-rejected and removed code-checks-pending labels Oct 12, 2022

Dr15Jones force-pushed the removeEDFilter branch from 23f751f to c529675 Compare October 12, 2022 15:06

cmsbuild added code-checks-pending and removed code-checks-rejected labels Oct 12, 2022

cmsbuild added code-checks-approved and removed code-checks-pending labels Oct 12, 2022

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

cmsbuild added the hold label Oct 12, 2022

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

cmsbuild mentioned this pull request Oct 13, 2022

Added transformAsync ability to modules #39557

Merged

Dr15Jones added 2 commits October 19, 2022 09:03

Removed use of edm::EDFilter

39fa6e2

This finished the migration away from the deprecated class.

Removed use of edm::EDProducer

8acff53

This finished the migration away from the deprecated class.

Dr15Jones force-pushed the removeEDFilter branch from c529675 to 8acff53 Compare October 19, 2022 14:25

cmsbuild added the tests-rejected label Jan 10, 2023

cmsbuild mentioned this pull request Jan 10, 2023

Removed legacy EDAnalyzer interface #40466

Merged

makortel reviewed Jan 10, 2023

View reviewed changes

cmsbuild added core-approved fully-signed and removed core-pending pending-signatures labels Jan 10, 2023

smuzaffar mentioned this pull request Jan 10, 2023

[BuildRules] Added support to skip header consistency checks for selected files cms-sw/cmsdist#8242

Merged

cmsbuild added orp-approved and removed orp-pending labels Jan 10, 2023

cmsbuild merged commit 806c56e into cms-sw:master Jan 10, 2023

makortel mentioned this pull request Jan 12, 2023

Migrate legacy EDModules to thread-efficient alternatives #36404

Closed

Dr15Jones deleted the removeEDFilter branch January 12, 2023 15:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Removed use of edm::EDFilter & edm::EDProducer #39710

Removed use of edm::EDFilter & edm::EDProducer #39710

Dr15Jones commented Oct 12, 2022

cmsbuild commented Oct 12, 2022

cmsbuild commented Oct 12, 2022

cmsbuild commented Oct 12, 2022

makortel commented Oct 12, 2022

makortel commented Oct 12, 2022

cmsbuild commented Oct 12, 2022

Dr15Jones commented Oct 12, 2022

cmsbuild commented Oct 12, 2022

Dr15Jones commented Jan 10, 2023

makortel Jan 10, 2023

makortel commented Jan 10, 2023

cmsbuild commented Jan 10, 2023

perrotta commented Jan 10, 2023

makortel commented Jan 10, 2023

smuzaffar commented Jan 10, 2023

smuzaffar commented Jan 10, 2023

Dr15Jones commented Jan 10, 2023

makortel commented Jan 10, 2023

smuzaffar commented Jan 10, 2023

perrotta commented Jan 10, 2023

smuzaffar commented Jan 10, 2023

smuzaffar commented Jan 10, 2023

perrotta commented Jan 10, 2023

perrotta commented Jan 10, 2023

smuzaffar commented Jan 11, 2023

perrotta commented Jan 11, 2023

		@@ -106,7 +105,6 @@ namespace edm {

		static const std::string& extendedBaseType(EDAnalyzer const*) { return kExtendedBaseForEDAnalyzer; }
		static const std::string& extendedBaseType(EDProducer const*) { return kExtendedBaseForEDProducer; }

Removed use of edm::EDFilter & edm::EDProducer #39710

Removed use of edm::EDFilter & edm::EDProducer #39710

Conversation

Dr15Jones commented Oct 12, 2022

PR description:

PR validation:

cmsbuild commented Oct 12, 2022

cmsbuild commented Oct 12, 2022

cmsbuild commented Oct 12, 2022

makortel commented Oct 12, 2022

makortel commented Oct 12, 2022

cmsbuild commented Oct 12, 2022

Dr15Jones commented Oct 12, 2022

cmsbuild commented Oct 12, 2022

Comparison Summary

Dr15Jones commented Jan 10, 2023

makortel Jan 10, 2023

Choose a reason for hiding this comment

makortel commented Jan 10, 2023

cmsbuild commented Jan 10, 2023

perrotta commented Jan 10, 2023

makortel commented Jan 10, 2023

smuzaffar commented Jan 10, 2023

smuzaffar commented Jan 10, 2023

Dr15Jones commented Jan 10, 2023

makortel commented Jan 10, 2023

smuzaffar commented Jan 10, 2023

perrotta commented Jan 10, 2023

smuzaffar commented Jan 10, 2023

smuzaffar commented Jan 10, 2023

perrotta commented Jan 10, 2023

perrotta commented Jan 10, 2023

smuzaffar commented Jan 11, 2023

perrotta commented Jan 11, 2023