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

Drop type specs for Trigger #37133

Merged
merged 1 commit into from
Mar 4, 2022
Merged

Drop type specs for Trigger #37133

merged 1 commit into from
Mar 4, 2022

Conversation

NiharSaha
Copy link
Contributor

@NiharSaha NiharSaha commented Mar 3, 2022 •
edited by perrotta
Loading

PR description:

Drop type specs in DQMOffline/Trigger python configuration files.

Central DQM migration campaign for drop type specs following
https://cms-sw.github.io/cms_coding_rules.html#6--packaging-rules-1
and update the safer syntax for existing parameters like: (1) drop type specifications where the original parameter exists. (2) using "clone" instead of "deepcopy" (3) move all parameters inside the clone

MET hanlde not valid" message now disappears, this was due to a wrong collection set BEFORE the PR, since Event Interpretation has been removed back in 2021.
This PR corrected above by setting to met = "pfMet", right above. Since the SoftMuHardJetMETSUSYmonitoring is cloned several times below, it seems it just complained but no crash. Now it should be reading an existing collection.

Moreover, the binning for diphotonMass55ANDnoPV module was corrected set to 50-150 in https://github.com/cms-sw/cmssw/pull/37133/files#diff-f0c298feb52e2a455f0a07a4ea80c12b3b660e38e6737d177570ccc9a94775d3R165

since it was wrongly set to 90-200 due to a typo in https://github.com/cms-sw/cmssw/pull/37133/files#diff-f0c298feb52e2a455f0a07a4ea80c12b3b660e38e6737d177570ccc9a94775d3L147

PR validation:

Tested in CMSSW_12_3_X via runTheMatrix.py -l limited -i all --ibeos

@cmsbuild cmsbuild added this to the CMSSW_12_3_X milestone Mar 3, 2022
@cmsbuild
Copy link
Contributor

cmsbuild commented Mar 3, 2022

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-37133/28666

@cmsbuild
Copy link
Contributor

cmsbuild commented Mar 3, 2022

A new Pull Request was created by @NiharSaha (Nihar Ranjan Saha) for master.

It involves the following packages:

  • DQMOffline/Trigger (dqm)

@emanueleusai, @ahmad3213, @cmsbuild, @jfernan2, @pmandrik, @pbo0, @rvenditti can you please review it and eventually sign? Thanks.
@sscruz, @missirol, @mtosi, @Fedespring, @calderona, @HuguesBrun, @jhgoh, @trocino, @cericeci, @rociovilar this is something you requested to watch as well.
@perrotta, @dpiparo, @qliphy you are the release manager for this.

cms-bot commands are listed here

@jfernan2
Copy link
Contributor

jfernan2 commented Mar 3, 2022

This PR replaces #36783 squashing all commits into a single one

@jfernan2
Copy link
Contributor

jfernan2 commented Mar 3, 2022

please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Mar 3, 2022

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-f581ae/22811/summary.html
COMMIT: 4facfeb
CMSSW: CMSSW_12_3_X_2022-03-03-1100/slc7_amd64_gcc10
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/37133/22811/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: 24 differences found in the comparisons
  • DQMHistoTests: Total files compared: 49
  • DQMHistoTests: Total histograms compared: 3987741
  • DQMHistoTests: Total failures: 8
  • DQMHistoTests: Total nulls: 4
  • DQMHistoTests: Total successes: 3987707
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 48 files compared)
  • Checked 204 log files, 45 edm output root files, 49 DQM output files
  • TriggerResults: no differences found

@jfernan2
Copy link
Contributor

jfernan2 commented Mar 4, 2022

+1
@NiharSaha do not forget to close #36783

@cmsbuild
Copy link
Contributor

cmsbuild commented Mar 4, 2022

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)

@NiharSaha
Copy link
Contributor Author

+1 @NiharSaha do not forget to close #36783

Yes @jfernan2. I have closed the PR #36783. Thank you.

@perrotta
Copy link
Contributor

perrotta commented Mar 4, 2022

I'm ready to merge this, but out, of curiosity: what was the fix that allowed to remove from output the error message

%MSG-w TopMonitor:  TopMonitor:SoftMuHardJetMETSUSYmonitoring_metPt  03-Mar-2022 12:40:15 CET Run: 1 Event: 3
MET handle not valid 

%MSG
%MSG-w TopMonitor:  TopMonitor:SoftMuHardJetMETSUSYmonitoring_jetPt  03-Mar-2022 12:40:15 CET Run: 1 Event: 3
MET handle not valid 

%MSG

I cannot see any obvious difference in practice in the configuration of SoftMuHardJetMETSUSYmonitoring_jetPt in the baseline and in this PR, but I am certainly missing something. (That was probably already discussed in #36783, but I did not manage to scroll through the whole long thread in that closed PR).

PS: if a bug fix is applied, discovered thanks to this campaign, it is proably worth to write it in the PR description

@jfernan2
Copy link
Contributor

jfernan2 commented Mar 4, 2022

Hi @perrotta
Yes, this was commented in: #36783 (comment)

@NiharSaha please include the following in the PR description:

"MET hanlde not valid" message now disappears, this was due to a wrong collection set BEFORE the PR, since Event Interpretation has been removed back in 2021.
This PR corrected above by setting to met = "pfMet", right above. Since the SoftMuHardJetMETSUSYmonitoring is cloned several times below, it seems it just complained but no crash. Now it should be reading an existing collection.

Moreover, the binning for diphotonMass55ANDnoPV module was corrected set to 50-150 in https://github.com/cms-sw/cmssw/pull/37133/files#diff-f0c298feb52e2a455f0a07a4ea80c12b3b660e38e6737d177570ccc9a94775d3R165

since it was wrongly set to 90-200 due to a typo in https://github.com/cms-sw/cmssw/pull/37133/files#diff-f0c298feb52e2a455f0a07a4ea80c12b3b660e38e6737d177570ccc9a94775d3L147

@perrotta
Copy link
Contributor

perrotta commented Mar 4, 2022

Thank you @jfernan2 (indeed, now I remember that we discussed it already in the previous PR)
Since I was there, I updated the PR description with your suggestion myself

@perrotta
Copy link
Contributor

perrotta commented Mar 4, 2022

+1

@cmsbuild cmsbuild merged commit b6201e3 into cms-sw:master Mar 4, 2022
@cmsbuild cmsbuild mentioned this pull request Mar 4, 2022
Merged
@NiharSaha
Copy link
Contributor Author

Thank you @jfernan2 (indeed, now I remember that we discussed it already in the previous PR) Since I was there, I updated the PR description with your suggestion myself

Thanks, @perrotta for updating the PR description. I somehow missed @jfernan2's suggestion to update the description as I was traveling. Good to see that the PR is already merged. I guess I have implemented all the comments in this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
code-checks-approved dqm-approved fully-signed orp-approved tests-approved
Projects
None yet
Milestone
CMSSW_12_3_X
Development

Successfully merging this pull request may close these issues.
4 participants
@NiharSaha @cmsbuild @jfernan2 @perrotta