Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove unnecessary calls to consumesMany in DQMRootOutputModule #41166

Closed
wants to merge 1 commit into from

Conversation

wddgit
Copy link
Contributor

@wddgit wddgit commented Mar 23, 2023

PR description:

Remove unnecessary calls to consumesMany in DQMRootOutputModule.

DQMTokens are used to order execution of modules when one module
depends on the actions of another module. DQMRootOutputModule
only has write methods. It does not have endRun or endLumi methods
so there is no reason for it to consume endRun or endLumi DQMToken
products. The write methods are already sequenced by the Framework
to execute after endRun or endLumi methods that could produce DQMTokens
are finished.

This was motivated by the migration to remove all consumesMany calls
from CMSSW. That function has been deprecated.

FYI @makortel @Dr15Jones

PR validation:

There should be no change in behavior. No tests were added.

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-41166/34839

  • This PR adds an extra 12KB to repository

@cmsbuild
Copy link
Contributor

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

It involves the following packages:

  • DQMServices/FwkIO (dqm)

@emanueleusai, @cmsbuild, @syuvivida, @rvenditti, @micsucmed, @pmandrik can you please review it and eventually sign? Thanks.
@barvic this is something you requested to watch as well.
@perrotta, @dpiparo, @rappoccio you are the release manager for this.

cms-bot commands are listed here

@wddgit
Copy link
Contributor Author

wddgit commented Mar 23, 2023

please test

@makortel
Copy link
Contributor

hold

I'd expect the tests to fail (but became curious to see if that actually happens).

@cmsbuild
Copy link
Contributor

Pull request has been put on hold by @makortel
They need to issue an unhold command to remove the hold state or L1 can unhold it for all

@cmsbuild cmsbuild added the hold label Mar 23, 2023
@cmsbuild
Copy link
Contributor

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-20de0c/31574/summary.html
COMMIT: c3833ea
CMSSW: CMSSW_13_1_X_2023-03-23-1100/el8_amd64_gcc11
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/41166/31574/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

  • You potentially removed 13 lines from the logs
  • Reco comparison results: 2 differences found in the comparisons
  • DQMHistoTests: Total files compared: 49
  • DQMHistoTests: Total histograms compared: 3552750
  • DQMHistoTests: Total failures: 1
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3552727
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 48 files compared)
  • Checked 213 log files, 164 edm output root files, 49 DQM output files
  • TriggerResults: no differences found

@makortel
Copy link
Contributor

Interesting, it worked. Looking from history, I'd guess I added the consumesMany() calls in #29586 in response to test failures in #29553 (comment) (other possibly relevant comment in that PR #29553 (comment)). I'd like to understand why this PR is not causing problems. Are all DQM modules put in Sequences instead of Tasks?

@emanueleusai
Copy link
Member

I believe this to be the case.

@emanueleusai
Copy link
Member

+1

@makortel
Copy link
Contributor

I think it would be dangerous to rely on all DQM modules to be in Sequences.

@wddgit
Copy link
Contributor Author

wddgit commented Apr 3, 2023

@makortel (I'm back from my vacation.)

We exchanged private emails with one comment from you that probably belongs in these PR comments instead. So I'm including it next:

"The consumption of the DQMTokens is indeed not needed for scheduling, but to make the dependence between DQMRootOutputModule and DQM EDModules visible so the DQM EDModules do not get removed by the deletion of non-consumed unscheduled modules." I didn't know about that second usage of DQMTokens.

Does this mean I should I close this PR and generate a new one adding support for GetterOfProducts in OutputModules and use that approach instead?

@makortel
Copy link
Contributor

makortel commented Apr 3, 2023

Hi @wddgit

Does this mean I should I close this PR and generate a new one adding support for GetterOfProducts in OutputModules and use that approach instead?

Yes, I think we should do that.

@wddgit wddgit closed this Apr 3, 2023
@wddgit wddgit deleted the removeUnnecessaryCall branch August 25, 2023 22:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants