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 depreciated DNN-based tau-Ids [12_4_X] #38164

Conversation

mbluj
Copy link
Contributor

@mbluj mbluj commented Jun 1, 2022

PR description:

This PR is to remove depreciated DNN-based tau-Ids: DPFIsolation (a dedicated module) and DeepTauID v1 (a v1-related part of the deepTauId module). These tau-Ids was superseded by deepTauID v2 and are not present in official workflows.

In addition two outdated and unused modules are also removed.

No changes are expected.

This PR is accompanied by another PR to cms-data removing model files of removed DNN-based tau-Ids (cms-data/RecoTauTag-TrainingFiles#9) or its updated version cms-data/RecoTauTag-TrainingFiles#10.

This is backport of #38063.

PR validation:

Limited matrix tests successful in original PR (#38063). Not repeated here as differences between master and 12_4_X are minimal (can be done on request).

if this PR is a backport please specify the original PR and why you need to backport that PR:

Backport of #38063.

Removal of outdated code and (more importantly) reduction of size of cms-distr thanks to removal of old model files when cms-data/RecoTauTag-TrainingFiles#9 merged. Finally it enables adding to cms-distr of 12_4_X new model files of newest deepTauID v2p5 requested in cms-data/RecoTauTag-TrainingFiles#10.

@cmsbuild
Copy link
Contributor

cmsbuild commented Jun 1, 2022

A new Pull Request was created by @mbluj for CMSSW_12_4_X.

It involves the following packages:

  • RecoTauTag/RecoTau (reconstruction)

@jpata, @cmsbuild, @clacaputo, @slava77 can you please review it and eventually sign? Thanks.
@mbluj, @azotz this is something you requested to watch as well.
@perrotta, @dpiparo, @qliphy you are the release manager for this.

cms-bot commands are listed here

@jpata
Copy link
Contributor

jpata commented Jun 1, 2022

Sorry if I missed it, but where was the backport requested and what's the motivation?

@mbluj
Copy link
Contributor Author

mbluj commented Jun 1, 2022

Sorry if I missed it, but where was the backport requested and what's the motivation?

The main motivation of the backport is to remove unused code and synchronize cms-data used by 12_4_X and master (12_5_X) which a) removes depreciated model files (size reduction of cms-distr) and b) enables coherent update of model files of deepTau v2p5 (on top of removal of the depreciated ones). However, if you think it that b) (most important for MC production with 124X) is not worth of merging this PR it can be closed. Then we will prepare different update of cms-data with updated deepTau v2p5 model files. I hope this comment is not confusing...

@clacaputo
Copy link
Contributor

Hi @mbluj , so the reason for the bp is intended to remove code not used in cmssw so that also the related model can be removed from 12_4_X cms-distrm right? Please, correct me if I'm wrong.

If so, I don't see any problem with doing so, maybe @cms-sw/orp-l2 has a different opinion

@mbluj
Copy link
Contributor Author

mbluj commented Jun 3, 2022

Hi @mbluj , so the reason for the bp is intended to remove code not used in cmssw so that also the related model can be removed from 12_4_X cms-distrm right? Please, correct me if I'm wrong.

It is correct.

@qliphy
Copy link
Contributor

qliphy commented Jun 3, 2022

Hi @mbluj , so the reason for the bp is intended to remove code not used in cmssw so that also the related model can be removed from 12_4_X cms-distrm right? Please, correct me if I'm wrong.

If so, I don't see any problem with doing so, maybe @cms-sw/orp-l2 has a different opinion

Fine with me under the context

@clacaputo
Copy link
Contributor

@cmsbuild please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Jun 3, 2022

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-9279aa/25255/summary.html
COMMIT: 23dc456
CMSSW: CMSSW_12_4_X_2022-06-03-1100/el8_amd64_gcc10
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/38164/25255/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: 10 differences found in the comparisons
  • DQMHistoTests: Total files compared: 50
  • DQMHistoTests: Total histograms compared: 3669912
  • DQMHistoTests: Total failures: 25
  • DQMHistoTests: Total nulls: 1
  • DQMHistoTests: Total successes: 3669864
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.004 KiB( 49 files compared)
  • DQMHistoSizes: changed ( 312.0 ): 0.004 KiB MessageLogger/Warnings
  • Checked 208 log files, 45 edm output root files, 50 DQM output files
  • TriggerResults: no differences found

@jpata
Copy link
Contributor

jpata commented Jun 6, 2022

enable profiling

@jpata
Copy link
Contributor

jpata commented Jun 6, 2022

@cmsbuild please test with cms-data/RecoTauTag-TrainingFiles#10

@cmsbuild
Copy link
Contributor

cmsbuild commented Jun 7, 2022

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-9279aa/25307/summary.html
COMMIT: 23dc456
CMSSW: CMSSW_12_4_X_2022-06-06-1100/el8_amd64_gcc10
Additional Tests: PROFILING
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/38164/25307/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: 99 differences found in the comparisons
  • DQMHistoTests: Total files compared: 50
  • DQMHistoTests: Total histograms compared: 3671502
  • DQMHistoTests: Total failures: 14
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3671466
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 49 files compared)
  • Checked 208 log files, 45 edm output root files, 50 DQM output files
  • TriggerResults: no differences found

@mbluj
Copy link
Contributor Author

mbluj commented Jun 8, 2022

@cmsbuild please test

Yes, the reco changes are from the data PR that is unrelated to this. Running again should return no changes.

Yes, I confirm, three tauIDs can be changed by the updated model files added in cms-data/RecoTauTag-TrainingFiles#10, while the rest should be unchanged. Test repeated without updated model is expected to show no changes.

@clacaputo
Copy link
Contributor

It seems the profiling job is stuck, but the comparison job ended successfully and plots can be checked here: https://cmssdt.cern.ch/SDT/jenkins-artifacts/baseLineComparisons/CMSSW_12_4_X_2022-06-07-2300+9279aa/50849/validateJR.html

As expected, the PR is not introducing any reco changes

@clacaputo
Copy link
Contributor

@cmsbuild please abort

@clacaputo
Copy link
Contributor

test parameters:

  • enable_test = none

@clacaputo
Copy link
Contributor

@cmsbuild please test

@clacaputo
Copy link
Contributor

As stated in #38164 (comment) , there are NO reco differences after test this PR without cms-data/RecoTauTag-TrainingFiles#10, but it seems that a profiling job pending is not letting the checks to finish.
@smuzaffar , could you please check? It seems the bot is ignoring enable none command

I'm ready to sign the PR as soon as the test finish

@cmsbuild
Copy link
Contributor

cmsbuild commented Jun 9, 2022

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-9279aa/25394/summary.html
COMMIT: 23dc456
CMSSW: CMSSW_12_4_X_2022-06-09-1100/el8_amd64_gcc10
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/38164/25394/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: 2 differences found in the comparisons
  • DQMHistoTests: Total files compared: 50
  • DQMHistoTests: Total histograms compared: 3671977
  • DQMHistoTests: Total failures: 8
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3671947
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 49 files compared)
  • Checked 208 log files, 45 edm output root files, 50 DQM output files
  • TriggerResults: no differences found

@smuzaffar
Copy link
Contributor

smuzaffar commented Jun 9, 2022

As stated in #38164 (comment) , there are NO reco differences after test this PR without cms-data/RecoTauTag-TrainingFiles#10, but it seems that a profiling job pending is not letting the checks to finish. @smuzaffar , could you please check? It seems the bot is ignoring enable none command

I'm ready to sign the PR as soon as the test finish

@clacaputo , there is a known issue with bot that it does not reset status for tests which were initially run (e.g. in this case profiling) but in latest tests they are dropped. Anyway, I have forced set the status for profiling and now PR tests results are now available

@clacaputo
Copy link
Contributor

clacaputo commented Jun 10, 2022

+reconstruction

@cmsbuild
Copy link
Contributor

This pull request is fully signed and it will be integrated in one of the next CMSSW_12_4_X IBs (tests are also fine) and once validation in the development release cycle CMSSW_12_5_X is complete. This pull request will now be reviewed by the release team before it's merged. @perrotta, @dpiparo, @qliphy (and backports should be raised in the release meeting by the corresponding L2)

@qliphy
Copy link
Contributor

qliphy commented Jun 10, 2022

+1
tested also with cms-sw/cmsdist#7918

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.

7 participants