BS online for 11 2 x #30591

gennai · 2020-07-08T08:20:43Z

PR description:

In this PR I modify the transient beamspot record to depend also from the offline beamspot record and added a number of plugins for its use in the HLT menu for Run3.

PR validation:

I have made some checks running the RecoVertex/BeamSpotProducer/test/readOnlineBeamSpotFromDB.py
script

cmsbuild · 2020-07-08T08:21:09Z

The code-checks are being triggered in jenkins.

cmsbuild · 2020-07-08T08:25:49Z

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-30591/16842

This PR adds an extra 36KB to repository

Code check has found code style and quality issues which could be resolved by applying following patch(s)

code-format:
https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-30591/16842/code-format.patch
e.g. curl https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-30591/16842/code-format.patch | patch -p1
You can also run scram build code-format to apply code format directly

cmsbuild · 2020-07-08T08:31:52Z

The code-checks are being triggered in jenkins.

cmsbuild · 2020-07-08T08:36:04Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-30591/16843

This PR adds an extra 36KB to repository

cmsbuild · 2020-07-08T08:36:30Z

A new Pull Request was created by @gennai (simone gennai) for master.

It involves the following packages:

CondFormats/DataRecord
RecoVertex/BeamSpotProducer

@perrotta, @slava77, @christopheralanwest, @tocheng, @cmsbuild, @jpata, @tlampen, @ggovi, @pohsun can you please review it and eventually sign? Thanks.
@makortel, @GiacomoSguazzoni, @JanFSchulte, @tocheng, @VinInn, @ebrondol, @rovere, @mmusich, @mtosi, @dgulhan, @seemasharmafnal this is something you requested to watch as well.
@silviodonato, @dpiparo you are the release manager for this.

cms-bot commands are listed here

gennai · 2020-07-08T08:38:08Z

@Martin-Grunewald @Sam-Harper you may want to have a look at this as well

gennai · 2020-07-08T08:38:40Z

Adding also @francescobrivio

RecoVertex/BeamSpotProducer/plugins/OfflineToTransientBeamSpotESProducer.cc

RecoVertex/BeamSpotProducer/plugins/OnlineBeamSpotESProducer.cc

Martin-Grunewald · 2020-07-08T09:01:35Z

RecoVertex/BeamSpotProducer/plugins/OnlineBeamSpotFromDB.cc

+  //std::cout << iEvent.getRun().beginTime().value() << std::endl;
+  //std::cout << iEvent.time().value() << std::endl;
+  std::cout << *mybeamspot << std::endl;
+  std::cout << *myGTbeamspot << std::endl;


MessageLogger...

Martin-Grunewald · 2020-07-08T09:03:33Z

@gennai
All new plugins either need a fillDescriptions method (even if there are no parameters!), or a cfi file in ../python.
This is needed such that ConfDB parsing can discover the new plugins (via the fillDescriptions-generated or the explicit cfi file).

gennai · 2020-07-08T09:05:30Z

Hi @Martin-Grunewald , thanks for spotting the cout they clearly should not be there, and also for the reminder of the fill description method! I will work on it asap.

Martin-Grunewald · 2020-07-08T09:14:03Z

RecoVertex/BeamSpotProducer/interface/OnlineBeamSpotFromDB.h

+#include "FWCore/Framework/interface/Event.h"
+#include "FWCore/ParameterSet/interface/ParameterSet.h"
+
+class OnlineBeamSpotFromDB : public edm::EDAnalyzer {


global/one/stream EDAnalyzer... not a legacy one

makortel

I'd also suggest to move the contents of the EDAnalyzer and ESProducers headers to the corresponding source files because the separate header files are not needed.

makortel · 2020-07-08T13:29:46Z

RecoVertex/BeamSpotProducer/plugins/OfflineToTransientBeamSpotESProducer.cc

+}
+
+OfflineToTransientBeamSpotESProducer::~OfflineToTransientBeamSpotESProducer() {
+  delete transientBS_;


This would delete also the product read from BeamSpotObjectsRcd which would lead to double-delete.

shall I remove the delete then? When running I didn't notice any error or warning messages, though.

so matti, I couldnt follow this specific double delete. Is this because transientBS_ is returned as a shared pointer by the producer? But in this case, due to the deletation function, the shared pointer wont delete the object. Or something else?

The current version of the code is ok (no delete) so further discussion may be moot, but earlier in the produce() there was an assignment

transientBS_ = &rec.get(bsOfflineToken_);

and a later delete transientBS_ would delete an object returned by rec.get(), which is owned by the EventSetup system.

makortel · 2020-07-08T13:35:36Z

RecoVertex/BeamSpotProducer/plugins/OfflineToTransientBeamSpotESProducer.cc

+  if (iRecord.tryToGetRecord<BeamSpotObjectsRcd>()) {
+    host->ifRecordChanges<BeamSpotObjectsRcd>(
+        iRecord, [this, h = host.get()](auto const& rec) { transientBS_ = &rec.get(bsOfflineToken_); });
+  }


I don't follow why the ESProductHost would be needed. How about something along

// in class declaration const BeamSpotObjects dummyBS_; // in here auto optionalRec = iRecord.tryToGetRecord<BeamSpotObjectsRcd>(); if (not optionalRec) { return std::shared_ptr<const BeamSpotObjects>(&dummyBS_), edm::do_nothing_deleter()); } return std::shared_ptr<const BeamSpotObjects>(&optionalRec->get(bsOfflineToken_), edm::do_nothing_deleter());

makortel · 2020-07-08T13:52:17Z

RecoVertex/BeamSpotProducer/plugins/OnlineBeamSpotESProducer.cc

+
+OnlineBeamSpotESProducer::~OnlineBeamSpotESProducer() {
+  delete theHLTBS_;
+  delete theLegacyBS_;


These may delete the ESProduct leading to double-delete.

This comment on the double-delete still holds though.

makortel · 2020-07-08T13:58:51Z

RecoVertex/BeamSpotProducer/plugins/OnlineBeamSpotESProducer.cc

+    transientBS_->SetPosition(0, 0, 0);
+    transientBS_->SetBeamWidthY(0.1);
+    transientBS_->SetBeamWidthX(0.1);
+    transientBS_->SetSigmaZ(5.0);


Modifying an ESProducer member variable that is returned as an ESProduct does not work with concurrent IOVs. I'm wondering if this code as written is intended for production use?

Assuming "no", and that the intention is to return the better of "HLT" and "legacy", how about something along

auto legacyRec = iRecord.tryToGetRecord<BeamSpotOnlineLegacyObjectsRcd>(); auto hltRec = iRecord.tryToGetRecord<BeamSpotOnlineHLTObjectsRcd>(); if (not legacyRec and not hltRec) { return std::shared_ptr<const BeamSpotObjects>(&fakeBS_, edm::do_nothing_deleter()); // or copy in case 'const BeamSpotObjects fakeBS_' member would not be preferred return std::make_shared<BeamSpotObjects>(fakeBS_); } const BeamSpotOnlineObjects* best; if (legacyRec and hltRec) { best = compareBS(&legacyRec->get(bsLegacyToken_), &hltRec->get(bsHLTToken_)); } else if (legacyRec) { best = &legacyRec->get(bsLegacytoken_); } else { best = &hltRec->get(bsHLTToken_); } return std::shared_ptr<const BeamSpotObjects>(best, edm::do_nothing_deleter());

I'd still suggest the above. It would avoid the edm::ESProductHost and any allocations that currently may lead to memory leak and double-delete.

…ientObjectsRcd.h

…ions

…ientObjectsRcd.h

cmsbuild · 2020-07-29T12:37:26Z

The code-checks are being triggered in jenkins.

gennai · 2020-07-29T12:37:42Z

Let me close this PR and open a cleaner one as I messed up quite heavily.

cmsbuild · 2020-07-29T12:45:46Z

-code-checks

ERROR: Build errors found during clang-tidy run.

CondFormats/DataRecord/interface/BeamSpotTransientObjectsRcd.h:30:1: error: version control conflict marker in file [clang-diagnostic-error]
<<<<<<< HEAD
^
Suppressed 1051 warnings (1050 in non-user code, 1 with check filters).
--
CondFormats/DataRecord/interface/BeamSpotTransientObjectsRcd.h:30:1: error: version control conflict marker in file [clang-diagnostic-error]
<<<<<<< HEAD
^
Suppressed 1124 warnings (1123 in non-user code, 1 with check filters).
--
CondFormats/DataRecord/interface/BeamSpotTransientObjectsRcd.h:30:1: error: version control conflict marker in file [clang-diagnostic-error]
<<<<<<< HEAD
^
Suppressed 1459 warnings (1458 in non-user code, 1 with check filters).
--
gmake: *** [config/SCRAM/GMake/Makefile.coderules:128: code-checks] Error 2
gmake: *** [There are compilation/build errors. Please see the detail log above.] Error 2

cmsbuild added this to the CMSSW_11_2_X milestone Jul 8, 2020

cmsbuild added alca-pending code-checks-pending comparison-pending db-pending orp-pending pending-signatures reconstruction-pending tests-pending labels Jul 8, 2020

cmsbuild added code-checks-rejected code-checks-pending and removed code-checks-pending code-checks-rejected labels Jul 8, 2020

cmsbuild added code-checks-approved and removed code-checks-pending labels Jul 8, 2020

Martin-Grunewald reviewed Jul 8, 2020

View reviewed changes

RecoVertex/BeamSpotProducer/plugins/OfflineToTransientBeamSpotESProducer.cc Outdated Show resolved Hide resolved

Martin-Grunewald reviewed Jul 8, 2020

View reviewed changes

RecoVertex/BeamSpotProducer/plugins/OnlineBeamSpotESProducer.cc Outdated Show resolved Hide resolved

Martin-Grunewald reviewed Jul 8, 2020

View reviewed changes

makortel reviewed Jul 8, 2020

View reviewed changes

cmsbuild mentioned this pull request Jul 14, 2020

Clean BuildFiles in and after Reco area #30679

Merged

gennai added 19 commits July 29, 2020 14:13

applied patch

ca19b47

applying code style to CondFormats/DataRecord/interface/BeamSpotTrans…

447141a

…ientObjectsRcd.h

applying suggestions from Martin, Sam and Matti

5ee4f4f

modified the OfflineToTransient ESProducer following Matti's suggest…

ade1e16

…ions

applied code style patch

17bc48e

resolved conflict

1edf14b

applied suggestion from Matti

4fa16a0

applied the code-checks patch

4e02869

removign useless helpfunction

ec160c2

added code-checks fix

d42ac62

moved to mpl vectors

56b285a

added offline beamspot record dependency to the transient one

7cb3150

added a number of plugins for onlineBeamSpot objects read/write

a021d39

added a number of plugins for onlineBeamSpots

d12caf1

reverted the transient class

3f3f94a

applied patch

8992320

applying code style to CondFormats/DataRecord/interface/BeamSpotTrans…

be5e2e5

…ientObjectsRcd.h

trying to rebase

c5d2573

last attempt

799ccc4

cmsbuild added code-checks-pending hlt-pending and removed code-checks-approved hlt-rejected labels Jul 29, 2020

gennai closed this Jul 29, 2020

cmsbuild added code-checks-rejected and removed code-checks-pending labels Jul 29, 2020

gennai mentioned this pull request Jul 29, 2020

Online BeamSpot ESProducer for Run3 workflow #30965

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BS online for 11 2 x #30591

BS online for 11 2 x #30591

gennai commented Jul 8, 2020

cmsbuild commented Jul 8, 2020

cmsbuild commented Jul 8, 2020

cmsbuild commented Jul 8, 2020

cmsbuild commented Jul 8, 2020

cmsbuild commented Jul 8, 2020

gennai commented Jul 8, 2020

gennai commented Jul 8, 2020

Martin-Grunewald Jul 8, 2020

Martin-Grunewald commented Jul 8, 2020

gennai commented Jul 8, 2020

Martin-Grunewald Jul 8, 2020

makortel left a comment

makortel Jul 8, 2020

gennai Jul 28, 2020

Sam-Harper Jul 28, 2020

makortel Jul 28, 2020

makortel Jul 8, 2020

makortel Jul 8, 2020

makortel Jul 28, 2020

makortel Jul 8, 2020

makortel Jul 28, 2020

cmsbuild commented Jul 29, 2020

gennai commented Jul 29, 2020

cmsbuild commented Jul 29, 2020

BS online for 11 2 x #30591

BS online for 11 2 x #30591

Conversation

gennai commented Jul 8, 2020

PR description:

PR validation:

cmsbuild commented Jul 8, 2020

cmsbuild commented Jul 8, 2020

cmsbuild commented Jul 8, 2020

cmsbuild commented Jul 8, 2020

cmsbuild commented Jul 8, 2020

gennai commented Jul 8, 2020

gennai commented Jul 8, 2020

Choose a reason for hiding this comment

Martin-Grunewald commented Jul 8, 2020

gennai commented Jul 8, 2020

Choose a reason for hiding this comment

makortel 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

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

cmsbuild commented Jul 29, 2020

gennai commented Jul 29, 2020

cmsbuild commented Jul 29, 2020