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

Phase 2 forwardport FastHisto PR1130 #42129

Merged
merged 6 commits into from
Jul 17, 2023

Conversation

NJManganelli
Copy link
Contributor

@NJManganelli NJManganelli commented Jun 29, 2023

This PR is a forward port of the FastHisto update in the Phase2 integration branch
cms-l1t-offline#1130

That PR grew overly large. It was refactored from 45 commits to 5, such that this PR can be divided into 3-4 pieces, should that be preferable to review (as separate PRs)

The first two commits are focused on reworking the EDM product workflow to match the GTT firmware. This consists of breaking up the "TrackSelection" into distinct TrackSelection (TS) and TrackVertexAssociation (TVA) modules. The emulation is updated to follow the firmware architecture, such that TS -> VertexFinding (VF) -> TVA -> TrackJets/MET, with separate configurable producers for VF, TrackJets, and MET. The TrackJets current behavior is retained as much as possible by currently making its TS module act in passthrough mode. Future PRs will update these separate TS/TVA modules with optimized selections from the GTT group. These changes are introduced now to focus the workflow-breaking updates into one (set of consecutive) PRs. Additionally, this fixes some naming of InputTags which were broken by the "l1t" naming update in September 2022. To simplify common workflows, common choices are propagated into the VertexProducer configuration, such that most uses can be simplified to load and add to Path/Task, rather than verbose configuration in all workflows.

  • 32ae7bc Split TrackSelection and TrackVertexAssociation, Triplicate for GTT, Propagate new workflow to central L1T and GTT configurations, fix l1t broken tags
  • 8791748 Propagate the TS and TVA split, new VertexFinder configuration, and updated JetFinding workflow to more L1T workflows

This commit is the core/seed of the PR, which change the FastHisto emulation algorithm to match firmware (HDLS GTT firmware version around June 2023). This involves updating the histogram binning to 256, removing track pt truncation, and introducing histogram bin truncation at a late stage.

  • 4246b05 FastHisto update to match firmware approximately for June 2023

This commit updates the "l1vertices" tag to "L1Vertices," to match the convention followed by most L1T products. L1{X} is the most common, followed by "Level1{X}" for some TrackTrigger products, and then less common ones.

  • 57125f0 Harmonize l1vertices to L1Vertices

This commit adds the new TS and TVA collections to the L1TrackObjectNtupleMaker, and inserts delete calls which have been historically missing.

  • c2e029e Update L1TrackObjectNtupleMaker

Validation:
scram b code-checks
scram b code-format
scram b

runTheMatrix.py -l limited -i all --ibeos passes in the cms-l1t-offline Phase2 integration branch. In central CMSSW,t has the same number of passes/fails as master.

Additionally, there was extensive performance testing by GTT, PFL1, and GT experts to ensure changes in performance were as expected.

Please let me know of necessary corrections or if this needs to be broken up into 3-4 distinct PRs.

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-42129/36108

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @NJManganelli (Nick) for master.

It involves the following packages:

  • DQMOffline/L1Trigger (dqm, l1)
  • L1Trigger/Configuration (l1)
  • L1Trigger/DemonstratorTools (l1)
  • L1Trigger/L1TTrackMatch (upgrade, l1)
  • L1Trigger/Phase2L1ParticleFlow (upgrade, l1)
  • L1Trigger/Phase2L1Taus (l1)
  • L1Trigger/VertexFinder (l1)

@aloeliger, @epalencia, @nothingface0, @emanueleusai, @cmsbuild, @AdrianoDee, @srimanob, @pmandrik, @syuvivida, @tjavaid, @micsucmed, @rvenditti can you please review it and eventually sign? Thanks.
@Martin-Grunewald, @missirol, @rociovilar 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

@epalencia
Copy link
Contributor

Please test

@cmsbuild
Copy link
Contributor

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-5bbc26/33441/summary.html
COMMIT: 8bfd955
CMSSW: CMSSW_13_2_X_2023-06-28-2300/el8_amd64_gcc11
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/42129/33441/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

  • You potentially removed 6 lines from the logs
  • ROOTFileChecks: Some differences in event products or their sizes found
  • Reco comparison results: 5 differences found in the comparisons
  • DQMHistoTests: Total files compared: 48
  • DQMHistoTests: Total histograms compared: 3193832
  • DQMHistoTests: Total failures: 48
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3193762
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 47 files compared)
  • Checked 207 log files, 159 edm output root files, 48 DQM output files
  • TriggerResults: no differences found

@emanueleusai
Copy link
Member

+1

  • DQM differenced in L1T/L1TPhase2/[TkMET,TkMET_Tracks,Correlator] as expected

@epalencia
Copy link
Contributor

+l1

@srimanob
Copy link
Contributor

srimanob commented Jul 3, 2023

Hi @NJManganelli
I just realize that L1T has a lot of complains in static checks. Some of them exists before, but is ignored or not realized that it exists. Could you please review the current log, and clarify if some of them comes from this PR.

Maybe open a issue, and follow up is a good to way to track issue, @cms-sw/l1-l2

@NJManganelli
Copy link
Contributor Author

Hi @srimanob , there was one warning tied to this PR, which concerned the config const-ness in an exists check (L1TrackVertexAssociationProducer.cc). That section needed to be revisited, so I took this opportunity to get feedback, and I've updated it according to recommendations from Andrea Bocci and Kevin Pedro (https://mattermost.web.cern.ch/cms-o-and-c/pl/htc3mmhj9jg67bqiz9ni3oxzwh)

The changes are currently in a separate PR at the head of the branch, and if approved of, I'll backport to PR1130 (and can also squash into the main commit concerning the TrackVertexAssociation, if that is highly desirable)

I've addressed the merge conflicts for L1Trigger/DemonstratorTools/python/GTTFileWriter_cff.py as well by rebasing.

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-42129/36237

  • This PR adds an extra 136KB to repository

@cmsbuild
Copy link
Contributor

Pull request #42129 was updated. @aloeliger, @epalencia, @nothingface0, @emanueleusai, @cmsbuild, @AdrianoDee, @srimanob, @pmandrik, @syuvivida, @tjavaid, @micsucmed, @rvenditti can you please check and sign again.

@emanueleusai
Copy link
Member

please test

@cmsbuild
Copy link
Contributor

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-5bbc26/33624/summary.html
COMMIT: ad4c326
CMSSW: CMSSW_13_2_X_2023-07-10-2300/el8_amd64_gcc11
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/42129/33624/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

  • You potentially removed 26 lines from the logs
  • ROOTFileChecks: Some differences in event products or their sizes found
  • Reco comparison results: 8 differences found in the comparisons
  • DQMHistoTests: Total files compared: 48
  • DQMHistoTests: Total histograms compared: 3193892
  • DQMHistoTests: Total failures: 48
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3193822
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 47 files compared)
  • Checked 207 log files, 159 edm output root files, 48 DQM output files
  • TriggerResults: no differences found

@emanueleusai
Copy link
Member

+1

  • resign

@epalencia
Copy link
Contributor

+l1

@srimanob
Copy link
Contributor

+Upgrade

There will be a follow up PR as mentioned here. Note that, on assignment, I am not sure why L1Trigger/Phase2L1Taus is not under upgrade.

@cmsbuild
Copy link
Contributor

This pull request is fully signed and it will be integrated in one of the next master IBs (tests are also fine). 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)

@NJManganelli
Copy link
Contributor Author

+Upgrade

There will be a follow up PR as mentioned here. Note that, on assignment, I am not sure why L1Trigger/Phase2L1Taus is not under upgrade.

Quickly correcting myself, it was a separate commit (not PR as I accidentally wrote) added into this PR at the head (which along with rebase triggered re-signing requests). Since the parallel PR1130 is still open in the integration branch, that commit will be added there (and any addenda from release managers) once this PR is certified okay.

@rappoccio
Copy link
Contributor

+1

@cmsbuild cmsbuild merged commit 2e9dc3e into cms-sw:master Jul 17, 2023
Dr15Jones pushed a commit to Dr15Jones/cmssw that referenced this pull request Jul 31, 2023
…itX-pr1130

Phase 2 forwardport FastHisto PR1130
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.

6 participants