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

Including more general DQM compare modules for pixel tracks objects #45206

Merged
merged 1 commit into from
Jun 27, 2024

Conversation

borzari
Copy link
Contributor

@borzari borzari commented Jun 12, 2024

PR description:

This PR is the replacement of #43964 with essentially the same changes, except for the Alpaka vs CUDA compare modules that are not needed anymore. It still includes generalized DQM compare modules for pixel rechits, tracks and vertices, which are templated if needed for other SoA types, an HLT customize function to include the module changes and pixel digi errors monitoring (that is also included in the DQM step), and the changes requested in #43964 (comment).

PR validation:

Executed the code with wf 12834.403 to check DQM plots and everything works as expected.

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:

This is not a backport. These changes will be backported to 14_0_X in a following PR.

Pinging @mmusich @makortel @fwyzard @AdrianoDee

@cmsbuild
Copy link
Contributor

cmsbuild commented Jun 12, 2024

cms-bot internal usage

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-45206/40555

@cmsbuild
Copy link
Contributor

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

It involves the following packages:

  • DQM/SiPixelHeterogeneous (dqm)
  • DataFormats/TrackSoA (heterogeneous, reconstruction)
  • DataFormats/TrackingRecHitSoA (heterogeneous, reconstruction)
  • EventFilter/SiPixelRawToDigi (reconstruction)
  • HLTrigger/Configuration (hlt)
  • RecoLocalTracker/SiPixelClusterizer (reconstruction)

@makortel, @tjavaid, @Martin-Grunewald, @mmusich, @syuvivida, @cmsbuild, @fwyzard, @antoniovagnerini, @nothingface0, @jfernan2, @mandrenguyen, @rvenditti can you please review it and eventually sign? Thanks.
@VourMa, @dkotlins, @missirol, @gpetruc, @rovere, @silviodonato, @felicepantaleo, @tsusa, @idebruyn, @JanFSchulte, @tvami, @mroguljic, @mtosi, @mmusich, @fioriNTU, @jandrea, @VinInn, @Martin-Grunewald, @threus, @ferencek, @GiacomoSguazzoni this is something you requested to watch as well.
@rappoccio, @antoniovilela, @sextonkennedy you are the release manager for this.

cms-bot commands are listed here

@@ -260,6 +260,90 @@ def checkHLTfor43774(process):
print('# TSG WARNING: check value of parameter "useAbs" in',filt,'(expect True but is False)!')

return process

def customizeHLTforPR(process):
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
def customizeHLTforPR(process):
def customizeHLTfor45206(process):

@@ -326,5 +410,6 @@ def customizeHLTforCMSSW(process, menuType="GRun"):
process = checkHLTfor43774(process)
process = customizeHLTfor44576(process)
process = customizeHLTfor45063(process)
process = customizeHLTforPR(process)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
process = customizeHLTforPR(process)
process = customizeHLTfor45206(process)

@@ -1,244 +0,0 @@
#include "DQMServices/Core/interface/MonitorElement.h"
Copy link
Contributor

Choose a reason for hiding this comment

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

@borzari, please do NOT delete the old plugins.
If you do that, when we we'll run (from the TSG side) the confDB parsing in next pre-release, the modules currently in the HLT menu will disappear, leading to have to redo the sequence by hand with the new plugins.
In the past what we did was:

  • add the new plugins in the master (and backport down to the cycle used by HLT - currently CMSSW_14_0_X)
  • JIRA ticket to change the sequence in ConfDB
  • PR to cmssw to remove the old plugins in the folllowing pre-release (that could be done by TSG).

See e.g. #40646 for some past instance.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mmusich I already reverted to the previous commit when the files weren't deleted and made the changes requested in your comments above, regarding changing the name of function customizeHLTforPR to customizeHLTfor45206. I am also going to clarify in the description of the PR that this will be backported to 14_0_X. Thank you for the suggestions

<class name="pixelTrack::TracksHostPhase1" ClassVersion="3">
<version ClassVersion="3" checksum="794224446"/>
</class>
<class name="pixelTrack::TracksHostPhase1"/>
Copy link
Contributor

Choose a reason for hiding this comment

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

out of curiosity (even if this lies beyond the HLT proper review) why these changes to the class version are needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I followed this comment #43964 (comment) from the previous PR. Maybe @makortel can provide more specific information about why this is the case now

Copy link
Contributor

Choose a reason for hiding this comment

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

Quoting @makortel:

Given that TrackingRecHitHostPhase1 itself is an instantiation of a class template, its version should not be tracked this way (but with CMS_CLASS_VERSION() in the class template definition). But given that schema evolution of PortableHostCollection is unexplored anyway (and therefore nothing is promised), I'd suggest to remove the class version for now (until the schema evolution is properly understood)

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-45206/40577

  • This PR adds an extra 24KB to repository

  • There are other open Pull requests which might conflict with changes you have proposed:

@cmsbuild
Copy link
Contributor

Pull request #45206 was updated. @mandrenguyen, @Martin-Grunewald, @fwyzard, @cmsbuild, @rvenditti, @tjavaid, @syuvivida, @mmusich, @jfernan2, @nothingface0, @makortel, @antoniovagnerini can you please check and sign again.

@mmusich
Copy link
Contributor

mmusich commented Jun 13, 2024

test parameters:

  • workflow_opts_gpu= -w upgrade
  • workflows_gpu= 12434.403, 12434.405, 12434.503

@mmusich
Copy link
Contributor

mmusich commented Jun 13, 2024

@cmsbuild, please test

@mmusich
Copy link
Contributor

mmusich commented Jun 19, 2024

DQMHistoSizes: changed ( 12434.403,... ): 55243.484 KiB SiPixelHeterogeneous/PixelErrorCompareGPUvsCPU
DQMHistoSizes: changed ( 12434.403,... ): 37.574 KiB SiPixelHeterogeneous/PixelErrorsCPU
DQMHistoSizes: changed ( 12434.403,... ): 37.574 KiB SiPixelHeterogeneous/PixelErrorsGPU
DQMHistoSizes: changed ( 12434.403,... ): 0.020 KiB SiPixelHeterogeneous/PixelTrackCompareDeviceVSHost

the first three changes are due to new additions to the relvals (these plots were not filled before, thanks for adding them!) while the fourth seems likely to come to some tiny residual discrepancy in the gpu vs cpu tracking code.

@mmusich
Copy link
Contributor

mmusich commented Jun 19, 2024

+hlt

@jfernan2
Copy link
Contributor

+1

@fwyzard
Copy link
Contributor

fwyzard commented Jun 19, 2024

+heterogeneous

@mmusich
Copy link
Contributor

mmusich commented Jun 25, 2024

@cms-sw/dqm-l2 please review this PR, it would be useful to have in the next 14.1.X pre-release.
@borzari can you please open a backport to CMSSW_14_0_X? If there are no objections, I think we should try to integrate this in the next 2024 menu (V1.4) @cms-sw/hlt-l2, @missirol FYI.

@Martin-Grunewald
Copy link
Contributor

Once the PR(s) are integrated, I assume an update of https://its.cern.ch/jira/browse/CMSHLT-3147 is warranted, to get the updated path configuration.

@mmusich
Copy link
Contributor

mmusich commented Jun 25, 2024

I assume an update of https://its.cern.ch/jira/browse/CMSHLT-3147 is warranted, to get the updated path configuration.

It will have to be a different ticket because since the persistence issue (see #44700) is not solved we still can't kick these plugins out of the menu. They will be simply replaced by this (better) version.
I can take care of opening the ticket.

@tjavaid
Copy link

tjavaid commented Jun 26, 2024

+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 now be reviewed by the release team before it's merged. @antoniovilela, @sextonkennedy, @rappoccio (and backports should be raised in the release meeting by the corresponding L2)

@antoniovilela
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.

8 participants