Initialize BranchDescription from Dictionary only if the branch is present #38806

makortel · 2022-07-20T21:09:44Z

PR description:

This PR limits the BranchDescription's ROOT dicrionary part be initialized only if the branch is present. This change allows a job to process a file that contains products whose parent product (that is not in the file, was either transient or dropped) dictionary does not exist anymore in the job. Resolves #38781.

I'm uncertain about the usefulness of the test added in this PR. It is sufficient to demonstrate that the changes of this PR work as intended, but it has some serious flaws for longer term.

PR validation:

Problematic job runs, framework unit tests pass.

cmsbuild · 2022-07-20T21:17:03Z

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-38806/31164

This PR adds an extra 68KB to repository
There are other open Pull requests which might conflict with changes you have proposed:
- File DataFormats/Provenance/interface/BranchDescription.h modified in PR(s): Add Transformer ability to modules #38454
- File IOPool/Input/src/PoolSource.cc modified in PR(s): Implement support for concurrent runs in the Framework #38801

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

cmsbuild · 2022-07-20T21:36:23Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-38806/31165

This PR adds an extra 68KB to repository
There are other open Pull requests which might conflict with changes you have proposed:
- File DataFormats/Provenance/interface/BranchDescription.h modified in PR(s): Add Transformer ability to modules #38454
- File IOPool/Input/src/PoolSource.cc modified in PR(s): Implement support for concurrent runs in the Framework #38801

cmsbuild · 2022-07-20T21:36:45Z

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

It involves the following packages:

DataFormats/Common (core)
DataFormats/Provenance (core)
DataFormats/TestObjects (core)
FWCore/Framework (core)
IOPool/Input (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

makortel · 2022-07-20T21:44:43Z

@cmsbuild, please test

makortel · 2022-07-20T21:47:48Z

@Dr15Jones @wddgit Can you think of any ill side effects that might not be covered by our unit tests?

wddgit · 2022-07-20T21:51:22Z

I'll take a look at it.

makortel · 2022-07-20T21:51:33Z

IOPool/Input/test/testNoParentDictionary.sh

+# Copy DataFormats/TestObjects from local/release
+# Plus minimal set of packages to run the test for the first time
+# TODO: this test doesn't really work on PRs that have code outside of
+# these packages that would break the functionality


Any ideas to improve this test are welcome. I tried to to symlink all files (except those matching to DataFormatsTestObjects) from ${OLD_CMSSW_BASE}/lib/${SCRAM_ARCH} into ${CMSSW_BASE}/lib/${SCRAM_ARCH} but that alone was not sufficient (the step2 job below failed as the code changes were not propagated).

Maybe this test could be sufficient until @smuzaffar gets back.

How about just manipulating what the PluginManager reads instead of having to do another compilation?

Put the dictionary generation of TransientIntParentT<1> and TransientIntParentT<2> into two different classes_def files so they are in different plugins

Write using TransientIntParentT<1> as you did

manipulate LD_LIBRARY_PATH to add a new directory to the beginning which has a .edmplugincache which lies about where to get TransientIntParentT<1> (points to a file that doesn't contain that dictionary)

I'm not sure I understood properly. The dictionaries end up in libraries, not plugins (right?). I can't quickly think of a way to play tricks with libraries and LD_LIBRARY_PATH without a second compilation, but maybe adjusting LD_LIBRARY_PATH with the second compilation would at least help to overcome the main deficiency of the current test.

Arg, I was thinking about ancient days where plugin manager provided the dictionaries. Still, the same idea might work if one manipulated a .rootmap file and LD_LIBRARY_PATH.

I finally figured out one way to improve the test such that it should work now in any PR/IB, fails in the IB without this PR, and does not require scram b.

cmsbuild · 2022-07-21T06:05:29Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-384df2/26357/summary.html
COMMIT: fa0f541
CMSSW: CMSSW_12_5_X_2022-07-20-1100/el8_amd64_gcc10
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/38806/26357/install.sh to create a dev area with all the needed externals and cmssw changes.

CMS deprecated warnings: 9 CMS deprecated warnings found, see summary page for details.

Comparison Summary

Summary:

No significant changes to the logs found
Reco comparison results: 4 differences found in the comparisons
DQMHistoTests: Total files compared: 50
DQMHistoTests: Total histograms compared: 3662417
DQMHistoTests: Total failures: 17
DQMHistoTests: Total nulls: 1
DQMHistoTests: Total successes: 3662377
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: -0.004 KiB( 49 files compared)
DQMHistoSizes: changed ( 312.0 ): -0.004 KiB MessageLogger/Warnings
Checked 208 log files, 45 edm output root files, 50 DQM output files
TriggerResults: no differences found

Dr15Jones · 2022-07-21T13:33:39Z

DataFormats/Provenance/interface/BranchDescription.h

@@ -158,9 +158,11 @@ namespace edm {
      BranchID switchAliasForBranchID_;

      // A TypeWithDict object for the wrapped object
+      // This is set if and only if the dropped_ is false


Would we do away with the use of TypeWithDict here for a later PR?

I would do that in a later PR.

Dr15Jones · 2022-07-21T13:34:45Z

DataFormats/Common/src/setIsMergeable.cc

@@ -17,7 +17,8 @@ namespace edm {
    // data member can only be true for run or lumi products.
    // It defaults to false. Also if it is true that means it
    // was already set.
-    if (desc.branchType() == InRun || desc.branchType() == InLumi) {
+    // Set it only for branches that are present
+    if (desc.present() and (desc.branchType() == InRun or desc.branchType() == InLumi)) {


Would we try to remove the concept of mergeable from BranchDescription in a later PR?

I would do that in a later PR.

Dr15Jones · 2022-07-21T13:40:15Z

IOPool/Input/test/testNoParentDictionary.sh

+# Copy DataFormats/TestObjects from local/release
+# Plus minimal set of packages to run the test for the first time
+# TODO: this test doesn't really work on PRs that have code outside of
+# these packages that would break the functionality


How about just manipulating what the PluginManager reads instead of having to do another compilation?

Put the dictionary generation of TransientIntParentT<1> and TransientIntParentT<2> into two different classes_def files so they are in different plugins

Write using TransientIntParentT<1> as you did

manipulate LD_LIBRARY_PATH to add a new directory to the beginning which has a .edmplugincache which lies about where to get TransientIntParentT<1> (points to a file that doesn't contain that dictionary)

wddgit · 2022-07-21T18:55:57Z

We already discussed that we keep BranchDescriptions for kept products and their ancestors. There is one other case where we also keep the BranchDescription. If there is a BranchAlias and we keep the alias BranchDescription, then we also keep the original BranchDescription. I'm not sure that this affects anything... Just mentioning it because we didn't include that when we were discussing it earlier.

wddgit · 2022-07-21T20:17:15Z

Is there anything that inhibits callWhenNewProductsRegistered from being called on BranchDescriptions not present? Code like this would fail in that case.

https://cmssdt.cern.ch/dxr/CMSSW/source/RecoMuon/L3MuonProducer/src/L3TkMuonProducer.cc#38

cmsbuild · 2022-08-16T22:11:07Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-38806/31595

This PR adds an extra 16KB to repository
There are other open Pull requests which might conflict with changes you have proposed:
- File DataFormats/Provenance/interface/BranchDescription.h modified in PR(s): Add Transformer ability to modules #38454
- File IOPool/Input/src/PoolSource.cc modified in PR(s): Implement support for concurrent runs in the Framework #38801

cmsbuild · 2022-08-16T22:11:32Z

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

makortel · 2022-08-16T22:25:14Z

@cmsbuild, please test

Last test after squashing the commits together

cmsbuild · 2022-08-17T06:02:46Z

-1

Failed Tests: UnitTests
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-384df2/26863/summary.html
COMMIT: a6ad1db
CMSSW: CMSSW_12_5_X_2022-08-16-2300/el8_amd64_gcc10
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/38806/26863/install.sh to create a dev area with all the needed externals and cmssw changes.

CMS deprecated warnings: 9 CMS deprecated warnings found, see summary page for details.

Unit Tests

I found errors in the following unit tests:

---> test testTauEmbeddingProducers had ERRORS

Comparison Summary

Summary:

No significant changes to the logs found
Reco comparison results: 2 differences found in the comparisons
DQMHistoTests: Total files compared: 51
DQMHistoTests: Total histograms compared: 3692500
DQMHistoTests: Total failures: 8
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3692470
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 50 files compared)
Checked 212 log files, 49 edm output root files, 51 DQM output files
TriggerResults: no differences found

makortel · 2022-08-17T13:07:19Z

+1

The unit test fails already in the IBs and is not related to this PR.

cmsbuild · 2022-08-17T13:07:49Z

This pull request is fully signed and it will be integrated in one of the next master IBs (but tests are reportedly failing). 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)

perrotta · 2022-08-17T15:41:47Z

+1

Unit test failure is also in the IB, and it will get fixed independently by [Fix] Make IncludeDY optional #39090

perrotta · 2022-08-17T15:43:30Z

merge

cmsbuild added this to the CMSSW_12_5_X milestone Jul 20, 2022

cmsbuild added code-checks-pending core-pending orp-pending pending-signatures tests-pending labels Jul 20, 2022

makortel mentioned this pull request Jul 20, 2022

Initialize BranchDescription from Dictionary only if the branch is present cms-sw/framework-team#417

Closed

2 tasks

cmsbuild added code-checks-rejected and removed code-checks-pending labels Jul 20, 2022

makortel force-pushed the transientParentDictionary branch from 38cce2e to fa0f541 Compare July 20, 2022 21:29

cmsbuild added code-checks-pending and removed code-checks-rejected labels Jul 20, 2022

cmsbuild added code-checks-approved and removed code-checks-pending labels Jul 20, 2022

cmsbuild added tests-started and removed tests-pending labels Jul 20, 2022

makortel commented Jul 20, 2022

View reviewed changes

makortel mentioned this pull request Jul 20, 2022

ROOT Dictionary Error For CUDADataFormats #38781

Closed

cmsbuild added tests-approved and removed tests-started labels Jul 21, 2022

Dr15Jones reviewed Jul 21, 2022

View reviewed changes

makortel force-pushed the transientParentDictionary branch from d629b72 to a6ad1db Compare August 16, 2022 22:03

cmsbuild added code-checks-pending tests-pending and removed tests-approved code-checks-approved labels Aug 16, 2022

cmsbuild added code-checks-approved and removed code-checks-pending labels Aug 16, 2022

cmsbuild added tests-started and removed tests-pending labels Aug 16, 2022

cmsbuild added tests-rejected and removed tests-started labels Aug 17, 2022

cmsbuild added core-approved fully-signed and removed core-pending pending-signatures labels Aug 17, 2022

cmsbuild added orp-approved and removed orp-pending labels Aug 17, 2022

cmsbuild merged commit 553b55a into cms-sw:master Aug 17, 2022

This was referenced Aug 17, 2022

Update cuda to 11.7.1 for gcc12 cms-sw/cmsdist#8025

Closed

[GCC Flags] drop -Wstrict-overflow which has been deprecated since gcc8 cms-sw/cmsdist#8026

Merged

[llvm] Update to version 14.0.6 cms-sw/cmsdist#7729

Closed

makortel deleted the transientParentDictionary branch August 18, 2022 17:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initialize BranchDescription from Dictionary only if the branch is present #38806

Initialize BranchDescription from Dictionary only if the branch is present #38806

makortel commented Jul 20, 2022

cmsbuild commented Jul 20, 2022

cmsbuild commented Jul 20, 2022

cmsbuild commented Jul 20, 2022

makortel commented Jul 20, 2022

makortel commented Jul 20, 2022

wddgit commented Jul 20, 2022

makortel Jul 20, 2022

Dr15Jones Jul 21, 2022

makortel Jul 21, 2022

Dr15Jones Jul 21, 2022

makortel Aug 6, 2022

cmsbuild commented Jul 21, 2022

Dr15Jones Jul 21, 2022

makortel Jul 21, 2022

Dr15Jones Jul 21, 2022

makortel Jul 21, 2022

Dr15Jones Jul 21, 2022

wddgit commented Jul 21, 2022

wddgit commented Jul 21, 2022

cmsbuild commented Aug 16, 2022

cmsbuild commented Aug 16, 2022

makortel commented Aug 16, 2022

cmsbuild commented Aug 17, 2022

makortel commented Aug 17, 2022

cmsbuild commented Aug 17, 2022

perrotta commented Aug 17, 2022

perrotta commented Aug 17, 2022

Initialize BranchDescription from Dictionary only if the branch is present #38806

Initialize BranchDescription from Dictionary only if the branch is present #38806

Conversation

makortel commented Jul 20, 2022

PR description:

PR validation:

cmsbuild commented Jul 20, 2022

cmsbuild commented Jul 20, 2022

cmsbuild commented Jul 20, 2022

makortel commented Jul 20, 2022

makortel commented Jul 20, 2022

wddgit commented Jul 20, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmsbuild commented Jul 21, 2022

Comparison Summary

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wddgit commented Jul 21, 2022

wddgit commented Jul 21, 2022

cmsbuild commented Aug 16, 2022

cmsbuild commented Aug 16, 2022

makortel commented Aug 16, 2022

cmsbuild commented Aug 17, 2022

Unit Tests

Comparison Summary

makortel commented Aug 17, 2022

cmsbuild commented Aug 17, 2022

perrotta commented Aug 17, 2022

perrotta commented Aug 17, 2022