Update LHCInfoProducer to use LHCInfoCombined

[francescobrivio]

PR description:

This PR updates the NanoAOD LHCInfoProducer module to be able to use both old LHCInfo (for Run 2) and new LHCInfoPer* records (for Run 3). In order to do so:

• I introduced a new run3_nanoAOD_ANY modifier (any feedback on whether this is done properly is appreciated)
  I used (~run3_common) modifier to differentiate between run3 and run2 workflows, as suggested during the PR review
• Additionally, I cleaned up some commented lines (referring to LHCInfo) in SiPixelStatusHarvester.cc and CTPPSDiamondDQMSource.cc.

Related topics/PRs:

• Partially addressing the issue in Remove "old" LHCInfo tag from Run3 GTs cms-AlCaDB/AlCaTools#87.
• Similar to CTPPS reco update to use LHCInfoPer* records #42515, CTPPS update Run3 Direct Simulation to use old LHCInfo after LHCInfo update #42890 and CTPPS update HLTPPSJetComparisonFilter to use LHCInfoCombined #43951

PR validation:

Code compiles, plus I successfully run few nano matrix tests (runTheMatrix.py -l 2500.31,2500.012 --ibeos -j 16).

Changes expected from this PR:

• none in the Run 2 workflows
• possibly some in the Run3 workflows, given that they have been consuming not-correct payloads so far (although AFAIK only PPS experts are interested in such information)

Backport:

A 140X backport will be provided once this PR is converged.

---

FYI @vavati @JanChyczynski @grzanka @Glitchmin

[cmsbuild]

cms-bot internal usage

[cmsbuild]

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-43956/38842

• This PR adds an extra 44KB to repository

• There are other open Pull requests which might conflict with changes you have proposed:

  • File PhysicsTools/NanoAOD/plugins/BuildFile.xml modified in PR(s): [NanoAOD,BTVNano] Btvnano integration as customized module #43485, Lepton time-life info for NanoAOD #43525
  • File PhysicsTools/NanoAOD/python/nano_cff.py modified in PR(s): Lepton time-life info for NanoAOD #43525

[cmsbuild]

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

It involves the following packages:

• CalibTracker/SiPixelQuality (alca)
• DQM/CTPPS (dqm)
• PhysicsTools/NanoAOD (xpog)

@rvenditti, @tjavaid, @syuvivida, @antoniovagnerini, @saumyaphor4252, @perrotta, @nothingface0, @cmsbuild, @consuegs, @hqucms, @vlimant can you please review it and eventually sign? Thanks.
@gpetruc, @mroguljic, @tvami, @grzanka, @ferencek, @mmusich, @rsreds, @AnnikaStein, @fabferro, @tocheng, @yuanchao, @dkotlins this is something you requested to watch as well.
@rappoccio, @antoniovilela, @sextonkennedy you are the release manager for this.

cms-bot commands are listed here

[francescobrivio]

enable nano

[perrotta]

please test

[vlimant]

Hello, I don't think we need the run3_nanoAOD_ANY modifier, but rather set it as true by default (run3 by default) and turn it off in the run2 modifiers.

[francescobrivio]

Hello, I don't think we need the run3_nanoAOD_ANY modifier, but rather set it as true by default (run3 by default) and turn it off in the run2 modifiers.

Hi @vlimant ok I can do that!
Should I then use run2_nanoAOD_ANYto modify the run2 workflows?

[francescobrivio]

@cmsbuild please abort

[vlimant]

Should I then use run2_nanoAOD_ANYto modify the run2 workflows?

yes please

[mmusich]

but rather set it as true by default (run3 by default) and turn it off in the run2 modifiers.

I am not advocating for the contrary, but it would be nice to have an "agreed upon" policy about this, most of the other modifiers work in the other way around (past = default, "new" feature activated by the modifier).

[vlimant]

there might be some policy somewhere. what I see as realistic is default is now, and past or future is obtained through modifiers.
for nano, as far as I can tell, it is this way

[vlimant]

+1

[vlimant]

do we need a 14.0 backport ?

[francescobrivio]

do we need a 14.0 backport ?

I'll open it in few minutes!

[perrotta]

+alca

• Modifications in the AlCa part are trivial
• (I would rather stick on the usual procedure of having the default set for the past eras, and then a modifier modifies it from a given era onward: but this is implemented in a file which is outside my jurisdiction, and I won't insist on it: in any case, please @francescobrivio update the PR description to correspond to what actually implemented in the PR)

[francescobrivio]

• in any case, please @francescobrivio update the PR description to correspond to what actually implemented in the PR)

I was doing it at the same time when you were writing 😄 It's updated now!

[vlimant]

please fwd me a place where it is said that our code base default must leave in the past, and that we need to enable the present with a modifier (if you can make sense of that to me, I am happy to modify things for that file outside of your juridiction)

[mmusich]

please fwd me a place where it is said that our code base default must leave in the past, and that we need to enable the present with a modifier

The whole rest of cmssw?

[vlimant]

does this sentence makes sense to you "our code base default must leave in the past, and that we need to enable the present with a modifier" I can understand the laziness of people trying to figure things out when implementing a new feature, but not having the today default be the two years ago situation

[mmusich]

does this sentence makes sense to you "our code base default must leave in the past, and that we need to enable the present with a modifier"

Maybe it does not, maybe it does. But try to reconstruct a sinlge 2023 event without the Era Run3 (default is Run1).

[perrotta]

does this sentence makes sense to you "our code base default must leave in the past, and that we need to enable the present with a modifier" I can understand the laziness of people trying to figure things out when implementing a new feature, but not having the today default be the two years ago situation

Because modifications are made starting from an initial config, and then for later eras one starts from the previous era + possible modifications. As such every era is simply based on the previous one. If we revert the order, then the modifications may become more complicate and mistakes much more probable. For example, in this case: if you want to extend nanoAOD to Run1, or some past special case, you must define another modification specific for it, while if there is a default that is modified starting from a given time onward everything is already automatically implemented.

[vlimant]

yes, run1 modifier (that is not in place already ?) would not even be needed here as it is "not run3" when run4 comes along, there is no need for modifier for the parameter ; less work less issue.

[makortel]

Generally the approach in the past for new parameters in existing modules has been that the default values of new parameters should preserve the earlier behavior of the module. To my understanding at least reconstruction and HLT have imposed such requirements in the past. Such requirement has been particularly strict for backports to production release cycles.

[makortel]

when run4 comes along, there is no need for modifier for the parameter

What do you mean? Do you mean at that point the support for NanoAOD in on Run 2 data would stop in master? (and therefore the customization via ~run3_common could be removed) I'd assume the future Run4 era (ModifierChain) would include run3_common Modifier, as Phase2 era does today.

[tjavaid]

+1

[cmsbuild]

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

[rappoccio]

+1