move online beam spot arbitration to the edproducer #47378

lguzzi · 2025-02-17T17:37:27Z

PR description:

This PR moves the online beam spot arbitration between the two DQM clients (legacy and hlt) to the EDProducer (RecoVertex/plugin/BeamSpotOnlineProducer.cc), removing the need for the ESProducer (RecoVertex/plugin/OnlineBeamSpotESProducer.cc). These modifications were asked in #39840 to allow selecting the proper event timestamp instead of the wall-clock time during the record selection.
The main modification is in RecoVertex/plugin/BeamSpotOnlineProducer.cc:

now reading the two DQM records (BeamSpotOnlineLegacyObjectsRcd and BeamSpotOnlineHLTObjectsRcd) instead of the transient record.
the useTransientRecord argument is renamed useBSOnlineRecords.
three arguments are added to the producer and used in the arbitration.
- sigmaZThreshold (default 2 cm) is used to accept or reject a single record based on the beam spot sigmaZ (must be higher than threshold to be accepted). This selection was done before in the ESProducer.
- sigmaXYThreshold (default 4 mum) is used to accept or reject a single record based on the beam spot sigmaX and sigmaY (must be higher than threshold to be accepted). This selection was done before in the ESProducer.
- timeThreshold (default 48 h) is is used to accept or reject a single record based on the elapsed time between the event and the record creation. This selection was done similarly in the ESProducer but is now using the event proper timestamp instead of the job wall-clock time.

The logic for the record arbitration is unchanged.
The ESProducer is also removed from the phase2 simplified HLT menu and the EDProducer is set accordingly.
This only affects online operations (running with useBSOnlineRecords=cms.bool(True)).

The OnlineBeamMonitor.cc has also been updated to fetch the information saved in the HLT collection instead of the transient record. This is done in the first event of each lumisection.

PR validation:

I validated this PR using the suggested recipe:

cd src
scram b distclean 
git cms-checkdeps -a -A
scram b -j 8
scram b runtests use-ibeos

gives no failures.

runTheMatrix.py -l limited -i all --ibeos

~~fails on 4 workflows for unrelated problems (input files not available on any site) [*].~~ gives no failures.

addOnTests.py

~~fails 14 times due to unavailable files [**].~~ gives no failures.

I also checked that re-running the HLT writes in the hltOnlineBeamSpot collection the same information as before (running on /HLTMonitor/Run2024I-Express-v2/FEVTHLTALL run=386852, see here).

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:

need a backport to 15_0_X

[*] https://lguzzi.web.cern.ch/lguzzi/BeamSpot/EDP_migration/test_runthematrix/
[**] https://lguzzi.web.cern.ch/lguzzi/BeamSpot/EDP_migration/addOnTests/

FYI @missirol @mtosi @rovere @VourMa @gennai @francescobrivio

cmsbuild · 2025-02-17T17:37:56Z

cms-bot internal usage

cmsbuild · 2025-02-17T17:39:38Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-47378/43735

There are other open Pull requests which might conflict with changes you have proposed:
- File DQM/Integration/python/clients/beam_dqm_sourceclient-live_cfg.py modified in PR(s): Remove the configuration of the legacy CUDA workflows #46076
- File DQM/Integration/python/clients/pixel_dqm_sourceclient-live_cfg.py modified in PR(s): Remove the configuration of the legacy CUDA workflows #46076
- File DQM/Integration/python/clients/sistrip_approx_dqm_sourceclient-live_cfg.py modified in PR(s): Remove the configuration of the legacy CUDA workflows #46076
- File DQM/Integration/python/clients/sistrip_dqm_sourceclient-live_cfg.py modified in PR(s): Remove the configuration of the legacy CUDA workflows #46076
- File HLTrigger/Configuration/python/customizeHLTforCMSSW.py modified in PR(s): CA Extension to strips #47090

cmsbuild · 2025-02-17T17:39:43Z

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

It involves the following packages:

Alignment/CommonAlignmentProducer (alca)
Configuration/DataProcessing (operations)
Configuration/StandardSequences (operations)
DQM/BeamMonitor (dqm, db)
DQM/Integration (dqm)
HLTrigger/Configuration (hlt)
RecoVertex/BeamSpotProducer (reconstruction, alca)

@Martin-Grunewald, @antoniovagnerini, @antoniovilela, @atpathak, @cmsbuild, @consuegs, @davidlange6, @fabiocos, @francescobrivio, @jfernan2, @mandrenguyen, @mmusich, @perrotta, @rappoccio, @rseidita can you please review it and eventually sign? Thanks.
@GiacomoSguazzoni, @JanChyczynski, @Martin-Grunewald, @PonIlya, @SohamBhattacharya, @VinInn, @VourMa, @adewit, @batinkov, @dgulhan, @fabiocos, @felicepantaleo, @francescobrivio, @makortel, @martinamalberti, @missirol, @mmusich, @mtosi, @pakhotin, @rovere, @rsreds, @sameasy, @slomeo, @threus, @tlampen, @tocheng, @yuanchao 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

missirol

Thanks for tackling this issue. I left a couple of comments as a non-expert.

missirol · 2025-02-17T17:56:42Z

RecoVertex/BeamSpotProducer/plugins/BeamSpotOnlineProducer.cc

@@ -12,7 +12,10 @@ ________________________________________________________________**/

 #include "CondFormats/BeamSpotObjects/interface/BeamSpotObjects.h"
 #include "CondFormats/DataRecord/interface/BeamSpotObjectsRcd.h"
+#include "CondFormats/BeamSpotObjects/interface/BeamSpotOnlineObjects.h"
 #include "CondFormats/DataRecord/interface/BeamSpotTransientObjectsRcd.h"


Should this (and any refs to "transient") be removed from this file ?

Actually, does the BeamSpotTransientObjectsRcd still serve any purpose after this PR?

well at DQM we are still using the ESProducer for the monitoring plots (we could in principle change that code to take the EDProducer actually). In fact, now that I think about it, we should also check the code that is run at HLT for the monitorElements.

well at DQM we are still using the ESProducer for the monitoring plots

which DQM? this PR seems to have removed it from the clients:

https://github.com/cms-sw/cmssw/pull/47378/files#diff-72385d5afa9e19f7453f2bbed300776e4d1221c10f97af51889bc87a01f02e8cL89

this line has been removed

missirol · 2025-02-17T18:04:15Z

DQM/Integration/python/clients/onlinebeammonitor_dqm_sourceclient-live_cfg.py

@@ -114,10 +114,6 @@
 # process.dqmSaverPB.tag         = 'OnlineBeamMonitor'


(The comment is not on this line specifically, but it applies to this file.)

This DQM config and the HLT menu used online both include an instance of the plugin OnlineBeamMonitor. Does the latter need any updates in light of this PR ?

I think so.
Who produces this product now

cmssw/DQM/BeamMonitor/plugins/OnlineBeamMonitor.cc

Line 279 in 365705a

if (auto bsTransientHandle = iSetup.getHandle(bsTransientToken_)) {

?

So , as I was writing before, both at HLT and in DQM that module is run. In DQM the configuration may still have the ESProducer, at HLT if nobody has changed the menu in confDB the ESProducer is still present.
Of course it would be better to modify the code of the OnlineBeamMonitor.cc to access the beam spot through the EDProducer.

at HLT if nobody has changed the menu in confDB the ESProducer is still present.

but this assumption is incorrect. The customization function here (proposed in this very PR) removes it:

cmssw/HLTrigger/Configuration/python/customizeHLTforCMSSW.py

Lines 161 to 162 in fa5f00f

for esprod in list(esproducers_by_type(process, "OnlineBeamSpotESProducer")):

delattr(process, esprod.label())

I didn't actually think about the DQM monitoring plots. I can leave it in the menu if it is needed.

I can leave it in the menu if it is needed.

I'd rather remove it and change the monitoring module, such that we monitor the same mechanism we use when running the HLT.

the online beam monitor has been updated to fetch information from the hlt collection instead of the transient record. This is done at the first event of each lumisection.

missirol · 2025-02-17T18:14:27Z

RecoVertex/BeamSpotProducer/plugins/BeamSpotOnlineProducer.cc

-  if (useTransientRecord_) {
-    fallBackToDB = processTransientRecord(iSetup, *result, shoutMODE);
+  if (useBSOnlineRecords_) {
+    fallBackToDB = processRecords(iEvent, iSetup, *result, shoutMODE);


Should the arbitration be done 1 time per lumisection, instead of 1 time per event ? (or, have you observed that the latter is not significantly slower than the former ?)

If desired, the arbitration could also be done only when either BeamSpotOnlineLegacyObjectsRcd or BeamSpotOnlineHLTObjectsRcd (or both) have transitioned to their next IOV.

In principle the checks need to be repeated for each lumisection, because the records may become stale (older than timeThreshold hours) at any moment.

Note that I don't really like that we cannot do these checks in an ESProducer, and we have to implement them in an EDProducer.

the arbitration has been moved to beginLuminosityBlock.

mmusich · 2025-02-17T18:29:45Z

fails 14 times due to unavailable files [**].

@lguzzi try doing this:

export CMS_PATH="/cvmfs/cms-ib.cern.ch"
export SITECONFIG_PATH="/cvmfs/cms-ib.cern.ch/SITECONF/local"

before executing the addOnTests.

makortel · 2025-02-17T18:49:14Z

RecoVertex/BeamSpotProducer/plugins/BeamSpotOnlineProducer.cc

+  auto const spotDBLegacy = getBeamSpotFromRecord(beamTokenLegacy_, iEvent, iSetup);
+  auto const spotDBHLT = getBeamSpotFromRecord(beamTokenHLT_, iEvent, iSetup);


Are the BeamSpotOnlineObjects objects copied (as opposed to passed by const reference) on purpose?

well, I thought there were no difference. I will change it to const&

makortel · 2025-02-17T18:54:16Z

RecoVertex/BeamSpotProducer/plugins/BeamSpotOnlineProducer.cc

-  if (useTransientRecord_) {
-    fallBackToDB = processTransientRecord(iSetup, *result, shoutMODE);
+  if (useBSOnlineRecords_) {
+    fallBackToDB = processRecords(iEvent, iSetup, *result, shoutMODE);


If desired, the arbitration could also be done only when either BeamSpotOnlineLegacyObjectsRcd or BeamSpotOnlineHLTObjectsRcd (or both) have transitioned to their next IOV.

mmusich · 2025-02-17T19:45:07Z

test parameters:

workflow = 1002.5

lguzzi · 2025-02-17T21:21:35Z

fails 14 times due to unavailable files [**].

@lguzzi try doing this:
export CMS_PATH="/cvmfs/cms-ib.cern.ch"
export SITECONFIG_PATH="/cvmfs/cms-ib.cern.ch/SITECONF/local"
before executing the addOnTests.

thanks, all tests succeed now

fwyzard · 2025-02-18T06:22:47Z

RecoVertex/BeamSpotProducer/plugins/BeamSpotOnlineProducer.cc

  return false;  // No fallback needed
 }

-void BeamSpotOnlineProducer::createBeamSpotFromTransient(const BeamSpotObjects& spotDB, reco::BeamSpot& result) const {
+template <typename TokenType>
+const BeamSpotOnlineObjects BeamSpotOnlineProducer::getBeamSpotFromRecord(const TokenType& token,


Suggested change

const BeamSpotOnlineObjects BeamSpotOnlineProducer::getBeamSpotFromRecord(const TokenType& token,

const BeamSpotOnlineObjects& BeamSpotOnlineProducer::getBeamSpotFromRecord(const TokenType& token,

?

I will add this in the next commit. It also requires modifications in the function body

mmusich · 2025-02-18T08:09:15Z

RecoVertex/BeamSpotProducer/plugins/BeamSpotOnlineProducer.cc

+  const auto& bs = setup.getData(token);
+  if (bs.sigmaZ() < sigmaZThreshold_ || bs.beamWidthX() < sigmaXYThreshold_ || bs.beamWidthY() < sigmaXYThreshold_ ||
+      bs.beamType() != reco::BeamSpot::Tracker) {
+    return fakeBS_;


in the OnlineBeamSpotESProducer we instrumented all the cases in which a fake BS is returned because of #45555, e.g. here:

cmssw/RecoVertex/BeamSpotProducer/plugins/OnlineBeamSpotESProducer.cc

Lines 166 to 168 in b8776f5

if (not legacyRec and not hltRec) {

edm::LogWarning("OnlineBeamSpotESProducer")

<< "None of the Beam Spots in ES are available! \n returning a fake one (fallback to PCL).";

or here

cmssw/RecoVertex/BeamSpotProducer/plugins/OnlineBeamSpotESProducer.cc

Lines 148 to 149 in b8776f5

edm::LogWarning("OnlineBeamSpotESProducer") << "Defaulting to fake (fallback to PCL) because the only payload "

"is either too old or does not pass the fit sanity checks";

please do the same.

I added back the warnings when the record does not pass the sanity check or the time threshold, I do not know however how to replicate tryToGetRecord

mmusich · 2025-02-18T08:26:05Z

RecoVertex/BeamSpotProducer/plugins/BeamSpotOnlineProducer.cc

-  if (spotDB.beamType() != 2) {
-    if (shoutMODE && beamTransientRcdESWatcher_.check(iSetup)) {
+  if (spotDB.beamType() != reco::BeamSpot::Tracker) {
+    if (shoutMODE && (beamLegacyRcdESWatcher_.check(iSetup) || beamHLTRcdESWatcher_.check(iSetup))) {
      edm::LogWarning("BeamSpotOnlineProducer")
          << "Online Beam Spot producer falls back to DB value due to fake beamspot in transient record.";


this warning now is outdated.

mmusich · 2025-02-18T08:28:52Z

RecoVertex/BeamSpotProducer/plugins/BeamSpotOnlineProducer.cc

+  fakeBS_.setCovariance(3, 3, 0.002);
+  fakeBS_.setCovariance(4, 4, 5e-11);
+  fakeBS_.setCovariance(5, 5, 5e-11);
+  fakeBS_.setCovariance(6, 6, 1e-09);


does setting all the data members really matter? IIUC the logic we never actually return a fake beamspot, so the only important point is to set the type, no?

Alignment/CommonAlignmentProducer/python/ALCARECOPromptCalibProdSiPixelAliHLT_cff.py

cmsbuild · 2025-02-18T16:21:30Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-47378/43764

There are other open Pull requests which might conflict with changes you have proposed:
- File DQM/Integration/python/clients/beam_dqm_sourceclient-live_cfg.py modified in PR(s): Remove the configuration of the legacy CUDA workflows #46076
- File DQM/Integration/python/clients/pixel_dqm_sourceclient-live_cfg.py modified in PR(s): Remove the configuration of the legacy CUDA workflows #46076
- File DQM/Integration/python/clients/sistrip_approx_dqm_sourceclient-live_cfg.py modified in PR(s): Remove the configuration of the legacy CUDA workflows #46076
- File DQM/Integration/python/clients/sistrip_dqm_sourceclient-live_cfg.py modified in PR(s): Remove the configuration of the legacy CUDA workflows #46076
- File HLTrigger/Configuration/python/HLT_75e33_cff.py modified in PR(s): Remove unused L1T paths from the past from HLT Upgrade Menu. #47379
- File HLTrigger/Configuration/python/HLT_75e33_timing_cff.py modified in PR(s): Remove unused L1T paths from the past from HLT Upgrade Menu. #47379
- File HLTrigger/Configuration/python/customizeHLTforCMSSW.py modified in PR(s): CA Extension to strips #47090

cmsbuild · 2025-03-05T14:45:43Z

Pull request #47378 was updated. @antoniovagnerini, @antoniovilela, @atpathak, @cmsbuild, @consuegs, @davidlange6, @fabiocos, @jfernan2, @mandrenguyen, @perrotta, @rappoccio, @rseidita can you please check and sign again.

mmusich · 2025-03-05T14:49:24Z

@cmsbuild, please test

cmsbuild · 2025-03-05T17:00:56Z

+1

Size: This PR adds an extra 24KB to repository
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-8b48bb/44821/summary.html
COMMIT: 8a2fae0
CMSSW: CMSSW_15_1_X_2025-03-05-1100/el8_amd64_gcc12
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/47378/44821/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

You potentially removed 44 lines from the logs
Reco comparison results: 587 differences found in the comparisons
DQMHistoTests: Total files compared: 52
DQMHistoTests: Total histograms compared: 4148451
DQMHistoTests: Total failures: 2755
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 4145676
DQMHistoTests: Total skipped: 20
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 51 files compared)
Checked 227 log files, 194 edm output root files, 52 DQM output files
TriggerResults: no differences found

perrotta · 2025-03-05T22:15:11Z

+alca

francescobrivio · 2025-03-06T08:00:30Z

@cms-sw/dqm-l2 @cms-sw/reconstruction-l2 a kind reminder to please review/sign this PR.

jfernan2 · 2025-03-06T15:36:07Z

+1

antoniovagnerini · 2025-03-07T15:50:47Z

+dqm

spurious differences seen from unstable WF 12834.7 (open issue spurious differences in outputs of Run-3 wfs *.7 #39803 )

cmsbuild · 2025-03-07T15:51:08Z

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

mandrenguyen · 2025-03-07T19:57:58Z

+1

cmsbuild added this to the CMSSW_15_1_X milestone Feb 17, 2025

cmsbuild added reconstruction-pending dqm-pending hlt-pending alca-pending db-pending operations-pending pending-signatures tests-pending orp-pending code-checks-pending tracking labels Feb 17, 2025

lguzzi force-pushed the bsEDP branch from d0e1a9d to fa5f00f Compare February 17, 2025 17:38

cmsbuild added code-checks-approved and removed code-checks-pending labels Feb 17, 2025

missirol reviewed Feb 17, 2025

View reviewed changes

makortel reviewed Feb 17, 2025

View reviewed changes

cmsbuild mentioned this pull request Feb 17, 2025

Remove unused L1T paths from the past from HLT Upgrade Menu. #47379

Merged

fwyzard reviewed Feb 18, 2025

View reviewed changes

mmusich reviewed Feb 18, 2025

View reviewed changes

cmsbuild added code-checks-pending and removed code-checks-approved labels Feb 18, 2025

cmsbuild removed the code-checks-pending label Feb 18, 2025

cmsbuild added tests-started and removed tests-pending labels Mar 5, 2025

cmsbuild added operations-approved tests-approved and removed operations-pending tests-started labels Mar 5, 2025

cmsbuild added alca-approved and removed alca-pending labels Mar 5, 2025

cmsbuild added reconstruction-approved and removed reconstruction-pending labels Mar 6, 2025

cmsbuild added dqm-approved fully-signed and removed dqm-pending pending-signatures labels Mar 7, 2025

lguzzi mentioned this pull request Mar 7, 2025

[15_0_X] move online beam spot arbitration to the edproducer #47532

Merged

cmsbuild added orp-approved and removed orp-pending labels Mar 7, 2025

cmsbuild merged commit e8f14aa into cms-sw:master Mar 7, 2025
12 checks passed

This was referenced Mar 12, 2025

Integrating Particle Flow Network (PFN) Models into TICLv5 #47550

Merged

remove hltOnlineBeamSpotESProducer_cfi from NGT scouting menu #47569

Merged

mmasciov mentioned this pull request Mar 21, 2025

move logic of HLT-Beamspot choice from ESProducer to EDProducer #39840

Open

		@@ -114,10 +114,6 @@
		# process.dqmSaverPB.tag = 'OnlineBeamMonitor'

	for esprod in list(esproducers_by_type(process, "OnlineBeamSpotESProducer")):
	delattr(process, esprod.label())

		auto const spotDBLegacy = getBeamSpotFromRecord(beamTokenLegacy_, iEvent, iSetup);
		auto const spotDBHLT = getBeamSpotFromRecord(beamTokenHLT_, iEvent, iSetup);

	const BeamSpotOnlineObjects BeamSpotOnlineProducer::getBeamSpotFromRecord(const TokenType& token,
	const BeamSpotOnlineObjects& BeamSpotOnlineProducer::getBeamSpotFromRecord(const TokenType& token,

	if (not legacyRec and not hltRec) {
	edm::LogWarning("OnlineBeamSpotESProducer")
	<< "None of the Beam Spots in ES are available! \n returning a fake one (fallback to PCL).";

	edm::LogWarning("OnlineBeamSpotESProducer") << "Defaulting to fake (fallback to PCL) because the only payload "
	"is either too old or does not pass the fit sanity checks";

move online beam spot arbitration to the edproducer #47378

move online beam spot arbitration to the edproducer #47378

Conversation

lguzzi commented Feb 17, 2025 • edited Loading

PR description:

PR validation:

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:

cmsbuild commented Feb 17, 2025 • edited Loading

cmsbuild commented Feb 17, 2025

cmsbuild commented Feb 17, 2025

missirol 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

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

mmusich commented Feb 17, 2025

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

mmusich commented Feb 17, 2025

lguzzi commented Feb 17, 2025

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 Feb 18, 2025

cmsbuild commented Mar 5, 2025

mmusich commented Mar 5, 2025

cmsbuild commented Mar 5, 2025

Comparison Summary

perrotta commented Mar 5, 2025

francescobrivio commented Mar 6, 2025

jfernan2 commented Mar 6, 2025

antoniovagnerini commented Mar 7, 2025

cmsbuild commented Mar 7, 2025

mandrenguyen commented Mar 7, 2025

lguzzi commented Feb 17, 2025 •

edited

Loading

cmsbuild commented Feb 17, 2025 •

edited

Loading