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
Migration of input collection parameters to untracked in DQM/BeamMonitor package #37848
Conversation
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-37848/29778
|
A new Pull Request was created by @NiharSaha (Nihar Saha) for master. It involves the following packages:
@malbouis, @pmandrik, @emanueleusai, @ahmad3213, @tvami, @cmsbuild, @jfernan2, @ggovi, @francescobrivio, @micsucmed, @rvenditti can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
@cmsbuild , please test |
My understanding is that if the module has a i.e.
could become
Although, I have to say that I didnt check if that given module has a |
-1 Failed Tests: RelVals-INPUT CMS deprecated warnings: 3 CMS deprecated warnings found, see summary page for details. RelVals-INPUTThe relvals timed out after 4 hours. Comparison SummarySummary:
|
I see that |
@cmsbuild , please test |
|
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-b75ee9/24511/summary.html CMS deprecated warnings: 3 CMS deprecated warnings found, see summary page for details. Comparison SummarySummary:
|
@mmusich Nihar is following the decision/prescription we took here: #36998 (comment) yes perhaps we should adjust the PR so that only the input collections are changed to untracked. |
Okay, let me try this out. Thank you for pointing this out. |
still not sure what's the benefit (the cost is a lot of code churn)
By all means, at least do that. |
Well. it is recommended not to write the explicit types for parameters modified when you You cannot avoid writing the explicit type if you define a module anew. Also this is independent from the existence of a fillDescription method in the C++ code that defines that module. Having a fillDescription method is needed to validate the parameters that you insert in the configuration, and fill with a default the ones that are not explicitely defined in the configuration. Of course, the fillDescription method also automatically provides you with a default configuration file from which you can always clone your module, thus avoiding to write the explicit types for the modified parameters in the cloned configuration. |
fe9cc2d
to
d00e750
Compare
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-37848/30851
|
Pull request #37848 was updated. @malbouis, @pmandrik, @emanueleusai, @ahmad3213, @tvami, @cmsbuild, @jfernan2, @ggovi, @francescobrivio, @micsucmed, @rvenditti can you please check and sign again. |
@cmsbuild , please test |
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-b75ee9/25974/summary.html Comparison SummarySummary:
|
+db
|
@NiharSaha did you check for other configuration files containing instances of the other analyzer codes you have changed? |
Hi @mmusich, I have checked for all the instances in all the configuration files for this package using LXR. There are four configuration files newly added to the new commit where the instances are present. There are two parameters viz In summary, there are 6 input collection parameters which are appeared in 7 configuration files for BeamMonitor. |
what about |
Yeah, I have checked that too using LXR. There is only one parameter in Please note that I am taking into consideration only the BeamMonitor configurations. |
+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 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) |
+1
|
PR description:
This PR has switched all the tracked parameters of DQM/BeamMonitor to untracked parameters to read input collections which is followed by the discussion: #36998 (comment)
PR validation:
Tested in CMSSW_12_4_X via runTheMatrix.py -l limited -i all --ibeos
if this PR is a backport please specify the original PR and why you need to backport that PR:
NA