Skip to content
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.

Sign up for GitHub

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

add onlineBeamSpotESProducer to BeamSpot_cfi: fix general Online BS swap case #35639

Merged
merged 2 commits into from
Oct 15, 2021
Merged

add onlineBeamSpotESProducer to BeamSpot_cfi: fix general Online BS swap case #35639

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9870cee
add onlineBeamSpotESProducer to BeamSpot_cfi: fix general Online BS s…
mmusich Oct 13, 2021
d6091aa
load the onlineBeamSpotESProducer only if the modifier is called
mmusich Oct 15, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions RecoVertex/BeamSpotProducer/python/BeamSpot_cfi.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,11 @@

offlineBeamSpot = cms.EDProducer("BeamSpotProducer")

import RecoVertex.BeamSpotProducer.onlineBeamSpotESProducer_cfi as _mod
BeamSpotESProducer = _mod.onlineBeamSpotESProducer.clone(
timeThreshold = 999999 # for express allow >48h old payloads for replays. DO NOT CHANGE
Copy link
Contributor

@francescobrivio francescobrivio Oct 13, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this mean that wherever this module is imported it gets this timeThreshold? I don't think this is the intended use for the HLT case, right @gennai ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't HLT fully dumped anyway?

)

import RecoVertex.BeamSpotProducer.BeamSpotOnline_cfi
_onlineBeamSpotProducer = RecoVertex.BeamSpotProducer.BeamSpotOnline_cfi.onlineBeamSpotProducer.clone()
mods.offlineToOnlineBeamSpotSwap.toReplaceWith(offlineBeamSpot, _onlineBeamSpotProducer)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

both visualization and express get their swap to online BS via this modifier:

  • Configuration/DataProcessing/python/Impl/pp.py self.visModifiers = modifyExpress
    • Configuration/DataProcessing/python/Modifiers.py modifyExpress = cms.ModifierChain(modifyPPData, _modsBS.offlineToOnlineBeamSpotSwap)
      • RecoVertex/BeamSpotProducer/python/Modifiers.py offlineToOnlineBeamSpotSwap = cms.Modifier()

I think that BeamSpotESProducer should be inserted in a consistent way. This would avoid the concerns about the HLT or other setups.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please make a concrete suggestion. By the way I think concerns on the HLT have been ruled out.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the alternative is to abandon this modifier-based modification and switch back to the RecoTLR.customiseExpress via visCustoms and expressCustoms

Copy link
Contributor

@francescobrivio francescobrivio Oct 14, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @slava77 sorry about my maybe naive question but I was under the (apparently wrong) impression that this

cmssw/Configuration/DataProcessing/python/RecoTLR.py

Lines 130 to 133 in 7d0e4f1

def customiseExpress(process):
process= customisePPData(process)
process = _swapOfflineBSwithOnline(process)
return process

was taking care of cutomizing the express reconstruction.
But I now learned that this is not the case and the customiseExpress is actually only used in relvals since #14350 (my ignorance for not knowing this). Are you now suggesting to revert that old PR? Or is there another smart way of implementing it?

At this point we most probably need another release in any case because without this PR the express reco will not work IIUC (just like it did not work in the DQM visualization clients) (FYI @perrotta @qliphy).
So if you have a suggestion on how to improve this PR it's very urgent!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

makeProcessModifier can be used to wrap a modifying function.
I was mostly concerned that, as I understood, onlineBeamSpotESProducer.clone( timeThreshold = 999999 is in general not consistent with a default instance and if we can avoid loading this ES producer for all, it would be better.

Do I understand correctly that this swap is also needed in some workflow based on cmsDriver?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

makeProcessModifier can be used to wrap a modifying function.

I am not very familiar with it. Anyway d6091aa it's my take at it.

Do I understand correctly that this swap is also needed in some workflow based on cmsDriver?

correct, but it's done via the customiseExpress function, already changed in PR #35373

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not very familiar with it. Anyway d6091aa it's my take at it.

did it fix the vis unit test?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, see #35642

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in particular https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-ddb8d0/19652/unitTests/src/DQM/Integration/test/TestDQMOnlineClient-visualization/testing.log