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

HLT Phase2 Removing Duplicate Modules #40912

Merged
merged 3 commits into from Mar 14, 2023

Conversation

beaucero
Copy link
Contributor

@beaucero beaucero commented Mar 1, 2023

PR description:

Some modules in HLT Phase2 menu are duplicated leading to a non optimum CPU usage. The duplicate modules are now removed and inputTag changed accordingly
See the presentation (only the 15 modules on the 2nd page have been worked on): https://indico.cern.ch/event/1259059/contributions/5288699/attachments/2601695/4492493/SB_RemovalDuplicate27Feb2023.pdf

PR validation:

Running Phase2 menu after the removal of the modules

@cmsbuild
Copy link
Contributor

cmsbuild commented Mar 1, 2023

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-40912/34392

  • This PR adds an extra 44KB to repository

@cmsbuild
Copy link
Contributor

cmsbuild commented Mar 1, 2023

A new Pull Request was created by @beaucero for master.

It involves the following packages:

  • HLTrigger/Configuration (hlt)

@cmsbuild, @missirol, @Martin-Grunewald can you please review it and eventually sign? Thanks.
@rchatter, @SohamBhattacharya, @argiro, @Martin-Grunewald, @missirol, @silviodonato, @thomreis, @simonepigazzini 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

@Martin-Grunewald
Copy link
Contributor

please test

Copy link
Contributor

@missirol missirol left a comment

Choose a reason for hiding this comment

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

@beaucero , I think some changes are necessary.

@missirol
Copy link
Contributor

missirol commented Mar 1, 2023

please abort

Tests didn't really start anyway. Better to clarify the review comments, first.

@fwyzard
Copy link
Contributor

fwyzard commented Mar 13, 2023

@cmsbuild, please test workflow 20834.76

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-40912/34590

  • This PR adds an extra 16KB to repository

@cmsbuild
Copy link
Contributor

Pull request #40912 was updated. @Martin-Grunewald, @missirol can you please check and sign again.

@beaucero
Copy link
Contributor Author

Corrections on the previous PR. Now everything should be OK:
20834.76_TTbar_14TeV+2026D88_HLTwDIGI75e33 Step0-PASSED Step1-PASSED Step2-PASSED Step3-PASSED Step4-PASSED - time date Mon Mar 13 17:21:52 2023-date Mon Mar 13 17:00:37 2023; exit: 0 0 0 0 0
1 1 1 1 1 tests passed, 0 0 0 0 0 failed
(and scram b was correctly performed before running!)

Copy link
Contributor

@missirol missirol left a comment

Choose a reason for hiding this comment

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

I leave some comments on the latest version.

Since Upgrade maintains this menu, I can sign the PR even if these comments are not addressed, as long as the tests pass.

@fwyzard
Copy link
Contributor

fwyzard commented Mar 13, 2023

please test

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-40912/34601

  • This PR adds an extra 48KB to repository

@cmsbuild
Copy link
Contributor

Pull request #40912 was updated. @Martin-Grunewald, @missirol can you please check and sign again.

@cmsbuild
Copy link
Contributor

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-40611c/31251/summary.html
COMMIT: 3d5d59b
CMSSW: CMSSW_13_1_X_2023-03-13-1100/el8_amd64_gcc11
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/40912/31251/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

  • You potentially added 6 lines to the logs
  • ROOTFileChecks: Some differences in event products or their sizes found
  • Reco comparison results: 410 differences found in the comparisons
  • DQMHistoTests: Total files compared: 49
  • DQMHistoTests: Total histograms compared: 3550756
  • DQMHistoTests: Total failures: 3
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3550731
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 48 files compared)
  • Checked 213 log files, 164 edm output root files, 49 DQM output files
  • TriggerResults: no differences found

@missirol
Copy link
Contributor

missirol commented Mar 14, 2023

+hlt

@beaucero @fwyzard

What's the preferred course of action? Integrate this PR first, then #41047 ? Or does #41047 supersede this one ?

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

@fwyzard
Copy link
Contributor

fwyzard commented Mar 14, 2023

I would say that this one can go ahead, while we decide what to do about the changes in #41047 .

@missirol
Copy link
Contributor

I would say that this one can go ahead, while we decide what to do about the changes in #41047 .

Okay, thanks.

@cms-sw/orp-l2 , this is ready for your review.

@rappoccio
Copy link
Contributor

+1

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

6 participants