Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove deprecated JetCorrector and related files #39953

Merged
merged 2 commits into from
Dec 12, 2022

Conversation

wddgit
Copy link
Contributor

@wddgit wddgit commented Nov 1, 2022

PR description:

Remove the deprecated ::JetCorrector class from CMSSW and other related files with it. The migration from ::JetCorrector to reco::JetCorrector started in 2014 and it is time to finally complete it. For the most part this PR removes unused dead code and simplifies things. It makes it impossible for someone to use the deprecated code and obvious failures will occur if they try. It reduces the amount of code we need to maintain and support.

Almost this entire PR consists of deletions. There is one exception to that. The exception is in RecoHI/HiJetAlgos/python/hiCaloJetsForTrk_cff.py. I migrated that file from using the old JetCorrector type to using the new reco::JetCorrector. Somehow that one was missed earlier.

There has been a long effort to migrate from the old JetCorrectors to the new JetCorrectors that has been in progress since 2014. Volker Adler did most of this migration long ago. I've recently migrated all the remaining C++ code. I've also migrated all the Python configuration code that is used in existing tests that I can find. And I will help if merging this PR reveals more cases of code in CMSSW that needs migration. There are 4 PRs that are submitted but not merged yet. These need to be merged before this PR or there will be compilation errors. These are #39875 #39864 #39847 and #39804.

I also deleted the obsolete central cff configuration files commonly imported to use the old JetCorrector class.

There are dozens of other configuration files that still reference the old cff files deleted by this PR. Those will be broken by this PR, but they are all old and look to be old obsolete code that is already broken. It is not worth the effort to deal with them at this point. If someone tries to run them they will fail with an obvious exception and then they can migrated to use the new JetCorrector class. There is a good chance none of them will ever be used again.

PR validation:

Relies on existing tests. Errors caused by this should result in obvious exceptions.

The one exception is the change in hiCaloJetsForTrk_cff.py. That is tested by runTheMatrix.py test 140.53. That is how I noticed it still needed to be migrated.

If other failures show up, I will be available to help migrate the affected code.

@cmsbuild
Copy link
Contributor

cmsbuild commented Nov 1, 2022

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-39953/32883

  • This PR adds an extra 20KB to repository

@cmsbuild
Copy link
Contributor

cmsbuild commented Nov 1, 2022

A new Pull Request was created by @wddgit (W. David Dagenhart) for master.

It involves the following packages:

  • DQM/Physics (dqm)
  • JetMETCorrections/Algorithms (analysis)
  • JetMETCorrections/Configuration (reconstruction)
  • JetMETCorrections/Modules (reconstruction)
  • JetMETCorrections/Objects (reconstruction)
  • PhysicsTools/PatAlgos (xpog, reconstruction)
  • RecoHI/HiJetAlgos (reconstruction)

@swertz, @vlimant, @micsucmed, @emanueleusai, @ahmad3213, @cmsbuild, @jfernan2, @clacaputo, @syuvivida, @pmandrik, @mandrenguyen, @rvenditti can you please review it and eventually sign? Thanks.
@rappoccio, @gouskos, @yslai, @schoef, @emilbols, @mbluj, @demuller, @seemasharmafnal, @mmarionncern, @jdolen, @ahinzmann, @dgulhan, @missirol, @azotz, @mandrenguyen, @yetkinyilmaz, @jdamgov, @nhanvtran, @gkasieczka, @hatakeyamak, @mariadalfonso, @clelange, @AlexDeMoor, @AnnikaStein, @jazzitup, @JyothsnaKomaragiri, @yenjie, @kurtejung, @gpetruc, @andrzejnovak 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

@wddgit
Copy link
Contributor Author

wddgit commented Nov 1, 2022

please test with #39875,#39864,#39847,#39804

FYI @Dr15Jones @makortel

inline bool JetCorrector::vectorialCorrection() const { return false; }

#endif
#error, obsolete header, use reco::JetCorrector in JetMETCorrections/JetCorrector/interface/JetCorrector.h instead
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be

Suggested change
#error, obsolete header, use reco::JetCorrector in JetMETCorrections/JetCorrector/interface/JetCorrector.h instead
#error "obsolete header, use reco::JetCorrector in JetMETCorrections/JetCorrector/interface/JetCorrector.h instead"

?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will fix that as soon as the tests finish. Thanks. No idea why I put a comma there...

@cmsbuild
Copy link
Contributor

cmsbuild commented Nov 1, 2022

-1

Failed Tests: HeaderConsistency
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-c6983d/28715/summary.html
COMMIT: 2ad4017
CMSSW: CMSSW_12_6_X_2022-11-01-1100/el8_amd64_gcc10
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/39953/28715/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

There are some workflows for which there are errors in the baseline:
534.0 step 1
536.0 step 1
The results for the comparisons for these workflows could be incomplete
This means most likely that the IB is having errors in the relvals.The error does NOT come from this pull request

Summary:

  • No significant changes to the logs found
  • Reco comparison results: 10 differences found in the comparisons
  • DQMHistoTests: Total files compared: 48
  • DQMHistoTests: Total histograms compared: 3416398
  • DQMHistoTests: Total failures: 12
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3416364
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 47 files compared)
  • Checked 206 log files, 48 edm output root files, 48 DQM output files
  • TriggerResults: no differences found

@makortel
Copy link
Contributor

makortel commented Nov 2, 2022

-1

Failed Tests: HeaderConsistency

Note that the header consistency check failure is caused by the #error, i.e. it is expected.

@cmsbuild
Copy link
Contributor

cmsbuild commented Nov 2, 2022

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-39953/32889

  • This PR adds an extra 20KB to repository

@makortel
Copy link
Contributor

hold

(seems that holding needs more power)

@cmsbuild
Copy link
Contributor

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

@cmsbuild cmsbuild added the hold label Nov 28, 2022
@wddgit
Copy link
Contributor Author

wddgit commented Dec 5, 2022

please test

@makortel I think we can remove the hold. I'm rerunning the tests to see if they will pass with no other PR's included. I think it should now that all the other PRs were merged.

@makortel
Copy link
Contributor

makortel commented Dec 5, 2022

unhold

Thanks for reminding

@cmsbuild cmsbuild removed the hold label Dec 5, 2022
@cmsbuild
Copy link
Contributor

cmsbuild commented Dec 5, 2022

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-c6983d/29464/summary.html
COMMIT: 44852c6
CMSSW: CMSSW_13_0_X_2022-12-05-1100/el8_amd64_gcc11
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/39953/29464/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: 25 differences found in the comparisons
  • DQMHistoTests: Total files compared: 48
  • DQMHistoTests: Total histograms compared: 3421337
  • DQMHistoTests: Total failures: 1170
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3420145
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 47 files compared)
  • Checked 206 log files, 158 edm output root files, 48 DQM output files
  • TriggerResults: no differences found

@makortel
Copy link
Contributor

makortel commented Dec 5, 2022

The comparison differences are in 11634.7, so this looks like another symptom of #39803 (comment)

@rappoccio
Copy link
Contributor

+1

Comparison differences are spurious.

@perrotta
Copy link
Contributor

perrotta commented Dec 12, 2022

merge

@cmsbuild cmsbuild merged commit d2b40c9 into cms-sw:master Dec 12, 2022
@perrotta
Copy link
Contributor

@wddgit after the merge of this PR there are unit test errors in Configuration/DataProcessing, apparently related to the relics JetCorrectionServicesAllAlgos_cff.py which is still imported in

  • HeavyIonsAnalysis/Configuration/python/HI_DiJetSkim_cff.py
  • HeavyIonsAnalysis/Configuration/python/analysisFilters_cff.py

In reality I see many more places in CMSSW where that config is imported, see e.g. https://cmssdt.cern.ch/lxr/search?%21v=CMSSW_13_0_X_2022-12-12-1100&_filestring=&_string=rrectionServicesAllAlgos_cff

Could you please have a look?

@wddgit
Copy link
Contributor Author

wddgit commented Dec 12, 2022

I'll take a look this morning.

@wddgit
Copy link
Contributor Author

wddgit commented Dec 12, 2022

I missed HI_DiJetSkim_cff.py. I'll work on a PR migrating that to use the new JetCorrector class as soon as possible and submit a PR. Hopefully, I can do that today. It will be my top priority. If there are any others I need to migrate to get the unit tests in Configuration/DataProcessing to succeed, then I will take care of those also. Thanks for identifying this problem.

This PR deleted several central cff files. Many dozens of existing cff or cfg files import those and are also broken by this PR. Your search found some of them, but many more are broken and will show up if one searches for use of other cff files deleted by this PR. There is a paragraph about this in the comment at the top of the PR. My proposal for dealing with this was to migrate the ones needed in existing tests, but not to migrate the rest. Many of them are obsolete and broken already. In most cases, running a configuration actually trying to use the old JetCorrectors would have already resulted in an exception being thrown by the Framework. The Framework has been behaving that way for about a year.

If someone identifies specific cases of broken cff or cfg files that need migrating and that are still in use and they need help with this, then I am willing to help migrate them. I don't mind doing the migration itself, but it's hard for me to fix problems unrelated JetCorrectors or write new tests in domains where I don't have any experience.

@perrotta
Copy link
Contributor

Thank you @wddgit
Please try to fix at least the files which are tested in the IB, as we wouldn't like to have a broke pre-release to be built in the next few days. Otherwise we will revert this one, and move it to the next pre-release.
Then, if you can prepare a list of all those "dozens of other configuration files that still reference the old cff files deleted by this PR" we can make a github issue with it, and ask people either to remove them, if not needed any more, or fix them.

@wddgit
Copy link
Contributor Author

wddgit commented Dec 12, 2022

The PR fixing the unit test is #40301, recently submitted.

I also submitted a new Issue with the list of files importing the deleted cff files. Issue #40304. Note that none of these are used in tests. As far as I know they are not used at all.

@wddgit
Copy link
Contributor Author

wddgit commented Dec 13, 2022

Starting with the IB after this was merged, CMSSW_13_0_X_2022-12-12-1100, the Static Analyzer test in the IB named "SA EventSetupRecord::get called" is now reporting this:

"no-package: {}"

I presume this means it found no problems. Clearing that was the main goal of this PR.

Also after the #40301 was merged last night, the unit tests all passed.

@perrotta
Copy link
Contributor

Good. Thank you @wddgit , also for the github issue

@wddgit wddgit deleted the removeDeprecatedJetCorrector branch January 13, 2023 17:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants