Fix isProcessWideService

[wddgit]

PR description:

Bug fix for the function isProcessWideService. It was incorrectly implemented in some Services.

For the most part this bug should not affect anything. This affects only jobs using SubProcess in the special case when certain Services have been defined in multiple SubProcesses. There is nothing that I am aware of currently that uses the SubProcess feature. Production does not. RelVal tests do not.

We might consider in the future improving the interface for this because 5 of the 10 services using this function had it implemented incorrectly. Although it probably is not worth the effort given how rare it is that a new service with this function is created... (I happened to notice because I was testing a new service. This problem was not reported by a user.)

The requirements are:

1. Definition of isProcessWideService must come before including ServiceMaker.h
2. Must be outside of the relevant class
3. Must be in the edm::service namespace

PR validation:

I manually tested all 10 of the relevant services. This should have no effect on existing unit tests and RelVal tests. I added to an existing SubProcess unit test verifying the behavior for 7 of the services.

[cmsbuild]

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-38279/30428

• This PR adds an extra 28KB to repository

• There are other open Pull requests which might conflict with changes you have proposed:

  • File FWCore/Services/plugins/CPU.cc modified in PR(s): Implement ResourceInformationService #37831

[cmsbuild]

A new Pull Request was created by @wddgit (W. David Dagenhart) for master.

It involves the following packages:

• CommonTools/UtilAlgos (reconstruction)
• FWCore/Services (core)
• IgTools/IgProf (core)

@smuzaffar, @Dr15Jones, @makortel, @clacaputo, @cmsbuild, @slava77, @jpata can you please review it and eventually sign? Thanks.
@makortel, @felicepantaleo 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

[wddgit]

please test

[Dr15Jones]

@wddgit Thanks for making the fix. Do you think it would be worth it to make a unit test for this functionality? Not necessarily testing them all, just one or two?

[wddgit]

Hmmmmm. If the point is to test the implementation in each service we should test them all... And it would take some thinking to come up with a way to test this and complicate the service with added test code. Hardly seems worth the effort given that we hardly use SubProcess at all to begin with. Mostly if it is correct when the Service is implemented, then it should stay that way. The most likely failure mode is someone noticing the include near the end of the service file and "cleaning it up" by moving it to the beginning or reordering the includes to alphabetize thinking they are helping...

If the point is to test the mechanism, then I could write a test service that throws if is it is constructed twice, define the function and then put it in a couple levels of a job with a SubProcess. Is that good enough?

[wddgit]

An idea. I could just configure the ones with parameter set validation with invalid parameters in the inner SubProcess. If they are ever constructed, then they fail parameter set validation...

[wddgit]

abort test

[wddgit]

I added to an existing unit to verify the behavior of 7 of the 10 functions with the function. Of the other 3, two don't have parameter set validation which the test depends on, and DependencyGraph ran really slow so I skipped it.

[cmsbuild]

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-38279/30429

• This PR adds an extra 40KB to repository

• There are other open Pull requests which might conflict with changes you have proposed:

  • File FWCore/Services/plugins/CPU.cc modified in PR(s): Implement ResourceInformationService #37831

[cmsbuild]

Pull request #38279 was updated. @smuzaffar, @Dr15Jones, @makortel, @clacaputo, @cmsbuild, @slava77, @jpata can you please check and sign again.

[wddgit]

please test

[cmsbuild]

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-b75f95/25348/summary.html
COMMIT: 12f4cf3
CMSSW: CMSSW_12_5_X_2022-06-07-2300/el8_amd64_gcc10
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/38279/25348/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: 50
• DQMHistoTests: Total histograms compared: 3652078
• DQMHistoTests: Total failures: 2
• DQMHistoTests: Total nulls: 0
• DQMHistoTests: Total successes: 3652054
• DQMHistoTests: Total skipped: 22
• DQMHistoTests: Total Missing objects: 0
• DQMHistoSizes: Histogram memory added: 0.0 KiB( 49 files compared)
• Checked 208 log files, 45 edm output root files, 50 DQM output files
• TriggerResults: no differences found

[Dr15Jones]

type bug-fix

[Dr15Jones]

+1

[clacaputo]

+reconstruction

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

[qliphy]

+1