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

[Backport 12_3_X] Update to DNN-based strategy for outside-in seed generation in Muon HLT #37467

Merged

Conversation

kondratyevd
Copy link
Contributor

This is a backport for #37437.
The backport is needed to integrate the OI seeding upgrade into the V2 HLT menu.
There are no changes to the code w.r.t. original PR.

@missirol

@cmsbuild
Copy link
Contributor

cmsbuild commented Apr 5, 2022

A new Pull Request was created by @kondratyevd (Dmitry Kondratyev) for CMSSW_12_3_X.

It involves the following packages:

  • RecoMuon/TrackerSeedGenerator (reconstruction)

@jpata, @cmsbuild, @clacaputo, @slava77 can you please review it and eventually sign? Thanks.
@HuguesBrun, @abbiendi, @Fedespring, @bellan, @sscruz, @jhgoh, @CeliaFernandez, @trocino, @cericeci, @rociovilar 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

@missirol
Copy link
Contributor

missirol commented Apr 5, 2022

urgent

Requested for HLT development in 12_3_0, see #37437 (comment).

@cmsbuild cmsbuild added the urgent label Apr 5, 2022
@missirol
Copy link
Contributor

missirol commented Apr 5, 2022

please test

The PR tests are not sensitive to this PR, see #37437 (comment).

@cmsbuild
Copy link
Contributor

cmsbuild commented Apr 5, 2022

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-a821c2/23662/summary.html
COMMIT: 4bb8edb
CMSSW: CMSSW_12_3_X_2022-04-05-1100/slc7_amd64_gcc10
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/37467/23662/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: 49
  • DQMHistoTests: Total histograms compared: 3697381
  • DQMHistoTests: Total failures: 19
  • DQMHistoTests: Total nulls: 1
  • DQMHistoTests: Total successes: 3697339
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.004 KiB( 48 files compared)
  • DQMHistoSizes: changed ( 312.0 ): 0.004 KiB MessageLogger/Warnings
  • Checked 204 log files, 45 edm output root files, 49 DQM output files
  • TriggerResults: no differences found

@JanFSchulte
Copy link
Contributor

Just a reminder that we also need cms-data/RecoMuon-TrackerSeedGenerator#4 for this update. So once it is merged we need a PR to update the version number of the relevant package in this file: https://github.com/cms-sw/cmsdist/blob/IB/CMSSW_12_3_X/master/data/cmsswdata.txt

For that we first need to know if the memory consumption reported in these profiles is acceptable: #37437 (comment)

@missirol
Copy link
Contributor

missirol commented Apr 6, 2022

I can't really judge this myself right away. This might be worth discussing in a TSG meeting to get better feedback (cc: @silviodonato). Or what is it already discussed in TSG?

If I look at the % values, this looks like a non-negligible increase, although it is a few percent of the total (<5%). At the same time, this looks like a very large increase compared to the previous model (x10 - x20 ?), if I understood correctly:

Was this increase expected? By the way, should the model be reviewed by a ML contact, to see if there are ways to improve it?

I don't think this necessarily stops this PR from proceeding, but these questions should be addressed before this is actually integrated in the HLT menu.

@smuzaffar
Copy link
Contributor

test parameters:

@smuzaffar
Copy link
Contributor

cmsdist PR to get the new data files is cms-sw/cmsdist#7755

@smuzaffar
Copy link
Contributor

please test

@kondratyevd
Copy link
Contributor Author

@missirol yes, the size increase is understood, it is simply due to the increased number of weights in the network.
We did not limit the network size during optimization, as we are not aware of what the memory limitation would be.

I suppose, we can re-optimize the network for a given limitation on the size of the model, if needed.

@cmsbuild
Copy link
Contributor

cmsbuild commented Apr 6, 2022

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-a821c2/23689/summary.html
COMMIT: 4bb8edb
CMSSW: CMSSW_12_3_X_2022-04-05-2300/slc7_amd64_gcc10
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/37467/23689/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-a821c2/23689/git-recent-commits.json
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-a821c2/23689/git-merge-result

Comparison Summary

Summary:

  • No significant changes to the logs found
  • Reco comparison results: 0 differences found in the comparisons
  • DQMHistoTests: Total files compared: 49
  • DQMHistoTests: Total histograms compared: 3697381
  • DQMHistoTests: Total failures: 70
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3697289
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 48 files compared)
  • Checked 204 log files, 45 edm output root files, 49 DQM output files
  • TriggerResults: no differences found

@perrotta
Copy link
Contributor

perrotta commented Apr 7, 2022

@cms-sw/reconstruction-l2 would you agree signing this PR, verbatim backport of #37437 ?
By mistake the cmsdist PR which updates the cms-data inputs needed for it was already merged, see cms-sw/cmsdist#7755. Of course, if there are objections to this backport we'll have to revert it before final 12_3_0.

@jpata
Copy link
Contributor

jpata commented Apr 7, 2022

+reconstruction

@cmsbuild
Copy link
Contributor

cmsbuild commented Apr 7, 2022

This pull request is fully signed and it will be integrated in one of the next CMSSW_12_3_X IBs (tests are also fine) and once validation in the development release cycle CMSSW_12_4_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)

@perrotta
Copy link
Contributor

perrotta commented Apr 7, 2022

+1

@cmsbuild cmsbuild merged commit e5d2c7c into cms-sw:CMSSW_12_3_X Apr 7, 2022
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.

None yet

7 participants