-
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
Nano: drop frozen V10 config, make triggerObjectTable more easily customizable #40321
Nano: drop frozen V10 config, make triggerObjectTable more easily customizable #40321
Conversation
enable nano |
-code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-40321/33402
Code check has found code style and quality issues which could be resolved by applying following patch(s)
|
ec5b78e
to
d42d61a
Compare
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-40321/33404
|
A new Pull Request was created by @swertz (Sébastien Wertz) for master. It involves the following packages:
@perrotta, @rappoccio, @swertz, @vlimant, @bbilin, @cmsbuild, @AdrianoDee, @srimanob, @kskovpen, @sunilUIET, @fabiocos, @davidlange6 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-041c1a/29628/summary.html Comparison SummarySummary:
NANO Comparison SummarySummary:
Nano size comparison Summary:
|
Looking at the jet trigger filters for Run2 It seems that the "Run3" trigger filters introduced in #39818 (therefore appearing in blue) are actually more relevant than the "Run2" filters previously present in UL nanoV9 (appearing in red), for 2017 and 2018 menus. The previous "Run2" filters only seem to make sense for 2016. I think for completeness we should make sure all the necessary Run2 filters are present, so I'll make further changes to add some of the filters from #39818 also for 2017 and 2018 eras. @johnalison does that make sense to you? |
+Upgrade From the upgrade related-code, only clean up in |
Hi @cms-sw/operations-l2 @cms-sw/pdmv-l2, do you have any comments on this? |
+pdmv |
please test |
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-041c1a/29875/summary.html Comparison SummarySummary:
NANO Comparison SummarySummary:
Nano size comparison Summary:
|
from Configuration.Eras.Modifier_run3_RPC_cff import run3_RPC | ||
|
||
Run3 = cms.ModifierChain(Run2_2018.copyAndExclude([run2_GEM_2017, ctpps_2018, run2_egamma_2018]), | ||
Run3 = cms.ModifierChain(Run2_2018.copyAndExclude([run2_GEM_2017, ctpps_2018, run2_egamma_2018, run2_HLTconditions_2018]), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@missirol @Martin-Grunewald could you please confirm that this is ok for HLT in Run3?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could you please confirm that this is ok for HLT in Run3?
Yes, it is okay for HLT.
To my knowledge, nothing that concerns HLT directly depends on this modifier. It is used in the HLT Offline DQM (FYI: @cms-sw/dqm-l2 (@ckoraka)), e.g.
https://github.com/cms-sw/cmssw/blob/a9b836229a8b2869e3d342b70dd1332e45e0681a/DQMOffline/Trigger/python/TopMonitoring_cff.py
For reference, this change was already discussed in #39082 and #39084 (neither of which was merged, despite what the GitHub labels say).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
|
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 be automatically merged. |
@swertz, many workflows ( [a]
|
Indeed, that directory is removed and I removed the associated nano-specific workflows from https://github.com/cms-sw/cmssw/blob/master/Configuration/PyReleaseValidation/python/relval_nano.py but I didn't notice that this step was making use of it: https://github.com/cms-sw/cmssw/blob/master/Configuration/PyReleaseValidation/python/relval_steps.py#L2800 How come this didn't come up in the PR tests? I can make a PR ASAP to switch that step to use the default nano configuration instead of the V10 one. |
We don't have the 2022 data workflow in runTheMatrix, Maybe we should add one, i.e. 140.106 for JetHT 2022B. Line to fix, https://github.com/cms-sw/cmssw/blob/master/Configuration/PyReleaseValidation/python/relval_steps.py#L2800 |
PR test runs a selected number of workflows and I guess non of those workflows were using PhysicsTools/NanoAOD/V10/nano_cff |
PR description:
PSet
instead of aVPSet
run2_HLTconditions_2018
modifier from theRun3
era. From a quick search I don't expect this to have unwanted effects elsewhere (note that this relates to the discussion in run2_common modifier is present in Run3 era #39958 as this is not the first time we've needed to introduce a different behaviour between Run2 and Run3 for things that were blindly inherited from theRun2_2018
era to theRun3
era, see e.g. Nano: restore Run2/3 consistency between electron/photon IDs; remove scale+smearing correction for Run3 #40209).PR validation:
Ran nano matrix workflows.