fix HGCAL Layer Cluster times in heterogeneous workflows at HLT #45838

AuroraPerego · 2024-08-30T08:07:14Z

PR description:

The times and timeErrors vectors used to store the rechits time were initialized with a size of 16, but then push_back was used to fill them leaving the first 16 entries as 0.
This PR removes the allocation of 16 elements in the constructor, using reserve() instead.
The change affects only heterogeneous workflows at the HLT.

PR validation:

Tested on wf 31834.492, the HGCAL Layer Clusters times are expected to change:

old -> pre fix
new -> post fix

FYI @rovere

cmsbuild · 2024-08-30T08:07:41Z

cms-bot internal usage

cmsbuild · 2024-08-30T08:09:37Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-45838/41582

cmsbuild · 2024-08-30T08:09:59Z

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

It involves the following packages:

RecoLocalCalo/HGCalRecProducers (upgrade, reconstruction)

@cmsbuild, @jfernan2, @mandrenguyen, @srimanob, @subirsarkar can you please review it and eventually sign? Thanks.
@apsallid, @bsunanda, @cseez, @edjtscott, @felicepantaleo, @hatakeyamak, @lecriste, @lgray, @missirol, @pfs, @rovere, @sameasy, @sethzenz, @vandreev11, @youyingli this is something you requested to watch as well.
@antoniovilela, @mandrenguyen, @rappoccio, @sextonkennedy you are the release manager for this.

cms-bot commands are listed here

rovere · 2024-08-30T12:28:26Z

thanks @AuroraPerego for the investigation and the fix.

rovere · 2024-08-30T12:28:32Z

@cmsbuild please test

cmsbuild · 2024-08-30T15:40:15Z

+1

Size: This PR adds an extra 20KB to repository
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-4551d6/41199/summary.html
COMMIT: 95e7d74
CMSSW: CMSSW_14_2_X_2024-08-29-2300/el8_amd64_gcc12
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/45838/41199/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

You potentially added 2 lines to the logs
Reco comparison results: 7 differences found in the comparisons
DQMHistoTests: Total files compared: 44
DQMHistoTests: Total histograms compared: 3328315
DQMHistoTests: Total failures: 6
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3328289
DQMHistoTests: Total skipped: 20
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 43 files compared)
Checked 193 log files, 163 edm output root files, 44 DQM output files
TriggerResults: no differences found

rovere · 2024-09-02T07:52:58Z

RecoLocalCalo/HGCalRecProducers/plugins/HGCalLayerClustersFromSoAProducer.cc

+    for (unsigned int i = 0; i < clusters->size(); ++i) {
+      times[i].reserve(16);
+      timeErrors[i].reserve(16);
+    }


Would it make sense to embed this loop into the previous one at lines 69 to 77? In this way, the heuristic is not even necessary and the size can be reserved correctly (i.e. the maximum possible, eventually some rechits will be discarded from the timing computation).

rovere · 2024-09-02T09:48:34Z

@cmsbuild please test

rovere · 2024-09-02T09:48:52Z

The bot seems to be a little stuck...

cmsbuild · 2024-09-02T09:51:37Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-45838/41597

cmsbuild · 2024-09-02T09:52:04Z

Pull request #45838 was updated. @jfernan2, @mandrenguyen, @srimanob, @subirsarkar can you please check and sign again.

cmsbuild · 2024-09-02T13:03:03Z

+1

Size: This PR adds an extra 20KB to repository
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-4551d6/41220/summary.html
COMMIT: c733b85
CMSSW: CMSSW_14_2_X_2024-09-01-2300/el8_amd64_gcc12
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/45838/41220/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

You potentially removed 176 lines from the logs
Reco comparison results: 10 differences found in the comparisons
DQMHistoTests: Total files compared: 44
DQMHistoTests: Total histograms compared: 3328315
DQMHistoTests: Total failures: 5
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3328290
DQMHistoTests: Total skipped: 20
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 43 files compared)
Checked 193 log files, 163 edm output root files, 44 DQM output files
TriggerResults: no differences found

mmusich · 2024-09-02T13:32:20Z

Tested on wf 31834.492, the HGCAL Layer Clusters times are expected to change:

is there a way to make this change appear in the bot tests? E.g. by running wf 31834.492 ?

AuroraPerego · 2024-09-02T15:36:45Z

is there a way to make this change appear in the bot tests? E.g. by running wf 31834.492 ?

I'm unsure because this workflow runs the producer I've changed, but the collection of clusters times is not saved in the event.

mmusich · 2024-09-02T15:40:20Z

I'm unsure because this workflow runs the producer I've changed, but the collection of clusters times is not saved in the event.

and does it have any bearing on trigger decisions?

AuroraPerego · 2024-09-02T15:49:33Z

and does it have any bearing on trigger decisions?

I doubt :)

mmusich · 2024-09-02T16:07:39Z

I doubt :)

I am confused. So this PR is of no consequence? what's the purpose?

rovere · 2024-09-02T16:31:12Z

when running CLUE on GPU at HLT for Phase2, the timing assigned to the clusters is wrong.
Regardless of its usage, we need to fix it.
This PR fixes it.

AuroraPerego · 2024-09-02T16:34:40Z

is there a way to make this change appear in the bot tests? E.g. by running wf 31834.492 ?

Thinking about it the effects should be visible downstream, e.g. in tracksters and candidates. They are saved in the event and their time is computed using the clusters times.

rovere · 2024-09-04T09:52:59Z

test parameters:

enable = gpu
workflows_gpu = 31834.492

rovere · 2024-09-04T09:53:29Z

please test

cmsbuild · 2024-09-04T13:23:04Z

-1

Failed Tests: RelVals-GPU
Size: This PR adds an extra 12KB to repository
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-4551d6/41273/summary.html
COMMIT: c733b85
CMSSW: CMSSW_14_2_X_2024-09-03-2300/el8_amd64_gcc12
Additional Tests: GPU
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/45838/41273/install.sh to create a dev area with all the needed externals and cmssw changes.

RelVals-GPU

ValueError: Undefined workflows: 31834.492

Comparison Summary

Summary:

You potentially added 1 lines to the logs
Reco comparison results: 84 differences found in the comparisons
DQMHistoTests: Total files compared: 44
DQMHistoTests: Total histograms compared: 3328501
DQMHistoTests: Total failures: 4603
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3323878
DQMHistoTests: Total skipped: 20
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 43 files compared)
Checked 193 log files, 163 edm output root files, 44 DQM output files
TriggerResults: no differences found

rovere · 2024-09-04T13:27:02Z

test parameters:

enable = gpu
workflows_gpu = 31834.492
workflow_opts = -w upgrade
workflow_opts_gpu = -w upgrade

rovere · 2024-09-04T13:28:14Z

please test

cmsbuild · 2024-09-04T16:39:03Z

+1

Size: This PR adds an extra 12KB to repository
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-4551d6/41284/summary.html
COMMIT: c733b85
CMSSW: CMSSW_14_2_X_2024-09-04-1100/el8_amd64_gcc12
Additional Tests: GPU
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/45838/41284/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

You potentially added 7 lines to the logs
Reco comparison results: 0 differences found in the comparisons
DQMHistoTests: Total files compared: 44
DQMHistoTests: Total histograms compared: 3328501
DQMHistoTests: Total failures: 0
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3328481
DQMHistoTests: Total skipped: 20
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 43 files compared)
Checked 193 log files, 163 edm output root files, 44 DQM output files
TriggerResults: no differences found

GPU Comparison Summary

Summary:

You potentially removed 24 lines from the logs
Reco comparison results: 28 differences found in the comparisons
DQMHistoTests: Total files compared: 7
DQMHistoTests: Total histograms compared: 137352
DQMHistoTests: Total failures: 769
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 136583
DQMHistoTests: Total skipped: 0
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 6 files compared)
Checked 25 log files, 30 edm output root files, 7 DQM output files
TriggerResults: no differences found

mmusich · 2024-09-04T16:51:26Z

+1

tbh, I am more confused than ever.
In wf 31834.492_TTbar_14TeV+2026D110SimOnGen_Patatrack_FullRecoAlpaka I see plenty of HLT changes log: all of them in tracking (in which I would have not anticipated any changes) and none in HGCal (where I would have expected some according to #45838 (comment)).
By no means I am a reviewer but I would be keen to understand better.

felicepantaleo · 2024-09-09T11:54:39Z

+1

tbh, I am more confused than ever. In wf 31834.492_TTbar_14TeV+2026D110SimOnGen_Patatrack_FullRecoAlpaka I see plenty of HLT changes log: all of them in tracking (in which I would have not anticipated any changes) and none in HGCal (where I would have expected some according to #45838 (comment)). By no means I am a reviewer but I would be keen to understand better.

The comparisons show tiny difference in pixel tracks offline alpaka GPU vs alpaka GPU if I understand correctly.
Are you surprised because you would have expected phase-2 Pixel Tracks alpaka running on GPU to be bit-by-bit reproducible?

cmsbuild added this to the CMSSW_14_2_X milestone Aug 30, 2024

cmsbuild added reconstruction-pending pending-signatures tests-pending orp-pending upgrade-pending code-checks-pending labels Aug 30, 2024

cmsbuild added code-checks-approved and removed code-checks-pending labels Aug 30, 2024

cmsbuild added tests-started and removed tests-pending labels Aug 30, 2024

cmsbuild added tests-approved and removed tests-started labels Aug 30, 2024

rovere reviewed Sep 2, 2024

View reviewed changes

use reserve instead of defining the size in the constructor

c733b85

AuroraPerego force-pushed the fixResize_pre7 branch from 95e7d74 to c733b85 Compare September 2, 2024 08:56

cmsbuild added tests-started code-checks-pending and removed tests-approved code-checks-approved labels Sep 2, 2024

cmsbuild removed the code-checks-pending label Sep 2, 2024

cmsbuild added the code-checks-approved label Sep 2, 2024

cmsbuild added tests-approved and removed tests-started labels Sep 2, 2024

cmsbuild added tests-started and removed tests-approved labels Sep 4, 2024

cmsbuild added tests-rejected and removed tests-started labels Sep 4, 2024

cmsbuild added tests-started and removed tests-rejected labels Sep 4, 2024

cmsbuild added tests-approved and removed tests-started labels Sep 4, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix HGCAL Layer Cluster times in heterogeneous workflows at HLT #45838

fix HGCAL Layer Cluster times in heterogeneous workflows at HLT #45838

AuroraPerego commented Aug 30, 2024

cmsbuild commented Aug 30, 2024 •

edited

Loading

cmsbuild commented Aug 30, 2024

cmsbuild commented Aug 30, 2024

rovere commented Aug 30, 2024

rovere commented Aug 30, 2024

cmsbuild commented Aug 30, 2024

rovere Sep 2, 2024

rovere commented Sep 2, 2024

rovere commented Sep 2, 2024

cmsbuild commented Sep 2, 2024

cmsbuild commented Sep 2, 2024

cmsbuild commented Sep 2, 2024

mmusich commented Sep 2, 2024

AuroraPerego commented Sep 2, 2024

mmusich commented Sep 2, 2024

AuroraPerego commented Sep 2, 2024

mmusich commented Sep 2, 2024

rovere commented Sep 2, 2024

AuroraPerego commented Sep 2, 2024

rovere commented Sep 4, 2024

rovere commented Sep 4, 2024

cmsbuild commented Sep 4, 2024

rovere commented Sep 4, 2024

rovere commented Sep 4, 2024

cmsbuild commented Sep 4, 2024

mmusich commented Sep 4, 2024 •

edited

Loading

felicepantaleo commented Sep 9, 2024

fix HGCAL Layer Cluster times in heterogeneous workflows at HLT #45838

Are you sure you want to change the base?

fix HGCAL Layer Cluster times in heterogeneous workflows at HLT #45838

Conversation

AuroraPerego commented Aug 30, 2024

PR description:

PR validation:

cmsbuild commented Aug 30, 2024 • edited Loading

cmsbuild commented Aug 30, 2024

cmsbuild commented Aug 30, 2024

rovere commented Aug 30, 2024

rovere commented Aug 30, 2024

cmsbuild commented Aug 30, 2024

Comparison Summary

rovere Sep 2, 2024

Choose a reason for hiding this comment

rovere commented Sep 2, 2024

rovere commented Sep 2, 2024

cmsbuild commented Sep 2, 2024

cmsbuild commented Sep 2, 2024

cmsbuild commented Sep 2, 2024

Comparison Summary

mmusich commented Sep 2, 2024

AuroraPerego commented Sep 2, 2024

mmusich commented Sep 2, 2024

AuroraPerego commented Sep 2, 2024

mmusich commented Sep 2, 2024

rovere commented Sep 2, 2024

AuroraPerego commented Sep 2, 2024

rovere commented Sep 4, 2024

rovere commented Sep 4, 2024

cmsbuild commented Sep 4, 2024

RelVals-GPU

Comparison Summary

rovere commented Sep 4, 2024

rovere commented Sep 4, 2024

cmsbuild commented Sep 4, 2024

Comparison Summary

GPU Comparison Summary

mmusich commented Sep 4, 2024 • edited Loading

felicepantaleo commented Sep 9, 2024

cmsbuild commented Aug 30, 2024 •

edited

Loading

mmusich commented Sep 4, 2024 •

edited

Loading