DRAFT: Base classes for refactor of CondTools/Hcal PopConAnalyzers and PopConSourceHandlers

[JanChyczynski]

PR description:

Reviewing #49556 I found several problems with proposed CondTools/Hcal/plugins/HcalPulseDelaysPopConAnalyzer.cc‎ and many other (~40) PopConAnalyzers in CondTools/Hcal.

This PR introduced a base class for these PopConAnalyzers and corresponding PopConSourceHandlers that fix the problems. As an example CastorElectronicsMapPopConAnalyzer is refactored using this base class.
Note: initObject was renamed to initPayload

With HCalPulseDelays as example, the problems are:

1. Defnining duplicate members m_source and m_populator results in 2 separate instances of these fields (e.g. HcalPulseDelaysPopConAnalyzer::m_source and PopConAnalyzer::m_source ). Its not a proper use of inheritance and is dangerous and error-prone with any future changes (eg. access to m_source from the base PopConAnalyzer will access a different instance of the source).
2. Re-implementing write() is a (potentially unecessary) code duplication
3. myDBObject should not be defined as a raw pointer. Running HcalPulseDelaysPopConAnalyzer.analyze mutliple times (i.e. for multiple events) would result with a memory leak. It would be much better to use unique_ptr and std::move when calling source.initObject() to transfer the ownership. Accordingly initObject() should take a unique_ptr as the parameter to express taking ownership.

Problem 3. is easy to fix by changing myDbObject and the parameter of initObject to unique_ptr .

Because m_source, m_populator, write and endJob are private is tricky to fix point 1. and 2. My idea is to move source.initObject(myDBObject); to .analyze(...) . This would allow not to redefine write() and using source() getter would allow not to redefine m_source.

It needs to be confirmed that this would result in the same logic and behaviour. The difference I see is that it would be then run for every event instead of once, in endJob() . From what I understand HcalPulseDelaysPopConAnalyzer (and other PopConAnalyzers with the same pattern) is not meant to be run for multiple events (as multiple execution of anaylze would result in a memory leak), thus this change of behaviour resulting from moving initObject to analyze is not a problem. If this is not appropriate, let me know and I'll present an alternative solution.

PR validation:

Tested by running dbwriteCastorElectronicsMap_cfg.py modified for writing to local sqlite and confirming that it wrote a payload to the db.

If this PR is a backport please specify the original PR and why you need to backport that PR. If this PR will be backported please specify to which release cycle the backport is meant for:

Before submitting your pull requests, make sure you followed this checklist:

• verify that the PR is really intended for the chosen branch
• verify that changes follow CMS Naming, Coding, And Style Rules
• verify that the PR passes the basic test procedure suggested in the CMSSW PR instructions

[cmsbuild]

cms-bot internal usage

[cmsbuild]

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-49905/47643

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-49905/47643/code-format.patch
  e.g. curl -k https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-49905/47643/code-format.patch | patch -p1
  You can also run scram build code-format to apply code format directly

[cmsbuild]

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-49905/47662

[cmsbuild]

Pull request #49905 was updated.

[perrotta]

Why are you removing the header guard?

[JanChyczynski]

I included the #pragma once header guard - is it ill-advised in CMSSW?

[perrotta]

Uhm, no idea... Probably you are right, even though I wouldn't (personally) modify it in already existing header files

[cmsbuild]

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-49905/47701

• There are other open Pull requests which might conflict with changes you have proposed:
  • File FWCore/Framework/test/stubs/ToyAnalyzers.cc modified in PR(s): [RFC] descriptions.addDefault -> descriptions.addWithDefaultLabel #44372

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-49905/47701/code-format.patch
  e.g. curl -k https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-49905/47701/code-format.patch | patch -p1
  You can also run scram build code-format to apply code format directly

[JanChyczynski]

I have a question:
I picked CastorElectronicsMap PopCon for implementing the example of using these base classes at random, not knowing anything about it besides reading the code. I've tested it by running dbwriteCastorElectronicsMap_cfg.py modified for writing to local sqlite and confirming that it wrote a payload to the db. Should I keep this as the example or should I remove changes to the CastorElectronicsMap classes before the merge?

Side note: initially I picked CastorChannelQuality but its test in dbwriteCastorChannelQuality_cfg.py was crashing even before my changes so I tried a different one.

[cmsbuild]

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-49905/47702

[cmsbuild]

Pull request #49905 was updated.

[perrotta]

I have a question: I picked CastorElectronicsMap PopCon for implementing the example of using these base classes at random, not knowing anything about it besides reading the code. I've tested it by running dbwriteCastorElectronicsMap_cfg.py modified for writing to local sqlite and confirming that it wrote a payload to the db. Should I keep this as the example or should I remove changes to the CastorElectronicsMap classes before the merge?

Having an example to look at is certainly preferable (I bet nobody but you will port the new code without such an example to refere at). On the other hand, Castor is some quite old code, and probably nobody is using and maintaining it, iow it is not even know if it still works as expected.

I don't know how many files are affected in cmssw? Is that a migration that you can afford doing yourself (for example, in this very same PR), or there are so many files involved, linked to different systems, that would suggest delegating it to the expert in the different areas? In the second case it would take longer, and a github issue should be opened to track it.

[JanChyczynski]

Having an example to look at is certainly preferable (I bet nobody but you will port the new code without such an example to refere at). On the other hand, Castor is some quite old code, and probably nobody is using and maintaining it, iow it is not even know if it still works as expected.

I don't know how many files are affected in cmssw? Is that a migration that you can afford doing yourself (for example, in this very same PR), or there are so many files involved, linked to different systems, that would suggest delegating it to the expert in the different areas? In the second case it would take longer, and a github issue should be opened to track it.

It involves around 40 PopCon Analyzers, so 120 files. I don't think it makes sense to inlude everything in this PR. Also #49556 would depend on these changes so I wouldn't delay merging this.