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.

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

Adding option to disable resetting L1 PS counters on ls change #37395

Merged
merged 2 commits into from Apr 5, 2022

Conversation

Sam-Harper
Copy link
Contributor

PR description:

This PR adds an option to disable resetting the prescale counters of the L1 trigger on lumi section change. This is needed for offline L1 reemulations where we only have ~23000 events per lumisection available (often much much less due to multithreading/job splitting) and this reset comes too soon and causes triggers to have a much higher effective prescale or not fire at all. This is based on the excellent debugging and investigations by @sanuvarghese

Also could experts just confirm why the L1 resets counters every lumisection? I'm just curious to why.

Finally as spotted by both Sanu and myself, we note that this code can not work with fractional prescales. What is the plan here?

PR validation:

Testing the L1 emulation workflow, prescale counts are now as expected.

if this PR is a backport please specify the original PR and why you need to backport that PR:

We'll need this in 12_3 as well, once we work out a plan for fixes.

@cmsbuild
Copy link
Contributor

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-37395/29050

  • This PR adds an extra 32KB to repository

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

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-37395/29052

  • This PR adds an extra 32KB to repository

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @Sam-Harper (Sam Harper) for master.

It involves the following packages:

  • L1Trigger/L1TGlobal (l1)

@epalencia, @cmsbuild, @cecilecaillol, @rekovic can you please review it and eventually sign? Thanks.
@Martin-Grunewald, @missirol this is something you requested to watch as well.
@perrotta, @dpiparo, @qliphy you are the release manager for this.

cms-bot commands are listed here

@Sam-Harper
Copy link
Contributor Author

@cmsbuild please test

@silviodonato
Copy link
Contributor

Also could experts just confirm why the L1 resets counters every lumisection? I'm just curious to why.

I'm not sure, maybe because they wanted to have the same results when running a single lumi section wrt multiple lumisection. This might be important especially for debugging. Anyway, you are adding a new option so this PR is harmless (and thanks a lot for the debugging the problem of the L1 skim).

@cecilecaillol
Copy link
Contributor

+l1

@cmsbuild
Copy link
Contributor

cmsbuild commented Apr 1, 2022

This pull request is fully signed and it will be integrated in one of the next master IBs after it passes the integration tests. This pull request will now be reviewed by the release team before it's merged. @perrotta, @dpiparo, @qliphy (and backports should be raised in the release meeting by the corresponding L2)

@qliphy
Copy link
Contributor

qliphy commented Apr 5, 2022

please abort
test stuck for one week

@qliphy
Copy link
Contributor

qliphy commented Apr 5, 2022

please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Apr 5, 2022

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-dfaf4a/23645/summary.html
COMMIT: ef923ca
CMSSW: CMSSW_12_4_X_2022-04-04-2300/slc7_amd64_gcc10
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/37395/23645/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

  • No significant changes to the logs found
  • Reco comparison results: 0 differences found in the comparisons
  • DQMHistoTests: Total files compared: 48
  • DQMHistoTests: Total histograms compared: 3593039
  • DQMHistoTests: Total failures: 2
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3593015
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 47 files compared)
  • Checked 200 log files, 45 edm output root files, 48 DQM output files
  • TriggerResults: no differences found

@missirol
Copy link
Contributor

missirol commented Apr 5, 2022

Also could experts just confirm why the L1 resets counters every lumisection? I'm just curious to why.

Finally as spotted by both Sanu and myself, we note that this code can not work with fractional prescales. What is the plan here?

I take it that these questions from the PR description won't be answered here by L1T. The integration of the PR can proceed regardless, as it simply adds a new option without changing defaults. I would suggest to integrate it soon, as I think it's best to backport it in time for 12_3_0 (it's needed by TSG for HLT-rate studies).

@qliphy
Copy link
Contributor

qliphy commented Apr 5, 2022

+1

@silviodonato
Copy link
Contributor

@rekovic @apana @cms-sw/l1-l2 do you know why the L1 resets counters every lumisection?

@mjeitler
Copy link

Hi @Sam-Harper ,
I was not aware of the reset of the prescale counters at the start of a lumi section until recently (and not aware of this thread either). When I discovered this recently I asked for a firmware fix. This is now ready and being tested, and we suggest to deploy it whenever we deploy the next menu. (If really needed, it could be done also before.)
Please let us know if this is OK with you or if this could now result in unwanted interference with the software fix you have developed?
Thanks,
Manfred (for the Vienna Global Trigger group)

@Sam-Harper
Copy link
Contributor Author

Hi Manfred,

Thanks. I dont think this interfers with this software fix, in fact this software fix is a necessary change offline and it should be updated to now enable it by default. One potential issue though and something for the L1 team to consider is how to handle the versioning. The behaviour of the L1 hardware will change mid run and thus do we want a mechanism to properly emulate this automatically either via some sort of GT payload or checking the firmware version/date.

On one hand, I dont like it'll be difficult to properly exactly emulate what the L1 is doing (without an automatic mechanism, you'll have to do it manually based on the runs you are using). On the other hand, prescale emulation is 1) rarely used and 2) imperfect in the sense that you need to run over all events L1 sees for a given seed to emulate it perfectly. And offline we have to change it anyways due to this.

So I cant see a scenario where we need to exactly emulate the prescales as done offline, given to do this we need to take all data the L1 sees. So I would go with setting this option as default and adding a comment to that this changed on date X/X/X for the L1 firmware.

However this is a trigger coordination call @silviodonato

@elfontan
Copy link
Contributor

Dear @Sam-Harper @silviodonato @missirol @sanuvarghese all,
a couple of updates as a follow up of this discussion:

  1. L1 PS counter reset: this update is finally going to be included online this week together with a new version of the L1 menu (L1Menu_Collisions2022_v1_4_0), so it might be good to proceed to the update of the emulator, too, in order to make the above option as default. We can provide the exact date X/X/X for the deployment of the new L1 firmware (@mjeitler @bundocka). If we agree with TSG coordination, would you like to proceed on your side @Sam-Harper @missirol given that you followed the previous steps?

  2. Regarding the code that was not working properly for fractional PS, the fix was included with PR#39464 by Molly Taylor.

Thanks a lot in advance,
--Elisa for the L1T group

@missirol
Copy link
Contributor

missirol commented Nov 25, 2022

Regarding 1., in my understanding all that's needed is in #40158 (no backports, no changes online).

@elfontan
Copy link
Contributor

The new L1 firmware with the above mentioned update have been deployed online during the collision run 362673 (L1 + HLT key : collisions2022/v350) on 25/11/2022. See (CMSLITDPG-980) for all details.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants