Modify L1TGlobal menu code and CondFormats to remove reinterpret_cast to and from utm firmware "es" types

[aloeliger]

PR description:

This PR changes L1TGlobal code and CondFormats to remove three instances of reinterpret_cast (one in L1TUtmTriggerMenuESProducer, and two in TriggerMenuParser) that were attempting to reinterpret pointers of utm Trigger Menu code/objects to CMSSW L1TUtm objects (with undefined behavior). To achieve this, constructors were added to equivalent L1TUtm objects that can accept firmware utm/es objects and create exact (or at the least, more defined) copies by copying values. These L1TUtm objects should now be used with preference in CMSSW and across L1TGlobal code.

This code should not produce any change in global trigger results. Any change in trigger results is incorrect and should not be merged.

These changes were requested in review of cms-sw/cmsdist#8352 and may address an issue seen in the testing of that PR. Regardless of whether it fixes that issue, these changes were requested.

These changes should in theory close #30819

@elfontan I figured you might like to be kept apprised of these changes.

PR validation:

All code compiles, and passes all unit tests, with special attention paid to the testL1TGlobalProducer, which produces identical results. Run the matrix seems to produce reasonable output. I have also confirmed (using print statements) that the now copied-by-value L1TUtmTriggerMenu members should be entirely identical to the L1TUtmTriggerMenu produced previously by reinterpret_cast

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:

This PR is not a backport. This PR may need to be backported to 13_0 to match the corresponding utm firmware update backport, if this PR solves the issue seen in cms-sw/cmsdist#8352. @epalencia may be able to clarify if that is necessary.

[cmsbuild]

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-41036/34572

• This PR adds an extra 48KB to repository

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

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

[cmsbuild]

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-41036/34573

• This PR adds an extra 48KB to repository

[cmsbuild]

A new Pull Request was created by @aloeliger (Andrew Loeliger) for master.

It involves the following packages:

• CondFormats/L1TObjects (l1, db, alca)
• L1Trigger/L1TGlobal (l1)

@epalencia, @cmsbuild, @saumyaphor4252, @aloeliger, @francescobrivio, @cecilecaillol, @tvami can you please review it and eventually sign? Thanks.
@Martin-Grunewald, @missirol, @mmusich, @tocheng, @seemasharmafnal this is something you requested to watch as well.
@perrotta, @dpiparo, @rappoccio you are the release manager for this.

cms-bot commands are listed here

[aloeliger]

please test

[arnobaer]

@aloeliger n.b. duplicating tmeventsetup functions getPrecision and getLut and getDeltaVector might create a lot of extra work as these functions are not only used by emulator but also for firmware generation and for offline rate studies outside the scope of CMSSW. I would argue that it would be better to use the functions provided by tmEventSetup.hh (changes to utm require frequent changes in these functions too).

Update: I just noticed these functions rely on es* classes, so then there is no way around it seems...

[aloeliger]

@arnobaer Yeah, that's the conclusion I came to unfortunately. Perhaps in the future we should discuss the addition of member setters in the respective L1TUtm and es objects (unless there is some code best practices as to why these things should be almost completely un-settable, they didn't even have constructor facing set options before I wrote them in...) that would allow a more seamless translation between the two, and allow some of the helping functions to remain synchronized and entirely in the utm code.

[arnobaer]

@aloeliger the duplicated functions are not up to date with the current utm_0.11.x release. This might result in differnt behaviour of the emulator. See https://gitlab.cern.ch/cms-l1t-utm/utm/-/blob/utm_0.11.1/tmEventSetup/tmEventSetup.cc

[cmsbuild]

-1

Failed Tests: Build
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-7f8219/31220/summary.html
COMMIT: 464f06c
CMSSW: CMSSW_13_1_X_2023-03-10-2300/el8_amd64_gcc11
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/41036/31220/install.sh to create a dev area with all the needed externals and cmssw changes.

Build

I found compilation error when building:

In file included from /data/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_13_1_X_2023-03-10-2300/src/CondFormats/L1TObjects/interface/L1TUtmScale.h:13,
                 from /data/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_13_1_X_2023-03-10-2300/src/CondFormats/L1TObjects/interface/L1TUtmTriggerMenu.h:14,
                 from /data/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_13_1_X_2023-03-10-2300/src/L1Trigger/L1TGlobal/interface/L1TGlobalUtil.h:12,
                 from /data/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_13_1_X_2023-03-10-2300/src/DQM/CastorMonitor/interface/CastorMonitorModule.h:17,
                 from /data/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_13_1_X_2023-03-10-2300/src/DQM/CastorMonitor/src/CastorMonitorModule.cc:6:
/data/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_13_1_X_2023-03-10-2300/src/CondFormats/L1TObjects/interface/L1TUtmBin.h:15:10: fatal error: tmEventSetup/esBin.hh: No such file or directory
   15 | #include "tmEventSetup/esBin.hh"
      |          ^~~~~~~~~~~~~~~~~~~~~~~
compilation terminated.
In file included from /data/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_13_1_X_2023-03-10-2300/src/CondFormats/L1TObjects/interface/L1TUtmScale.h:13,
                 from /data/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_13_1_X_2023-03-10-2300/src/CondFormats/L1TObjects/interface/L1TUtmTriggerMenu.h:14,


[aloeliger]

@arnobaer Okay. I'll take a look at it when I fix the compilation error. Strange that that's happening, it compiles when I try it.

[smuzaffar]

looks like https://github.com/cms-sw/cmssw/blob/master/DQM/CastorMonitor/interface/CastorMonitorModule.h#L17 ( #include "L1Trigger/L1TGlobal/interface/L1TGlobalUtil.h" ) is not needed. This include need utm dependency in DQM/CastorMonitor . So either cleanup the include statement or add utm dependency in DQM/CastorMonitor/BuildFile.xml

[smuzaffar]

https://github.com/cms-sw/cmssw/blob/master/DQM/CastorMonitor/interface/CastorMonitorModule.h#L101 can also go away

[aloeliger]

Unit tests failures seem unrelated to the changes here. And the squashing should not have changed whether unit tests pass or not. Failures are likely related to #41075 and should hopefully be fixed by #41116

[tvami]

@cmsbuild , please test with #41116

[aloeliger]

Right, I always forget about that command. Thank you.

[cmsbuild]

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-7f8219/31495/summary.html
COMMIT: 998ad83
CMSSW: CMSSW_13_1_X_2023-03-21-1100/el8_amd64_gcc11
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/41036/31495/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

• You potentially removed 9 lines from the logs
• Reco comparison results: 8 differences found in the comparisons
• DQMHistoTests: Total files compared: 49
• DQMHistoTests: Total histograms compared: 3550915
• DQMHistoTests: Total failures: 7
• DQMHistoTests: Total nulls: 0
• DQMHistoTests: Total successes: 3550886
• DQMHistoTests: Total skipped: 22
• DQMHistoTests: Total Missing objects: 0
• DQMHistoSizes: Histogram memory added: 0.0 KiB( 48 files compared)
• Checked 213 log files, 164 edm output root files, 49 DQM output files
• TriggerResults: no differences found

[aloeliger]

+l1

I don't know if I should be signing my own PRs, but it passes all menu unit tests, and no difference in trigger results is seen.

[tvami]

@cms-sw/dqm-l2 do you have any comments?

[emanueleusai]

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

[perrotta]

Is this include needed?

[aloeliger]

@perrotta I believe so, there are a number of enum types used in the code that I didn't want to change: https://gitlab.cern.ch/cms-l1t-utm/utm/-/blob/master/tmEventSetup/include/utm/tmEventSetup/esTypes.hh

[perrotta]

+1

[elfontan]

Hello @aloeliger,
I just realized for some merging conflicts in the TriggerMenuParser when trying to cherry-pick some other uGT emulator changes to 130X that at the end this PR was not backported to 13_0_X. Could you (and @epalencia) clarify if this is not needed at all for the 130X CMSSW release that will be used for the data-taking with the new utm?
Thank you in advance!

Cheers,
--Elisa

[aloeliger]

@elfontan nothing in here should be necessary for data-taking and back-porting, this is just a fix going forward. If, for whatever reason, the undefined utm behavior breaks in production, we could back-port this. It shouldn't be necessary though. It can work on all utm versions v0.11.2 and forward.