added the possibility to lock a record #33659

gennai · 2021-05-07T12:42:09Z

PR description:

I have added the possibility to lock a record

PR validation:

tested with unitTest

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

cmsbuild · 2021-05-07T12:47:16Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-33659/22551

This PR adds an extra 44KB to repository
There are other open Pull requests which might conflict with changes you have proposed:
- File DQM/Integration/python/clients/beam_dqm_sourceclient-live_cfg.py modified in PR(s): CondDB frontierKey update #33643, Redesign all GPU workflows to detect if a GPU is present, and fall back to CPU otherwise #33428
- File DQM/Integration/python/clients/beamfake_dqm_sourceclient-live_cfg.py modified in PR(s): CondDB frontierKey update #33643, Redesign all GPU workflows to detect if a GPU is present, and fall back to CPU otherwise #33428
- File DQM/Integration/python/clients/beamhlt_dqm_sourceclient-live_cfg.py modified in PR(s): CondDB frontierKey update #33643
- File DQM/Integration/python/clients/beamhltfake_dqm_sourceclient-live_cfg.py modified in PR(s): CondDB frontierKey update #33643

cmsbuild · 2021-05-07T12:47:42Z

A new Pull Request was created by @gennai (simone gennai) for master.

It involves the following packages:

DQM/BeamMonitor
DQM/Integration

@andrius-k, @kmaeshima, @ErnestaP, @ahmad3213, @cmsbuild, @jfernan2, @rvenditti can you please review it and eventually sign? Thanks.
@threus, @batinkov, @battibass this is something you requested to watch as well.
@silviodonato, @dpiparo, @qliphy you are the release manager for this.

cms-bot commands are listed here

jfernan2 · 2021-05-07T14:18:19Z

please test

cmsbuild · 2021-05-07T14:24:44Z

-1

Failed Tests: Build Python3
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-f6f4b1/14944/summary.html
COMMIT: 7e321e5
CMSSW: CMSSW_12_0_X_2021-05-06-2300/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/33659/14944/install.sh to create a dev area with all the needed externals and cmssw changes.

Build

I found compilation warning when building: See details on the summary page.

#Python3

I found errors:

*** Error compiling 'src/DQM/Integration/python/clients/beam_dqm_sourceclient-live_cfg.py'...

cmsbuild · 2021-05-07T15:10:59Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-33659/22557

This PR adds an extra 44KB to repository
There are other open Pull requests which might conflict with changes you have proposed:
- File DQM/Integration/python/clients/beam_dqm_sourceclient-live_cfg.py modified in PR(s): CondDB frontierKey update #33643, Redesign all GPU workflows to detect if a GPU is present, and fall back to CPU otherwise #33428
- File DQM/Integration/python/clients/beamfake_dqm_sourceclient-live_cfg.py modified in PR(s): CondDB frontierKey update #33643, Redesign all GPU workflows to detect if a GPU is present, and fall back to CPU otherwise #33428
- File DQM/Integration/python/clients/beamhlt_dqm_sourceclient-live_cfg.py modified in PR(s): CondDB frontierKey update #33643
- File DQM/Integration/python/clients/beamhltfake_dqm_sourceclient-live_cfg.py modified in PR(s): CondDB frontierKey update #33643

cmsbuild · 2021-05-07T15:11:24Z

Pull request #33659 was updated. @andrius-k, @kmaeshima, @ErnestaP, @ahmad3213, @cmsbuild, @jfernan2, @rvenditti can you please check and sign again.

gennai · 2021-05-10T16:44:35Z

@jfernan2 could you trigger the test, or is there anything else I should do for the warnings?

jfernan2 · 2021-05-10T17:21:39Z

please test

cmsbuild · 2021-05-10T19:54:18Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-f6f4b1/14974/summary.html
COMMIT: 1982ccf
CMSSW: CMSSW_12_0_X_2021-05-10-1100/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/33659/14974/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: 8 differences found in the comparisons
DQMHistoTests: Total files compared: 37
DQMHistoTests: Total histograms compared: 2663174
DQMHistoTests: Total failures: 12
DQMHistoTests: Total nulls: 1
DQMHistoTests: Total successes: 2663139
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: -0.004 KiB( 36 files compared)
DQMHistoSizes: changed ( 312.0 ): -0.004 KiB MessageLogger/Warnings
Checked 155 log files, 37 edm output root files, 37 DQM output files
TriggerResults: no differences found

jfernan2 · 2021-05-10T20:06:24Z

@gennai since this PR is affecting Online DQM mainly, could you make a 11_3_X PR version to test it at P5 machines? Next MWGR is planning to use 11_3_X, unless you don't plan to use it by then

gennai · 2021-05-10T20:08:37Z

I am already working on it, I was waiting this PR to be scrutinized to apply the same comments to the back porting

gennai · 2021-05-10T20:28:25Z

Backporting is #33684

Best,
S.

jfernan2 · 2021-05-11T11:09:35Z

Thank you! @gennai

jfernan2 · 2021-05-12T16:16:32Z

+1
Backport tested succesfully in Online DQM at P5

cmsbuild · 2021-05-12T16:16:54Z

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

silviodonato

I would also suggest (not related to this PR) to use clone

process.dqmBeamMonitor = process.dqmFakeBeamMonitor.clone(
   OnlineMode = True,
  useLockRecords =  useLockRecords,
  [...]
)

instead of

process.dqmBeamMonitor = process.dqmFakeBeamMonitor.clone()
process.dqmBeamMonitor.useLockRecords = useLockRecords
process.dqmBeamMonitor.OnlineMode = True
[...]

in the first case the mis-spelled variable name would give error. See for instance https://github.com/cms-sw/cmssw/pull/33046/files
and
When one customizes an existing parameter in clone(), Modifier.toModify(), or in assignment, explicit types on the right hand side should be avoided. in https://cms-sw.github.io/cms_coding_rules.html

silviodonato · 2021-05-12T16:59:27Z

DQM/BeamMonitor/plugins/BeamMonitor.cc

@@ -137,6 +137,7 @@ BeamMonitor::BeamMonitor(const ParameterSet& ps)
  minNrVertices_ = ps.getParameter<ParameterSet>("PVFitter").getUntrackedParameter<unsigned int>("minNrVerticesForFit");
  minVtxNdf_ = ps.getParameter<ParameterSet>("PVFitter").getUntrackedParameter<double>("minVertexNdf");
  minVtxWgt_ = ps.getParameter<ParameterSet>("PVFitter").getUntrackedParameter<double>("minVertexMeanWeight");
+  useLockRecords_ = ps.getUntrackedParameter<bool>("useLockRecords");


Isn't better to have it as a tracked parameter? (same comment for other parameters above, not related to this PR)

Hi @silviodonato , thanks for the tips. Regarding the clone comment if you wish I can change it and commit. Let me know if I should do it or not.
For the untracked vs tracked parameters I have simply followed what was already there for the others, I have no strong feeling about it.

silviodonato · 2021-05-13T12:27:06Z

+1
I got confused with FakeBeamMonitor.cc that was made recently.
So I forward the comments to @cms-sw/dqm-l2.
Ideally the untracked parameters should be used only for those parameters that does not change the physics result.

jfernan2 · 2021-05-13T14:06:23Z

@silviodonato I can create a general PR modifying all the python config files in DQM packages if needed, including those at Online.

About tracked/untracked parameters, the Provenance is lost in DQM plain root files which are the final endproduct of DQM modules after harvesting, right? Though it might be tracked up in the EDM root files for intermediate MEs not produced in Harvesting step

silviodonato · 2021-05-13T14:36:11Z

@silviodonato I can create a general PR modifying all the python config files in DQM packages if needed, including those at Online.

It is not urgent, but it would be a nice style improvement. Reco is making many PRs in that direction (eg. #30947)

About tracked/untracked parameters, the Provenance is lost in DQM plain root files which are the final endproduct of DQM modules after harvesting, right? Though it might be tracked up in the EDM root files for intermediate MEs not produced in Harvesting step

Ok, good point. I thought that it might be important to know if the module ran with useLockRecords True or False but, as you said, we cannot have this piece of information in plain DQM ROOT files.

jfernan2 · 2021-05-13T14:53:50Z

OK, I will take care of this cloning restyling for master branch in the near future. Thanks!

gennai · 2021-05-16T08:30:37Z

About the tracked or untracked parameter, I do not think It would be interest anyway in keep track of it. It is something which does not affect the beamspot computation per se.

added the possibility to lock a record

7e321e5

cmsbuild added this to the CMSSW_12_0_X milestone May 7, 2021

cmsbuild added code-checks-pending dqm-pending orp-pending pending-signatures tests-pending labels May 7, 2021

cmsbuild added code-checks-approved and removed code-checks-pending labels May 7, 2021

cmsbuild added tests-started and removed tests-pending labels May 7, 2021

cmsbuild added tests-rejected and removed tests-started labels May 7, 2021

fixed the compilation error

1982ccf

cmsbuild added code-checks-pending tests-pending and removed code-checks-approved tests-rejected labels May 7, 2021

cmsbuild added code-checks-approved and removed code-checks-pending labels May 7, 2021

cmsbuild added tests-started and removed tests-pending labels May 10, 2021

cmsbuild added tests-approved and removed tests-started labels May 10, 2021

gennai mentioned this pull request May 10, 2021

Introducing the beamspot tag lock systems #33684

Merged

cmsbuild added dqm-approved fully-signed and removed dqm-pending pending-signatures labels May 12, 2021

silviodonato reviewed May 12, 2021

View reviewed changes

cmsbuild added orp-approved and removed orp-pending labels May 13, 2021

cmsbuild merged commit d7fa76b into cms-sw:master May 13, 2021

This was referenced Jun 15, 2021

Drop type specs for DT DQM #34144

Merged

Drop Type in toModify for DTDQM #34153

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

added the possibility to lock a record #33659

added the possibility to lock a record #33659

gennai commented May 7, 2021

cmsbuild commented May 7, 2021

cmsbuild commented May 7, 2021

jfernan2 commented May 7, 2021

cmsbuild commented May 7, 2021

cmsbuild commented May 7, 2021

cmsbuild commented May 7, 2021

gennai commented May 10, 2021

jfernan2 commented May 10, 2021

cmsbuild commented May 10, 2021

jfernan2 commented May 10, 2021

gennai commented May 10, 2021

gennai commented May 10, 2021

jfernan2 commented May 11, 2021

jfernan2 commented May 12, 2021

cmsbuild commented May 12, 2021

silviodonato left a comment

silviodonato May 12, 2021

gennai May 12, 2021

silviodonato commented May 13, 2021

jfernan2 commented May 13, 2021 •

edited

silviodonato commented May 13, 2021

jfernan2 commented May 13, 2021

gennai commented May 16, 2021

added the possibility to lock a record #33659

added the possibility to lock a record #33659

Conversation

gennai commented May 7, 2021

PR description:

PR validation:

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

cmsbuild commented May 7, 2021

cmsbuild commented May 7, 2021

jfernan2 commented May 7, 2021

cmsbuild commented May 7, 2021

Build

cmsbuild commented May 7, 2021

cmsbuild commented May 7, 2021

gennai commented May 10, 2021

jfernan2 commented May 10, 2021

cmsbuild commented May 10, 2021

Comparison Summary

jfernan2 commented May 10, 2021

gennai commented May 10, 2021

gennai commented May 10, 2021

jfernan2 commented May 11, 2021

jfernan2 commented May 12, 2021

cmsbuild commented May 12, 2021

silviodonato left a comment

Choose a reason for hiding this comment

silviodonato May 12, 2021

Choose a reason for hiding this comment

gennai May 12, 2021

Choose a reason for hiding this comment

silviodonato commented May 13, 2021

jfernan2 commented May 13, 2021 • edited

silviodonato commented May 13, 2021

jfernan2 commented May 13, 2021

gennai commented May 16, 2021

jfernan2 commented May 13, 2021 •

edited