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

Improve SwitchProducer error message for inconsistent output products #31414

Merged
merged 1 commit into from Sep 10, 2020

Conversation

makortel
Copy link
Contributor

@makortel makortel commented Sep 9, 2020

PR description:

This PR resolves (partially) #31230. The exception message includes now a suggestion to use EDAlias, and lists the products (friendly class name and product instance name) for all cases, along

  SwitchProducer s has a case s@test1 that does not produce a product BranchKey
  (edmtestIntProduct, s, foo, TEST) that is produced by the chosen case
  s@test2. If the intention is to produce only a subset of the products listed
  below, each case with more products needs to be replacated with an EDAlias to
  only the necessary products, and the EDProducer itself needs to be moved to a
  Task.
  
  Products for case s@test1 (friendly class name, product instance name):
   edmtestIntProduct 
  Products for case s@test2 (friendly class name, product instance name):
   edmtestIntProduct 
   edmtestIntProduct foo

PR validation:

Framework unit tests succeed

@cmsbuild
Copy link
Contributor

cmsbuild commented Sep 9, 2020

The code-checks are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

cmsbuild commented Sep 9, 2020

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-31414/18281

@cmsbuild
Copy link
Contributor

cmsbuild commented Sep 9, 2020

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

It involves the following packages:

FWCore/Framework
FWCore/Integration

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

cms-bot commands are listed here

@makortel
Copy link
Contributor Author

makortel commented Sep 9, 2020

@cmsbuild, please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Sep 9, 2020

The tests are being triggered in jenkins.

REQUIRE_THROWS(edm::test::TestProcessor(config1),
Catch::Contains("with a product") && Catch::Contains("that is not produced by the chosen case"));
REQUIRE_THROWS_WITH(
edm::test::TestProcessor(config2),
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one (and similar change below) actually fix a bug in the test as well. This test should have used config2 already before, but was not caught with the Catch::Contains because the test mistakenly used REQUIRE_THROWS instead of REQUIRE_THROWS_WITH.

@fwyzard
Copy link
Contributor

fwyzard commented Sep 9, 2020

Thank, this will definitely come useful for building a combined CPU/GPU workflow for the Pixel, ECAL and HCAL workflows !

<< proc_pset.getParameter<edm::ParameterSet>(switchLabel)
.getUntrackedParameter<std::string>("@chosen_case")
<< ". If the intention is to produce only a subset of the products listed below, each case with "
"more products needs to be replacated with an EDAlias to only the necessary products, and the "
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

replacate -> replicate

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @wddgit! Although I probably meant "replaced".

@cmsbuild
Copy link
Contributor

cmsbuild commented Sep 9, 2020

+1
Tested at: 6860ce4
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-30a1aa/9236/summary.html
CMSSW: CMSSW_11_2_X_2020-09-09-1100
SCRAM_ARCH: slc7_amd64_gcc820

@cmsbuild
Copy link
Contributor

cmsbuild commented Sep 9, 2020

Comparison job queued.

@cmsbuild
Copy link
Contributor

cmsbuild commented Sep 9, 2020

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-30a1aa/9236/summary.html

Comparison Summary:

  • No significant changes to the logs found
  • Reco comparison results: 4 differences found in the comparisons
  • DQMHistoTests: Total files compared: 35
  • DQMHistoTests: Total histograms compared: 2609667
  • DQMHistoTests: Total failures: 7
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 2609638
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 34 files compared)
  • Checked 149 log files, 22 edm output root files, 35 DQM output files

…consistency exception

Also fix two mistakes in the unit test.
@cmsbuild
Copy link
Contributor

cmsbuild commented Sep 9, 2020

The code-checks are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

cmsbuild commented Sep 9, 2020

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-31414/18282

@cmsbuild
Copy link
Contributor

cmsbuild commented Sep 9, 2020

Pull request #31414 was updated. @makortel, @smuzaffar, @cmsbuild, @Dr15Jones can you please check and sign again.

@makortel
Copy link
Contributor Author

makortel commented Sep 9, 2020

@cmsbuild, please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Sep 9, 2020

The tests are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

+1
Tested at: c4c0f52
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-30a1aa/9238/summary.html
CMSSW: CMSSW_11_2_X_2020-09-09-1100
SCRAM_ARCH: slc7_amd64_gcc820

@cmsbuild
Copy link
Contributor

Comparison job queued.

@cmsbuild
Copy link
Contributor

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-30a1aa/9238/summary.html

Comparison Summary:

  • No significant changes to the logs found
  • Reco comparison results: 0 differences found in the comparisons
  • DQMHistoTests: Total files compared: 35
  • DQMHistoTests: Total histograms compared: 2609667
  • DQMHistoTests: Total failures: 1
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 2609644
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 34 files compared)
  • Checked 149 log files, 22 edm output root files, 35 DQM output files

@makortel
Copy link
Contributor Author

+1

@cmsbuild
Copy link
Contributor

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)

@silviodonato
Copy link
Contributor

+1

@cmsbuild cmsbuild merged commit 67425f0 into cms-sw:master Sep 10, 2020
@makortel makortel deleted the switchProducerTypesInException branch September 10, 2020 23:40
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

5 participants