DQM modules to monitor Pixel RecHits for heterogenous workflows #37174

sroychow · 2022-03-08T13:55:14Z

PR description:

This PR introduces Rechi monitoring modules for pixels for CPU and GPU workflows. There is a base class where the histo booking, filling are done. Related talk

PR validation:

Checks done with 11634.501, 11634.506

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

cmsbuild · 2022-03-08T14:03:20Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-37174/28739

This PR adds an extra 20KB to repository

cmsbuild · 2022-03-08T14:03:44Z

A new Pull Request was created by @sroychow (Suvankar Roy Chowdhury) for master.

It involves the following packages:

DQM/SiPixelPhase1Heterogeneous (dqm)

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

tsusa · 2022-03-08T14:04:55Z

please test

sroychow · 2022-03-08T14:05:13Z

enable gpu

mmusich

some quick preliminary comments.

DQM/SiPixelPhase1Heterogeneous/plugins/SiPixelPhase1MonitorRecHitsSoABase.cc

mmusich · 2022-03-08T14:11:03Z

DQM/SiPixelPhase1Heterogeneous/plugins/SiPixelPhase1MonitorRecHitsSoABase.cc

+  }
+}
+
+void SiPixelPhase1MonitorRecHitsSoABase::fillHistosForRecHit(const DetId& id, const float xG, const float yG, const float zG, const float rG, const float fphi, const uint32_t charge, const int16_t sizeX, const int16_t sizeY) {


instead of having a large set of arguments passed, wouldn't it be more elegant to store them in a POD struct or something to that effect?

tsusa · 2022-03-08T15:00:32Z

urgent
needed for GPU/CPU comparison in 123X

DQM/SiPixelPhase1Heterogeneous/plugins/SiPixelPhase1MonitorRecHitsSoABase.cc

perrotta · 2022-03-08T15:20:47Z

DQM/SiPixelPhase1Heterogeneous/plugins/SiPixelPhase1MonitorRecHitsSoABase.h

+//
+#ifndef DQM_SiPixelPhase1Heterogeneous_SiPixelPhase1MonitorRecHitsSoABase_h
+#define DQM_SiPixelPhase1Heterogeneous_SiPixelPhase1MonitorRecHitsSoABase_h
+#include "FWCore/Framework/interface/ESHandle.h"


not needed?

DQM/SiPixelPhase1Heterogeneous/plugins/SiPixelPhase1MonitorRecHitsSoABase.cc

mmusich · 2022-03-08T15:31:00Z

DQM/SiPixelPhase1Heterogeneous/plugins/SiPixelPhase1MonitorRecHitsSoACPU.cc

+//
+// Author: Alessandro Rossi, Suvankar Roy Chowdhury
+//
+#include "DataFormats/Math/interface/approx_atan2.h"


what is this needed for?

for phi conversion from short to float (short2phi) on line 55

fwyzard · 2022-03-08T16:47:08Z

assign heterogeneous

cmsbuild · 2022-03-08T16:47:32Z

New categories assigned: heterogeneous

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

mmusich

this other set is minor and I guess it could go into a follow-up.

mmusich · 2022-03-08T16:09:01Z

DQM/SiPixelPhase1Heterogeneous/plugins/SiPixelPhase1MonitorRecHitsSoACPU.cc

+  static void fillDescriptions(edm::ConfigurationDescriptions& descriptions);
+
+private:
+  edm::EDGetTokenT<TrackingRecHit2DCPU> tokenSoAHitsCPU_;


could be const (if then put into the initializer list)

mmusich · 2022-03-08T16:10:03Z

DQM/SiPixelPhase1Heterogeneous/plugins/SiPixelPhase1MonitorRecHitsSoAGPU.cc

+  edm::EDGetTokenT<cms::cuda::Product<TrackingRecHit2DGPU>> tokenHitsGPU_;
+  edm::EDGetTokenT<SiPixelClusterCollectionNew> clusterToken_;


Suggested change

edm::EDGetTokenT<cms::cuda::Product<TrackingRecHit2DGPU>> tokenHitsGPU_;

edm::EDGetTokenT<SiPixelClusterCollectionNew> clusterToken_;

const edm::EDGetTokenT<cms::cuda::Product<TrackingRecHit2DGPU>> tokenHitsGPU_;

const edm::EDGetTokenT<SiPixelClusterCollectionNew> clusterToken_;

if they're put into the initializer's list at line 40-41.

also the convention for the member data is inconsistent (either use leading m_ or trailing _)

DQM/SiPixelPhase1Heterogeneous/plugins/SiPixelPhase1MonitorRecHitsSoAGPU.cc

fwyzard · 2022-03-08T16:56:43Z

DQM/SiPixelPhase1Heterogeneous/plugins/SiPixelPhase1MonitorRecHitsSoABase.cc

+    : geomToken_(esConsumes<TrackerGeometry, TrackerDigiGeometryRecord, edm::Transition::BeginRun>()),
+      topoToken_(esConsumes<TrackerTopology, TrackerTopologyRcd, edm::Transition::BeginRun>()) {
+  topFolderName_ = iConfig.getParameter<std::string>("TopFolderName");  //"SiPixelHeterogeneous/PixelRecHitsSoA";
+  onGPU_ = iConfig.getParameter<bool>("onGPU");                         //unused now, but may be needed


I would prefer not to have a parameter without any effect, because setting it to True or False will be misleading :-(

Instead, better add it later, if it becomes needed.

fwyzard · 2022-03-08T17:00:40Z

@sroychow @arossi83 if you haven't already, could you check that the new module works correctly

in the 11634.501 workflow on a machine without GPUs (e.g. lxplus)
in the 11634.502 workflow on a machine without GPUs (e.g. lxplus)
in the 11634.502 workflow on a machine with GPUs (e.g. lxplus-gpu)

and that the plots look OK in the three cases ?

cmsbuild · 2022-03-08T18:04:26Z

-1

Failed Tests: UnitTests
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-5c87f9/22939/summary.html
COMMIT: 886edba
CMSSW: CMSSW_12_3_X_2022-03-08-1100/slc7_amd64_gcc10
Additional Tests: GPU
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/37174/22939/install.sh to create a dev area with all the needed externals and cmssw changes.

Unit Tests

I found errors in the following unit tests:

---> test test-das-selected-lumis had ERRORS

GPU Comparison Summary

Summary:

No significant changes to the logs found
Reco comparison results: 6 differences found in the comparisons
DQMHistoTests: Total files compared: 4
DQMHistoTests: Total histograms compared: 19811
DQMHistoTests: Total failures: 1928
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 17883
DQMHistoTests: Total skipped: 0
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 7479.093 KiB( 3 files compared)
DQMHistoSizes: changed ( 11634.506 ): 7479.093 KiB SiPixelHeterogeneous/PixelRecHitsSoA
Checked 12 log files, 9 edm output root files, 4 DQM output files
TriggerResults: no differences found

Comparison Summary

Summary:

No significant changes to the logs found
Reco comparison results: 8 differences found in the comparisons
DQMHistoTests: Total files compared: 49
DQMHistoTests: Total histograms compared: 3985573
DQMHistoTests: Total failures: 15
DQMHistoTests: Total nulls: 1
DQMHistoTests: Total successes: 3985535
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: -0.004 KiB( 48 files compared)
DQMHistoSizes: changed ( 312.0 ): -0.004 KiB MessageLogger/Warnings
Checked 204 log files, 45 edm output root files, 49 DQM output files
TriggerResults: no differences found

makortel · 2022-03-08T16:52:19Z

DQM/SiPixelPhase1Heterogeneous/BuildFile.xml

@@ -0,0 +1,3 @@
+<export>
+  <lib name="1"/>
+</export>


This file shouldn't be needed.

makortel · 2022-03-08T16:54:32Z

DQM/SiPixelPhase1Heterogeneous/plugins/SiPixelPhase1MonitorRecHitsSoABase.h

+  static void fillPSetDescription(edm::ParameterSetDescription& desc);
+
+protected:
+  edm::ParameterSet config_;


Appears to be unused

Suggested change

edm::ParameterSet config_;

makortel · 2022-03-08T17:38:37Z

DQM/SiPixelPhase1Heterogeneous/plugins/SiPixelPhase1MonitorRecHitsSoACPU.cc

+  const auto& rhsoaHandle = iEvent.getHandle(tokenSoAHitsCPU_);
+  if (!rhsoaHandle.isValid())
+    return;
+  auto const& rhsoa = iEvent.get(tokenSoAHitsCPU_);  //*((rhsoaHandle.product())->get());


Suggested change

auto const& rhsoa = iEvent.get(tokenSoAHitsCPU_); //*((rhsoaHandle.product())->get());

auto const& rhsoa = *rhsoaHandle;

would be cheaper and more idiomatic.

makortel · 2022-03-08T17:39:32Z

DQM/SiPixelPhase1Heterogeneous/plugins/SiPixelPhase1MonitorRecHitsSoAGPU.cc

+  const auto& rhsoaHandle = iEvent.getHandle(tokenHitsGPU_);
+  if (!rhsoaHandle.isValid())
+    return;
+  const auto& rho = iEvent.get(tokenHitsGPU_);


Suggested change

const auto& rho = iEvent.get(tokenHitsGPU_);

const auto& rho = *rhsoaHandle;

makortel · 2022-03-08T19:01:08Z

DQM/SiPixelPhase1Heterogeneous/plugins/SiPixelPhase1MonitorRecHitsSoAGPU.cc

+  m_store32 = rhsoa.localCoordToHostAsync(ctx.stream());
+  m_hitsModuleStart = rhsoa.hitsModuleStartToHostAsync(ctx.stream());
+  auto xl = m_store32.get();
+  auto yl = xl + m_nHits;


There is no synchronization between these lines and where xl, yl, or m_hitsModuleStart are used.

The preferred solution would be to use ExternalWork, but apparently the DQMEDAnalyzer base class does not support additional extensions.

In general the next best solution would be to add an additional ExternalWork-EDProducer to do the transfer. (hopefully in the near future we'd find a way to avoid the explicit transfer-EDProducers, but it's possible that will be only for Alpaka-based code)

At minimum cudaCheck(cudaStreamSynchronize(ctx.stream())); would be needed (guarantees safety, but is not necessarily as efficient).

makortel · 2022-03-08T19:02:14Z

DQM/SiPixelPhase1Heterogeneous/plugins/SiPixelPhase1MonitorRecHitsSoAGPU.cc

+  cms::cuda::host::unique_ptr<float[]> m_store32;
+  cms::cuda::host::unique_ptr<uint32_t[]> m_hitsModuleStart;


These would be better as local variables (because they are used in only one member function).

arossi83 · 2022-03-09T00:04:11Z

@sroychow @arossi83 if you haven't already, could you check that the new module works correctly

in the 11634.501 workflow on a machine without GPUs (e.g. lxplus)

in the 11634.502 workflow on a machine without GPUs (e.g. lxplus)

in the 11634.502 workflow on a machine with GPUs (e.g. lxplus-gpu)

and that the plots look OK in the three cases ?

@fwyzard the module works correctly in 11634.501 wf on lxplus and on 11634.502 wf on lxplus-gpu and plots are OK but it crashes on 11634.502 wf on lxplus.
We use the gpu procModifier to select which product to consume so in this "mixed" case (gpu procModifier but on a machine without a GPU) we try to access the CUDA RecHit anyway and this generates the crash.
According to this there is no a mechanism to avoid this actually. if you have any suggestion please let us know.

cmsbuild · 2022-03-21T17:36:05Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-37174/28926

This PR adds an extra 16KB to repository

fwyzard · 2022-03-21T17:59:43Z

please test

cmsbuild · 2022-03-22T00:26:49Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-5c87f9/23258/summary.html
COMMIT: cf7abce
CMSSW: CMSSW_12_4_X_2022-03-21-1100/slc7_amd64_gcc10
Additional Tests: GPU
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/37174/23258/install.sh to create a dev area with all the needed externals and cmssw changes.

GPU Comparison Summary

Summary:

No significant changes to the logs found
Reco comparison results: 24 differences found in the comparisons
DQMHistoTests: Total files compared: 4
DQMHistoTests: Total histograms compared: 19811
DQMHistoTests: Total failures: 2364
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 17447
DQMHistoTests: Total skipped: 0
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 8031.437 KiB( 3 files compared)
DQMHistoSizes: changed ( 11634.506 ): 8031.437 KiB SiPixelHeterogeneous/PixelRecHitsSoA
Checked 12 log files, 9 edm output root files, 4 DQM output files
TriggerResults: no differences found

Comparison Summary

@slava77 comparisons for the following workflows were not done due to missing matrix map:

/data/cmsbld/jenkins/workspace/compare-root-files-short-matrix/data/PR-5c87f9/39434.911_TTbar_14TeV+2026D88_DD4hep+TTbar_14TeV_TuneCP5_GenSimHLBeamSpot14+DigiTrigger+RecoGlobal+HARVESTGlobal

Summary:

No significant changes to the logs found
Reco comparison results: 2 differences found in the comparisons
DQMHistoTests: Total files compared: 49
DQMHistoTests: Total histograms compared: 3695650
DQMHistoTests: Total failures: 51178
DQMHistoTests: Total nulls: 64
DQMHistoTests: Total successes: 3644386
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.004 KiB( 48 files compared)
DQMHistoSizes: changed ( 312.0 ): 0.004 KiB MessageLogger/Warnings
Checked 204 log files, 45 edm output root files, 49 DQM output files
TriggerResults: no differences found

jfernan2 · 2022-03-22T07:34:37Z

+1

fwyzard · 2022-03-22T08:04:19Z

+1

cmsbuild · 2022-03-22T08:04:43Z

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, @qliphy (and backports should be raised in the release meeting by the corresponding L2)

perrotta · 2022-03-22T10:59:25Z

+1

fwyzard · 2022-10-11T07:03:45Z

Yes, that seems a reasonable approach.

cmsbuild added this to the CMSSW_12_3_X milestone Mar 8, 2022

cmsbuild added code-checks-pending dqm-pending orp-pending pending-signatures tests-pending labels Mar 8, 2022

cmsbuild added code-checks-approved and removed code-checks-pending labels Mar 8, 2022

cmsbuild added tests-started and removed tests-pending labels Mar 8, 2022

mmusich reviewed Mar 8, 2022

View reviewed changes

cmsbuild added the urgent label Mar 8, 2022

perrotta reviewed Mar 8, 2022

View reviewed changes

mmusich reviewed Mar 8, 2022

View reviewed changes

cmsbuild added the heterogeneous-pending label Mar 8, 2022

mmusich reviewed Mar 8, 2022

View reviewed changes

fwyzard reviewed Mar 8, 2022

View reviewed changes

cmsbuild added tests-rejected and removed tests-started labels Mar 8, 2022

makortel reviewed Mar 8, 2022

View reviewed changes

cmsbuild added code-checks-approved and removed code-checks-pending labels Mar 21, 2022

cmsbuild added tests-started and removed tests-pending labels Mar 21, 2022

cmsbuild added tests-approved and removed tests-started labels Mar 22, 2022

cmsbuild added dqm-approved and removed dqm-pending labels Mar 22, 2022

cmsbuild added fully-signed heterogeneous-approved and removed pending-signatures heterogeneous-pending labels Mar 22, 2022

cmsbuild added orp-approved and removed orp-pending labels Mar 22, 2022

cmsbuild merged commit a294783 into cms-sw:master Mar 22, 2022

		edm::EDGetTokenT<cms::cuda::Product<TrackingRecHit2DGPU>> tokenHitsGPU_;
		edm::EDGetTokenT<SiPixelClusterCollectionNew> clusterToken_;

	auto const& rhsoa = iEvent.get(tokenSoAHitsCPU_); //*((rhsoaHandle.product())->get());
	auto const& rhsoa = *rhsoaHandle;

	const auto& rho = iEvent.get(tokenHitsGPU_);
	const auto& rho = *rhsoaHandle;

		cms::cuda::host::unique_ptr<float[]> m_store32;
		cms::cuda::host::unique_ptr<uint32_t[]> m_hitsModuleStart;

DQM modules to monitor Pixel RecHits for heterogenous workflows #37174

DQM modules to monitor Pixel RecHits for heterogenous workflows #37174

Conversation

sroychow commented Mar 8, 2022

PR description:

PR validation:

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

cmsbuild commented Mar 8, 2022

cmsbuild commented Mar 8, 2022

tsusa commented Mar 8, 2022

sroychow commented Mar 8, 2022

mmusich left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tsusa commented Mar 8, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fwyzard commented Mar 8, 2022

cmsbuild commented Mar 8, 2022

mmusich left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fwyzard commented Mar 8, 2022

cmsbuild commented Mar 8, 2022

Unit Tests

GPU Comparison Summary

Comparison Summary

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arossi83 commented Mar 9, 2022 • edited

cmsbuild commented Mar 21, 2022

fwyzard commented Mar 21, 2022

cmsbuild commented Mar 22, 2022

GPU Comparison Summary

Comparison Summary

jfernan2 commented Mar 22, 2022

fwyzard commented Mar 22, 2022

cmsbuild commented Mar 22, 2022

perrotta commented Mar 22, 2022

fwyzard commented Oct 11, 2022 via email

arossi83 commented Mar 9, 2022 •

edited