Migrate JetMET and PF codes to new PoolDBOutputService methods #36214

laurenhay · 2021-11-23T09:05:11Z

PR description:

As introduced in issue #28, we need to migrate to the new methods replacing writeOne --> writeOneIOV, createNewIOV --> createOneIOV, and appendSinceTime --> appendOneIOV.
The codes containing PoolDBOutputService in JetMET that were addressed are:
JetMETCorrections/Modules/plugins/JetCorrectorDBWriter.cc
JetMETCorrections/Modules/plugins/JetResolutionDBWriter.cc
JetMETCorrections/Modules/plugins/METCorrectorDBWriter.cc
JetMETCorrections/Modules/plugins/QGLikelihoodDBWriter.cc
JetMETCorrections/Modules/plugins/QGLikelihoodSystematicsDBWriter.cc

JetMETCorrections/FFTJetModules/plugins/FFTJetCorrectorDBWriter.cc was addressed already in #35781
Planned to also address RecoParticleFlow/PFClusterTools/test/ProducePFCalibrationObject.cc but this was addressed in #36196

PR validation:

CMSSW compiles
scram b runtests
cmsRun JetCorrectionDBWriter_cfg.py runs without issue

Backport:

NA

@kirschen @panwarlsweet @juska

cmsbuild · 2021-11-23T09:11:04Z

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-36214/26823

This PR adds an extra 20KB to repository
There are other open Pull requests which might conflict with changes you have proposed:
- File RecoParticleFlow/PFClusterTools/test/ProducePFCalibrationObject.cc modified in PR(s): Migrate misc scripts to new PoolDBOutputService methods #36196

Code check has found code style and quality issues which could be resolved by applying following patch(s)

code-format:
https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-36214/26823/code-format.patch
e.g. curl -k https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-36214/26823/code-format.patch | patch -p1
You can also run scram build code-format to apply code format directly

cmsbuild · 2021-11-23T09:58:46Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-36214/26824

This PR adds an extra 20KB to repository
There are other open Pull requests which might conflict with changes you have proposed:
- File RecoParticleFlow/PFClusterTools/test/ProducePFCalibrationObject.cc modified in PR(s): Migrate misc scripts to new PoolDBOutputService methods #36196

cmsbuild · 2021-11-23T09:59:07Z

A new Pull Request was created by @laurenhay (Lauren Hay) for master.

It involves the following packages:

JetMETCorrections/Modules (reconstruction)
RecoParticleFlow/PFClusterTools (reconstruction)

@jpata, @cmsbuild, @slava77 can you please review it and eventually sign? Thanks.
@rappoccio, @gouskos, @ahinzmann, @schoef, @mmarionncern, @lgray, @jdamgov, @jdolen, @nhanvtran, @gkasieczka, @clelange, @hatakeyamak, @mariadalfonso, @seemasharmafnal, @cbernet 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

slava77 · 2021-11-23T13:41:43Z

@cmsbuild please test

tvami · 2021-11-23T15:27:01Z

JetMETCorrections/Modules/plugins/JetResolutionDBWriter.cc

@@ -46,7 +46,7 @@ void JetResolutionDBWriter::beginJob() {
  if (s.isAvailable()) {
    std::cout << "Setting up payload record " << m_record << std::endl;
    cond::Time_t sinceTime = s->isNewTagRequest(m_record) ? s->beginOfTime() : s->currentTime();
-    s->writeOne<JME::JetResolutionObject>(jerObject, sinceTime, m_record);
+    s->writeOneIOV<JME::JetResolutionObject>(jerObject, sinceTime, m_record);


Suggested change

s->writeOneIOV<JME::JetResolutionObject>(jerObject, sinceTime, m_record);

s->writeOneIOV(jerObject, sinceTime, m_record);

This would also work

tvami · 2021-11-23T15:28:37Z

assign db

cmsbuild · 2021-11-23T15:28:56Z

New categories assigned: db

@ggovi,@francescobrivio,@malbouis,@tvami you have been requested to review this Pull request/Issue and eventually sign? Thanks

francescobrivio · 2021-11-23T15:36:12Z

JetMETCorrections/Modules/plugins/JetCorrectorDBWriter.cc

    else
-      s->appendSinceTime<JetCorrectorParametersCollection>(payload, 111, payloadTag);
+      s->appendOneIOV<JetCorrectorParametersCollection>(payload, 111, payloadTag);


May I ask what is the point of using a since 111 ? Is it just a placeholder instead of using beginOfTime or currentTime?

I believe this was a placeholder for currentTime(), but after reading the slides again I realize I can work around this by replacing this section with s->writeOneIOV so I'm doing this now.

cmsbuild · 2021-11-23T16:46:03Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-a370d9/20708/summary.html
COMMIT: 11bd492
CMSSW: CMSSW_12_2_X_2021-11-23-1100/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/36214/20708/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: 0 differences found in the comparisons
DQMHistoTests: Total files compared: 42
DQMHistoTests: Total histograms compared: 3247025
DQMHistoTests: Total failures: 0
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3247003
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 41 files compared)
Checked 177 log files, 37 edm output root files, 42 DQM output files
TriggerResults: no differences found

tvami · 2021-12-02T13:34:40Z

+db

code chance in line w PR description
no failures in the Jenkins tests

cmsbuild · 2021-12-02T13:35:06Z

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)

slava77 · 2021-12-02T16:04:38Z

@smuzaffar
in this case we had some code from JetMETCorrections/Modules (fully in reco) moved to a non-reco category.
I'd expect that the bot should still require a reco signature to approve the move.
Please check if the label logic can be updated.
Thank you.

smuzaffar · 2021-12-02T16:29:37Z

ah right, looks like bot ignore the deleted file category ( though it properly mentioned it in #36214 (comment) ) . I will fix this @slava77

tvami · 2021-12-02T16:37:48Z

@slava77 should we put this PR to hold?
@laurenhay can you please put back the fully RECO code to the original placement? I really meant to just move the DB related code under CondTools

cmsbuild · 2021-12-02T16:58:30Z

Pull request #36214 was updated. @jpata, @slava77 can you please check and sign again.

smuzaffar · 2021-12-02T17:00:51Z

@slava77 , bot is fixed now and it should consider also the renamed files.

jpata · 2021-12-03T08:16:03Z

I have the impression from the commits that only ALCA-related code was moved (DBReader, DBWriter) from the reco package, which sounds perfectly reasonable.

It was just the fact that reco sig was not formally required to move code (reco-related or not) out of reco packages that needed to be addressed, and I think this was already quickly fixed by Shahzad.

So when I look what's remaining under https://github.com/cms-sw/cmssw/tree/2fee4242c25ca83143f2db99a4504ec749a37d13/JetMETCorrections/Modules/plugins, it's now more clearly reco code.

I don't think this PR needs to be held up by reco, but let me know if I missed something.

tvami · 2021-12-03T13:07:26Z

@jpata thanks, in that case, can you please sign the PR? (since now the signature is required from your side)

jpata · 2021-12-03T13:41:50Z

+reconstruction

technical for reco, moves ALCA-related modules out of a JetMETCorrections/Modules
no reco changes expected/observed

cmsbuild · 2021-12-03T13:42:16Z

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)

perrotta · 2021-12-04T09:52:06Z

+1

Technical

cmsbuild added this to the CMSSW_12_2_X milestone Nov 23, 2021

cmsbuild added code-checks-pending orp-pending pending-signatures reconstruction-pending tests-pending labels Nov 23, 2021

cmsbuild added code-checks-rejected and removed code-checks-pending labels Nov 23, 2021

code checks and removing PF changes addressed in other PR

11bd492

cmsbuild added code-checks-pending and removed code-checks-rejected labels Nov 23, 2021

cmsbuild added code-checks-approved and removed code-checks-pending labels Nov 23, 2021

laurenhay force-pushed the DBOutput_PFandJetMET branch from 5f70209 to 11bd492 Compare November 23, 2021 10:11

laurenhay marked this pull request as ready for review November 23, 2021 10:13

cmsbuild added tests-started and removed tests-pending labels Nov 23, 2021

tvami reviewed Nov 23, 2021

View reviewed changes

cmsbuild added the db-pending label Nov 23, 2021

francescobrivio reviewed Nov 23, 2021

View reviewed changes

cmsbuild added tests-approved and removed tests-started labels Nov 23, 2021

Replacing createOneIOV+appendOneIOV blocks with writeOneIOV

633accd

cmsbuild added tests-approved and removed new-package-pending labels Dec 2, 2021

cmsbuild added db-approved fully-signed and removed db-pending pending-signatures labels Dec 2, 2021

cmsbuild added pending-signatures reconstruction-pending and removed fully-signed labels Dec 2, 2021

cmsbuild added fully-signed reconstruction-approved and removed reconstruction-pending pending-signatures labels Dec 3, 2021

cmsbuild added orp-approved and removed orp-pending labels Dec 4, 2021

cmsbuild merged commit 04ffa39 into cms-sw:master Dec 4, 2021

This was referenced Dec 5, 2021

Move to root 6.24 cms-sw/cmsdist#7445

Merged

[ASAN] fix unit tests by setting NO_TEST_PREFIX flag for ASAN IB #36365

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate JetMET and PF codes to new PoolDBOutputService methods #36214

Migrate JetMET and PF codes to new PoolDBOutputService methods #36214

laurenhay commented Nov 23, 2021 •

edited

Loading

cmsbuild commented Nov 23, 2021

cmsbuild commented Nov 23, 2021

cmsbuild commented Nov 23, 2021

slava77 commented Nov 23, 2021

tvami Nov 23, 2021

tvami commented Nov 23, 2021

cmsbuild commented Nov 23, 2021

francescobrivio Nov 23, 2021

laurenhay Nov 25, 2021

cmsbuild commented Nov 23, 2021

tvami commented Dec 2, 2021

cmsbuild commented Dec 2, 2021

slava77 commented Dec 2, 2021

smuzaffar commented Dec 2, 2021

tvami commented Dec 2, 2021

cmsbuild commented Dec 2, 2021

smuzaffar commented Dec 2, 2021

jpata commented Dec 3, 2021

tvami commented Dec 3, 2021

jpata commented Dec 3, 2021

cmsbuild commented Dec 3, 2021

perrotta commented Dec 4, 2021

	s->writeOneIOV<JME::JetResolutionObject>(jerObject, sinceTime, m_record);
	s->writeOneIOV(jerObject, sinceTime, m_record);

Migrate JetMET and PF codes to new PoolDBOutputService methods #36214

Migrate JetMET and PF codes to new PoolDBOutputService methods #36214

Conversation

laurenhay commented Nov 23, 2021 • edited Loading

PR description:

PR validation:

Backport:

cmsbuild commented Nov 23, 2021

cmsbuild commented Nov 23, 2021

cmsbuild commented Nov 23, 2021

slava77 commented Nov 23, 2021

tvami Nov 23, 2021

Choose a reason for hiding this comment

tvami commented Nov 23, 2021

cmsbuild commented Nov 23, 2021

francescobrivio Nov 23, 2021

Choose a reason for hiding this comment

laurenhay Nov 25, 2021

Choose a reason for hiding this comment

cmsbuild commented Nov 23, 2021

Comparison Summary

tvami commented Dec 2, 2021

cmsbuild commented Dec 2, 2021

slava77 commented Dec 2, 2021

smuzaffar commented Dec 2, 2021

tvami commented Dec 2, 2021

cmsbuild commented Dec 2, 2021

smuzaffar commented Dec 2, 2021

jpata commented Dec 3, 2021

tvami commented Dec 3, 2021

jpata commented Dec 3, 2021

cmsbuild commented Dec 3, 2021

perrotta commented Dec 4, 2021

laurenhay commented Nov 23, 2021 •

edited

Loading