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.

Sign up for GitHub

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

Jump to bottom

Pixel Heterogeneous DQM: add modules for Tracks and Vertices @cpu / @gpu comparisons #37619

Merged
merged 13 commits into from Apr 28, 2022
Merged

Pixel Heterogeneous DQM: add modules for Tracks and Vertices @cpu / @gpu comparisons #37619

merged 13 commits into from Apr 28, 2022

Conversation

mmusich
Copy link
Contributor

@mmusich mmusich commented Apr 19, 2022 •
edited

PR description:

Title says it all.
The code is actually from @sroychow and @arossi83.
Taking the liberty to pushing this through to start getting ahead with possible comments (and possibly getting merged in 12_4_0_pre3?)

PR validation:

Tested running wf. runTheMatrix.py --what gpu -l 11634.503 on lxplus-gpu successfully.

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

N/A

@cmsbuild cmsbuild added this to the CMSSW_12_4_X milestone Apr 19, 2022
@mmusich
Copy link
Contributor Author

mmusich commented Apr 19, 2022

@cms-sw/heterogeneous-l2 @VinInn comments welcome.

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-37619/29387

@cmsbuild
Copy link
Contributor

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

It involves the following packages:

  • Configuration/ProcessModifiers (operations)
  • DQM/SiPixelPhase1Heterogeneous (dqm)

@perrotta, @pmandrik, @emanueleusai, @ahmad3213, @cmsbuild, @jfernan2, @qliphy, @rvenditti, @micsucmed, @fabiocos, @davidlange6 can you please review it and eventually sign? Thanks.
@hdelanno, @makortel, @Martin-Grunewald, @missirol, @fioriNTU, @jandrea, @idebruyn, @threus, @fabiocos 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

@mmusich mmusich mentioned this pull request Apr 19, 2022
Merged
@mmusich
Copy link
Contributor Author

mmusich commented Apr 19, 2022

enable gpu

fwyzard
fwyzard reviewed Apr 19, 2022
View reviewed changes
DQM/SiPixelPhase1Heterogeneous/plugins/SiPixelPhase1CompareTrackSoA.cc
for (auto gid : looseTrkidxGPU) {
float etagpu = tsoaGPU.eta(gid);
float phigpu = tsoaGPU.phi(gid);
float dr2 = reco::deltaR2(etacpu, phicpu, etagpu, phigpu);
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe take into account also the pT of the two tracks ?
Though, I'm not sure what would be better:

  • to consider tracks with a large pT difference as "not matched"
  • to consider tracks with a large pT difference as "matched" and showing up as outliers on the pT-vs-pT plot

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, as @sroychow was anticipated we're looking at a way of displaying this differences with 1D histograms, commit from him should come in the next hours.

@fwyzard
Copy link
Contributor

fwyzard commented Apr 19, 2022 •
edited

I've left a first round of comments covering only the C++ part of the code.

There is one thing that I did not take into account: both for tracks and vertices, the matching is done all-vs-all, while it could be optimised e.g. sorting the vertices by z or organising the tracks in an eta/phi grid.
I assume that for a DQM module writing code that is easier to implement and maintain is more important that the runtime complexity of the algorithm.

@fwyzard
Copy link
Contributor

fwyzard commented Apr 19, 2022

Taking the liberty to pushing this through to start getting ahead with possible comments (and possibly getting merged in 12_4_0_pre3?)

OK for me -- let's see if we manage to review it and address any issues in time :-)

@fwyzard
Copy link
Contributor

fwyzard commented Apr 19, 2022

enable gpu

@fwyzard
Copy link
Contributor

fwyzard commented Apr 19, 2022

please test

@AdrianoDee
Copy link
Contributor

+upgrade

  • successful tests;
  • *GPUValidation wf modifications in the matrix activate the GPUvsCPU validation.

@emanueleusai
Copy link
Member

+1

@kskovpen
Copy link
Contributor

+pdmv

@cmsbuild cmsbuild mentioned this pull request Apr 28, 2022
Merged
@perrotta
Copy link
Contributor

@mmusich I am a bit confused: you write that "we need #37665 merged first", but tests apparently succeed even without it, and this PR seems to me ready to get merged otherwise. So, should we really wait for #37665 before merging this one?

@mmusich
Copy link
Contributor Author

mmusich commented Apr 28, 2022

Hi @perrotta sorry for the confusion.
#37665 offers some opportunity for reduction of memory consumption, but it's not strictly needed to merge this PR.
I wrote that comment when I was thinking #37665 could be merged fast and this PR could be adjusted accordingly.
Now, it seems that #37665 requires a little bit more work, so I would be in favor of merging this as is to get operational confidence with the workflow, and I can come back to the memory reduction once #37665 is ready.
Please notice that the net overall memory consumption for the affected workflow is reduced with this PR

DQMHistoSizes: Histogram memory added: -12948.525000000001 KiB( 6 files compared)

because some other histograms are removed.

@perrotta
Copy link
Contributor

+1

@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 be automatically merged.

@cmsbuild cmsbuild merged commit b2ddcce into cms-sw:master Apr 28, 2022
@cmsbuild cmsbuild mentioned this pull request Apr 29, 2022
Merged
@fwyzard
Copy link
Contributor

fwyzard commented Apr 29, 2022

@mmusich @cms-sw/dqm-l2
do you think you could backport this PR to CMSSW 12.3.x, in order to deploy it online in the coming weeks, so we can start testing the GPU-vs-CPU comparison in the online DQM ?

By the way, if I remember correctly we have the GPU-vs-CPU DQM workflows only for MC... who should start adding them for data ? Do you foresee any issues ?

@mmusich mmusich mentioned this pull request Apr 29, 2022
Merged
@mmusich
Copy link
Contributor Author

mmusich commented Apr 29, 2022 •
edited

do you think you could backport this PR to CMSSW 12.3.x, in order to deploy it online in the coming weeks, so we can start testing the GPU-vs-CPU comparison in the online DQM ?

here it is: #37737
I'll flag it at today's operations meeting.

By the way, if I remember correctly we have the GPU-vs-CPU DQM workflows only for MC... who should start adding them for data ? Do you foresee any issues ?

I personally don't see issues. Perhaps to discuss at the operations meeting as well?

@mmusich mmusich deleted the gpuvalidPixtrkvtx_12_4_X branch May 6, 2022 11:00
This was referenced May 6, 2022
Merged
Merged
@mmusich mmusich mentioned this pull request May 17, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@makortel makortel makortel left review comments

@fwyzard fwyzard fwyzard left review comments

Assignees
No one assigned
Labels
code-checks-approved dqm-approved fully-signed operations-approved orp-approved pdmv-approved tests-approved upgrade-approved
Projects
None yet
Milestone
CMSSW_12_4_X
Development

Successfully merging this pull request may close these issues.

None yet
9 participants
@mmusich @cmsbuild @fwyzard @sroychow @emanueleusai @AdrianoDee @kskovpen @perrotta @makortel