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.

Sign up for GitHub

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

Jump to bottom

Remove long type in DQMServices and Integration [12_0_X] #35307

Merged
merged 14 commits into from
Sep 29, 2021
Merged

Remove long type in DQMServices and Integration [12_0_X] #35307

merged 14 commits into from
Sep 29, 2021

Conversation

jfernan2
Copy link
Contributor

PR description:

Remove long type in DQM Online packages due to python3.

PR validation:

Tested in Online DQM at P5

@cmsbuild cmsbuild added this to the CMSSW_12_1_X milestone Sep 16, 2021
@jfernan2 jfernan2 changed the base branch from master to CMSSW_12_0_X September 16, 2021 16:00
@cmsbuild cmsbuild modified the milestones: CMSSW_12_1_X, CMSSW_12_0_X Sep 16, 2021
perrotta
perrotta reviewed Sep 29, 2021
View reviewed changes
DQM/Integration/python/clients/sistrip_dqm_sourceclient-live_cfg.py Outdated
@@ -243,7 +243,7 @@
process.SiStripAnalyserCosmic.TkMapCreationFrequency = -1
process.SiStripAnalyserCosmic.ShiftReportFrequency = -1
process.SiStripAnalyserCosmic.StaticUpdateFrequency = 5
process.SiStripAnalyserCosmic.MonitorSiStripBackPlaneCorrection = False
process.SiStripAnalyserCosmic.MonitorSiStripBackPlaneCorrection = cms.bool(False)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jfernan2 the changes in this file are the only differences wrt the master version of this PR that I can't understand.
MonitorSiStripBackPlaneCorrection is defined, for example, in

cmssw/DQM/SiStripMonitorClient/python/SiStripClientConfigP5_cff.py

Line 39 in 292e6ce

MonitorSiStripBackPlaneCorrection = OnDemandMonitoring.MonitorSiStripBackPlaneCorrection,
: then what is the reason to add the explicit types here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR was forwardported to master in #35366 , not the other way around, and that's where I received comments to drop that type reconfiguring files in SiStripAnalyserCosmic

I can put them in synch, despite both work, the master one is more optimal of course.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, please, try to sync them. I mean, only these explicit types, not the other code-format related changes.
We still have some time before cutting 12_0_2, and since you are responsible to sign this for dqm, I expect that it can be merged right after the automatic tests are concluded.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mean, only these explicit types, not the other code-format related changes

This is not possible, only a full synch works. The other changes were needed to drop those type specs

Anyway, I will try to synch asap

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mean, only these explicit types, not the other code-format related changes

This is not possible, only a full synch works. The other changes were needed to drop those type specs

Anyway, I will try to synch asap

Wait, if so we can let the backport PR as it is now.

However, I still don't understand why do you need to "drop those type specs": there is nothing to drop, because in the original 12_0_X configs the explicit types were not defined, and you only added them here. And given that MonitorSiStripBackPlaneCorrection is already defined in 12_0_X, as I pointed out above, I don't think (but I didn't try it...) that they are needed either. Could you please explain?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then, it must be a mistake from my side, you are right. Let me sync the two PRs so that we have both releases in agreement.
Thanks

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. In the process I found an addtional change needed in the just merged master PR #35366
I will create a small PR for the modification. I apologize for the inconvenience. Thanks

@cmsbuild
Copy link
Contributor

Pull request #35307 was updated. @emanueleusai, @ahmad3213, @cmsbuild, @jfernan2, @pmandrik, @pbo0, @rvenditti can you please check and sign again.

@cmsbuild
Copy link
Contributor

Pull request #35307 was updated. @emanueleusai, @ahmad3213, @cmsbuild, @jfernan2, @pmandrik, @pbo0, @rvenditti can you please check and sign again.

@jfernan2
Copy link
Contributor Author

please test

@jfernan2 jfernan2 mentioned this pull request Sep 29, 2021
Merged
@cmsbuild
Copy link
Contributor

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-096124/19229/summary.html
COMMIT: 64eff07
CMSSW: CMSSW_12_0_X_2021-09-28-2300/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/35307/19229/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: 4 differences found in the comparisons
  • DQMHistoTests: Total files compared: 39
  • DQMHistoTests: Total histograms compared: 2998564
  • DQMHistoTests: Total failures: 5
  • DQMHistoTests: Total nulls: 1
  • DQMHistoTests: Total successes: 2998536
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: -0.004 KiB( 38 files compared)
  • DQMHistoSizes: changed ( 312.0 ): -0.004 KiB MessageLogger/Warnings
  • Checked 165 log files, 37 edm output root files, 39 DQM output files
  • TriggerResults: no differences found

@jfernan2
Copy link
Contributor Author

+1

@cmsbuild
Copy link
Contributor

This pull request is fully signed and it will be integrated in one of the next CMSSW_12_0_X IBs (tests are also fine) and once validation in the development release cycle CMSSW_12_1_X is complete. 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

+1

@cmsbuild cmsbuild merged commit eae51dc into cms-sw:CMSSW_12_0_X Sep 29, 2021
@cmsbuild cmsbuild mentioned this pull request Sep 30, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@perrotta perrotta perrotta left review comments

Assignees
No one assigned
Labels
backport-ok dqm-approved fully-signed orp-approved tests-approved
Projects
None yet
Milestone
CMSSW_12_0_X
Development

Successfully merging this pull request may close these issues.
5 participants
@jfernan2 @cmsbuild @qliphy @perrotta @pmandrik