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

template DQM/SiPixelPhase1Heterogenous class to be able to handle the Phase-2 SoA objects #40264

Merged
merged 5 commits into from
Dec 13, 2022

Conversation

mmusich
Copy link
Contributor

@mmusich mmusich commented Dec 8, 2022

PR description:

This is a follow-up to #39771 and #38761 in order to be able to fully monitor the SoA products for pixel rechits, tracks and vertices for both the phase-1 and phase-2 upgrade detectors.
As promised in #39771 I also deliver the renaming of all classes and packages from SiPixelPhase1Heterogeneous to SiPixelHeterogeneous in commit 64ac830

PR validation:

Run successfully

runTheMatrix.py --what upgrade -l 20834.507 -t 4 -j 8

and obtained plots for RecHits, Tracks and Vertices.

RecHits Tracks Vertices
Screenshot from 2022-12-08 20-37-13 Screenshot from 2022-12-08 20-36-37 Screenshot from 2022-12-08 20-40-31

If this PR is a backport please specify the original PR and why you need to backport that PR. If this PR will be backported please specify to which release cycle the backport is meant for:

N/A

FYI
@AdrianoDee @sroychow

@cmsbuild
Copy link
Contributor

cmsbuild commented Dec 8, 2022

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-40264/33315

  • This PR adds an extra 36KB to repository

@cmsbuild
Copy link
Contributor

cmsbuild commented Dec 8, 2022

A new Pull Request was created by @mmusich (Marco Musich) for master.

It involves the following packages:

  • DQM/SiPixelHeterogeneous (****)
  • DQM/SiPixelPhase1Heterogeneous (dqm)
  • DQMOffline/Configuration (dqm)

The following packages do not have a category, yet:

DQM/SiPixelHeterogeneous
Please create a PR for https://github.com/cms-sw/cms-bot/blob/master/categories_map.py to assign category

@emanueleusai, @ahmad3213, @cmsbuild, @jfernan2, @syuvivida, @pmandrik, @micsucmed, @rvenditti can you please review it and eventually sign? Thanks.
@hdelanno, @fioriNTU, @jandrea, @idebruyn, @threus, @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

@sroychow
Copy link
Contributor

sroychow commented Dec 8, 2022

enable gpu

@sroychow
Copy link
Contributor

sroychow commented Dec 8, 2022

please test

@@ -239,6 +239,15 @@ def customizeHLTfor38761(process):

return process

def customizeHLTfor40264(process):
for producer in producers_by_type(process, "SiPixelPhase1MonitorVertexSoA"):
producer._TypedParameterizable__type = "SiPixelMonitorVertexSoA"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this feels hacky... Is there any better option to rename on the fly?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know, but, since this function should soon [*] disappear, I think it is okay here as long as the config dumps are as expected.

There is a recurring issue with renaming plugins used in HLT menus (see #38761 (comment), for example): when the menus are 'migrated in ConfDB' to a newer CMSSW release, ConfDB will only see that the old plugin does not exist anymore (without figuring out its new name). To mitigate this problem, it is useful to have a (pre)release where both old and new plugins exist, so HLT can do this migration more easily.

Afaiu, this means this PR would also include something like

using SiPixelPhase1CompareVertexSoA = SiPixelCompareVertexSoA;
DEFINE_FWK_MODULE(SiPixelPhase1CompareVertexSoA);

(Then, the HLT customisation could even be removed, but it could stay as a reminder for HLT.)

We could then open an issue for the removal of duplicate plugins left by this PR (and by #38761), and solve it early next year.

[*] TSG expects to move HLT menus to 13_0_X in January.

Copy link
Contributor Author

@mmusich mmusich Dec 9, 2022

Choose a reason for hiding this comment

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

Thanks Marino @missirol for the suggestion. I have implemented it in dfe16cb.
I expect that - eventually - after the alpaka migration is completed, these DQM plugins could be removed from the HLT menu itself and then run on DQM machines as part of a standalone client (as for the other subsystems). But that's likely for after January, so let's go with the way you propose.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks, sounds good.

Fwiw, playing around with the python customisation, this is arguably less hacky (but I'm not asking to make further changes):

process.hltXYZ = cms.EDProducer('NewName', **process.hltXYZ.parameters_())

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does the last point in https://its.cern.ch/jira/browse/CMSHLT-2603 imply we can go ahead an remove

using SiPixelPhase1CompareVertexSoA = SiPixelCompareVertexSoA;
DEFINE_FWK_MODULE(SiPixelPhase1CompareVertexSoA);

and

using SiPixelPhase1MonitorVertexSoA = SiPixelMonitorVertexSoA;
DEFINE_FWK_MODULE(SiPixelPhase1MonitorVertexSoA);

or will it be done together with an update of the HLT menu?

Copy link
Contributor

Choose a reason for hiding this comment

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

The HLT menus first need to be updated in the release (will be done tomorrow). The duplicate plugins can then removed (I would prefer that's done in a separate PR).

@cmsbuild
Copy link
Contributor

cmsbuild commented Dec 8, 2022

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-40264/33319

  • This PR adds an extra 40KB to repository

@cmsbuild
Copy link
Contributor

cmsbuild commented Dec 9, 2022

Pull request #40264 was updated. @Martin-Grunewald, @emanueleusai, @ahmad3213, @cmsbuild, @missirol, @jfernan2, @syuvivida, @pmandrik, @micsucmed, @rvenditti can you please check and sign again.

@mmusich
Copy link
Contributor Author

mmusich commented Dec 9, 2022

@cmsbuild, please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Dec 9, 2022

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-cb40cc/29533/summary.html
COMMIT: dfe16cb
CMSSW: CMSSW_13_0_X_2022-12-08-1100/el8_amd64_gcc11
Additional Tests: GPU
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/40264/29533/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: 26 differences found in the comparisons
  • DQMHistoTests: Total files compared: 48
  • DQMHistoTests: Total histograms compared: 3421214
  • DQMHistoTests: Total failures: 1189
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3420003
  • 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

GPU Comparison Summary

Summary:

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

@missirol
Copy link
Contributor

+hlt

@emanueleusai
Copy link
Member

+1

  • dqm comparisons in failures in CPU appear to be spurious.

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

@perrotta
Copy link
Contributor

+1

@cmsbuild cmsbuild merged commit eddbea4 into cms-sw:master Dec 13, 2022
@mmusich mmusich deleted the SoAMonitoringInPhase2 branch January 9, 2023 11:16
missirol added a commit to missirol/cmssw that referenced this pull request Jan 31, 2023
missirol added a commit to missirol/cmssw that referenced this pull request Jan 31, 2023
cmsbuild added a commit that referenced this pull request Feb 1, 2023
…61and40264

remove duplicate plugins left by #38761 and #40264
fllor pushed a commit to fllor/cmssw that referenced this pull request Feb 20, 2023
NJManganelli pushed a commit to NJManganelli/cmssw that referenced this pull request Feb 24, 2023
aloeliger pushed a commit to aloeliger/cmssw that referenced this pull request Feb 24, 2023
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