Fix SiPixelRawToClusterGPUKernel for spurious ROCs

[fwyzard]

PR description:

Make the implementation of the pixel unpacker running on GPU skip spurious ROCs similar to the legacy version of the unpacker.

Fixes an HLT crash that has been happening more or less regularly for the past two months.

See #39045 for more details on the problem, investigation, and tests.

Also, changes the debug function parameter to a template parameter, to enable or disable the printouts at compile time.

PR validation:

Running the HLT over the error stream files does not crash any more.

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:

To be backported to 12.4.x and 12.5.x for data taking.

[fwyzard]

type bugfix

[fwyzard]

enable gpu

[fwyzard]

please test

[fwyzard]

assign trk-dpg

[cmsbuild]

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-39711/32544

• This PR adds an extra 20KB to repository

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

  • File RecoLocalTracker/SiPixelClusterizer/plugins/SiPixelRawToClusterGPUKernel.cu modified in PR(s): [DO NOT MERGE] remove duplicate pixels #37359, Tracker Traits and Enabling Phase2 for Inner Tracker Reconstruction on GPU #38761

[cmsbuild]

New categories assigned: trk-dpg

@connorpa,@sroychow you have been requested to review this Pull request/Issue and eventually sign? Thanks

[cmsbuild]

A new Pull Request was created by @fwyzard (Andrea Bocci) for master.

It involves the following packages:

• RecoLocalTracker/SiPixelClusterizer (reconstruction)

@connorpa, @mandrenguyen, @clacaputo, @sroychow can you please review it and eventually sign? Thanks.
@mtosi, @VourMa, @felicepantaleo, @GiacomoSguazzoni, @JanFSchulte, @rovere, @VinInn, @mroguljic, @missirol, @dkotlins, @ferencek, @gpetruc, @mmusich, @threus, @tvami 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

[cmsbuild]

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-28a6a6/28249/summary.html
COMMIT: 85fea7e
CMSSW: CMSSW_12_6_X_2022-10-13-2300/el8_amd64_gcc10
Additional Tests: GPU
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/39711/28249/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: 0 differences found in the comparisons
• Reco comparison had 6 failed jobs
• DQMHistoTests: Total files compared: 49
• DQMHistoTests: Total histograms compared: 3392309
• DQMHistoTests: Total failures: 0
• DQMHistoTests: Total nulls: 0
• DQMHistoTests: Total successes: 3392287
• DQMHistoTests: Total skipped: 22
• DQMHistoTests: Total Missing objects: 0
• DQMHistoSizes: Histogram memory added: 0.0 KiB( 48 files compared)
• Checked 204 log files, 49 edm output root files, 49 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
• Reco comparison had 3 failed jobs
• DQMHistoTests: Total files compared: 4
• DQMHistoTests: Total histograms compared: 19876
• DQMHistoTests: Total failures: 74
• DQMHistoTests: Total nulls: 0
• DQMHistoTests: Total successes: 19802
• 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

[rappoccio]

Looks like the relval failures were spurious, let's see if we can get the 12.6 PR integrated today so we can test in an IB and then merge the back ports.

[fwyzard]

You realise that every single failure observed so far was spurious, right ?
And that the decision to merge this fix now or delay it further is completely arbitrary, and is not based on any objective check ?

[rappoccio]

Yes, and I'm hoping RECO and TRK DPG will give signatures ASAP so we can get this in today.

[fwyzard]

I see, thanks.

[mandrenguyen]

Well, the "tests-approved" now, but there are some segfaults shown in the comparisons:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/baseLineComparisons/CMSSW_12_6_X_2022-10-13-2300+28a6a6/53395/validateJR.html

Does anyone understand where these are coming from?

[fwyzard]

plotting variable: HBHEDataFramesSorted_simHcalUnsuppressedDigis__RECO.obj.obj@.size()
/cvmfs/cms-ib.cern.ch/nweek-02754/el8_amd64_gcc10/lcg/root/6.24.07-743e141b5605864d08d853874a243e48/etc//cling/std.modulemap:368:10: error: module 'std.codecvt' is incompatible with feature 'header_existence'
  module "codecvt" {
         ^
/cvmfs/cms-ib.cern.ch/nweek-02754/el8_amd64_gcc10/external/gcc/10.3.0-84898dea653199466402e67d73657f10/bin/../lib/gcc/x86_64-redhat-linux-gnu/10.3.0/../../../../include/c++/10.3.0/bits/fs_path.h:40:10: note: submodule of top-level module 'std' implicitly imported here
#include <codecvt>

No idea, but clearly unrelated to this fix.

[missirol]

Does anyone understand where these are coming from?

No, but they also appear elsewhere, #39691:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/baseLineComparisons/CMSSW_12_6_X_2022-10-13-2300+234fc6/53398/validateJR.html

( they seem to come from some RECO plotting script, validate.C ? )

My guess is that none of those wfs runs pixel-gpu reco, which is all this PR touches (so the issue is clearly unrelated to this PR). Note that this plotting failure does not show up in the 12_4_X backport of this PR.

[rappoccio]

I agree that this looks unrelated to this PR. I suggest we can approve this one and make an issue to fix these unrelated WFs.

[rappoccio]

Issue made here.

[mandrenguyen]

+reconstruction

[sroychow]

+1

[cmsbuild]

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)

[rappoccio]

+1

[rappoccio]

Updated plan on the other PR, discussion can continue there:

#39045 (comment)