Merge pull request #15740 from Dr15Jones/fixModuleConsistencyCheck

Fix module run order consistency check

DQMServices/FwkIO/plugins/DQMRootOutputModule.cc

@@ -271,11 +271,11 @@ DQMRootOutputModule::DQMRootOutputModule(edm::ParameterSet const& pset):
 edm::one::OutputModuleBase::OutputModuleBase(pset),
 edm::one::OutputModule<>(pset),
 m_fileName(pset.getUntrackedParameter<std::string>("fileName")),
-m_logicalFileName(pset.getUntrackedParameter<std::string>("logicalFileName","")),
+m_logicalFileName(pset.getUntrackedParameter<std::string>("logicalFileName")),
 m_file(0),
 m_treeHelpers(kNIndicies,boost::shared_ptr<TreeHelperBase>()),
 m_presentHistoryIndex(0),
-m_filterOnRun(pset.getUntrackedParameter<unsigned int>("filterOnRun",0)),
+m_filterOnRun(pset.getUntrackedParameter<unsigned int>("filterOnRun")),
 m_enableMultiThread(false),
 m_fullNameBufferPtr(&m_fullNameBuffer),
 m_indicesTree(0)
@@ -599,14 +599,23 @@ void DQMRootOutputModule::finishEndFile() {
 //
 void
 DQMRootOutputModule::fillDescriptions(edm::ConfigurationDescriptions& descriptions) {
-  //The following says we do not know what parameters are allowed so do no validation
-  // Please change this to state exactly what you do use, even if it is no parameters
   edm::ParameterSetDescription desc;
-  desc.setUnknown();
+
+  desc.addUntracked<std::string>("fileName");
+  desc.addUntracked<std::string>("logicalFileName","");
+  desc.addUntracked<unsigned int>("filterOnRun",0)
+    ->setComment("Only write the run with this run number. 0 means write all runs.");
+  desc.addOptionalUntracked<int>("splitLevel", 99)
+    ->setComment("UNUSED Only here to allow older configurations written for PoolOutputModule to work.");
+  edm::OutputModule::fillDescription(desc, std::vector<std::string>(1U, std::string("drop *")));
+
+  edm::ParameterSetDescription dataSet;
+  dataSet.setAllowAnything();
+  desc.addUntracked<edm::ParameterSetDescription>("dataset", dataSet)
+    ->setComment("PSet is only used by Data Operations and not by this module.");
+
   descriptions.addDefault(desc);
 
-  //NOTE: when actually filling this in, do not forget to add a untracked PSet 'dataset'
-  // which is used for bookkeeping by the DMWM
 }
 
 

FWCore/Framework/interface/EDConsumerBase.h

@@ -84,11 +84,6 @@ namespace edm {
     typedef ProductLabels Labels;
     void labelsForToken(EDGetToken iToken, Labels& oLabels) const;
 
-    void modulesDependentUpon(std::string const& iProcessName,
-                              std::string const& iModuleLabel,
-                              bool iPrint,
-                              std::vector<char const*>& oModuleLabels) const;
-
     void modulesWhoseProductsAreConsumed(std::vector<ModuleDescription const*>& modules,
                                          ProductRegistry const& preg,
                                          std::map<std::string, ModuleDescription const*> const& labelsToDesc,

FWCore/Framework/interface/EventProcessor.h

@@ -318,6 +318,8 @@ namespace edm {
     typedef std::set<std::pair<std::string, std::string> > ExcludedData;
     typedef std::map<std::string, ExcludedData> ExcludedDataMap;
     ExcludedDataMap                               eventSetupDataToExcludeFromPrefetching_;
+
+    bool printDependencies_ = false;
   }; // class EventProcessor
 
   //--------------------------------------------------------------------

FWCore/Framework/interface/OutputModule.h

@@ -75,7 +75,7 @@ namespace edm {
     SelectedProductsForBranchType const& keptProducts() const {return keptProducts_;}
     std::array<bool, NumBranchTypes> const& hasNewlyDroppedBranch() const {return hasNewlyDroppedBranch_;}
 
-    static void fillDescription(ParameterSetDescription & desc);
+    static void fillDescription(ParameterSetDescription & desc, std::vector<std::string> const& iDefaultOutputCommands = ProductSelectorRules::defaultSelectionStrings());
     static void fillDescriptions(ConfigurationDescriptions& descriptions);
     static const std::string& baseType();
     static void prevalidate(ConfigurationDescriptions& );

FWCore/Framework/interface/PathsAndConsumesOfModules.h

@@ -68,5 +68,9 @@ namespace edm {
     Schedule const* schedule_;
     std::shared_ptr<ProductRegistry const> preg_;
   };
+
+  void
+  checkForModuleDependencyCorrectness(edm::PathsAndConsumesOfModulesBase const& iPnC,
+                                      bool iPrintDependencies);
 }
 #endif

FWCore/Framework/interface/ProductSelectorRules.h

@@ -43,8 +43,9 @@ namespace edm {
 
     bool keepAll() const {return keepAll_;}
 
-    static void fillDescription(ParameterSetDescription& desc, char const* parameterName);
+    static void fillDescription(ParameterSetDescription& desc, char const* parameterName, std::vector<std::string> const& defaultStrings = defaultSelectionStrings());
 
+    static const std::vector<std::string>& defaultSelectionStrings();
   private:
     class Rule {
     public:

FWCore/Framework/interface/Schedule.h

@@ -263,9 +263,6 @@ namespace edm {
 
   private:
 
-    /// Check that the schedule is actually runable
-    void checkForCorrectness() const;
-
     void limitOutput(ParameterSet const& proc_pset, BranchIDLists const& branchIDLists);
 
     std::shared_ptr<TriggerResultInserter const> resultsInserter() const {return get_underlying_safe(resultsInserter_);}
@@ -285,7 +282,6 @@ namespace edm {
     edm::propagate_const<std::unique_ptr<SystemTimeKeeper>> summaryTimeKeeper_;
 
     bool                           wantSummary_;
-    bool                           printDependencies_;
 
     volatile bool           endpathsAreActive_;
   };

FWCore/Framework/interface/stream/EDAnalyzerAdaptorBase.h

@@ -93,11 +93,6 @@ namespace edm {
 
       const EDConsumerBase* consumer() const;
 
-      void modulesDependentUpon(std::string const& iProcessName,
-                                std::string const& iModuleLabel,
-                                bool iPrint,
-                                std::vector<char const*>& oModuleLabels) const;
-
       void modulesWhoseProductsAreConsumed(std::vector<ModuleDescription const*>& modules,
                                            ProductRegistry const& preg,
                                            std::map<std::string, ModuleDescription const*> const& labelsToDesc,

FWCore/Framework/interface/stream/ProducingModuleAdaptorBase.h

@@ -83,11 +83,6 @@ namespace edm {
                         ProductResolverIndexHelper const&,
                         bool iPrefetchMayGet);
 
-      void modulesDependentUpon(std::string const& iProcessName,
-                                std::string const& iModuleLabel,
-                                bool iPrint,
-                                std::vector<char const*>& oModuleLabels) const;
-
       void modulesWhoseProductsAreConsumed(std::vector<ModuleDescription const*>& modules,
                                            ProductRegistry const& preg,
                                            std::map<std::string, ModuleDescription const*> const& labelsToDesc,

FWCore/Framework/src/EDConsumerBase.cc

@@ -383,35 +383,6 @@ namespace {
   };
 }
 
-void
-EDConsumerBase::modulesDependentUpon(std::string const& iProcessName,
-                                     std::string const& iModuleLabel,
-                                     bool iPrint,
-                                     std::vector<char const*>& oModuleLabels) const {
-  std::set<char const*, CharStarComp> uniqueModules;
-  for(unsigned int index = 0, iEnd = m_tokenInfo.size(); index < iEnd; ++index) {
-    auto const& info = m_tokenInfo.get<kLookupInfo>(index);
-    if(not info.m_index.skipCurrentProcess()) {
-      auto const& labels = m_tokenInfo.get<kLabels>(index);
-      unsigned int const start = labels.m_startOfModuleLabel;
-      char const* processName = &(m_tokenLabels[start+labels.m_deltaToProcessName]);
-      if(iPrint) {
-        LogAbsolute("ModuleDependency") << "ModuleDependency '" << iModuleLabel <<
-        "' may consume product of type '" << info.m_type.className() << 
-        "' with input tag '" << &(m_tokenLabels[start]) <<
-        ':' << &(m_tokenLabels[start+labels.m_deltaToProductInstance]) <<
-        ':' << processName << "'";
-      }
-      if((not processName) or processName[0]==0 or iProcessName == processName) {
-        uniqueModules.insert(&(m_tokenLabels[start]));
-      }
-    }
-  }
-
-  oModuleLabels = std::vector<const char*>(uniqueModules.begin(),uniqueModules.end());
-}
-
-
 namespace {
   void
   insertFoundModuleLabel(const char* consumedModuleLabel,
@@ -469,7 +440,8 @@ EDConsumerBase::modulesWhoseProductsAreConsumed(std::vector<ModuleDescription co
   for(auto itInfo = m_tokenInfo.begin<kLookupInfo>(),itEnd = m_tokenInfo.end<kLookupInfo>();
       itInfo != itEnd; ++itInfo,++itKind,++itLabels) {
 
-    if(itInfo->m_branchType == InEvent) {
+    if(itInfo->m_branchType == InEvent and
+       (not itInfo->m_index.skipCurrentProcess())) {
 
       const unsigned int labelStart = itLabels->m_startOfModuleLabel;
       const char* consumedModuleLabel = &(m_tokenLabels[labelStart]);

FWCore/Framework/src/EventProcessor.cc

@@ -82,6 +82,7 @@
 #include <sys/fcntl.h>
 #include <unistd.h>
 
+
 //Used for CPU affinity
 #ifndef __APPLE__
 #include <sched.h>
@@ -106,9 +107,8 @@ namespace {
     }
   private:
     edm::ActivityRegistry* reg_; // We do not use propagate_const because the registry itself is mutable.
-
-
   };
+
 }
 
 namespace edm {
@@ -480,6 +480,8 @@ namespace edm {
     }
     IllegalParameters::setThrowAnException(optionsPset.getUntrackedParameter<bool>("throwIfIllegalParameter", true));
 
+    printDependencies_ =  optionsPset.getUntrackedParameter("printDependencies", false);
+
     // Now do general initialization
     ScheduleItems items;
 
@@ -605,6 +607,9 @@ namespace edm {
                                  preallocations_.numberOfThreads());
     actReg_->preallocateSignal_(bounds);
     pathsAndConsumesOfModules_.initialize(schedule_.get(), preg());
+
+    //NOTE: this may throw
+    checkForModuleDependencyCorrectness(pathsAndConsumesOfModules_, printDependencies_);
     actReg_->preBeginJobSignal_(pathsAndConsumesOfModules_, processContext_);
 
     //NOTE:  This implementation assumes 'Job' means one call

FWCore/Framework/src/OutputModule.cc

@@ -407,8 +407,8 @@ namespace edm {
   }
 
   void
-  OutputModule::fillDescription(ParameterSetDescription& desc) {
-    ProductSelectorRules::fillDescription(desc, "outputCommands");
+  OutputModule::fillDescription(ParameterSetDescription& desc, std::vector<std::string> const& defaultOutputCommands) {
+    ProductSelectorRules::fillDescription(desc, "outputCommands",defaultOutputCommands);
     EventSelector::fillDescription(desc);
   }
 

FWCore/Framework/src/PathsAndConsumesOfModules.cc

@@ -2,6 +2,8 @@
 
 #include "FWCore/Framework/interface/Schedule.h"
 #include "FWCore/Framework/src/Worker.h"
+#include "throwIfImproperDependencies.h"
+
 #include "FWCore/Utilities/interface/EDMException.h"
 
 #include <algorithm>
@@ -92,4 +94,122 @@ namespace edm {
     }
     return iter->second;
   }
+
+  //====================================
+  // checkForCorrectness algorithm
+  //
+  // The code creates a 'dependency' graph between all
+  // modules. A module depends on another module if
+  // 1) it 'consumes' data produced by that module
+  // 2) it appears directly after the module within a Path
+  //
+  // If there is a cycle in the 'dependency' graph then
+  // the schedule may be unrunnable. The schedule is still
+  // runnable if all cycles have at least two edges which
+  // connect modules only by Path dependencies (i.e. not
+  // linked by a data dependency).
+  //
+  //  Example 1:
+  //  C consumes data from B
+  //  Path 1: A + B + C
+  //  Path 2: B + C + A
+  //
+  //  Cycle: A after C [p2], C consumes B, B after A [p1]
+  //  Since this cycle has 2 path only edges it is OK since
+  //  A and (B+C) are independent so their run order doesn't matter
+  //
+  //  Example 2:
+  //  B consumes A
+  //  C consumes B
+  //  Path: C + A
+  //
+  //  Cycle: A after C [p], C consumes B, B consumes A
+  //  Since this cycle has 1 path only edge it is unrunnable.
+  //
+  //  Example 3:
+  //  A consumes B
+  //  B consumes C
+  //  C consumes A
+  //  (no Path since unscheduled execution)
+  //
+  //  Cycle: A consumes B, B consumes C, C consumes A
+  //  Since this cycle has 0 path only edges it is unrunnable.
+  //====================================
+
+
+  void checkForModuleDependencyCorrectness(edm::PathsAndConsumesOfModulesBase const& iPnC,
+                                           bool iPrintDependencies) {
+    using namespace edm::graph;
+    //Need to lookup ids to names quickly
+    std::unordered_map<unsigned int, std::string> moduleIndexToNames;
+
+    //for testing, state that TriggerResults is at the end of all paths
+    const std::string kTriggerResults("TriggerResults");
+    unsigned int kTriggerResultsIndex = kInvalidIndex;
+    for(auto const& description: iPnC.allModules()) {
+      moduleIndexToNames.insert(std::make_pair(description->id(),
+                                               description->moduleLabel()));
+      if(kTriggerResults == description->moduleLabel()) {
+        kTriggerResultsIndex = description->id();
+      }
+    }
+
+    //If a module to module dependency comes from a path, remember which path
+    EdgeToPathMap edgeToPathMap;
+
+    //determine the path dependencies
+    std::vector<std::string> pathNames = iPnC.paths();
+    const unsigned int kFirstEndPathIndex = pathNames.size();
+    pathNames.insert(pathNames.end(), iPnC.endPaths().begin(), iPnC.endPaths().end());
+    std::vector<std::vector<unsigned int>> pathIndexToModuleIndexOrder(pathNames.size());
+    {
+
+      for(unsigned int pathIndex = 0; pathIndex != pathNames.size(); ++pathIndex) {
+        std::set<unsigned int> alreadySeenIndex;
+
+        std::vector<ModuleDescription const*> const* moduleDescriptions;
+        if(pathIndex < kFirstEndPathIndex) {
+          moduleDescriptions= &(iPnC.modulesOnPath(pathIndex));
+        } else {
+          moduleDescriptions= &(iPnC.modulesOnEndPath(pathIndex-kFirstEndPathIndex));
+        }
+        unsigned int lastModuleIndex = kInvalidIndex;
+        auto& pathOrder =pathIndexToModuleIndexOrder[pathIndex];
+        pathOrder.reserve(moduleDescriptions->size());
+        for(auto const& description: *moduleDescriptions) {
+          auto found = alreadySeenIndex.insert(description->id());
+          if(found.second) {
+            //first time for this path
+            unsigned int const moduleIndex = description->id();
+            pathOrder.push_back(moduleIndex);
+            if(lastModuleIndex  != kInvalidIndex ) {
+              edgeToPathMap[std::make_pair(moduleIndex,lastModuleIndex)].push_back(pathIndex);
+            }
+            lastModuleIndex = moduleIndex;
+          }
+        }
+        //Stick TriggerResults at end of paths
+        if( pathIndex < kFirstEndPathIndex) {
+          if( (lastModuleIndex  != kInvalidIndex) and (kTriggerResultsIndex != kInvalidIndex) ) {
+            edgeToPathMap[std::make_pair(kTriggerResultsIndex,lastModuleIndex)].push_back(pathIndex);
+          }
+        }
+      }
+    }
+    {
+      //determine the data dependencies
+      for(auto const& description: iPnC.allModules()) {
+        unsigned int const moduleIndex = description->id();
+        auto const& dependentModules = iPnC.modulesWhoseProductsAreConsumedBy(moduleIndex);
+        for(auto const& depDescription: dependentModules) {
+          edgeToPathMap[std::make_pair(moduleIndex, depDescription->id())].push_back(kDataDependencyIndex);
+          if(iPrintDependencies) {
+            edm::LogAbsolute("ModuleDependency") << "ModuleDependency '" << description->moduleLabel() <<
+            "' depends on data products from module '" << depDescription->moduleLabel()<<"'";
+          }
+        }
+      }
+    }
+    graph::throwIfImproperDependencies(edgeToPathMap,pathIndexToModuleIndexOrder,pathNames,moduleIndexToNames);
+  }
 }

FWCore/Framework/src/ProductSelectorRules.cc

@@ -222,9 +222,15 @@ typedef std::vector<edm::BranchDescription const*> VCBDP;
       partial_match(processName_, branch->processName());
   }
 
+  const std::vector<std::string>&
+  ProductSelectorRules::defaultSelectionStrings() {
+    static const std::vector<std::string> s_defaultStrings(1U, std::string("keep *"));
+    return s_defaultStrings;
+  }
+
   void
-  ProductSelectorRules::fillDescription(ParameterSetDescription& desc, char const* parameterName) {
-    std::vector<std::string> defaultStrings(1U, std::string("keep *"));
+  ProductSelectorRules::fillDescription(ParameterSetDescription& desc, char const* parameterName, std::vector<std::string> const& defaultStrings) {
+    ;
     desc.addUntracked<std::vector<std::string> >(parameterName, defaultStrings)
         ->setComment("Specifies which branches are kept or dropped.");
   }