-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Conversation
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-38250/30400
|
A new Pull Request was created by @elfontan (Elisa Fontanesi) for master. It involves the following packages:
@epalencia, @cmsbuild, @cecilecaillol, @rekovic can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
please test |
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-336a93/25288/summary.html Comparison SummarySummary:
|
+l1 |
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) |
@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 |
Good morning @perrotta, Cheers, |
Ciao Elisa. I suspect that the tests that you mention would give the very same identical results whether you
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? |
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...) |
+1
|
Hi @perrotta , @elfontan
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. |
@panoskatsoulis what I find is missing specifically for this kind of pull requests is indeed "testing the Menu itself". 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. |
I see your point and will discuss it with the L1 people to conclude to such a workflow |
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
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 L1TRIGGEREXT_DIR=${CMSSW_BASE}/src/CondTools/L1TriggerExt/test
+[ -d ${L1TRIGGEREXT_DIR} ] || L1TRIGGEREXT_DIR=${CMSSW_RELEASE_BASE}/src/CondTools/L1TriggerExt/test |
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:
PR validation:
Basic tests performed successfully starting from CMSSW_12_4_0_pre3.