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

New set of files coherent with the new v1_1_0 L1Menu #38250

Merged
merged 1 commit into from
Jun 12, 2022

Conversation

elfontan
Copy link
Contributor

@elfontan elfontan commented Jun 5, 2022

PR description:

A full set of xml files for the L1 emulation of prescales and masks coherent with the updated L1 menu for Run 3 has been created and pushed to L1Trigger-L1TGlobal (PR#11). Once that these new files will be available in cms-data, we will proceed with the coherent update of these files. The update is targeting CMSSW_12_4_0. The backport of this PR is #38331.
The current version of the updated L1 Menu is L1Menu_Collisions2022_v1_1_0.

In the context of the trigger studies for the preparation of the Run 3 menu (L1+HLT), we recently faced an issue related to the emulation of the L1 prescales. Two different issues were found out:

  • format of the PS table (some documentation and a recipe can be found here);
  • usage of the fractional PS (PR#37046).

PR validation:

Basic tests performed successfully starting from CMSSW_12_4_0_pre3.

cmsrel CMSSW_12_4_0_pre3/src
cd CMSSW_12_4_0_pre3/src
cmsenv
git cms-addpkg L1Trigger/Configuration
git cms-addpkg L1Trigger/L1TGlobal
cd L1Trigger/L1TGlobal
mkdir -p data/Luminosity/startup
wget https://raw.githubusercontent.com/cms-l1-dpg/L1MenuRun3/master/development/L1Menu_Collisions2022_v1_1_0/L1Menu_Collisions2022_v1_1_0.xml
cd -
scram b distclean
git cms-checkdeps -a -A
scram b -j 8
scram b runtests
scram build code-checks
scram build code-format

@cmsbuild
Copy link
Contributor

cmsbuild commented Jun 5, 2022

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-38250/30400

  • This PR adds an extra 24KB to repository

@cmsbuild
Copy link
Contributor

cmsbuild commented Jun 5, 2022

A new Pull Request was created by @elfontan (Elisa Fontanesi) for master.

It involves the following packages:

  • L1Trigger/Configuration (l1)
  • 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

@epalencia
Copy link
Contributor

please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Jun 5, 2022

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-336a93/25288/summary.html
COMMIT: df9c038
CMSSW: CMSSW_12_5_X_2022-06-05-0000/el8_amd64_gcc10
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/38250/25288/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: 2 differences found in the comparisons
  • DQMHistoTests: Total files compared: 50
  • DQMHistoTests: Total histograms compared: 3651603
  • DQMHistoTests: Total failures: 8
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3651573
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 49 files compared)
  • Checked 208 log files, 45 edm output root files, 50 DQM output files
  • TriggerResults: no differences found

@epalencia
Copy link
Contributor

+l1

@cmsbuild
Copy link
Contributor

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

@perrotta
Copy link
Contributor

@elfontan I understand that the new menu is not testable in the PR tests, otherwise this PR would have failed having been run without including cms-data/L1Trigger-L1TGlobal#11
As such, the PR tests you are listing in the PR description cannot have had any effect.
Could you please explain how the new menu was tested? Shouldn't a unit test which tests the currently used menu be made available, so that it can be kept tested at every IB?

@elfontan
Copy link
Contributor Author

Good morning @perrotta,
thanks for the message.
I actually forgot to add in the list of test the intermediate step of adding manually the L1 menu in L1Trigger/L1TGlobal/data/Luminosity/startup (as clarified in #38331 instead).
I modified the initial description also here now.
Moreover, we tested the menu via cmsDriver and hltGetConfiguration commands: should I also specify some testing commands?

Cheers,
--Elisa

@perrotta
Copy link
Contributor

Ciao Elisa.

I suspect that the tests that you mention would give the very same identical results whether you

wget https://raw.githubusercontent.com/cms-l1-dpg/L1MenuRun3/master/development/L1Menu_Collisions2022_v1_1_0/L1Menu_Collisions2022_v1_1_0.xml

or not: am I correct?

As such, you are not testing the actual new L1 menu. Is there any test code or script that you ran, that was actually picking the new menu? For example, the L1Trigger/L1TGlobal/test/runGlobalFakeInputProducer.py that you modified with this PR?

IOW, how can we get assured that the new menu included here actually runs without crashes or other kind of unwanted issues?

@elfontan
Copy link
Contributor Author

Hi @perrotta, I believe that this discussion is similar to what we discussed last time regarding the PS and masks files. They are not part of any official workflow atm that allows the testing. There are O2O scripts that might cause a failure if the files of interest are not found, and I think that @panoskatsoulis was working on including some of the O2O tests as central tests (correct me if I am wrong Panos!). So I am not sure about what I could test. (This is also why I was keeping the PR as draft for a while waiting to have the PR to cms-data merged...)

@perrotta
Copy link
Contributor

+1

  • Ok, let have this merged even if there is nothing in the usual workflows or unit tests that can be used to inspect the correctness of the new L1T menu: it is left to the responsability of the L1 group to make sure that it can run online

@panoskatsoulis
Copy link
Contributor

Hi @perrotta , @elfontan
I think we need to brakedown this discussion in 2 items, because I think the following issues are somehow mixed in the discussion

  • testing the Menu itself
  • testing the O2O Mechanism itself

While the unit tests implemented ~2 months ago are able to test the O2O machinery, they are not in place test changes in the Menu itself. Also technically atm the unit tests are not triggered because the tests are not in the same pkg (L1TriggerConfig/L1TConfigProducers) with the files changed here.
We could eventually somehow link them but this needs some disscussion between L1 people first

@perrotta
Copy link
Contributor

@panoskatsoulis what I find is missing specifically for this kind of pull requests is indeed "testing the Menu itself".
The 228k lines of xml code in cms-data/L1Trigger-L1TGlobal#11 are added without even a checking in the release that there are no formal flaws. I don't mean "validation", which is something that is demanded to the L1 group: I would like just to check that "it doesn't crash" when included in a workflow that runs in CMSSW.

A simple unit test that read those files and do some basic actions meant to verify that they can run at every IB is more than welcome.

@panoskatsoulis
Copy link
Contributor

I see your point and will discuss it with the L1 people to conclude to such a workflow
@epalencia @cecilecaillol

@missirol
Copy link
Contributor

@elfontan @panoskatsoulis

Also technically atm the unit tests are not triggered because the tests are not in the same pkg

After the fix in #38331 (comment), a PR like this might very well trigger the O2O unit tests that require the correct xml inputs, like it happened (and failed) in #38331. The failure was caused by the fact that the unit test depends on yet another package CondTools/L1TriggerExt, which "check-deps" (correctly) does not check out.

So I am not sure about what I could test.

Given the situation above, I think a future PR like this one should be tested with something like #38331 (comment). At the same time, I think one could make a minor improvement to the L1T-O2O unit test [*]; with said improvement, one would only need to test together with the relevant cms-data PR, without having to also request cms-addpkg = CondTools/L1TriggerExt.

[*]
https://github.com/cms-sw/cmssw/blob/master/L1TriggerConfig/L1TConfigProducers/scripts/runL1-O2O-scramTests.sh#L77

L1TRIGGEREXT_DIR=${CMSSW_BASE}/src/CondTools/L1TriggerExt/test
+[ -d ${L1TRIGGEREXT_DIR} ] || L1TRIGGEREXT_DIR=${CMSSW_RELEASE_BASE}/src/CondTools/L1TriggerExt/test

@elfontan elfontan deleted the EF_updatesForL1Menuv110 branch January 19, 2023 13:05
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.

6 participants