Merge pull request #17752 from wddgit/fixProvenanceRetriever

Fix parentage provenance in corner case
cms-sw · Mar 7, 2017 · b07e1ba · b07e1ba
2 parents 79af08b + fff43f6
commit b07e1ba
Show file tree

Hide file tree

Showing 10 changed files with 174 additions and 5 deletions.
diff --git a/DataFormats/Provenance/interface/ProductProvenanceRetriever.h b/DataFormats/Provenance/interface/ProductProvenanceRetriever.h
@@ -67,6 +67,8 @@ namespace edm {
 
     void mergeProvenanceRetrievers(std::shared_ptr<ProductProvenanceRetriever> other);
 
+    void mergeParentProcessRetriever(ProductProvenanceRetriever const& provRetriever);
+
     void deepCopy(ProductProvenanceRetriever const&);
 
     void reset();
@@ -82,6 +84,7 @@ namespace edm {
     mutable tbb::concurrent_unordered_set<ProductProvenance, ProductProvenanceHasher, ProductProvenanceEqual> entryInfoSet_;
     mutable std::atomic<const std::set<ProductProvenance>*> readEntryInfoSet_;
     edm::propagate_const<std::shared_ptr<ProductProvenanceRetriever>> nextRetriever_;
+    edm::propagate_const<ProductProvenanceRetriever const*> parentProcessRetriever_;
     std::shared_ptr<const ProvenanceReaderBase> provenanceReader_;
     unsigned int transitionIndex_;
   };

diff --git a/DataFormats/Provenance/src/ProductProvenanceRetriever.cc b/DataFormats/Provenance/src/ProductProvenanceRetriever.cc
@@ -14,6 +14,7 @@ namespace edm {
       entryInfoSet_(),
       readEntryInfoSet_(),
       nextRetriever_(),
+      parentProcessRetriever_(nullptr),
       provenanceReader_(),
       transitionIndex_(iTransitionIndex){
   }
@@ -22,6 +23,7 @@ namespace edm {
       entryInfoSet_(),
       readEntryInfoSet_(),
       nextRetriever_(),
+      parentProcessRetriever_(nullptr),
       provenanceReader_(reader.release()),
       transitionIndex_(std::numeric_limits<unsigned int>::max())
   {
@@ -86,6 +88,7 @@ namespace edm {
     if(nextRetriever_) {
       nextRetriever_->reset();
     }
+    parentProcessRetriever_ = nullptr;
   }
 
   void
@@ -102,11 +105,19 @@ namespace edm {
     nextRetriever_ = other;
   }
 
+  void
+  ProductProvenanceRetriever::mergeParentProcessRetriever(ProductProvenanceRetriever const& provRetriever) {
+    parentProcessRetriever_ = &provRetriever;
+  }
+
   ProductProvenance const*
   ProductProvenanceRetriever::branchIDToProvenance(BranchID const& bid) const {
     ProductProvenance ei(bid);
     auto it = entryInfoSet_.find(ei);
     if(it == entryInfoSet_.end()) {
+      if (parentProcessRetriever_) {
+        return parentProcessRetriever_->branchIDToProvenance(bid);
+      }
       //check in source
       readProvenance();
       auto ptr =readEntryInfoSet_.load();

diff --git a/FWCore/Framework/interface/EventPrincipal.h b/FWCore/Framework/interface/EventPrincipal.h
@@ -75,7 +75,8 @@ namespace edm {
                             EventSelectionIDVector&& eventSelectionIDs,
                             BranchListIndexes&& branchListIndexes,
                             ProductProvenanceRetriever const& provRetriever,
-                            DelayedReader* reader = nullptr);
+                            DelayedReader* reader = nullptr,
+                            bool deepCopyRetriever = true);
 
 
     void clearEventPrincipal();

diff --git a/FWCore/Framework/src/EventPrincipal.cc b/FWCore/Framework/src/EventPrincipal.cc
@@ -63,9 +63,14 @@ namespace edm {
         EventSelectionIDVector&& eventSelectionIDs,
         BranchListIndexes&& branchListIndexes,
         ProductProvenanceRetriever const& provRetriever,
-        DelayedReader* reader) {
+        DelayedReader* reader,
+        bool deepCopyRetriever) {
     eventSelectionIDs_ = eventSelectionIDs;
-    provRetrieverPtr_->deepCopy(provRetriever);
+    if (deepCopyRetriever) {
+      provRetrieverPtr_->deepCopy(provRetriever);
+    } else {
+      provRetrieverPtr_->mergeParentProcessRetriever(provRetriever);
+    }
     branchListIndexes_ = branchListIndexes;
     if(branchIDListHelper_->hasProducedProducts()) {
       // Add index into BranchIDListRegistry for products produced this process

diff --git a/FWCore/Framework/src/SubProcess.cc b/FWCore/Framework/src/SubProcess.cc
@@ -352,12 +352,14 @@ namespace edm {
     processHistoryRegistry.registerProcessHistory(principal.processHistory());
     BranchListIndexes bli(principal.branchListIndexes());
     branchIDListHelper_->fixBranchListIndexes(bli);
+    bool deepCopyRetriever = false;
     ep.fillEventPrincipal(aux,
                           processHistoryRegistry,
                           std::move(esids),
                           std::move(bli),
                           *(principal.productProvenanceRetrieverPtr()),//NOTE: this transfers the per product provenance
-                          principal.reader());
+                          principal.reader(),
+                          deepCopyRetriever);
     ep.setLuminosityBlockPrincipal(principalCache_.lumiPrincipalPtr());
     propagateProducts(InEvent, principal, ep);
 

diff --git a/FWCore/Integration/test/BuildFile.xml b/FWCore/Integration/test/BuildFile.xml
@@ -188,7 +188,7 @@
     <use   name="FWCore/Framework"/>
     <use   name="FWCore/MessageLogger"/>
   </library>
-  <library   file="TestFindProduct.cc,ManyProductProducer.cc" name="FWCoreIntegrationTestFindProduct">
+  <library   file="TestFindProduct.cc,ManyProductProducer.cc,TestParentage.cc" name="FWCoreIntegrationTestFindProduct">
     <flags   EDM_PLUGIN="1"/>
     <use   name="FWCore/Framework"/>
     <use   name="FWCore/ParameterSet"/>

diff --git a/FWCore/Integration/test/TestParentage.cc b/FWCore/Integration/test/TestParentage.cc
@@ -0,0 +1,113 @@
+
+#include "DataFormats/Common/interface/Handle.h"
+#include "DataFormats/Provenance/interface/BranchID.h"
+#include "DataFormats/Provenance/interface/Parentage.h"
+#include "DataFormats/Provenance/interface/ProductProvenance.h"
+#include "DataFormats/Provenance/interface/ProductProvenanceRetriever.h"
+#include "DataFormats/Provenance/interface/Provenance.h"
+#include "DataFormats/TestObjects/interface/ToyProducts.h"
+#include "FWCore/Framework/interface/EDAnalyzer.h"
+#include "FWCore/Framework/interface/Event.h"
+#include "FWCore/Framework/interface/MakerMacros.h"
+#include "FWCore/ParameterSet/interface/ParameterSet.h"
+#include "FWCore/Utilities/interface/InputTag.h"
+#include "FWCore/Utilities/interface/EDGetToken.h"
+
+#include <iostream>
+#include <set>
+#include <string>
+#include <vector>
+
+namespace {
+  void getAncestors(edm::Event const& e,
+                    edm::BranchID const& branchID,
+                    std::set<edm::BranchID>& ancestors) {
+    edm::Provenance prov = e.getProvenance(branchID);
+    for (auto const& parent : prov.productProvenance()->parentage().parents()) {
+      ancestors.insert(parent);
+      getAncestors(e, parent, ancestors);
+    }
+  }
+
+  // Does the same thing as the previous function in a different
+  // way. The previous function goes through the links in the
+  // ProductsResolver which for SubProcesses could lead to a different
+  // retriever. In SubProcesses, the following function follows the
+  // links in the retrievers themselves. Both should give the same answer.
+  void getAncestorsFromRetriever(edm::ProductProvenanceRetriever const* retriever,
+                                 edm::BranchID const& branchID,
+                                 std::set<edm::BranchID>& ancestors) {
+    edm::ProductProvenance const* productProvenance = retriever->branchIDToProvenance(branchID);
+    if (productProvenance) {
+      for (auto const& parent : productProvenance->parentage().parents()) {
+        ancestors.insert(parent);
+        getAncestorsFromRetriever(retriever, parent, ancestors);
+      }
+    }
+  }
+}
+
+namespace edmtest {
+
+  class TestParentage : public edm::EDAnalyzer { 
+  public:
+
+    explicit TestParentage(edm::ParameterSet const& pset);
+    virtual ~TestParentage();
+
+    virtual void analyze(edm::Event const& e, edm::EventSetup const& es) override;
+
+  private:
+
+    edm::InputTag inputTag_;
+    edm::EDGetTokenT<IntProduct> token_;
+    std::vector<std::string> expectedAncestors_;
+  };
+
+  TestParentage::TestParentage(edm::ParameterSet const& pset) :
+    inputTag_(pset.getParameter<edm::InputTag>("inputTag")),
+    expectedAncestors_(pset.getParameter<std::vector<std::string> >("expectedAncestors")) {
+
+    token_ = consumes<IntProduct>(inputTag_);
+  }
+
+  TestParentage::~TestParentage() {}
+
+  void
+  TestParentage::analyze(edm::Event const& e, edm::EventSetup const&) {
+
+    edm::Handle<IntProduct> h;
+    e.getByToken(token_, h);
+
+    edm::Provenance const* prov = h.provenance();
+    std::set<edm::BranchID> ancestors;
+    getAncestors(e, prov->branchID(), ancestors);
+
+    std::set<std::string> ancestorLabels;
+    for (edm::BranchID const& ancestor : ancestors) {
+      edm::Provenance ancestorProv = e.getProvenance(ancestor);
+      ancestorLabels.insert(ancestorProv.moduleLabel());
+    }
+    std::set<std::string> expectedAncestors(expectedAncestors_.begin(), expectedAncestors_.end());
+    if (ancestorLabels != expectedAncestors) {
+      std::cerr << "TestParentage::analyze: ancestors do not match expected ancestors" << std::endl;
+      abort();
+    }
+    edm::ProductProvenanceRetriever const* retriever = prov->store();
+    std::set<edm::BranchID> ancestorsFromRetriever;
+    getAncestorsFromRetriever(retriever, prov->branchID(), ancestorsFromRetriever);
+
+    std::set<std::string> ancestorLabels2;
+    for (edm::BranchID const& ancestor : ancestorsFromRetriever) {
+      edm::Provenance ancestorProv = e.getProvenance(ancestor);
+      ancestorLabels2.insert(ancestorProv.moduleLabel());
+    }
+    if (ancestorLabels2 != expectedAncestors) {
+      std::cerr << "TestParentage::analyze: ancestors do not match expected ancestors (parentage from retriever)" << std::endl;
+      abort();
+    }
+  }
+} // namespace edmtest
+
+using edmtest::TestParentage;
+DEFINE_FWK_MODULE(TestParentage);
diff --git a/FWCore/Integration/test/run_SubProcess.sh b/FWCore/Integration/test/run_SubProcess.sh
@@ -37,6 +37,9 @@ pushd ${LOCAL_TMP_DIR}
   echo cmsRun testSubProcessUnscheduled_cfg.py
   cmsRun -p ${LOCAL_TEST_DIR}/testSubProcessUnscheduled_cfg.py || die "cmsRun testSubProcessUnscheduled_cfg.py" $?
 
+  echo cmsRun testSubProcessUnscheduledRead_cfg.py
+  cmsRun -p ${LOCAL_TEST_DIR}/testSubProcessUnscheduledRead_cfg.py || die "cmsRun testSubProcessUnscheduledRead_cfg.py" $?
+
 popd
 
 exit 0
diff --git a/FWCore/Integration/test/testSubProcessUnscheduledRead_cfg.py b/FWCore/Integration/test/testSubProcessUnscheduledRead_cfg.py
@@ -0,0 +1,16 @@
+import FWCore.ParameterSet.Config as cms
+
+process = cms.Process( "TEST2" )
+
+process.source = cms.Source("PoolSource",
+    fileNames = cms.untracked.vstring(
+        'file:testSubProcessUnscheduled.root'
+    )
+)
+
+process.test = cms.EDAnalyzer("TestParentage",
+                                 inputTag = cms.InputTag("final"),
+                                 expectedAncestors = cms.vstring("two", "ten", "adder")
+)
+process.o = cms.EndPath(process.test)
+
diff --git a/FWCore/Integration/test/testSubProcessUnscheduled_cfg.py b/FWCore/Integration/test/testSubProcessUnscheduled_cfg.py
@@ -36,3 +36,18 @@
 subprocess.final = cms.EDProducer("AddIntsProducer", labels = cms.vstring('adder'))
 
 subprocess.subpath = cms.Path( subprocess.final )
+
+subprocess.test = cms.EDAnalyzer("TestParentage",
+                                 inputTag = cms.InputTag("final"),
+                                 expectedAncestors = cms.vstring("two", "ten", "adder")
+)
+
+subprocess.out = cms.OutputModule("PoolOutputModule",
+    fileName = cms.untracked.string('testSubProcessUnscheduled.root'),
+    outputCommands = cms.untracked.vstring(
+        'drop *', 
+        'keep *_final_*_*',
+    )
+)
+subprocess.o = cms.EndPath(subprocess.test * subprocess.out)
+