Fix BranchChildren/Parentage problems that occur with SubProcess and …

…EDAlias

Also adds unit tests to cover these cases.

DataFormats/Provenance/interface/BranchChildren.h

@@ -13,6 +13,8 @@ BranchChildren: Dependency information between branches.
 
 namespace edm {
 
+  class BranchDescription;
+
   class BranchChildren {
   private:
     typedef std::set<BranchID> BranchIDSet;
@@ -31,18 +33,28 @@ namespace edm {
     // Look up all the descendants of the given parent, and insert them
     // into descendants. N.B.: this does not clear out descendants first;
     // it only appends *new* elements to the collection.
-    void appendToDescendants(BranchID parent, BranchIDSet& descendants) const;
+    void appendToDescendants(BranchDescription const& parent,
+                             BranchIDSet& descendants,
+                             std::map<BranchID, BranchID> const& droppedToKeptAlias) const;
 
     // const accessor for the data
     map_t const&
     childLookup() const {
       return childLookup_;
     }
 
+    map_t&
+    childLookup() {
+      return childLookup_;
+    }
+
   private:
     map_t childLookup_;
 
-    void append_(map_t const& lookup, BranchID item, BranchIDSet& itemSet) const;
+    void append_(map_t const& lookup,
+                 BranchID item,
+                 BranchIDSet& itemSet,
+                 std::map<BranchID, BranchID> const& droppedToKeptAlias) const;
   };
 
 }

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();}
     std::string const& branchName() const {return stable().branchName();}
     std::string const& className() const {return stable().className();}
     std::string const& moduleLabel() const {return stable().moduleLabel();}

DataFormats/Provenance/interface/StableProvenance.h

@@ -37,6 +37,7 @@ namespace edm {
     std::shared_ptr<BranchDescription const> const& constBranchDescriptionPtr() const {return branchDescription_;}
 
     BranchID const& branchID() const {return branchDescription().branchID();}
+    BranchID const& originalBranchID() const {return branchDescription().originalBranchID();}
     std::string const& branchName() const {return branchDescription().branchName();}
     std::string const& className() const {return branchDescription().className();}
     std::string const& moduleLabel() const {return branchDescription().moduleLabel();}

DataFormats/Provenance/interface/SubProcessParentageHelper.h

@@ -0,0 +1,36 @@
+#ifndef DataFormats_Provenance_SubProcessParentageHelper_h
+#define DataFormats_Provenance_SubProcessParentageHelper_h
+
+// This class is used to properly fill Parentage in SubProcesses.
+// In particular it helps filling the BranchChildren container
+// that is used when dropping descendants of products that
+// have been dropped on input.
+//
+// This class is only filled for SubProcesses. Its data member
+// only has entries for products produced in a prior SubProcess
+// or the top level Process in the same overall process.
+
+#include "DataFormats/Provenance/interface/BranchID.h"
+
+#include <vector>
+
+namespace edm {
+
+  class ProductRegistry;
+
+  class SubProcessParentageHelper {
+  public:
+
+    void update(SubProcessParentageHelper const& parentSubProcessParentageHelper,
+                ProductRegistry const& parentProductRegistry);
+
+    std::vector<BranchID> const& producedProducts() const {
+      return producedProducts_;
+    }
+
+  private:
+
+    std::vector<BranchID> producedProducts_;
+  };
+}
+#endif

DataFormats/Provenance/src/BranchChildren.cc

@@ -1,16 +1,34 @@
 #include "DataFormats/Provenance/interface/BranchChildren.h"
 
+#include "DataFormats/Provenance/interface/BranchDescription.h"
+
 namespace edm {
   void
-  BranchChildren::append_(map_t const& lookup, BranchID item, BranchIDSet& itemSet) const {
+  BranchChildren::append_(map_t const& lookup,
+                          BranchID item,
+                          BranchIDSet& itemSet,
+                          std::map<BranchID, BranchID> const& droppedToKeptAlias) const {
     auto const iter = lookup.find(item);
     if(iter != lookup.end()) {
       BranchIDSet const& branchIDs = iter->second;
       for(BranchID const& branchID : branchIDs) {
-        // Insert the BranchID of the parents(children) into the set of ancestors(descendants).
+        auto it = droppedToKeptAlias.find(branchID);
+        // Insert the BranchID of the children into the set of descendants.
         // If the insert succeeds, append recursively.
-        if(itemSet.insert(branchID).second) {
-          append_(lookup, branchID, itemSet);
+        if (it == droppedToKeptAlias.end()) {
+          // Normal case. Not an EDAlias.
+          if (itemSet.insert(branchID).second) {
+            append_(lookup, branchID, itemSet, droppedToKeptAlias);
+          }
+        } else {
+          // In this case, we want to put the EDAlias in the
+          // set of things to drop because that is what really
+          // needs to be dropped, but the recursive search in
+          // the lookup map must continue with the original BranchID
+          // because that is what the lookup map contains.
+          if (itemSet.insert(it->second).second) {
+            append_(lookup, branchID, itemSet, droppedToKeptAlias);
+          }
         }
       }
     }
@@ -32,8 +50,13 @@ namespace edm {
   }
 
   void
-  BranchChildren::appendToDescendants(BranchID parent, BranchIDSet& descendants) const {
-    descendants.insert(parent);
-    append_(childLookup_, parent, descendants);
+  BranchChildren::appendToDescendants(BranchDescription const& parent,
+                                      BranchIDSet& descendants,
+                                      std::map<BranchID, BranchID> const& droppedToKeptAlias) const {
+    descendants.insert(parent.branchID());
+    // A little tricky here. The child lookup map is filled with the
+    // BranchID of the original product even if there was an EDAlias
+    // and the EDAlias was saved and the original branch dropped.
+    append_(childLookup_, parent.originalBranchID(), descendants, droppedToKeptAlias);
   }
 }

DataFormats/Provenance/src/Provenance.cc

@@ -32,7 +32,7 @@ namespace edm {
     if(!store_) {
       return nullptr;
     }
-    return store_->branchIDToProvenance(branchID());
+    return store_->branchIDToProvenance(originalBranchID());
   }
 
   void

DataFormats/Provenance/src/SubProcessParentageHelper.cc

@@ -0,0 +1,22 @@
+#include "DataFormats/Provenance/interface/SubProcessParentageHelper.h"
+
+#include "DataFormats/Provenance/interface/BranchDescription.h"
+#include "DataFormats/Provenance/interface/ProductRegistry.h"
+#include "FWCore/Utilities/interface/BranchType.h"
+
+namespace edm {
+
+  void SubProcessParentageHelper::
+  update(SubProcessParentageHelper const& parentSubProcessParentageHelper,
+         ProductRegistry const& parentProductRegistry) {
+
+    *this = parentSubProcessParentageHelper;
+
+    for(auto const& prod : parentProductRegistry.productList()) {
+      BranchDescription const& desc = prod.second;
+      if (desc.produced() && desc.branchType() == InEvent && !desc.isAlias()) {
+        producedProducts_.push_back(desc.branchID());
+      }
+    }
+  }
+}

FWCore/Framework/interface/OutputModuleDescription.h

@@ -12,14 +12,20 @@ output module that does not come in through the ParameterSet
 namespace edm {
 
   class BranchIDListHelper;
+  class SubProcessParentageHelper;
+
   struct OutputModuleDescription {
     //OutputModuleDescription() : maxEvents_(-1) {}
-    explicit OutputModuleDescription(BranchIDLists const& branchIDLists, int maxEvents = -1) :
+    explicit OutputModuleDescription(BranchIDLists const& branchIDLists,
+                                     int maxEvents = -1,
+                                     SubProcessParentageHelper const* subProcessParentageHelper = nullptr) :
       branchIDLists_(&branchIDLists),
-      maxEvents_(maxEvents)
+      maxEvents_(maxEvents),
+      subProcessParentageHelper_(subProcessParentageHelper)
     {}
     BranchIDLists const* branchIDLists_;
     int maxEvents_;
+    SubProcessParentageHelper const* subProcessParentageHelper_;
   };
 }
 

FWCore/Framework/interface/Schedule.h

@@ -107,6 +107,7 @@ namespace edm {
   struct TriggerTimingReport;
   class ModuleRegistry;
   class ThinnedAssociationsHelper;
+  class SubProcessParentageHelper;
   class TriggerResultInserter;
   class WaitingTaskHolder;
 
@@ -124,6 +125,7 @@ namespace edm {
              ProductRegistry& pregistry,
              BranchIDListHelper& branchIDListHelper,
              ThinnedAssociationsHelper& thinnedAssociationsHelper,
+             SubProcessParentageHelper const* subProcessParentageHelper,
              ExceptionToActionTable const& actions,
              std::shared_ptr<ActivityRegistry> areg,
              std::shared_ptr<ProcessConfiguration> processConfiguration,
@@ -271,7 +273,9 @@ namespace edm {
 
   private:
 
-    void limitOutput(ParameterSet const& proc_pset, BranchIDLists const& branchIDLists);
+    void limitOutput(ParameterSet const& proc_pset,
+                     BranchIDLists const& branchIDLists,
+                     SubProcessParentageHelper const* subProcessParentageHelper);
 
     std::shared_ptr<TriggerResultInserter const> resultsInserter() const {return get_underlying_safe(resultsInserter_);}
     std::shared_ptr<TriggerResultInserter>& resultsInserter() {return get_underlying_safe(resultsInserter_);}

FWCore/Framework/interface/ScheduleItems.h

@@ -23,6 +23,7 @@ namespace edm {
   class SignallingProductRegistry;
   class StreamID;
   class PreallocationConfiguration;
+  class SubProcessParentageHelper;
 
   struct ScheduleItems {
     ScheduleItems();
@@ -60,13 +61,15 @@ namespace edm {
     std::shared_ptr<BranchIDListHelper>& branchIDListHelper() {return get_underlying_safe(branchIDListHelper_);}
     std::shared_ptr<ThinnedAssociationsHelper const> thinnedAssociationsHelper() const {return get_underlying_safe(thinnedAssociationsHelper_);}
     std::shared_ptr<ThinnedAssociationsHelper>& thinnedAssociationsHelper() {return get_underlying_safe(thinnedAssociationsHelper_);}
+    std::shared_ptr<SubProcessParentageHelper>& subProcessParentageHelper() {return get_underlying_safe(subProcessParentageHelper_);}
     std::shared_ptr<ProcessConfiguration const> processConfiguration() const {return get_underlying_safe(processConfiguration_);}
     std::shared_ptr<ProcessConfiguration>& processConfiguration() {return get_underlying_safe(processConfiguration_);}
 
     std::shared_ptr<ActivityRegistry> actReg_; // We do not use propagate_const because the registry itself is mutable.
     edm::propagate_const<std::shared_ptr<SignallingProductRegistry>> preg_;
     edm::propagate_const<std::shared_ptr<BranchIDListHelper>> branchIDListHelper_;
     edm::propagate_const<std::shared_ptr<ThinnedAssociationsHelper>> thinnedAssociationsHelper_;
+    edm::propagate_const<std::shared_ptr<SubProcessParentageHelper>> subProcessParentageHelper_;
     std::unique_ptr<ExceptionToActionTable const> act_table_;
     edm::propagate_const<std::shared_ptr<ProcessConfiguration>> processConfiguration_;
   };

FWCore/Framework/interface/SubProcess.h

@@ -34,6 +34,7 @@ namespace edm {
   class ProductRegistry;
   class PreallocationConfiguration;
   class ThinnedAssociationsHelper;
+  class SubProcessParentageHelper;
   class WaitingTaskHolder;
 
   namespace eventsetup {
@@ -46,6 +47,7 @@ namespace edm {
                std::shared_ptr<ProductRegistry const> parentProductRegistry,
                std::shared_ptr<BranchIDListHelper const> parentBranchIDListHelper,
                ThinnedAssociationsHelper const& parentThinnedAssociationsHelper,
+               SubProcessParentageHelper const& parentSubProcessParentageHelper,
                eventsetup::EventSetupsController& esController,
                ActivityRegistry& parentActReg,
                ServiceToken const& token,
@@ -262,6 +264,7 @@ namespace edm {
     std::shared_ptr<ProductRegistry const>        preg_;
     edm::propagate_const<std::shared_ptr<BranchIDListHelper>> branchIDListHelper_;
     edm::propagate_const<std::shared_ptr<ThinnedAssociationsHelper>> thinnedAssociationsHelper_;
+    edm::propagate_const<std::shared_ptr<SubProcessParentageHelper>> subProcessParentageHelper_;
     std::unique_ptr<ExceptionToActionTable const> act_table_;
     std::shared_ptr<ProcessConfiguration const>   processConfiguration_;
     ProcessContext                                processContext_;

FWCore/Framework/interface/one/OutputModuleBase.h

@@ -50,6 +50,7 @@ namespace edm {
   class ActivityRegistry;
   class ProductRegistry;
   class ThinnedAssociationsHelper;
+  class SubProcessParentageHelper;
   class WaitingTask;
 
   template <typename T> class OutputModuleCommunicatorT;
@@ -98,7 +99,11 @@ namespace edm {
       BranchIDLists const* branchIDLists();
 
       ThinnedAssociationsHelper const* thinnedAssociationsHelper() const;
-
+
+      SubProcessParentageHelper const* subProcessParentageHelper() const {
+         return subProcessParentageHelper_;
+      }
+
       const ModuleDescription& moduleDescription() const {
         return moduleDescription_;
       }
@@ -174,6 +179,7 @@ namespace edm {
       edm::propagate_const<std::unique_ptr<BranchIDLists>> branchIDLists_;
       BranchIDLists const* origBranchIDLists_;
 
+      SubProcessParentageHelper const* subProcessParentageHelper_;
 
       edm::propagate_const<std::unique_ptr<ThinnedAssociationsHelper>> thinnedAssociationsHelper_;
       std::map<BranchID, bool> keepAssociation_;

FWCore/Framework/src/Event.cc

@@ -199,7 +199,7 @@ namespace edm {
 
   void
   Event::addToGotBranchIDs(Provenance const& prov) const {
-    gotBranchIDs_.insert(prov.branchID());
+    gotBranchIDs_.insert(prov.originalBranchID());
   }
 
   ProcessHistory const&

FWCore/Framework/src/EventProcessor.cc

@@ -5,6 +5,7 @@
 #include "DataFormats/Provenance/interface/ParameterSetID.h"
 #include "DataFormats/Provenance/interface/ParentageRegistry.h"
 #include "DataFormats/Provenance/interface/ProcessHistoryRegistry.h"
+#include "DataFormats/Provenance/interface/SubProcessParentageHelper.h"
 
 #include "FWCore/Framework/interface/CommonParams.h"
 #include "FWCore/Framework/interface/EDLooperBase.h"
@@ -569,6 +570,7 @@ namespace edm {
                                  preg(),
                                  branchIDListHelper(),
                                  *thinnedAssociationsHelper_,
+                                 SubProcessParentageHelper(),
                                  *espController_,
                                  *actReg_,
                                  token,

FWCore/Framework/src/ProductResolvers.cc

@@ -587,7 +587,7 @@ namespace edm {
   }
 
   ProductProvenance const* ParentProcessProductResolver::productProvenancePtr_() const {
-    return provRetriever_? provRetriever_->branchIDToProvenance(bd_->branchID()): nullptr;
+    return provRetriever_? provRetriever_->branchIDToProvenance(bd_->originalBranchID()): nullptr;
   }
 
   void ParentProcessProductResolver::resetProductData_(bool deleteEarly) {
@@ -608,7 +608,28 @@ namespace edm {
     << "ParentProcessProductResolver::putOrMergeProduct_(std::unique_ptr<WrapperBase> edp) not implemented and should never be called.\n"
     << "Contact a Framework developer\n";
   }
-
+
+  void ParentProcessProductResolver::throwNullRealProduct() const {
+    // In principle, this ought to be fixed. I noticed one hits this error
+    // when in a SubProcess and calling the Event::getProvenance function
+    // with a BranchID to a branch from an earlier SubProcess or the top
+    // level process and this branch is not kept in this SubProcess. It might
+    // be possible to hit this in other contexts. I say it ought to be
+    // fixed because one does not encounter this issue if the SubProcesses
+    // are split into genuinely different processes (in principle that
+    // ought to give identical behavior and results). No user has ever
+    // reported this issue which has been around for some time and it was only
+    // noticed when testing some rare corner cases after modifying Core code.
+    // After discussing this with Chris we decided that at least for the moment
+    // there are higher priorities than fixing this ... I converted it so it
+    // causes an exception instead of a seg fault. The issue that may need to
+    // be addressed someday is how ProductResolvers for non-kept branches are
+    // connected to earlier SubProcesses.
+    throw Exception(errors::LogicError)
+      << "ParentProcessProductResolver::throwNullRealProduct RealProduct pointer not set in this context.\n"
+      << "Contact a Framework developer\n";
+  }
+
   NoProcessProductResolver::
   NoProcessProductResolver(std::vector<ProductResolverIndex> const&  matchingHolders,
                            std::vector<bool> const& ambiguous) :

FWCore/Framework/src/ProductResolvers.h

@@ -269,7 +269,11 @@ namespace edm {
                                  ModuleCallingContext const* mcc) const override {
       realProduct_->prefetchAsync( waitTask, *parentPrincipal_, skipCurrentProcess, sra, mcc);
     }
-    virtual bool unscheduledWasNotRun_() const override {return realProduct_->unscheduledWasNotRun();}
+    virtual bool unscheduledWasNotRun_() const override {
+      if (realProduct_) return realProduct_->unscheduledWasNotRun();
+      throwNullRealProduct();
+      return false;
+    }
     virtual bool productUnavailable_() const override {return realProduct_->productUnavailable();}
     virtual bool productResolved_() const override final { return realProduct_->productResolved(); }
     virtual bool productWasDeleted_() const override {return realProduct_->productWasDeleted();}
@@ -288,6 +292,7 @@ namespace edm {
     virtual ProductProvenance const* productProvenancePtr_() const override;
     virtual void resetProductData_(bool deleteEarly) override;
     virtual bool singleProduct_() const override;
+    void throwNullRealProduct() const;
 
     ProductResolverBase const* realProduct_;
     std::shared_ptr<BranchDescription const> bd_;