Fix BranchChildren/Parentage problems that occur with SubProcess and EDAlias #17950

wddgit · 2017-03-15T22:04:14Z

Also adds unit tests to cover these cases.

…EDAlias Also adds unit tests to cover these cases.

In the previous commit, I added pruning of BranchChildren. It was a nice little algorithm and I wanted to get it into the git history. But after I wrote it I realized one cannot prune with this simple algorithm with secondary file input.

cmsbuild · 2017-03-15T22:04:33Z

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

It involves the following packages:

DataFormats/Provenance
FWCore/Framework
FWCore/Integration
IOPool/Input
IOPool/Output

@cmsbuild, @smuzaffar, @Dr15Jones, @davidlange6 can you please review it and eventually sign? Thanks.
@Martin-Grunewald, @wmtan this is something you requested to watch as well.
@Muzaffar, @davidlange6, @smuzaffar you are the release manager for this.

cms-bot commands are listed here #13028

Dr15Jones · 2017-03-15T23:46:15Z

please test

cmsbuild · 2017-03-15T23:46:31Z

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/18470/console Started: 2017/03/16 00:46

cmsbuild · 2017-03-16T08:30:38Z

-1

Tested at: 6cb2b78

You can see the results of the tests here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-17950/18470/summary.html

I found follow errors while testing this PR

Failed tests: UnitTests

Unit Tests:

I found errors in the following unit tests:

---> test TestSubProcess had ERRORS

cmsbuild · 2017-03-16T08:30:40Z

Comparison job queued.

cmsbuild · 2017-03-16T11:16:27Z

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

There are some workflows for which there are errors in the baseline:
25.0 step 5
The results for the comparisons for these workflows could be incomplete
This means most likely that the IB is having errors in the relvals.The error does NOT come from this pull request

Dr15Jones · 2017-03-16T12:54:04Z

Please test

cmsbuild · 2017-03-16T12:54:21Z

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/18489/console Started: 2017/03/16 15:03

Dr15Jones

Done with my review.

Dr15Jones · 2017-03-16T13:48:24Z

DataFormats/Provenance/interface/Provenance.h

@@ -45,6 +45,7 @@ namespace edm {

    ProductProvenance const* productProvenance() const;
    BranchID const& branchID() const {return stable().branchID();}
+    BranchID const& originalBranchID() const {return stable().originalBranchID();}


Please add a comment explaining the difference between branchID and originalBranchID

Dr15Jones · 2017-03-16T13:53:22Z

FWCore/Framework/src/ScheduleItems.cc

@@ -36,6 +38,7 @@ namespace edm {
      preg_(new SignallingProductRegistry(preg)),
      branchIDListHelper_(new BranchIDListHelper),
      thinnedAssociationsHelper_(new ThinnedAssociationsHelper),
+      subProcessParentageHelper_(new SubProcessParentageHelper),


How about switching to std::make_shared<>()? That is actually more memory efficient

Dr15Jones · 2017-03-16T13:54:09Z

FWCore/Framework/src/ScheduleItems.cc

@@ -141,6 +144,7 @@ namespace edm {
                     *preg_,
                     *branchIDListHelper_,
                     *thinnedAssociationsHelper_,
+                     subProcessParentageHelper_ ? &*subProcessParentageHelper_ : nullptr,


probably simpler to do subProcessParentageHelper_ ? subProcessParentageHelper_.get() : nullptr

Dr15Jones · 2017-03-16T13:57:52Z

IOPool/Input/src/RootFile.h

@@ -227,7 +227,7 @@ namespace edm {
    std::shared_ptr<LuminosityBlockAuxiliary> fillLumiAuxiliary();
    std::shared_ptr<RunAuxiliary> fillRunAuxiliary();
    std::string const& newBranchToOldBranch(std::string const& newBranch) const;
-    void markBranchToBeDropped(bool dropDescendants, BranchID const& branchID, std::set<BranchID>& branchesToDrop) const;
+    void markBranchToBeDropped(bool dropDescendants, BranchDescription const& branchy, std::set<BranchID>& branchesToDrop, std::map<BranchID, BranchID> const& droppedToKeptAlias) const;


branchy seems like an odd name

Dr15Jones · 2017-03-20T14:22:16Z

@davidlange6 as far as David Dagenhart and I can tell, the unit test failure is from the problem fixed by #17946 and not from this pull request.

davidlange6 · 2017-03-20T14:35:43Z

please test ok, lets test again

…

On Mar 20, 2017, at 3:22 PM, Chris Jones ***@***.***> wrote: @davidlange6 as far as David Dagenhart and I can tell, the unit test failure is from the problem fixed by #17946 and not from this pull request. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

cmsbuild · 2017-03-20T14:36:03Z

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/18562/console Started: 2017/03/20 16:51

cmsbuild · 2017-03-20T19:42:34Z

+1
Tested at: 33148b9
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-17950/18562/summary.html

cmsbuild · 2017-03-20T19:42:38Z

Comparison job queued.

davidlange6 · 2017-03-20T20:32:24Z

+1

cmsbuild · 2017-03-20T23:08:10Z

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

The intent is that dropOnInput removes entries in the ProductRegistry in addition to dropping the products. The recent commit inadvertently disabled that for branches that were already dropped. And also this exposed the fact that GetterOfProducts requires dictionaries for all products in the ProductRegistry, even the dropped ones. And this should not be necessary and wastes a little CPU, so that is fixed also.

Fix problem introduced by recent PR #17950 related to dropOnInput

dmitrijus · 2017-03-24T10:39:52Z

Hi @wddgit, can this be backported to 90x?

This problem causes a segfault in IOPool/Streamer modules (when using data from GEN-SIM), this PR fixes it.

wddgit · 2017-03-24T15:47:03Z

@dmitrijus We can probably backport the change, but we are interested in understanding how and why the seg fault is occurring. Can you tell me how to reproduce it so I can investigate this? If you already know exactly how the seg fault is occurring, can you explain it? Thanks.

dmitrijus · 2017-03-24T19:29:46Z

@wddgit
To reproduce merge #18066 and use
cmsRun DQMStreamerOutputRepackerTest_cfg.py <any_root_with_mc_data>.root

The crash is a (double) null-pointer dereference in this line,
https://github.com/cms-sw/cmssw/blob/master/IOPool/Streamer/src/StreamSerializer.cc#L149

wddgit · 2017-03-24T21:13:07Z

@Dr15Jones I reproduced the problem. If you write a file containing an EDAlias to a ROOT format output file and then in a subsequent process you read that file and try to run the streamer output module, then a seg fault occurs. The problem is indeed in the line he pointed to in StreamSerializer.cc. It successfully get the product using the EDAlias, then using the EDAlias BranchID tries to get the ProductProvenance, but that is stored keyed on the original BranchID not the EDAlias BranchID so it fails and returns a nullptr. The streamer output module does not check the pointer and dereferences and there is a seg fault. This was fixed by this PR in 9_1_X, but not backported to 9_0_X

I reproduced the error running a Framework unit test to make the file with the EDAlias
(cmsRun FWCore/Integration/test/testGetBy1_cfg.py)

and the following to read it and seg fault. It has nothing to do with the DQM PR.

import FWCore.ParameterSet.Config as cms

process = cms.Process("PROD2")

process.source = cms.Source("PoolSource",
    fileNames = cms.untracked.vstring(
        'file:testGetBy1.root'
    )
)

process.out = cms.OutputModule("EventStreamFileWriter",
    fileName = cms.untracked.string('testSeriesOfProcessesHLT.dat'),
    compression_level = cms.untracked.int32(1),
    use_compression = cms.untracked.bool(True),
    max_event_size = cms.untracked.int32(7000000)
)

process.e = cms.EndPath(process.out)

Should I backport the entire PR (and the other related PR that followed) to 9_0_X? This has probably been there for a while. It is somewhat unusual to run streamer output on a file containing an EDAlias and that is probably why no one noticed before.

wddgit · 2017-03-24T21:15:40Z

I did not try it but this might also fail if you dropped the parentage and tried to run streamer output. So it could be looked at as also a bug in streamer output.

Dr15Jones · 2017-03-24T21:24:12Z

Let's start with a fix to the Streamer. Both in 9_1 and 9_0.

wddgit · 2017-03-27T15:53:05Z

I'm working on fixing the Streamer IO. It actually is not built to handle a nullptr for the ProductProvenance at all so this is more than a one or two line fix. I'll need to trace this all the way through streamer input and output and then make several little fixes in the entire streamer input and output chain to make it work.

dmitrijus · 2017-03-28T12:05:55Z

@wddgit Thanks!

wddgit · 2017-03-30T17:32:50Z

@dmitrijus I submitted pull requests for the Streamer Output Module problem. PR #18126 was already merged into 9_1_X and PR #18135 is the back port to 9_0_X which was just submitted (not merged yet). These should be sufficient to avoid the seg fault you saw.
@Dr15Jones My understanding is that we are not backporting this PR because we want to maintain stability in 9_0_X, avoid the effort to resolve conflicts, and the risk associated with it. Let me know if you want me to backport this also.

wddgit added 2 commits March 15, 2017 16:25

Fix BranchChildren/Parentage problems that occur with SubProcess and …

6e61eb6

…EDAlias Also adds unit tests to cover these cases.

Remove pruning of BranchChildren

6cb2b78

In the previous commit, I added pruning of BranchChildren. It was a nice little algorithm and I wanted to get it into the git history. But after I wrote it I realized one cannot prune with this simple algorithm with secondary file input.

cmsbuild added this to the Next CMSSW_9_1_X milestone Mar 15, 2017

cmsbuild added comparison-pending core-pending orp-pending pending-signatures tests-pending labels Mar 15, 2017

cmsbuild added tests-started and removed tests-pending labels Mar 15, 2017

cmsbuild added tests-rejected and removed tests-started labels Mar 16, 2017

cmsbuild added comparison-available and removed comparison-pending labels Mar 16, 2017

cmsbuild added comparison-pending tests-pending and removed comparison-available tests-rejected labels Mar 16, 2017

cmsbuild added tests-started and removed tests-pending labels Mar 16, 2017

Dr15Jones suggested changes Mar 16, 2017

View reviewed changes

Resolve Chris's comments for last two commits

33148b9

cmsbuild added tests-pending tests-started and removed tests-started tests-pending labels Mar 20, 2017

cmsbuild added tests-approved and removed tests-started labels Mar 20, 2017

cmsbuild added orp-approved and removed orp-rejected labels Mar 20, 2017

cmsbuild merged commit ae40518 into cms-sw:master Mar 20, 2017

cmsbuild added comparison-available and removed comparison-pending labels Mar 20, 2017

cmsbuild added a commit that referenced this pull request Mar 22, 2017

Merge pull request #18022 from wddgit/from-CMSSW_9_1_X_2017-03-20-2300

c3b387a

Fix problem introduced by recent PR #17950 related to dropOnInput

wddgit deleted the fixBranchChildren branch August 2, 2017 14:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix BranchChildren/Parentage problems that occur with SubProcess and EDAlias #17950

Fix BranchChildren/Parentage problems that occur with SubProcess and EDAlias #17950

wddgit commented Mar 15, 2017

cmsbuild commented Mar 15, 2017

Dr15Jones commented Mar 15, 2017

cmsbuild commented Mar 15, 2017 •

edited

cmsbuild commented Mar 16, 2017

cmsbuild commented Mar 16, 2017

cmsbuild commented Mar 16, 2017

Dr15Jones commented Mar 16, 2017

cmsbuild commented Mar 16, 2017 •

edited

Dr15Jones left a comment

Dr15Jones Mar 16, 2017

Dr15Jones Mar 16, 2017

Dr15Jones Mar 16, 2017

Dr15Jones Mar 16, 2017

Dr15Jones commented Mar 20, 2017

davidlange6 commented Mar 20, 2017 via email

cmsbuild commented Mar 20, 2017 •

edited

cmsbuild commented Mar 20, 2017

cmsbuild commented Mar 20, 2017

davidlange6 commented Mar 20, 2017

cmsbuild commented Mar 20, 2017

dmitrijus commented Mar 24, 2017 •

edited

wddgit commented Mar 24, 2017

dmitrijus commented Mar 24, 2017

wddgit commented Mar 24, 2017

wddgit commented Mar 24, 2017

Dr15Jones commented Mar 24, 2017

wddgit commented Mar 27, 2017

dmitrijus commented Mar 28, 2017

wddgit commented Mar 30, 2017

Fix BranchChildren/Parentage problems that occur with SubProcess and EDAlias #17950

Fix BranchChildren/Parentage problems that occur with SubProcess and EDAlias #17950

Conversation

wddgit commented Mar 15, 2017

cmsbuild commented Mar 15, 2017

Dr15Jones commented Mar 15, 2017

cmsbuild commented Mar 15, 2017 • edited

cmsbuild commented Mar 16, 2017

cmsbuild commented Mar 16, 2017

cmsbuild commented Mar 16, 2017

Dr15Jones commented Mar 16, 2017

cmsbuild commented Mar 16, 2017 • edited

Dr15Jones left a comment

Choose a reason for hiding this comment

Dr15Jones Mar 16, 2017

Choose a reason for hiding this comment

Dr15Jones Mar 16, 2017

Choose a reason for hiding this comment

Dr15Jones Mar 16, 2017

Choose a reason for hiding this comment

Dr15Jones Mar 16, 2017

Choose a reason for hiding this comment

Dr15Jones commented Mar 20, 2017

davidlange6 commented Mar 20, 2017 via email

cmsbuild commented Mar 20, 2017 • edited

cmsbuild commented Mar 20, 2017

cmsbuild commented Mar 20, 2017

davidlange6 commented Mar 20, 2017

cmsbuild commented Mar 20, 2017

dmitrijus commented Mar 24, 2017 • edited

wddgit commented Mar 24, 2017

dmitrijus commented Mar 24, 2017

wddgit commented Mar 24, 2017

wddgit commented Mar 24, 2017

Dr15Jones commented Mar 24, 2017

wddgit commented Mar 27, 2017

dmitrijus commented Mar 28, 2017

wddgit commented Mar 30, 2017

cmsbuild commented Mar 15, 2017 •

edited

cmsbuild commented Mar 16, 2017 •

edited

cmsbuild commented Mar 20, 2017 •

edited

dmitrijus commented Mar 24, 2017 •

edited