Patatrack integration - Hcal conditions (13/N) #32039

mariadalfonso · 2020-11-05T16:00:50Z

Hcal conditions on GPU are move in the appropriate CondFormats/HcalObjects package
to be used by HCAL-local reco in #31720

@fwyzard @perrotta @makortel @slava77 @jpata

cmsbuild · 2020-11-05T16:01:12Z

The code-checks are being triggered in jenkins.

cmsbuild · 2020-11-05T16:09:54Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-32039/19590

This PR adds an extra 32KB to repository

cmsbuild · 2020-11-05T16:10:15Z

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

It involves the following packages:

CondFormats/HcalObjects

@yuanchao, @christopheralanwest, @tocheng, @cmsbuild, @tlampen, @ggovi, @pohsun can you please review it and eventually sign? Thanks.
@mmusich, @abdoulline, @tocheng, @seemasharmafnal this is something you requested to watch as well.
@silviodonato, @dpiparo, @qliphy you are the release manager for this.

cms-bot commands are listed here

makortel · 2020-11-06T15:22:14Z

CondFormats/HcalObjects/interface/HcalCombinedRecordsGPU.h

+
+using HcalConvertedPedestalsRcd = HcalCombinedRecord<HcalPedestalsRcd, HcalQIEDataRcd, HcalQIETypesRcd>;
+
+using HcalConvertedEffectivePedestalsRcd = HcalCombinedRecord<HcalPedestalsRcd, HcalQIEDataRcd, HcalQIETypesRcd>;


By quick look HcalConvertedPedestalsRcd and HcalConvertedPedestalsRcd are identical (in fact they very same C++ type). What motivates the duplication?

makortel · 2020-11-06T15:22:30Z

CondFormats/HcalObjects/interface/HcalCombinedRecordsGPU.h

+    HcalCombinedRecord<HcalPedestalsRcd, HcalPedestalWidthsRcd, HcalQIEDataRcd, HcalQIETypesRcd>;
+
+using HcalConvertedEffectivePedestalWidthsRcd =
+    HcalCombinedRecord<HcalPedestalsRcd, HcalPedestalWidthsRcd, HcalQIEDataRcd, HcalQIETypesRcd>;


Same with HcalConvertedPedestalWidthsRcd and HcalConvertedEffectivePedestalWidthsRcd.

Looks like nobody really knows, so I'll just remove the duplicate ones and check that everything still works.

Pedestal and EffectivePedestal (mean and widths) are actually two distinct payload.
We derive this one with bias voltage on and one with bias voltage off.

We need both.
we can remove the code duplicate, but we actually need to read two different numbers.

Two (or more) payload (EventSetup product) types are fine, I was asking about the Record types (that determine the IOVs). Framework supports arbitrary number of products and product types in a Record.

makortel · 2020-11-06T15:28:19Z

CondFormats/HcalObjects/interface/HcalConvertedPedestalWidthsGPU.h

+
+#ifndef __CUDACC__
+#include "HeterogeneousCore/CUDAUtilities/interface/HostAllocator.h"
+#include "HeterogeneousCore/CUDACore/interface/ESProduct.h"


Is a dependence on CUDA acceptable for CondFormats? Or would we need e.g. CUDACondFormats for these? (these are not supposed to be stored in the CondDB in any case)

Just to note that in the next revision of the CUDA framework support I hope to remove the explicit use of cms::cuda::ESProduct as a member of the "payload", and instead wrap the payload similarly to cms::cuda::Product<T> (for EDProducts) for th ES product.

Just to note that in the next revision of the CUDA framework support I hope to remove the explicit use of cms::cuda::ESProduct as a member of the "payload"

That would remove the dependency on CUDA from this package ?

Just to note that in the next revision of the CUDA framework support I hope to remove the explicit use of cms::cuda::ESProduct as a member of the "payload"

That would remove the dependency on CUDA from this package ?

Very likely yes. Essentially e.g. HcalConvertedPedestalWidthsGPU would change such that (e.g.)

HcalConvertedPedestalWidthsGPU::Product becomes HcalConvertedPedestalWidthsGPU

The logic from HcalConvertedPedestalWidthsGPU::getProduct() moves into the ESProducer

OK, then I would leave this part as is for the time being.

Just to note that in the next revision of the CUDA framework support I hope to remove the explicit use of cms::cuda::ESProduct as a member of the "payload"

That would remove the dependency on CUDA from this package ?

Very likely yes.

On a further thought I have to take it back. If the payload uses e.g. cms::cuda::device::unique_ptr (which I hope the CUDA ESProducts would move to at that point), the dependence on HeterogeneousCore/CUDAUtilities would be unavoidable.

makortel · 2020-11-06T15:40:38Z

CondFormats/HcalObjects/interface/HcalConvertedPedestalWidthsGPU.h

+  ~HcalConvertedPedestalWidthsGPU() = default;
+
+  // get device pointers
+  Product const& getProduct(cudaStream_t) const;


I'm concerned by this function (and many below) effectively returning "const pointer to non-const" instead of "(const) pointers to const". Then it would be very easy (as in compiler would not catch) to have code that accidentally modifies the pointed-to data (which is not allowed for ES products).

do you think that changing the definition of Product to

struct Product { ~Product(); edm::propagate_const<float*> values; };

would fix this potential issue ?

I believe edm::propagate_const would solve issue. I suppose it would require declaring ~all methods of propagate_const as constexpr for it to work in the device code.

Actually these changes compile fine:

diff --git a/CondFormats/HcalObjects/interface/HcalConvertedPedestalWidthsGPU.h b/CondFormats/HcalObjects/interface/HcalConvertedPedestalWidthsGPU.h index e3adb59ca54e..1afad7ca3a66 100644 --- a/CondFormats/HcalObjects/interface/HcalConvertedPedestalWidthsGPU.h +++ b/CondFormats/HcalObjects/interface/HcalConvertedPedestalWidthsGPU.h @@ -5,6 +5,7 @@ #include "CondFormats/HcalObjects/interface/HcalPedestalWidths.h" #include "CondFormats/HcalObjects/interface/HcalQIEData.h" #include "CondFormats/HcalObjects/interface/HcalQIETypes.h" +#include "FWCore/Utilities/interface/propagate_const.h" #ifndef __CUDACC__ #include "HeterogeneousCore/CUDAUtilities/interface/HostAllocator.h" @@ -15,7 +16,7 @@ class HcalConvertedPedestalWidthsGPU { public: struct Product { ~Product(); - float* values; + edm::propagate_const<float*> values; }; #ifndef __CUDACC__ diff --git a/CondFormats/HcalObjects/src/HcalConvertedPedestalWidthsGPU.cc b/CondFormats/HcalObjects/src/HcalConvertedPedestalWidthsGPU.cc index 1d7ca5b3394c..0af2b5eaade2 100644 --- a/CondFormats/HcalObjects/src/HcalConvertedPedestalWidthsGPU.cc +++ b/CondFormats/HcalObjects/src/HcalConvertedPedestalWidthsGPU.cc @@ -145,6 +145,8 @@ HcalConvertedPedestalWidthsGPU::Product const& HcalConvertedPedestalWidthsGPU::g auto const& product = product_.dataForCurrentDeviceAsync( cudaStream, [this](HcalConvertedPedestalWidthsGPU::Product& product, cudaStream_t cudaStream) { // malloc - cudaCheck(cudaMalloc((void**)&product.values, this->values_.size() * sizeof(float))); + float* values; + cudaCheck(cudaMalloc(&values, values_.size() * sizeof(float))); + product.values = values; // transfer

Actually these changes compile fine:

Ah, right, probably HcalConvertedPedestalWidthsGPU::Product is not passed to device code as such, but the individual arrays themselves.

@makortel can you confirm that the implementation that uses edm::propagate_const_array is now fine ?

can you confirm that the implementation that uses edm::propagate_const_array is now fine ?

Visually the use of edm::propagate_const_array looks good now, thanks.

makortel · 2020-11-06T16:54:47Z

CondFormats/HcalObjects/src/HcalPedestalsGPU.cc

+// FIXME: add proper getters to conditions
+HcalPedestalsGPU::HcalPedestalsGPU(HcalPedestals const& pedestals)
+    : unitIsADC_{pedestals.isADC()},
+      totalChannels_{pedestals.getAllContainers()[0].second.size() + pedestals.getAllContainers()[1].second.size()},


Let me make a note on a semi-random place. The HcalPedestals::getAllContainers() seems to have lot's of overhead (constructing vector<pair<string, vector<HcalPedestal>>> on each call). I wonder if this could be (eventually) improved, event if the price is paid only at IOV changes.

makortel · 2020-11-06T16:58:20Z

CondFormats/HcalObjects/interface/HcalCombinedRecordsGPU.h

+#include "CondFormats/DataRecord/interface/HcalPedestalWidthsRcd.h"
+#include "CondFormats/DataRecord/interface/HcalPedestalsRcd.h"
+#include "CondFormats/DataRecord/interface/HcalQIEDataRcd.h"
+#include "CondFormats/DataRecord/interface/HcalQIETypesRcd.h"


This file is the only one pulling in the dependence on CondFormats/DataRecord. I wonder if it would be better to place these Record definitions in other package to avoid that (e.g. in CondFormats/DataRecord).

fwyzard · 2020-11-13T15:41:26Z

please test

cmsbuild · 2020-11-13T15:41:47Z

The tests are being triggered in jenkins.

CMSSW_11_2_X_2020-11-12-2300/slc7_amd64_gcc820: https://cmssdt.cern.ch/jenkins/job/ib-run-pr-tests/10735/console Started: 2020/11/13 16:44

cmsbuild · 2020-11-13T17:38:36Z

+1
Tested at: b7e1199
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-5865d8/10735/summary.html
CMSSW: CMSSW_11_2_X_2020-11-12-2300
SCRAM_ARCH: slc7_amd64_gcc820

cmsbuild · 2020-11-13T17:38:38Z

Comparison job queued.

fwyzard · 2020-11-13T18:10:35Z

@yuanchao, @christopheralanwest, @tocheng, @tlampen, @ggovi, @pohsun any comments from your side ?

cmsbuild · 2020-11-13T18:53:58Z

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-5865d8/10735/summary.html

Comparison Summary:

No significant changes to the logs found
Reco comparison results: 0 differences found in the comparisons
DQMHistoTests: Total files compared: 35
DQMHistoTests: Total histograms compared: 2529296
DQMHistoTests: Total failures: 1
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 2529273
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 34 files compared)
Checked 148 log files, 22 edm output root files, 35 DQM output files

fwyzard · 2020-11-17T23:21:55Z

please test with #31720

cmsbuild · 2020-11-17T23:22:17Z

The tests are being triggered in jenkins.
Tested with other pull request(s) #31720

CMSSW_11_2_X_2020-11-17-2300/slc7_amd64_gcc820: https://cmssdt.cern.ch/jenkins/job/ib-run-pr-tests/10822/console Started: 2020/11/18 02:52

cmsbuild · 2020-11-26T13:34:21Z

The tests are being triggered in jenkins.
Tested with other pull request(s) #31720

CMSSW_11_2_X_2020-11-25-2300/slc7_amd64_gcc820: https://cmssdt.cern.ch/jenkins/job/ib-run-pr-tests/11069/console Started: 2020/11/26 14:35

cmsbuild · 2020-11-26T15:45:32Z

+1
Tested at: 041e3b9
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-6a9587/11069/summary.html
CMSSW: CMSSW_11_2_X_2020-11-25-2300
SCRAM_ARCH: slc7_amd64_gcc820

cmsbuild · 2020-11-26T15:45:37Z

Comparison job queued.

cmsbuild · 2020-11-26T17:33:09Z

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-6a9587/11069/summary.html

Comparison Summary:

No significant changes to the logs found
Reco comparison results: 4 differences found in the comparisons
DQMHistoTests: Total files compared: 36
DQMHistoTests: Total histograms compared: 2747014
DQMHistoTests: Total failures: 7
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 2746985
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 35 files compared)
Checked 153 log files, 34 edm output root files, 36 DQM output files

perrotta · 2020-11-30T13:29:29Z

@cms-sw/alca-l2 do you have any comment here? There are other PRs queued while waiting for this one...

christopheralanwest · 2020-11-30T16:55:38Z

How exactly will these conditions be used? Will they ever require new conditions to be added to the GTs or will they simply reformat the existing conditions in a way that is convenient for GPUs?

mariadalfonso · 2020-11-30T17:06:51Z

How exactly will these conditions be used? Will they ever require new conditions to be added to the GTs or will they simply reformat the existing conditions in a way that is convenient for GPUs?

no new conditions will be introduced.
It's just a reformat the existing conditions convenient for GPUs as you say.

christopheralanwest · 2020-12-01T20:46:46Z

+alca

ggovi · 2020-12-02T09:49:30Z

+1

cmsbuild · 2020-12-02T09:49:55Z

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

silviodonato · 2020-12-02T11:26:53Z

+1

cmsbuild added this to the CMSSW_11_2_X milestone Nov 5, 2020

cmsbuild added alca-pending code-checks-pending comparison-pending db-pending orp-pending pending-signatures tests-pending labels Nov 5, 2020

cmsbuild added code-checks-approved and removed code-checks-pending labels Nov 5, 2020

makortel reviewed Nov 6, 2020

View reviewed changes

cmsbuild mentioned this pull request Nov 6, 2020

Squash all Patatrack developments on top of CMSSW_11_3_0_pre5 #27983

Closed

fwyzard mentioned this pull request Nov 9, 2020

Patatrack integration - HCAL local reconstruction (8/N) #31720

Merged

cmsbuild added tests-started and removed tests-pending labels Nov 13, 2020

cmsbuild added tests-approved and removed tests-started labels Nov 13, 2020

cmsbuild added comparison-available and removed comparison-pending labels Nov 13, 2020

cmsbuild added requires-external tests-started and removed tests-pending labels Nov 26, 2020

cmsbuild added tests-approved and removed tests-started labels Nov 26, 2020

cmsbuild added comparison-available and removed comparison-pending labels Nov 26, 2020

cmsbuild added alca-approved and removed alca-pending labels Dec 1, 2020

cmsbuild added db-approved fully-signed and removed db-pending pending-signatures labels Dec 2, 2020

cmsbuild added orp-approved and removed orp-pending labels Dec 2, 2020

cmsbuild merged commit 719f135 into cms-sw:master Dec 2, 2020

This was referenced Dec 10, 2020

Patatrack integration - ECAL local reconstruction (7/N) #31719

Merged

Open issues regarding the ECAL local reconstruction on GPU #32480

Open

makortel mentioned this pull request Jan 25, 2021

Heterogeneous HGCAL RecHit Calibration #32683

Merged


		using HcalConvertedPedestalsRcd = HcalCombinedRecord<HcalPedestalsRcd, HcalQIEDataRcd, HcalQIETypesRcd>;

		using HcalConvertedEffectivePedestalsRcd = HcalCombinedRecord<HcalPedestalsRcd, HcalQIEDataRcd, HcalQIETypesRcd>;

Patatrack integration - Hcal conditions (13/N) #32039

Patatrack integration - Hcal conditions (13/N) #32039

Conversation

mariadalfonso commented Nov 5, 2020 • edited Loading

cmsbuild commented Nov 5, 2020

cmsbuild commented Nov 5, 2020

cmsbuild commented Nov 5, 2020

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

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

Choose a reason for hiding this comment

fwyzard Nov 18, 2020 • edited Loading

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 Nov 13, 2020

cmsbuild commented Nov 13, 2020 • edited Loading

cmsbuild commented Nov 13, 2020

cmsbuild commented Nov 13, 2020

fwyzard commented Nov 13, 2020

cmsbuild commented Nov 13, 2020

fwyzard commented Nov 17, 2020

cmsbuild commented Nov 17, 2020 • edited Loading

cmsbuild commented Nov 26, 2020 • edited Loading

cmsbuild commented Nov 26, 2020

cmsbuild commented Nov 26, 2020

cmsbuild commented Nov 26, 2020

perrotta commented Nov 30, 2020

christopheralanwest commented Nov 30, 2020

mariadalfonso commented Nov 30, 2020

christopheralanwest commented Dec 1, 2020

ggovi commented Dec 2, 2020

cmsbuild commented Dec 2, 2020

silviodonato commented Dec 2, 2020

mariadalfonso commented Nov 5, 2020 •

edited

Loading

fwyzard Nov 18, 2020 •

edited

Loading

cmsbuild commented Nov 13, 2020 •

edited

Loading

cmsbuild commented Nov 17, 2020 •

edited

Loading

cmsbuild commented Nov 26, 2020 •

edited

Loading