Removed use of edm::EDFilter

[Dr15Jones]

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]

-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): #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]

+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): #39557

[cmsbuild]

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]

@cmsbuild, please test

[makortel]

hold

This is more for 13_0_X.

[cmsbuild]

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]

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]

-1

Failed Tests: HeaderConsistency Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-4364ad/28200/summary.html COMMIT: c529675433a05c2ee8ea9ab22d3b378b5959a7b7 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:

• @wddgit cms-sw/cmssw#39700
• @cecilecaillol cms-sw/cmssw#39704

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

[cmsbuild]

+code-checks

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

• This PR adds an extra 28KB to repository

[cmsbuild]

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

[Dr15Jones]

please test with #39750

[cmsbuild]

-1

Failed Tests: HeaderConsistency Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-4364ad/28368/summary.html COMMIT: 8acff5386bc7a3d371acc8f22517fa4b5b45b1fc CMSSW: CMSSW_12_6_X_2022-10-19-1100/el8_amd64_gcc10 User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/39710/28368/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: 8 differences found in the comparisons
• Reco comparison had 6 failed jobs
• DQMHistoTests: Total files compared: 48
• DQMHistoTests: Total histograms compared: 3391158
• DQMHistoTests: Total failures: 6
• DQMHistoTests: Total nulls: 0
• DQMHistoTests: Total successes: 3391130
• DQMHistoTests: Total skipped: 22
• DQMHistoTests: Total Missing objects: 0
• DQMHistoSizes: Histogram memory added: 0.0 KiB( 47 files compared)
• Checked 201 log files, 48 edm output root files, 48 DQM output files
• TriggerResults: no differences found

[makortel]

unhold

[makortel]

@Dr15Jones The IBs should be clean from legacy EDAnalyzers as well starting from CMSSW_13_0_X_2023-01-08-0000. Could you (rebase and) update this PR to remove edm::EDAnalyzer as well?

[Dr15Jones]

please test

[makortel]

@Dr15Jones The IBs should be clean from legacy EDAnalyzers as well starting from CMSSW_13_0_X_2023-01-08-0000. Could you (rebase and) update this PR to remove edm::EDAnalyzer as well?

Just for the record, we just agreed that if this PR's tests succeed, we'll go with this PR as it is, and open a new PR for the legacy EDAnalyzers

[cmsbuild]

-1

Failed Tests: HeaderConsistency Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-4364ad/29859/summary.html COMMIT: 8acff5386bc7a3d371acc8f22517fa4b5b45b1fc CMSSW: CMSSW_13_0_X_2023-01-09-1100/el8_amd64_gcc11 User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/39710/29859/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: 11 differences found in the comparisons
• DQMHistoTests: Total files compared: 49
• DQMHistoTests: Total histograms compared: 3555538
• DQMHistoTests: Total failures: 9
• DQMHistoTests: Total nulls: 0
• DQMHistoTests: Total successes: 3555507
• DQMHistoTests: Total skipped: 22
• DQMHistoTests: Total Missing objects: 0
• DQMHistoSizes: Histogram memory added: 0.0 KiB( 48 files compared)
• Checked 211 log files, 162 edm output root files, 49 DQM output files
• TriggerResults: no differences found

[Dr15Jones]

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]

+1

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

[cmsbuild]

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]

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]

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]

@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]

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]

@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]

@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]

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]

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]

@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]

https://github.com/cms-sw/cmsdist/pull/8242 should allow to ignore selected files for header consistency checks