es get() migration of L1CondDBPayloadWriter and L1CondDBPayloadWriterExt #36383

tvami · 2021-12-07T00:35:37Z

PR description:

Part of the migration in #31061

According to the comment #31061 (comment) this is the last DB signature for the es get() functions

PR validation:

Code compiles

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

N/A

cc @makortel

tvami · 2021-12-07T00:36:00Z

@cmsbuild , please test

tvami · 2021-12-07T00:44:03Z

code-checks

cmsbuild · 2021-12-07T01:17:38Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-36383/27162

This PR adds an extra 16KB to repository

cmsbuild · 2021-12-07T01:18:03Z

A new Pull Request was created by @tvami (Tamas Vami) for master.

It involves the following packages:

CondTools/L1Trigger (db, l1)
CondTools/L1TriggerExt (db)

@malbouis, @rekovic, @ggovi, @francescobrivio, @cecilecaillol, @tvami can you please review it and eventually sign? Thanks.
@mmusich 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

CondTools/L1TriggerExt/plugins/L1CondDBPayloadWriterExt.cc

makortel · 2021-12-07T01:44:45Z

Thanks @tvami! But I'm afraid the bodies of L1CondDBPayloadWriter and L1CondDBPayloadWriterExt are not sufficient. The EDModule report
https://cmssdt.cern.ch/SDT/jenkins-artifacts/ib-static-analysis/CMSSW_12_2_X_2021-12-05-0000/slc7_amd64_gcc900/reports/eventsetuprecord-get.yaml
shows call stack of e.g.

  - 'L1CondDBPayloadWriter::analyze(const edm::Event &, const edm::EventSetup &); l1t::DataWriter::writePayload(const edm::EventSetup &, const std::basic_string<char, std::char_traits<char>, std::allocator<char> > &); l1t::WriterProxy::save(const edm::EventSetup &) const; l1t::WriterProxyT<L1CaloEcalScaleRcd, L1CaloEcalScale>::save(const edm::EventSetup &) const; '

pointing to

cmssw/CondTools/L1Trigger/interface/WriterProxy.h

Lines 50 to 57 in ea84b43

    
           // load record and type from EventSetup and save them in db 
        
           edm::ESHandle<Type> handle; 
        
           try { 
        
             setup.get<Record>().get(handle); 
        
           } catch (l1t::DataAlreadyPresentException& ex) { 
        
             return std::string(); 
        
           }

(what exactly is the try-catch attempting to do?)

tvami · 2021-12-07T01:52:36Z

(what exactly is the try-catch attempting to do?)

Maybe @rekovic could comment? Or @hjkwon260 ?

cmsbuild · 2021-12-07T04:37:48Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-d91bad/21035/summary.html
COMMIT: ea84b43
CMSSW: CMSSW_12_2_X_2021-12-06-2300/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/36383/21035/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: 3250608
DQMHistoTests: Total failures: 0
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3250586
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

cmsbuild · 2021-12-07T13:29:09Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-36383/27182

This PR adds an extra 16KB to repository

cmsbuild · 2021-12-07T13:29:34Z

Pull request #36383 was updated. @malbouis, @cmsbuild, @rekovic, @ggovi, @francescobrivio, @cecilecaillol, @tvami can you please check and sign again.

tvami · 2021-12-07T13:33:51Z

@cmsbuild , please test

cmsbuild · 2021-12-07T15:59:07Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-36383/27189

This PR adds an extra 16KB to repository

cmsbuild · 2021-12-07T15:59:26Z

Pull request #36383 was updated. @malbouis, @rekovic, @ggovi, @francescobrivio, @cecilecaillol, @tvami can you please check and sign again.

cmsbuild · 2021-12-07T19:41:57Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-d91bad/21059/summary.html
COMMIT: 0e1479a
CMSSW: CMSSW_12_3_X_2021-12-06-2300/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/36383/21059/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: 42
DQMHistoTests: Total histograms compared: 3250608
DQMHistoTests: Total failures: 5
DQMHistoTests: Total nulls: 1
DQMHistoTests: Total successes: 3250580
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: -0.004 KiB( 41 files compared)
DQMHistoSizes: changed ( 312.0 ): -0.004 KiB MessageLogger/Warnings
Checked 177 log files, 37 edm output root files, 42 DQM output files
TriggerResults: no differences found

tvami · 2021-12-07T21:01:58Z

So I can't do more with this PR, any other updates need to come from the L1T team, and so that should happen in a separate PR

malbouis · 2021-12-07T21:42:24Z

+db

esConsume migration. Remaining of migration to be done by L1 team.
tests pass.

tvami · 2021-12-07T21:46:26Z

So I can't do more with this PR, any other updates need to come from the L1T team, and so that should happen in a separate PR

Hi @perrotta @qliphy so we have a fully technical PR migrating the last DB signature piece of the esConsumes migration. Please consider merging it w/o the L1T signature. They'll have to follow up about the WriterProxy anyway in another PR....

tvami · 2021-12-15T15:09:29Z

Hi @perrotta @qliphy I know it's funny to say that we should skip a L1T signature for a package that's almost fully under L1T.... but it's been 8 days with no news and this is a technical migration after all... Please let me know what you think!

perrotta · 2021-12-15T15:15:02Z

@rekovic please have a look and comment and/or sign

cecilecaillol · 2021-12-16T08:36:09Z

+l1

cmsbuild · 2021-12-16T08:36:26Z

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)

tvami · 2021-12-17T14:52:01Z

Hi @cecilecaillol thanks a lot for signing this! Can you please confirm that you'll take care of WriteProxyT in a follow-up PR? thanks!

perrotta · 2021-12-17T16:34:58Z

+1

@cms-sw/l1-l2 please arrange with @tvami for possible further cleanups of CondTools/L1Trigger/interface/WriterProxy.h, as discussed above

panoskatsoulis · 2022-04-17T09:18:14Z

Just to add some details from a private chat with @tvami. A challenge for migrating the helper code (WriteProxyT) is that the WriteProxyT instance (i.e. (record, type) pair) is constructed via a factory based on a string here

cmssw/CondTools/L1Trigger/src/DataWriter.cc

Line 17 in 3fe21cf

std::unique_ptr<WriterProxy> writer(factory->create(recordType + "@Writer"));

The string is passed in in

cmssw/CondTools/L1Trigger/plugins/L1CondDBPayloadWriter.cc

Line 136 in 3fe21cf

token = m_writer.writePayload(iSetup, it->first);

and the strings originate from another EventSetup product

cmssw/CondTools/L1Trigger/plugins/L1CondDBPayloadWriter.cc

Lines 116 to 117 in 3fe21cf

L1TriggerKey::RecordToKey::const_iterator it = key->recordToKeyMap().begin();

L1TriggerKey::RecordToKey::const_iterator end = key->recordToKeyMap().end();

cmssw/CondTools/L1Trigger/plugins/L1CondDBPayloadWriter.cc

Line 87 in 3fe21cf

key = iSetup.getHandle(l1TriggerKeyToken_);

which makes it impossible to call esConsumes() at the L1CondDBPayloadWriter module construction time.

Would it be feasible to specify these strings in configuration instead of an EventSetup product? @cms-sw/l1-l2

Also, @tvami can you please elaborate better on this issue?
It's not followed up and seems that creates problems on 12_3
Whatever is constructed by the factory, should have its own constructor, not?

tvami · 2022-04-17T15:12:33Z

Hi @panoskatsoulis

about the header question: the header is not removed, it's just simply moved into the cc file, as per the CMS coding rule 6.7
about es get() migration of L1CondDBPayloadWriter and L1CondDBPayloadWriterExt #36383 (comment) : @makortel probably could elaborate more, but I think the problem is that token = m_writer.writePayload(iSetup, it->first); should take the object directly in that code, instead of taking it through the factory. What is really the reason to have writePayload instead of a direct approach in l1conddbpayloadwriter.cc?

panoskatsoulis · 2022-04-17T15:36:47Z

Hi @tvami , I prepared a PR about this #37601
in short the problem was that there was not obvious way to run esConsume in some constructor when the Writer was being created by the Factory.
The main reason is that esConsumes is a method of ESConsumerBase from where every module in the cmssw inherits (but not the WriterProxy)
When I use it base class for WriterProxy (and ProxyT), i was able to use the ESGetToken without problem.

makortel · 2022-04-18T15:43:13Z

2. about es get() migration of L1CondDBPayloadWriter and L1CondDBPayloadWriterExt #36383 (comment) : @makortel probably could elaborate more, but I think the problem is that token = m_writer.writePayload(iSetup, it->first); should take the object directly in that code, instead of taking it through the factory. What is really the reason to have writePayload instead of a direct approach in l1conddbpayloadwriter.cc?

(mostly for the record here) The framework needs to know at EDModule construction time the EventSetup data products the EDModule consumes. Currently the WriterProxyT constructor is called from L1CondDBPayloadWriter::analyze(), which is too late. I outlined a possible way forward in #37602 (comment).

es get() migration of L1CondDBPayloadWriter and L1CondDBPayloadWriterExt

ea84b43

cmsbuild added this to the CMSSW_12_3_X milestone Dec 7, 2021

cmsbuild added code-checks-pending db-pending l1-pending orp-pending pending-signatures tests-started labels Dec 7, 2021

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

makortel reviewed Dec 7, 2021

View reviewed changes

CondTools/L1TriggerExt/plugins/L1CondDBPayloadWriterExt.cc Show resolved Hide resolved

cmsbuild added tests-approved and removed tests-started labels Dec 7, 2021

Remove L1CondDBPayloadWriterExt.h

e9025cb

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

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

cmsbuild added tests-approved and removed tests-started labels Dec 7, 2021

cmsbuild added db-approved and removed db-pending labels Dec 7, 2021

cmsbuild added fully-signed l1-approved and removed l1-pending pending-signatures labels Dec 16, 2021

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

cmsbuild merged commit 2e12139 into cms-sw:master Dec 17, 2021

mmusich mentioned this pull request Apr 17, 2022

Fix typo in L1T prescales files #37582

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

es get() migration of L1CondDBPayloadWriter and L1CondDBPayloadWriterExt #36383

es get() migration of L1CondDBPayloadWriter and L1CondDBPayloadWriterExt #36383

tvami commented Dec 7, 2021

tvami commented Dec 7, 2021

tvami commented Dec 7, 2021

cmsbuild commented Dec 7, 2021

cmsbuild commented Dec 7, 2021

makortel commented Dec 7, 2021

tvami commented Dec 7, 2021

cmsbuild commented Dec 7, 2021

cmsbuild commented Dec 7, 2021

cmsbuild commented Dec 7, 2021

tvami commented Dec 7, 2021

cmsbuild commented Dec 7, 2021

cmsbuild commented Dec 7, 2021

cmsbuild commented Dec 7, 2021

tvami commented Dec 7, 2021

malbouis commented Dec 7, 2021

tvami commented Dec 7, 2021

tvami commented Dec 15, 2021

perrotta commented Dec 15, 2021

cecilecaillol commented Dec 16, 2021

cmsbuild commented Dec 16, 2021

tvami commented Dec 17, 2021

perrotta commented Dec 17, 2021

panoskatsoulis commented Apr 17, 2022 •

edited

Loading

tvami commented Apr 17, 2022

panoskatsoulis commented Apr 17, 2022 •

edited

Loading

makortel commented Apr 18, 2022

es get() migration of L1CondDBPayloadWriter and L1CondDBPayloadWriterExt #36383

es get() migration of L1CondDBPayloadWriter and L1CondDBPayloadWriterExt #36383

Conversation

tvami commented Dec 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:

tvami commented Dec 7, 2021

tvami commented Dec 7, 2021

cmsbuild commented Dec 7, 2021

cmsbuild commented Dec 7, 2021

makortel commented Dec 7, 2021

tvami commented Dec 7, 2021

cmsbuild commented Dec 7, 2021

Comparison Summary

cmsbuild commented Dec 7, 2021

cmsbuild commented Dec 7, 2021

tvami commented Dec 7, 2021

cmsbuild commented Dec 7, 2021

cmsbuild commented Dec 7, 2021

cmsbuild commented Dec 7, 2021

Comparison Summary

tvami commented Dec 7, 2021

malbouis commented Dec 7, 2021

tvami commented Dec 7, 2021

tvami commented Dec 15, 2021

perrotta commented Dec 15, 2021

cecilecaillol commented Dec 16, 2021

cmsbuild commented Dec 16, 2021

tvami commented Dec 17, 2021

perrotta commented Dec 17, 2021

panoskatsoulis commented Apr 17, 2022 • edited Loading

tvami commented Apr 17, 2022

panoskatsoulis commented Apr 17, 2022 • edited Loading

makortel commented Apr 18, 2022

panoskatsoulis commented Apr 17, 2022 •

edited

Loading

panoskatsoulis commented Apr 17, 2022 •

edited

Loading