Migration of pixel code to esConsumes

[ferencek]

PR description:

This PR addresses the esConsumes migration (#31061) for the SiPixel code. This is a purely framework-related update so no changes in the output are expected. In the process some limited cleanup of header file includes was done. As a by-product, the code in RecoLocalTracker/SiPixelClusterizer/test/ was fully migrated to consumes (there were still a lot of getByLabel calls which were migrated to getByToken). Code touched in #31697 intentionally skipped in this PR.

PR validation:

Code compiles and explicitly tested by running workflow 4.53.

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

No backport needed.

[cmsbuild]

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-32093/19739

• This PR adds an extra 96KB to repository

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

[ferencek]

@makortel, here the token is defined with the EndLuminosityBlock transition because the event setup is accessed in SiPixelEDAClient::dqmEndLuminosityBlock(...). However, the edm::ESHandle assigned in the SiPixelEDAClient::dqmEndLuminosityBlock(...) is later also used in SiPixelEDAClient::dqmEndJob(...). I am not sure if this could have any unwanted implications.

[makortel]

Unfortunately use of EventSetup products at endJob() (or at endProcessBlockProduce() to which dqmEndJob() translates now) is not allowed (framework may clean up the object before reaching that point).

Best option would be to move all computations using SiPixelFedCablingMap to dqmEndLuminosityBlock(). If that is not possible (or feasible), I believe the best workaround would be to copy the SiPixelFedCablingMap (or necessary parts of it if the full object is no not needed) from dqmEndLuminosityBlock() to dqmEndJob().

[ferencek]

Here is my attempt at implementing a workaround 6c41889

[ferencek]

@makortel, how to handle edm::ESHandle in SiPixelClusterModule::book(...) given the fact that this is eventually called from SiPixelClusterSource::bookHistograms...)? I assume a edm::ConsumesCollector can be defined in SiPixelClusterSource and passed on to the ctor of SiPixelClusterModule but what transition type would be appropriate here?

[makortel]

how to handle edm::ESHandle in SiPixelClusterModule::book(...) given the fact that this is eventually called from SiPixelClusterSource::bookHistograms...)? I assume a edm::ConsumesCollector can be defined in SiPixelClusterSource and passed on to the ctor of SiPixelClusterModule but what transition type would be appropriate here?

Correct, edm::ConsumesCollector can be used also for esConsumes(). The bookHistograms() is always called in the beginRun() of the base class, so the transition is edm::Transition::BeginRun.

[ferencek]

how to handle edm::ESHandle in SiPixelClusterModule::book(...) given the fact that this is eventually called from SiPixelClusterSource::bookHistograms...)? I assume a edm::ConsumesCollector can be defined in SiPixelClusterSource and passed on to the ctor of SiPixelClusterModule but what transition type would be appropriate here?

Correct, edm::ConsumesCollector can be used also for esConsumes(). The bookHistograms() is always called in the beginRun() of the base class, so the transition is edm::Transition::BeginRun.

OK, I implemented this in 226be17

[cmsbuild]

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-32093/19771

• This PR adds an extra 152KB to repository

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

[ferencek]

(accepted a wrong autocomplete).

right, this unfortunately means that some else is now receiving notifications from this thread. Try to avoid that next time.

Unfortunately yes, my bad. Hopefully they know how to unwatch a thread ;)

The factorization was intended to make people's lives easier now and potentially simplify any future integration into the official CMSSW code.

my point is that the code which is present there has some amount of overlap with what is currently present in cmssw, so in order to avoid maintaining twice the same code in two repos, the one in cmssw could be deleted as your choice was to factor code out of it anyway.

This would require a more extensive audit of the pixel code so perhaps something to think about.

We cannot rectify the pixel code history in this PR.

absolutely, just a suggestion for the future ;)

I would add for the future in an ideal world ;)

[makortel]

Perhaps @makortel has a suggestion?

If the class is not used, and not intended to be used in the future, the easiest "fix" would be to remove it.

If the class needs to stay for some reason, passing the ESProduct in the function is a viable option. But I'd suggest to leave the ESHandle out of the interface (unless for some reason the function itself should throw an exception in case of an invalid ESHandle).

[ferencek]

Perhaps @makortel has a suggestion?

If the class is not used, and not intended to be used in the future, the easiest "fix" would be to remove it.

If the class needs to stay for some reason, passing the ESProduct in the function is a viable option. But I'd suggest to leave the ESHandle out of the interface (unless for some reason the function itself should throw an exception in case of an invalid ESHandle).

@mmusich, @tsusa, @tvami, do you know who might be the best person to ask about removing this class. Perhaps @dkotlins knows since he commented out this class in RecoLocalTracker/SiPixelRecHits/interface/PixelCPEGeneric.h in this commit 13716b1#diff-05be511f209b7f09de4edad2afede653443033d9a218a6a719f7ac190ef37634

[mmusich]

Just reporting the outcome of the private discussion within the DPG.
SiPixelCPEGenericDBErrorParametrization and friends are used to populate DB objects of the type SiPixelCPEGenericErrorParm.
This CondFormat is what we used to have to assign CPE errors before the GenError from templates existed.
It is now deprecated and we're essentially waiting to see all the Global Tags to be cleaned up before removing the Condition format and associated record, and later clean all the client code.

[ferencek]

Just reporting the outcome of the private discussion within the DPG.
SiPixelCPEGenericDBErrorParametrization and friends are used to populate DB objects of the type SiPixelCPEGenericErrorParm.
This CondFormat is what we used to have to assign CPE errors before the GenError from templates existed.
It is now deprecated and we're essentially waiting to see all the Global Tags to be cleaned up before removing the Condition format and associated record, and later clean all the client code.

And just to add, the PR is therefore complete and ready for review and sign-off by the relevant code responsibles.

[jpata]

+reconstruction

• technical, observed changes only in the workflow 11642.911 which do not come from this PR
• changes in reco areas are minimal and in line with the description

[jfernan2]

+1

[civanch]

+1

[christopheralanwest]

+1

[cmsbuild]

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. @silviodonato, @dpiparo, @qliphy (and backports should be raised in the release meeting by the corresponding L2)

[qliphy]

+1