-
Notifications
You must be signed in to change notification settings - Fork 4.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Produce a fake BeamSpot
object in BeamSpotOnlineProducer
when using transient record logic and OnlineBeamSpotESProducer
returned a fake
#41597
Produce a fake BeamSpot
object in BeamSpotOnlineProducer
when using transient record logic and OnlineBeamSpotESProducer
returned a fake
#41597
Conversation
…by letting the BeamSpotOnlineProducer deal automatically with the arbitrated one
…FromOfflineConverter
…accept the online beamspot objects stored in DB even if they are older than 48h
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-41597/35488
|
A new Pull Request was created by @mmusich (Marco Musich) for master. It involves the following packages:
@Martin-Grunewald, @clacaputo, @cmsbuild, @missirol, @saumyaphor4252, @tvami, @mandrenguyen, @francescobrivio can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
@cmsbuild, please test |
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-1c9357/32504/summary.html Comparison SummarySummary:
|
+1
|
I'm afraid I need a clarification on the rationale behind this, and how it can impact HLT online in Run 3. I asked @francescobrivio offline, and I understood that the main use case to favour the fake BS is to use that at HLT (instead of the BS from PCL) after a technical stop, in case the BS position has changed. This is basically the case 'both online payloads are too old', as far as I understand. In the ESProducer, the requirement for using one of the online tags is that the payload is recent enough, and also that the corresponding BS is "good" ( I'm wondering what happens if, in the middle of a run, both DQM clients return a "bad" BS for a given LS: in that case, with this PR, would HLT switch to a fake BS until one of the clients returns a "good" BS again ? |
yes, excepted that I am not sure this has ever happened in practice (I should add that depending on how old is the last PCL update, there is no guarantee that using that as opposed to the fake one is favored). |
I also don't rember ever seeing this happen so far.
I agree. @missirol I guess your point here (IIUC) is if we should change the I am not sure if this is possible/advisable.... @mmusich @gennai what do you think? |
Case in point I am not sure how you could instruct the framework to read an IoV different than the one used for processing the data. I guess you could add a 3rd online tag that stores the values of the already arbitrated beamspot with a latency with respect to the LS processed, but this looks a complication over an already fragile mechanism. |
The fwk implementation was indeed my main concern. I agree with you Marco! |
IF we want to differentiate the cases in which the transient beamspot is too old from the one in which it is not good (and fall back to DB in that case), I guess we could propagate forward the info from the ESProducer by means of the beamspot type,e.g. use unknown (-1) instead of fake (0). Let me know if you prefer that. |
But no actual differentiation of treatment is possible right? So the propagating forward of the BS type would be only for the logging. In that case I think it can be postponed to a subsequent PR if deemed necessary in the future. |
It is possible, just as we check here for beam types !=2
we can treat the case 0 and the case -1 differently.
but we can speciate (of course the logic becomes more complicated) |
After private discussion with Marco we concluded that, for the moment, this PR is the best way to proceed forward. Further discussion is needed in case we want to modify the logic and can be left to a following development. @cms-sw/reconstruction-l2 a kind ping for your review and signature when possible |
+reconstruction |
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, @rappoccio (and backports should be raised in the release meeting by the corresponding L2) |
+1 |
PR description:
While working on #41193, I have noticed that despite
OnlineBeamSpotESProducer
has a refined logic in order to put into the event setup a fake BeamSpot when certain conditions are not met:cmssw/RecoVertex/BeamSpotProducer/plugins/OnlineBeamSpotESProducer.cc
Lines 168 to 174 in a52f2d1
the current
BeamSpotOnlineProducer
logic, ifuseTransientRecord_
is true and theOnlineBeamSpotESProducer
returned a fake beamspot, just falls back to DB (reading the content ofBeamSpotObjectsRcd
, populated by the PCL in realtime workflows) - which I have understood from the Beam Spot experts (@gennai) is not the right / expected behaviour.cmssw/RecoVertex/BeamSpotProducer/plugins/BeamSpotOnlineProducer.cc
Lines 104 to 111 in a52f2d1
In this PR (in commit ec786fe) I change the current behaviour, such that instead now it will indeed produce a fake BS for consumption online (as it was originally devised).
This has a consequence, that in the case in which we want indeed to fall-back to DB (e.g. for the Phase-2 HLT testing case, after #41193), the change will alter physics in an undesired way. For this reason, it has been necessary to supply to the
auto:phase2_realistic
GlobalTag key the rightBeamSpotOnline*ObjectsRcd
payloads in order for theOnlineBeamSpotESProducer
to not pick up a fake BeamSpot. This is done in commit 5fbea8a, while commit 30121dd sets the the time threshold to1e6
seconds (as it's done for the Run-3 menus, see https://github.com/cms-sw/cmssw/pull/41193/files#r1152358315) to the same effect.The difference in Global Tags is:
To supply such payloads, I have found convenient to create a new plugin
BeamSpotOnlineFromOfflineConverter
that taking aBeamSpotObject
in input creates an sqlite file in output containing a payload of the typeBeamSpotOnlineObjects
. This has been added in commit e870b17, while commits a03a99d and a03a99d add a utility accessor to theBeamSpotOnlineObjects
condtion format to be able to readily copy from aBeamSpotObjects
payload (friend class) all the common parameters. Finally commit ecd817c adds this converter to the battery of unit tests of theCondTools/BeamSpot
package.PR validation:
cmssw
complies. Passes back unit tests.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:
To be backported down to 13.0.X for 2023 data-taking purposes
@francescobrivio @dzuolo