Navigation Menu

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

Call callWhenNewProductsRegistered() callbacks only for branches that are present #38872

Merged

Conversation

makortel
Copy link
Contributor

PR description:

This is a preparatory PR towards #38806. This change avoids user surprises when the BranchDescription no longer has type information for dropped branches. An analysis of all current users of callWhenNewProductsRegistered() in #38806 (comment) showed that all of them either explicitly limited themselves to present branches, or implicilty assumed the branch is present. I can't think of any useful use case for user code to make decisions based on dropped branches. For inspecting product history up to dropped products provenance is likely better way.

I made this change as a separate PR so that the change in behavior becomes clearer in release notes, and we can see any possible side effect in the IBs separately from #38806.

PR validation:

Framework unit tests pass.

… are present

This change avoids user surprises when the BranchDescription no longer
has type information for dropped branches. An analysis of all current
users of callWhenNewProductsRegistered() showed that all of them
either explicitly limited themselves to present branches, or
implicilty assumed the branch is present. I can't think of any useful
use case for user code to make decisions based on dropped branches.
For inspecting product history up to dropped products provenance is
likely better way.
@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-38872/31293

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @makortel (Matti Kortelainen) for master.

It involves the following packages:

  • DataFormats/Provenance (core)
  • FWCore/Framework (core)

@cmsbuild, @smuzaffar, @Dr15Jones, @makortel can you please review it and eventually sign? Thanks.
@wddgit, @rovere this is something you requested to watch as well.
@perrotta, @dpiparo, @qliphy, @rappoccio you are the release manager for this.

cms-bot commands are listed here

@Dr15Jones
Copy link
Contributor

looks good to me

@makortel
Copy link
Contributor Author

@cmsbuild, please test

@makortel
Copy link
Contributor Author

@cmsbuild, please abort

@makortel
Copy link
Contributor Author

@cmsbuild, please test

@cmsbuild
Copy link
Contributor

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-155cbd/26530/summary.html
COMMIT: 41452dc
CMSSW: CMSSW_12_5_X_2022-07-29-1100/el8_amd64_gcc10
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/38872/26530/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: 51
  • DQMHistoTests: Total histograms compared: 3668050
  • DQMHistoTests: Total failures: 12
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3668016
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 50 files compared)
  • Checked 210 log files, 47 edm output root files, 51 DQM output files
  • TriggerResults: no differences found

@makortel
Copy link
Contributor Author

4.53 shows the same difference as in #38603.

@makortel
Copy link
Contributor Author

makortel commented Aug 3, 2022

@cmsbuild, please test

Refresh to see if the comparison differences are gone

@cmsbuild
Copy link
Contributor

cmsbuild commented Aug 3, 2022

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-155cbd/26621/summary.html
COMMIT: 41452dc
CMSSW: CMSSW_12_5_X_2022-08-03-1100/el8_amd64_gcc10
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/38872/26621/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: 6 differences found in the comparisons
  • DQMHistoTests: Total files compared: 51
  • DQMHistoTests: Total histograms compared: 3691510
  • DQMHistoTests: Total failures: 13
  • DQMHistoTests: Total nulls: 1
  • DQMHistoTests: Total successes: 3691474
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: -0.004 KiB( 50 files compared)
  • DQMHistoSizes: changed ( 312.0 ): -0.004 KiB MessageLogger/Warnings
  • Checked 212 log files, 49 edm output root files, 51 DQM output files
  • TriggerResults: no differences found

@makortel
Copy link
Contributor Author

makortel commented Aug 3, 2022

+1

Comparisons are now clean.

@cmsbuild
Copy link
Contributor

cmsbuild commented Aug 3, 2022

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

@qliphy
Copy link
Contributor

qliphy commented Aug 4, 2022

+1

@cmsbuild cmsbuild merged commit 57ca824 into cms-sw:master Aug 4, 2022
@makortel makortel deleted the callWhenNewProductsRegisteredPresentOnly branch August 5, 2022 13:38
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