diff --git a/FWCore/Framework/src/PathsAndConsumesOfModules.cc b/FWCore/Framework/src/PathsAndConsumesOfModules.cc index 646d22834cebe..86d57a4b4f964 100644 --- a/FWCore/Framework/src/PathsAndConsumesOfModules.cc +++ b/FWCore/Framework/src/PathsAndConsumesOfModules.cc @@ -3,11 +3,12 @@ #include "FWCore/Framework/interface/Schedule.h" #include "FWCore/Framework/interface/ModuleProcessName.h" #include "FWCore/Framework/src/Worker.h" -#include "throwIfImproperDependencies.h" #include "FWCore/Utilities/interface/EDMException.h" #include +#include +#include namespace edm { PathsAndConsumesOfModules::PathsAndConsumesOfModules() = default; @@ -266,8 +267,71 @@ namespace edm { // Since this cycle has 0 path only edges it is unrunnable. //==================================== + namespace { + struct ModuleStatus { + std::vector dependsOn_; + std::vector pathsOn_; + unsigned long long lastSearch = 0; + bool onPath_ = false; + bool wasRun_ = false; + }; + + struct PathStatus { + std::vector modulesOnPath_; + unsigned long int activeModuleSlot_ = 0; + unsigned long int nModules_ = 0; + unsigned int index_ = 0; + bool endPath_ = false; + }; + + class CircularDependencyException {}; + + bool checkIfCanRun(unsigned long long searchIndex, + unsigned int iModuleToCheckID, + std::vector& iModules, + std::vector& stackTrace) { + auto& status = iModules[iModuleToCheckID]; + if (status.wasRun_) { + return true; + } + + if (status.lastSearch == searchIndex) { + //check to see if the module is already on the stack + // checking searchIndex is insufficient as multiple modules + // in this search may be dependent upon the same module + auto itFound = std::find(stackTrace.begin(), stackTrace.end(), iModuleToCheckID); + if (itFound != stackTrace.end()) { + stackTrace.push_back(iModuleToCheckID); + throw CircularDependencyException(); + } + //we have already checked this module's dependencies during this search + return false; + } + stackTrace.push_back(iModuleToCheckID); + status.lastSearch = searchIndex; + + bool allDependenciesRan = true; + for (auto index : status.dependsOn_) { + auto& dep = iModules[index]; + if (dep.onPath_) { + if (not dep.wasRun_) { + allDependenciesRan = false; + } + } else if (not checkIfCanRun(searchIndex, index, iModules, stackTrace)) { + allDependenciesRan = false; + } + } + if (allDependenciesRan) { + status.wasRun_ = true; + } + stackTrace.pop_back(); + + return allDependenciesRan; + } + } // namespace void checkForModuleDependencyCorrectness(edm::PathsAndConsumesOfModulesBase const& iPnC, bool iPrintDependencies) { - using namespace edm::graph; + constexpr auto kInvalidIndex = std::numeric_limits::max(); + //Need to lookup ids to names quickly std::unordered_map moduleIndexToNames; @@ -278,8 +342,8 @@ namespace edm { const std::string kPathStatusInserter("PathStatusInserter"); const std::string kEndPathStatusInserter("EndPathStatusInserter"); unsigned int kTriggerResultsIndex = kInvalidIndex; + ModuleStatus triggerResultsStatus; unsigned int largestIndex = 0; - unsigned int kPathToTriggerResultsDependencyLastIndex = kInvalidIndex; for (auto const& description : iPnC.allModules()) { moduleIndexToNames.insert(std::make_pair(description->id(), description->moduleLabel())); if (kTriggerResults == description->moduleLabel()) { @@ -288,196 +352,345 @@ namespace edm { if (description->id() > largestIndex) { largestIndex = description->id(); } + if (description->moduleName() == kPathStatusInserter) { + triggerResultsStatus.dependsOn_.push_back(description->id()); + } if (description->moduleName() == kPathStatusInserter || description->moduleName() == kEndPathStatusInserter) { pathStatusInserterModuleLabelToModuleID[description->moduleLabel()] = description->id(); } } - kPathToTriggerResultsDependencyLastIndex = largestIndex; - /* - { - //We need to explicitly check that modules on Paths do not try to read data from - // Modules which are only on EndPaths. The circular dependency finder has been - // known to miss these. - std::unordered_set modulesOnlyOnEndPaths; - auto const& endPaths = iPnC.endPaths(); - for( unsigned int pathIndex = 0; pathIndex != endPaths.size(); ++pathIndex) { - auto const& moduleDescriptions = iPnC.modulesOnEndPath(pathIndex); - for(auto const& description: moduleDescriptions) { - modulesOnlyOnEndPaths.insert(description->id()); - } - } + std::vector statusOfModules(largestIndex + 1); + for (auto const& nameID : pathStatusInserterModuleLabelToModuleID) { + statusOfModules[nameID.second].onPath_ = true; + } + if (kTriggerResultsIndex != kInvalidIndex) { + statusOfModules[kTriggerResultsIndex] = std::move(triggerResultsStatus); + } + + std::vector statusOfPaths(iPnC.paths().size() + iPnC.endPaths().size()); - std::unordered_set modulesOnPaths; - auto const& paths = iPnC.paths(); - for( unsigned int pathIndex = 0; pathIndex != paths.size(); ++pathIndex) { - auto const& moduleDescriptions = iPnC.modulesOnPath(pathIndex); - for(auto const& description: moduleDescriptions) { - auto itFind =modulesOnlyOnEndPaths.find(description->id()); - if(modulesOnlyOnEndPaths.end() != itFind) { - modulesOnlyOnEndPaths.erase(itFind); + //If there are no paths, no modules will run so nothing to check + if (statusOfPaths.empty()) { + return; + } + + { + auto nPaths = iPnC.paths().size(); + for (unsigned int p = 0; p < nPaths; ++p) { + auto& status = statusOfPaths[p]; + status.index_ = p; + status.modulesOnPath_.reserve(iPnC.modulesOnPath(p).size() + 1); + std::unordered_set uniqueModules; + for (auto const& mod : iPnC.modulesOnPath(p)) { + if (uniqueModules.insert(mod->id()).second) { + status.modulesOnPath_.push_back(mod->id()); + statusOfModules[mod->id()].onPath_ = true; + statusOfModules[mod->id()].pathsOn_.push_back(p); } - modulesOnPaths.insert(description->id()); } + status.nModules_ = uniqueModules.size() + 1; + + //add the PathStatusInserter at the end + auto found = pathStatusInserterModuleLabelToModuleID.find(iPnC.paths()[p]); + assert(found != pathStatusInserterModuleLabelToModuleID.end()); + status.modulesOnPath_.push_back(found->second); } - - for(auto moduleIndex : modulesOnPaths) { - auto const& dependentModules = iPnC.modulesWhoseProductsAreConsumedBy(moduleIndex); - for(auto const& depDescription: dependentModules) { - auto itFind = modulesOnlyOnEndPaths.find(depDescription->id()); - if(itFind != modulesOnlyOnEndPaths.end()) { - throw edm::Exception(edm::errors::ScheduleExecutionFailure, "Unrunnable schedule\n") - <<"The module "<id()]<<" which is on an EndPath."; + } + { + auto offset = iPnC.paths().size(); + auto nPaths = iPnC.endPaths().size(); + for (unsigned int p = 0; p < nPaths; ++p) { + auto& status = statusOfPaths[p + offset]; + status.endPath_ = true; + status.index_ = p; + status.modulesOnPath_.reserve(iPnC.modulesOnEndPath(p).size() + 1); + std::unordered_set uniqueModules; + for (auto const& mod : iPnC.modulesOnEndPath(p)) { + if (uniqueModules.insert(mod->id()).second) { + status.modulesOnPath_.push_back(mod->id()); + statusOfModules[mod->id()].onPath_ = true; + statusOfModules[mod->id()].pathsOn_.push_back(p + offset); } } - } + status.nModules_ = uniqueModules.size() + 1; + //add the EndPathStatusInserter at the end + auto found = pathStatusInserterModuleLabelToModuleID.find(iPnC.endPaths()[p]); + assert(found != pathStatusInserterModuleLabelToModuleID.end()); + status.modulesOnPath_.push_back(found->second); + } } - */ - //If a module to module dependency comes from a path, remember which path - EdgeToPathMap edgeToPathMap; + for (auto const& description : iPnC.allModules()) { + unsigned int const moduleIndex = description->id(); + auto const& dependentModules = iPnC.modulesWhoseProductsAreConsumedBy(moduleIndex); + auto& deps = statusOfModules[moduleIndex]; + deps.dependsOn_.reserve(dependentModules.size()); + for (auto const& depDescription : dependentModules) { + if (iPrintDependencies) { + edm::LogAbsolute("ModuleDependency") + << "ModuleDependency '" << description->moduleLabel() << "' depends on data products from module '" + << depDescription->moduleLabel() << "'"; + } + deps.dependsOn_.push_back(depDescription->id()); + } + } - //Need to be able to quickly look up which paths a module appears on - std::unordered_map> moduleIndexToPathIndex; + unsigned int nPathsFinished = 0; + + //if a circular dependency exception happens, stackTrace has the info + std::vector stackTrace; + bool madeForwardProgress = true; + try { + //'simulate' the running of the paths. On each step mark each module as 'run' + // if all the module's dependencies were fulfilled in a previous step + unsigned long long searchIndex = 0; + while (madeForwardProgress and nPathsFinished != statusOfPaths.size()) { + madeForwardProgress = false; + for (auto& p : statusOfPaths) { + //the path has already completed in an earlier pass + if (p.activeModuleSlot_ == p.nModules_) { + continue; + } + ++searchIndex; + bool didRun = checkIfCanRun(searchIndex, p.modulesOnPath_[p.activeModuleSlot_], statusOfModules, stackTrace); + if (didRun) { + madeForwardProgress = true; + ++p.activeModuleSlot_; + if (p.activeModuleSlot_ == p.nModules_) { + ++nPathsFinished; + } + } + } + } + } catch (CircularDependencyException const&) { + //the last element in stackTrace must appear somewhere earlier in stackTrace + std::ostringstream oStr; + + unsigned int lastIndex = stackTrace.front(); + bool firstSkipped = false; + for (auto id : stackTrace) { + if (firstSkipped) { + oStr << " module '" << moduleIndexToNames[lastIndex] << "' depends on " << moduleIndexToNames[id] << "\n"; + } else { + firstSkipped = true; + } + lastIndex = id; + } + throw edm::Exception(edm::errors::ScheduleExecutionFailure, "Unrunnable schedule\n") + << "Circular module dependency found in configuration\n" + << oStr.str(); + } - //determine the path dependencies - std::vector pathNames = iPnC.paths(); - const unsigned int kFirstEndPathIndex = pathNames.size(); + auto pathName = [&](PathStatus const& iP) { + if (iP.endPath_) { + return iPnC.endPaths()[iP.index_]; + } + return iPnC.paths()[iP.index_]; + }; - const std::string kPathEnded("@PathEnded"); - const std::string kEndPathStart("@EndPathStart"); + //The program would deadlock + if (not madeForwardProgress) { + std::ostringstream oStr; + auto modIndex = std::numeric_limits::max(); + unsigned int presentPath; + for (auto itP = statusOfPaths.begin(); itP != statusOfPaths.end(); ++itP) { + auto const& p = *itP; + if (p.activeModuleSlot_ == p.nModules_) { + continue; + } + //this path is stuck + modIndex = p.modulesOnPath_[p.activeModuleSlot_]; + presentPath = itP - statusOfPaths.begin(); + break; + } + //NOTE the following should always be true as at least 1 path should be stuc. + // I've added the condition just to be paranoid. + if (modIndex != std::numeric_limits::max()) { + struct ProgressInfo { + ProgressInfo(unsigned int iMod, unsigned int iPath, bool iPreceeds = false) + : moduleIndex_(iMod), pathIndex_(iPath), preceeds_(iPreceeds) {} - //The finished processing depends on all paths and end paths - const std::string kFinishedProcessing("@FinishedProcessing"); - const unsigned int kFinishedProcessingIndex{0}; - moduleIndexToNames.insert(std::make_pair(kFinishedProcessingIndex, kFinishedProcessing)); + ProgressInfo(unsigned int iMod) : moduleIndex_(iMod), pathIndex_{}, preceeds_(false) {} - pathNames.insert(pathNames.end(), iPnC.endPaths().begin(), iPnC.endPaths().end()); - std::vector> pathIndexToModuleIndexOrder(pathNames.size()); - { - for (unsigned int pathIndex = 0; pathIndex != pathNames.size(); ++pathIndex) { - std::set alreadySeenIndex; + unsigned int moduleIndex_ = std::numeric_limits::max(); + std::optional pathIndex_; + bool preceeds_; - std::vector 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() + 1); - 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); - auto& paths = moduleIndexToPathIndex[moduleIndex]; - paths.push_back(pathIndex); - if (lastModuleIndex != kInvalidIndex) { - edgeToPathMap[std::make_pair(moduleIndex, lastModuleIndex)].push_back(pathIndex); + bool operator==(ProgressInfo const& iOther) const { + return moduleIndex_ == iOther.moduleIndex_ and pathIndex_ == iOther.pathIndex_; + } + }; + + std::vector progressTrace; + progressTrace.emplace_back(modIndex, presentPath); + + //The following starts from the first found unrun module on a path. It then finds + // the first modules it depends on that was not run. If that module is on a Task + // it then repeats the check for that module's dependencies. If that module is on + // a path, it checks to see if that module is the first unrun module of a path + // and if so it repeats the check for that module's dependencies, if not it + // checks the dependencies of the stuck module on that path. + // Eventually, all these checks should allow us to find a cycle of modules. + + //NOTE: the only way foundUnrunModule should ever by false by the end of the + // do{}while loop is if there is a bug in the algorithm. I've included it to + // try to avoid that case causing an infinite loop in the program. + bool foundUnrunModule; + do { + //check dependencies looking for stuff not run and on a path + foundUnrunModule = false; + for (auto depMod : statusOfModules[modIndex].dependsOn_) { + auto const& depStatus = statusOfModules[depMod]; + if (not depStatus.wasRun_ and depStatus.onPath_) { + foundUnrunModule = true; + //last run on a path? + bool lastOnPath = false; + unsigned int foundPath; + for (auto pathOn : depStatus.pathsOn_) { + auto const& depPaths = statusOfPaths[pathOn]; + if (depPaths.modulesOnPath_[depPaths.activeModuleSlot_] == depMod) { + lastOnPath = true; + foundPath = pathOn; + break; + } + } + if (lastOnPath) { + modIndex = depMod; + progressTrace.emplace_back(modIndex, foundPath); + } else { + //some earlier module on the same path is stuck + progressTrace.emplace_back(depMod, depStatus.pathsOn_[0]); + auto const& depPath = statusOfPaths[depStatus.pathsOn_[0]]; + modIndex = depPath.modulesOnPath_[depPath.activeModuleSlot_]; + progressTrace.emplace_back(modIndex, depStatus.pathsOn_[0], true); + } + break; } - lastModuleIndex = moduleIndex; } - } - //Have TriggerResults depend on the end of all paths - // Have all EndPaths depend on TriggerResults - auto labelToID = pathStatusInserterModuleLabelToModuleID.find(pathNames[pathIndex]); - if (labelToID == pathStatusInserterModuleLabelToModuleID.end()) { - // should never happen - throw Exception(errors::LogicError) - << "PathsAndConsumesOfModules::moduleDescription:checkForModuleDependencyCorrectness Could not find " - "PathStatusInserter\n"; - } - unsigned int pathStatusInserterModuleID = labelToID->second; - if (pathIndex < kFirstEndPathIndex) { - if ((lastModuleIndex != kInvalidIndex)) { - edgeToPathMap[std::make_pair(pathStatusInserterModuleID, lastModuleIndex)].push_back(pathIndex); - moduleIndexToNames.insert(std::make_pair(pathStatusInserterModuleID, kPathEnded)); - if (kTriggerResultsIndex != kInvalidIndex) { - edgeToPathMap[std::make_pair(kTriggerResultsIndex, pathStatusInserterModuleID)].push_back( - kDataDependencyIndex); + if (not foundUnrunModule) { + //check unscheduled modules + for (auto depMod : statusOfModules[modIndex].dependsOn_) { + auto const& depStatus = statusOfModules[depMod]; + if (not depStatus.wasRun_ and not depStatus.onPath_) { + foundUnrunModule = true; + progressTrace.emplace_back(depMod); + modIndex = depMod; + break; + } + } + } + } while (foundUnrunModule and (0 == std::count(progressTrace.begin(), + progressTrace.begin() + progressTrace.size() - 1, + progressTrace.back()))); + + auto printTrace = [&](auto& oStr, auto itBegin, auto itEnd) { + for (auto itTrace = itBegin; itTrace != itEnd; ++itTrace) { + if (itTrace != itBegin) { + if (itTrace->preceeds_) { + oStr << " and follows module '" << moduleIndexToNames[itTrace->moduleIndex_] << "' on the path\n"; + } else { + oStr << " and depends on module '" << moduleIndexToNames[itTrace->moduleIndex_] << "'\n"; + } + } + if (itTrace + 1 != itEnd) { + if (itTrace->pathIndex_) { + oStr << " module '" << moduleIndexToNames[itTrace->moduleIndex_] << "' is on path '" + << pathName(statusOfPaths[*itTrace->pathIndex_]) << "'"; + } else { + oStr << " module '" << moduleIndexToNames[itTrace->moduleIndex_] << "' is in a task"; + } } - //Need to make dependency for finished process - edgeToPathMap[std::make_pair(kFinishedProcessingIndex, pathStatusInserterModuleID)].push_back( - kDataDependencyIndex); - pathOrder.push_back(pathStatusInserterModuleID); } + }; + + if (not foundUnrunModule) { + //If we get here, this suggests a problem with either the algorithm that finds problems or the algorithm + // that attempts to report the problem + oStr << "Algorithm Error, unable to find problem. Contact framework group.\n Traced problem this far\n"; + printTrace(oStr, progressTrace.begin(), progressTrace.end()); } else { - if ((not moduleDescriptions->empty())) { - if (kTriggerResultsIndex != kInvalidIndex) { - ++kPathToTriggerResultsDependencyLastIndex; - edgeToPathMap[std::make_pair(moduleDescriptions->front()->id(), kPathToTriggerResultsDependencyLastIndex)] - .push_back(pathIndex); - moduleIndexToNames.insert(std::make_pair(kPathToTriggerResultsDependencyLastIndex, kEndPathStart)); - edgeToPathMap[std::make_pair(kPathToTriggerResultsDependencyLastIndex, kTriggerResultsIndex)].push_back( - kDataDependencyIndex); - pathOrder.insert(pathOrder.begin(), kPathToTriggerResultsDependencyLastIndex); + printTrace( + oStr, std::find(progressTrace.begin(), progressTrace.end(), progressTrace.back()), progressTrace.end()); + } + } + //the schedule deadlocked + throw edm::Exception(edm::errors::ScheduleExecutionFailure, "Unrunnable schedule\n") + << "The Path/EndPath configuration could cause the job to deadlock\n" + << oStr.str(); + } + + //NOTE: although the following conditions are not needed for safe running, they are + // policy choices the collaboration has made. + + //Check to see if for each path if the order of the modules is correct based on dependencies + for (auto& p : statusOfPaths) { + for (unsigned long int i = 0; p.nModules_ > 0 and i < p.nModules_ - 1; ++i) { + auto moduleID = p.modulesOnPath_[i]; + if (not statusOfModules[moduleID].dependsOn_.empty()) { + for (unsigned long int j = i + 1; j < p.nModules_; ++j) { + auto testModuleID = p.modulesOnPath_[j]; + for (auto depModuleID : statusOfModules[moduleID].dependsOn_) { + if (depModuleID == testModuleID) { + throw edm::Exception(edm::errors::ScheduleExecutionFailure, "Unrunnable schedule\n") + << "Dependent module later on Path\n" + << " module '" << moduleIndexToNames[moduleID] << "' depends on '" + << moduleIndexToNames[depModuleID] << "' which is later on path " << pathName(p); + } } - //Need to make dependency for finished process - ++kPathToTriggerResultsDependencyLastIndex; - edgeToPathMap[std::make_pair(pathStatusInserterModuleID, lastModuleIndex)].push_back(pathIndex); - moduleIndexToNames.insert(std::make_pair(pathStatusInserterModuleID, kPathEnded)); - edgeToPathMap[std::make_pair(kFinishedProcessingIndex, pathStatusInserterModuleID)].push_back( - kDataDependencyIndex); - pathOrder.push_back(pathStatusInserterModuleID); } } } } - { - //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) { - if (iPrintDependencies) { - edm::LogAbsolute("ModuleDependency") - << "ModuleDependency '" << description->moduleLabel() << "' depends on data products from module '" - << depDescription->moduleLabel() << "'"; + + //HLT wants all paths to be equivalent. If a path has a module A that needs data from module B and module B appears on one path + // as module A then B must appear on ALL paths that have A. + unsigned int modIndex = 0; + for (auto& mod : statusOfModules) { + for (auto& depIndex : mod.dependsOn_) { + std::size_t count = 0; + std::size_t nonEndPaths = 0; + for (auto modPathID : mod.pathsOn_) { + if (statusOfPaths[modPathID].endPath_) { + continue; } - //see if all paths containing this module also contain the dependent module earlier in the path - // if it does, then treat this only as a path dependency and not a data dependency as this - // simplifies the circular dependency checking logic - auto depID = depDescription->id(); - auto itPathsFound = moduleIndexToPathIndex.find(moduleIndex); - bool keepDataDependency = true; - auto itDepsPathsFound = moduleIndexToPathIndex.find(depID); - if (itPathsFound != moduleIndexToPathIndex.end() and itDepsPathsFound != moduleIndexToPathIndex.end()) { - keepDataDependency = false; - for (auto const pathIndex : itPathsFound->second) { - for (auto idToCheck : pathIndexToModuleIndexOrder[pathIndex]) { - if (idToCheck == depID) { - //found dependent module first so check next path - break; - } - if (idToCheck == moduleIndex) { - //did not find dependent module earlier on path so - // must keep data dependency - keepDataDependency = true; - break; - } - } - if (keepDataDependency) { - break; - } + ++nonEndPaths; + for (auto depPathID : statusOfModules[depIndex].pathsOn_) { + if (depPathID == modPathID) { + ++count; + break; } } - if (keepDataDependency) { - edgeToPathMap[std::make_pair(moduleIndex, depID)].push_back(kDataDependencyIndex); + } + if (count != 0 and count != nonEndPaths) { + std::ostringstream onStr; + std::ostringstream missingStr; + + for (auto modPathID : mod.pathsOn_) { + if (statusOfPaths[modPathID].endPath_) { + continue; + } + bool found = false; + for (auto depPathID : statusOfModules[depIndex].pathsOn_) { + if (depPathID == modPathID) { + found = true; + } + } + auto& s = statusOfPaths[modPathID]; + if (found) { + onStr << pathName(s) << " "; + } else { + missingStr << pathName(s) << " "; + } } + throw edm::Exception(edm::errors::ScheduleExecutionFailure, "Unrunnable schedule\n") + << "Paths are non consistent\n" + << " module '" << moduleIndexToNames[modIndex] << "' depends on '" << moduleIndexToNames[depIndex] + << "' which appears on paths\n " << onStr.str() << "\nbut is missing from\n " << missingStr.str(); } } - } - // Don't bother if there are no modules in any paths (the - // dependence check crashes if the configuration has only Paths - // with Tasks with modules, but nothing to trigger any work to - // run) - if (not moduleIndexToPathIndex.empty()) { - graph::throwIfImproperDependencies(edgeToPathMap, pathIndexToModuleIndexOrder, pathNames, moduleIndexToNames); + ++modIndex; } } } // namespace edm diff --git a/FWCore/Framework/src/throwIfImproperDependencies.cc b/FWCore/Framework/src/throwIfImproperDependencies.cc deleted file mode 100644 index fc3aa87610fed..0000000000000 --- a/FWCore/Framework/src/throwIfImproperDependencies.cc +++ /dev/null @@ -1,517 +0,0 @@ -// -*- ++ -*- -// -// Package: FWCore/Framework -// Function: throwIfImproperDependencies -// -// Implementation: -// [Notes on implementation] -// -// Original Author: root -// Created: Tue, 06 Sep 2016 16:04:28 GMT -// - -// system include files - -// user include files -#include "FWCore/Framework/src/throwIfImproperDependencies.h" -#include "FWCore/Utilities/interface/EDMException.h" - -#include "boost/graph/graph_traits.hpp" -#include "boost/graph/adjacency_list.hpp" -#include "boost/graph/depth_first_search.hpp" -#include "boost/graph/visitors.hpp" - -namespace { - //==================================== - // 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. - //==================================== - - typedef std::pair SimpleEdge; - typedef std::map> EdgeToPathMap; - - typedef boost::adjacency_list Graph; - - typedef boost::graph_traits::edge_descriptor Edge; - - template - std::unordered_set intersect(std::unordered_set const& iLHS, std::unordered_set const& iRHS) { - std::unordered_set result; - if (iLHS.size() < iRHS.size()) { - result.reserve(iLHS.size()); - for (auto const& l : iLHS) { - if (iRHS.find(l) != iRHS.end()) { - result.insert(l); - } - } - return result; - } - result.reserve(iRHS.size()); - for (auto const& r : iRHS) { - if (iLHS.find(r) != iLHS.end()) { - result.insert(r); - } - } - return result; - } - - struct cycle_detector : public boost::dfs_visitor<> { - static const unsigned int kRootVertexIndex = 0; - - cycle_detector(EdgeToPathMap const& iEdgeToPathMap, - std::vector> const& iPathIndexToModuleIndexOrder, - std::vector const& iPathNames, - std::unordered_map const& iModuleIndexToNames) - : m_edgeToPathMap(iEdgeToPathMap), - m_pathIndexToModuleIndexOrder(iPathIndexToModuleIndexOrder), - m_pathNames(iPathNames), - m_indexToNames(iModuleIndexToNames) {} - - bool compare(Edge const& iLHS, Edge const& iRHS) const; - - void tree_edge(Edge const& iEdge, Graph const& iGraph) { - auto const& index = get(boost::vertex_index, iGraph); - - auto in = index[source(iEdge, iGraph)]; - for (auto it = m_stack.begin(); it != m_stack.end(); ++it) { - if (in == index[source(*it, iGraph)]) { - //this vertex is now being used to probe a new edge - // so we should drop the rest of the tree - m_stack.erase(it, m_stack.end()); - break; - } - } - - m_stack.push_back(iEdge); - } - - void finish_vertex(unsigned int iVertex, Graph const& iGraph) { - if (not m_stack.empty()) { - auto const& index = get(boost::vertex_index, iGraph); - - if (iVertex == index[source(m_stack.back(), iGraph)]) { - m_stack.pop_back(); - } - } - } - - //Called if a cycle happens - void back_edge(Edge const& iEdge, Graph const& iGraph) { - auto const& index = get(boost::vertex_index, iGraph); - - if (kRootVertexIndex != index[source(m_stack.front(), iGraph)]) { - //this part of the graph is not connected to data processing - return; - } - - m_stack.push_back(iEdge); - - auto pop_stack = [](std::vector* stack) { stack->pop_back(); }; - std::unique_ptr, decltype(pop_stack)> guard(&m_stack, pop_stack); - - //This edge has not been added to the stack yet - // making a copy allows us to add it in but not worry - // about removing it at the end of the routine - std::vector tempStack; - - tempStack = findMinimumCycle(m_stack, iGraph); - checkCycleForProblem(tempStack, iGraph); - for (auto const& edge : tempStack) { - unsigned int in = index[source(edge, iGraph)]; - unsigned int out = index[target(edge, iGraph)]; - - m_verticiesInFundamentalCycles.insert(in); - m_verticiesInFundamentalCycles.insert(out); - } - - //NOTE: Need to remove any 'extra' bits at beginning of stack - // which may not be part of the cycle - m_fundamentalCycles.emplace_back(std::move(tempStack)); - } - - void forward_or_cross_edge(Edge iEdge, Graph const& iGraph) { - typedef typename boost::property_map::type IndexMap; - IndexMap const& index = get(boost::vertex_index, iGraph); - - if (kRootVertexIndex != index[source(m_stack.front(), iGraph)]) { - //this part of the graph is not connected to data processing - return; - } - - const unsigned int out = index[target(iEdge, iGraph)]; - - //If this is a crossing edge whose out vertex is part of a fundamental cycle - // then this path is also part of a cycle - if (m_verticiesInFundamentalCycles.end() == m_verticiesInFundamentalCycles.find(out)) { - return; - } - - for (auto const& cycle : m_fundamentalCycles) { - //Is the out vertex in this cycle? - auto itStartMatch = cycle.end(); - for (auto it = cycle.begin(); it != cycle.end(); ++it) { - unsigned int inCycle = index[source(*it, iGraph)]; - - if (out == inCycle) { - itStartMatch = it; - break; - } - } - if (itStartMatch == cycle.end()) { - //this cycle isn't the one which uses the vertex from the stack - continue; - } - - //tempStack will hold a stack that could have been found by depth first - // search if module to index ordering had been different - m_stack.push_back(iEdge); - auto pop_stack = [](std::vector* stack) { stack->pop_back(); }; - std::unique_ptr, decltype(pop_stack)> guard(&m_stack, pop_stack); - auto tempStack = findMinimumCycle(m_stack, iGraph); - - //the set of 'in' verticies presently in the stack is used to find where an 'out' - // vertex from the fundamental cycle connects into the present stack - std::set verticiesInStack; - for (auto const& edge : tempStack) { - verticiesInStack.insert(index[source(edge, iGraph)]); - } - - //Now find place in the fundamental cycle that attaches to the stack - // First see if that happens later in the stack - auto itLastMatch = cycle.end(); - for (auto it = itStartMatch; it != cycle.end(); ++it) { - unsigned int outCycle = index[target(*it, iGraph)]; - if (verticiesInStack.end() != verticiesInStack.find(outCycle)) { - itLastMatch = it; - break; - } - } - if (itLastMatch == cycle.end()) { - //See if we can find the attachment to the stack earlier in the cycle - tempStack.insert(tempStack.end(), itStartMatch, cycle.end()); - for (auto it = cycle.begin(); it != itStartMatch; ++it) { - unsigned int outCycle = index[target(*it, iGraph)]; - if (verticiesInStack.end() != verticiesInStack.find(outCycle)) { - itLastMatch = it; - break; - } - } - if (itLastMatch == cycle.end()) { - //need to use the full cycle - //NOTE: this should just retest the same cycle but starting - // from a different position. If everything is correct, then - // this should also pass so in principal we could return here. - //However, as long as this isn't a performance problem, having - // this additional check could catch problems in the algorithm. - tempStack.insert(tempStack.end(), cycle.begin(), itStartMatch); - } else { - tempStack.insert(tempStack.end(), cycle.begin(), itLastMatch + 1); - } - } else { - if ((itStartMatch == cycle.begin()) and (cycle.end() == (itLastMatch + 1))) { - //This is just the entire cycle starting where we've already started - // before. Given the cycle was OK before, it would also be OK this time - return; - } - tempStack.insert(tempStack.end(), itStartMatch, itLastMatch + 1); - } - - tempStack = findMinimumCycle(tempStack, iGraph); - checkCycleForProblem(tempStack, iGraph); - } - } - - private: - std::string const& pathName(unsigned int iIndex) const { return m_pathNames[iIndex]; } - - std::string const& moduleName(unsigned int iIndex) const { - auto itFound = m_indexToNames.find(iIndex); - assert(itFound != m_indexToNames.end()); - return itFound->second; - } - - void throwOnError(std::vector const& iEdges, - boost::property_map::type const& iIndex, - Graph const& iGraph) const { - std::stringstream oStream; - oStream << "Module run order problem found: \n"; - bool first_edge = true; - for (auto const& edge : iEdges) { - unsigned int in = iIndex[source(edge, iGraph)]; - unsigned int out = iIndex[target(edge, iGraph)]; - - if (first_edge) { - first_edge = false; - } else { - oStream << ", "; - } - oStream << moduleName(in); - - auto iFound = m_edgeToPathMap.find(SimpleEdge(in, out)); - bool pathDependencyOnly = true; - for (auto dependency : iFound->second) { - if (dependency == edm::graph::kDataDependencyIndex) { - pathDependencyOnly = false; - break; - } - } - if (pathDependencyOnly) { - oStream << " after " << moduleName(out) << " [path " << pathName(iFound->second[0]) << "]"; - } else { - oStream << " consumes " << moduleName(out); - } - } - oStream << "\n Running in the threaded framework would lead to indeterminate results." - "\n Please change order of modules in mentioned Path(s) to avoid inconsistent module ordering."; - - throw edm::Exception(edm::errors::ScheduleExecutionFailure, "Unrunnable schedule\n") << oStream.str() << "\n"; - } - - std::vector findMinimumCycle(std::vector const& iCycleEdges, Graph const& iGraph) const { - //Remove unnecessary edges - // The graph library scans the verticies so we have edges in the list which are - // not part of the cycle but are associated to a vertex contributes to the cycle. - // To find these unneeded edges we work backwards on the edge list looking for cases - // where the 'in' on the previous edge is not the 'out' for the next edge. When this - // happens we know that there are additional edges for that same 'in' which can be - // removed. - - typedef typename boost::property_map::type IndexMap; - IndexMap const& index = get(boost::vertex_index, iGraph); - - std::vector reducedEdges; - reducedEdges.reserve(iCycleEdges.size()); - reducedEdges.push_back(iCycleEdges.back()); - unsigned int lastIn = index[source(iCycleEdges.back(), iGraph)]; - const unsigned int finalVertex = index[target(iCycleEdges.back(), iGraph)]; - for (auto it = iCycleEdges.rbegin() + 1; it != iCycleEdges.rend(); ++it) { - unsigned int in = index[source(*it, iGraph)]; - unsigned int out = index[target(*it, iGraph)]; - if (lastIn == out) { - reducedEdges.push_back(*it); - lastIn = in; - if (in == finalVertex) { - break; - } - } - } - std::reverse(reducedEdges.begin(), reducedEdges.end()); - - return reducedEdges; - } - - void checkCycleForProblem(std::vector const& iCycleEdges, Graph const& iGraph) { - //For a real problem, we need at least one data dependency - // we already know we originate from a path because all tests - // require starting from the root node which connects to all paths - bool hasDataDependency = false; - //Since we are dealing with a circle, we initialize the 'last' info with the end of the graph - typedef typename boost::property_map::type IndexMap; - IndexMap const& index = get(boost::vertex_index, iGraph); - - unsigned int lastIn = index[source(iCycleEdges.back(), iGraph)]; - unsigned int lastOut = index[target(iCycleEdges.back(), iGraph)]; - bool lastEdgeHasDataDepencency = false; - - std::unordered_set lastPathsSeen; - - //If a data dependency appears to make us jump off a path but that module actually - // appears on the path that was left, we need to see if we later come back to that - // path somewhere before that module. If not than it is a false cycle - std::unordered_multimap pathToModulesWhichMustAppearLater; - bool moduleAppearedEarlierInPath = false; - - for (auto dependency : m_edgeToPathMap.find(SimpleEdge(lastIn, lastOut))->second) { - if (dependency != edm::graph::kDataDependencyIndex) { - lastPathsSeen.insert(dependency); - } else { - lastEdgeHasDataDepencency = true; - } - } - //Need to check that the - bool minimumInitialPathsSet = false; - std::unordered_set initialPaths(lastPathsSeen); - std::unordered_set sharedPaths; - for (auto const& edge : iCycleEdges) { - unsigned int in = index[source(edge, iGraph)]; - unsigned int out = index[target(edge, iGraph)]; - - auto iFound = m_edgeToPathMap.find(SimpleEdge(in, out)); - std::unordered_set pathsOnEdge; - bool edgeHasDataDependency = false; - for (auto dependency : iFound->second) { - if (dependency == edm::graph::kDataDependencyIndex) { - //need to count only if this moves us to a new path - hasDataDependency = true; - edgeHasDataDependency = true; - } else { - pathsOnEdge.insert(dependency); - - auto const& pathIndicies = m_pathIndexToModuleIndexOrder[dependency]; - auto pathToCheckRange = pathToModulesWhichMustAppearLater.equal_range(dependency); - for (auto it = pathToCheckRange.first; it != pathToCheckRange.second;) { - auto moduleIDToCheck = it->second; - if (moduleIDToCheck == in or moduleIDToCheck == out) { - auto toErase = it; - ++it; - pathToModulesWhichMustAppearLater.erase(toErase); - continue; - } - bool alreadyAdvanced = false; - for (auto pathIndex : pathIndicies) { - if (pathIndex == out) { - //we must have skipped over the module so the earlier worry about the - // module being called on the path was wrong - auto toErase = it; - ++it; - alreadyAdvanced = true; - pathToModulesWhichMustAppearLater.erase(toErase); - break; - } - if (pathIndex == moduleIDToCheck) { - //module still earlier on the path - break; - } - } - if (not alreadyAdvanced) { - ++it; - } - } - } - } - sharedPaths = intersect(pathsOnEdge, lastPathsSeen); - if (sharedPaths.empty()) { - minimumInitialPathsSet = true; - if ((not edgeHasDataDependency) and (not lastEdgeHasDataDepencency) and (not lastPathsSeen.empty())) { - //If we jumped from one path to another without a data dependency - // than the cycle is just because two independent modules were - // scheduled in different arbitrary order on different paths - return; - } - if (edgeHasDataDependency and not lastPathsSeen.empty()) { - //If the paths we were on had this module we are going to earlier - // on their paths than we do not have a real cycle - bool atLeastOnePathFailed = false; - std::vector pathsToWatch; - pathsToWatch.reserve(lastPathsSeen.size()); - for (auto seenPath : lastPathsSeen) { - if (pathsOnEdge.end() == pathsOnEdge.find(seenPath)) { - //we left this path so we now need to see if the module 'out' - // is on this path ahead of the module 'in' - bool foundOut = false; - for (auto seenPathIndex : m_pathIndexToModuleIndexOrder[seenPath]) { - if (seenPathIndex == out) { - foundOut = true; - pathsToWatch.push_back(seenPath); - } - if (seenPathIndex == lastOut) { - if (not foundOut) { - atLeastOnePathFailed = true; - } - break; - } - if (atLeastOnePathFailed) { - break; - } - } - } - } - //If all the paths have the module earlier in their paths - // then there was no need to jump between paths to get it - // and this breaks the data cycle - if (not atLeastOnePathFailed) { - moduleAppearedEarlierInPath = true; - for (auto p : pathsToWatch) { - pathToModulesWhichMustAppearLater.emplace(p, out); - } - } - } - lastPathsSeen = pathsOnEdge; - } else { - lastPathsSeen = sharedPaths; - if (not minimumInitialPathsSet) { - initialPaths = sharedPaths; - } - } - lastOut = out; - lastEdgeHasDataDepencency = edgeHasDataDependency; - } - if (moduleAppearedEarlierInPath and not pathToModulesWhichMustAppearLater.empty()) { - return; - } - if (not hasDataDependency) { - return; - } - if ((not initialPaths.empty()) and intersect(initialPaths, sharedPaths).empty()) { - //The effective start and end paths for the first graph - // node do not match. This can happen if the node - // appears on multiple paths - return; - } - throwOnError(iCycleEdges, index, iGraph); - } - - EdgeToPathMap const& m_edgeToPathMap; - std::vector> const& m_pathIndexToModuleIndexOrder; - std::vector const& m_pathNames; - std::unordered_map m_indexToNames; - std::unordered_map> m_pathToModuleIndex; - - std::vector m_stack; - std::vector> m_fundamentalCycles; - std::set m_verticiesInFundamentalCycles; - }; -} // namespace - -void edm::graph::throwIfImproperDependencies(EdgeToPathMap const& iEdgeToPathMap, - std::vector> const& iPathIndexToModuleIndexOrder, - std::vector const& iPathNames, - std::unordered_map const& iModuleIndexToNames) { - //Now use boost graph library to find cycles in the dependencies - std::vector outList; - outList.reserve(iEdgeToPathMap.size()); - for (auto const& edgeInfo : iEdgeToPathMap) { - outList.push_back(edgeInfo.first); - } - - Graph g(outList.begin(), outList.end(), iModuleIndexToNames.size()); - - cycle_detector detector(iEdgeToPathMap, iPathIndexToModuleIndexOrder, iPathNames, iModuleIndexToNames); - boost::depth_first_search(g, boost::visitor(detector)); -} diff --git a/FWCore/Framework/src/throwIfImproperDependencies.h b/FWCore/Framework/src/throwIfImproperDependencies.h deleted file mode 100644 index 6912577710bc8..0000000000000 --- a/FWCore/Framework/src/throwIfImproperDependencies.h +++ /dev/null @@ -1,49 +0,0 @@ -#ifndef FWCore_Framework_throwIfImproperDependencies_h -#define FWCore_Framework_throwIfImproperDependencies_h -// -*- C++ -*- -// -// Package: FWCore/Framework -// Function: throwIfImproperDependencies -// -/**\function throwIfImproperDependencies throwIfImproperDependencies.h "throwIfImproperDependencies.h" - - Description: Function which uses the graph of dependencies to determine if there are any cycles - - Usage: - - -*/ -// -// Original Author: Chris Jones -// Created: Tue, 06 Sep 2016 16:04:26 GMT -// - -// system include files -#include -#include -#include -#include -#include - -// user include files - -// forward declarations - -namespace edm { - namespace graph { - constexpr auto kInvalidIndex = std::numeric_limits::max(); - //This index is used as the Path index for the case where we are - // describing a data dependency and not a dependency on a Path - constexpr auto kDataDependencyIndex = std::numeric_limits::max(); - - using SimpleEdge = std::pair; - using EdgeToPathMap = std::map>; - - void throwIfImproperDependencies(EdgeToPathMap const&, - std::vector> const& iPathIndexToModuleIndexOrder, - std::vector const& iPathNames, - std::unordered_map const& iModuleIndexToNames); - } // namespace graph -}; // namespace edm - -#endif diff --git a/FWCore/Framework/test/BuildFile.xml b/FWCore/Framework/test/BuildFile.xml index 970412381d6a9..329e5cdacd736 100644 --- a/FWCore/Framework/test/BuildFile.xml +++ b/FWCore/Framework/test/BuildFile.xml @@ -189,7 +189,7 @@ - + @@ -396,3 +396,11 @@ + + + + + + + + diff --git a/FWCore/Framework/test/throwIfImproperDependencies_t.cppunit.cc b/FWCore/Framework/test/checkForModuleDependencyCorrectness_t.cppunit.cc similarity index 61% rename from FWCore/Framework/test/throwIfImproperDependencies_t.cppunit.cc rename to FWCore/Framework/test/checkForModuleDependencyCorrectness_t.cppunit.cc index 9daaf5207f9d7..ae0e48814510b 100644 --- a/FWCore/Framework/test/throwIfImproperDependencies_t.cppunit.cc +++ b/FWCore/Framework/test/checkForModuleDependencyCorrectness_t.cppunit.cc @@ -10,18 +10,164 @@ #include #include #include +#include -#include "FWCore/Framework/src/throwIfImproperDependencies.h" +#include "FWCore/Framework/interface/PathsAndConsumesOfModules.h" #include "FWCore/Utilities/interface/Exception.h" +#include "DataFormats/Provenance/interface/ParameterSetID.h" +#include "DataFormats/Provenance/interface/ModuleDescription.h" +#include "DataFormats/Provenance/interface/ProcessConfiguration.h" #include "cppunit/extensions/HelperMacros.h" -class test_throwIfImproperDependencies : public CppUnit::TestFixture { - CPPUNIT_TEST_SUITE(test_throwIfImproperDependencies); +using ModuleDependsOnMap = std::map>; +using PathToModules = std::unordered_map>; + +namespace { + class PathsAndConsumesOfModulesForTest : public edm::PathsAndConsumesOfModulesBase { + public: + PathsAndConsumesOfModulesForTest(ModuleDependsOnMap const&, PathToModules const&); + + private: + std::vector const& doPaths() const final { return m_paths; } + std::vector const& doEndPaths() const final { return m_endPaths; } + std::vector const& doAllModules() const final { return m_modules; } + edm::ModuleDescription const* doModuleDescription(unsigned int moduleID) const final { return m_modules[moduleID]; } + std::vector const& doModulesOnPath(unsigned int pathIndex) const final { + return m_modulesOnPath[pathIndex]; + } + std::vector const& doModulesOnEndPath(unsigned int endPathIndex) const final { + return m_modulesOnEndPath[endPathIndex]; + } + std::vector const& doModulesWhoseProductsAreConsumedBy( + unsigned int moduleID, edm::BranchType branchType) const final { + return m_modulesWhoseProductsAreConsumedBy[moduleID]; + } + std::vector doConsumesInfo(unsigned int moduleID) const final { + return m_moduleConsumesInfo[moduleID]; + } + unsigned int doLargestModuleID() const final { + if (m_modules.empty()) { + return 0; + } + return m_modules.size() - 1; + } + + std::vector m_paths; + std::vector m_endPaths; + std::vector m_modules; + std::vector> m_moduleConsumesInfo; + std::vector> m_modulesOnPath; + std::vector> m_modulesOnEndPath; + std::vector> m_modulesWhoseProductsAreConsumedBy; + std::vector m_cache; + + static unsigned int indexForModule(std::string const& iName, + std::unordered_map& modsToIndex, + std::unordered_map& indexToMods) { + auto found = modsToIndex.find(iName); + unsigned int fromIndex; + if (found == modsToIndex.end()) { + fromIndex = modsToIndex.size(); + modsToIndex.emplace(iName, fromIndex); + indexToMods.emplace(fromIndex, iName); + } else { + fromIndex = found->second; + } + return fromIndex; + } + }; + PathsAndConsumesOfModulesForTest::PathsAndConsumesOfModulesForTest(ModuleDependsOnMap const& iModDeps, + PathToModules const& iPaths) { + //setup module indicies + std::unordered_map modsToIndex; + std::unordered_map indexToMods; + + const edm::ProcessConfiguration pc("TEST", edm::ParameterSetID{}, "CMSSW_x_y_z", "??"); + + //In actual configuration building, the source is always assigned id==0 + m_cache.emplace_back( + edm::ParameterSetID{}, "source", "source", &pc, indexForModule("source", modsToIndex, indexToMods)); + + for (auto const& md : iModDeps) { + auto const lastSize = modsToIndex.size(); + auto index = indexForModule(md.first, modsToIndex, indexToMods); + if (index == lastSize) { + m_cache.emplace_back(edm::ParameterSetID{}, md.first, md.first, &pc, index); + } + } + m_paths.reserve(iPaths.size()); + for (auto const& pToM : iPaths) { + m_paths.push_back(pToM.first); + + for (auto const& mod : pToM.second) { + auto const lastSize = modsToIndex.size(); + unsigned int index = indexForModule(mod, modsToIndex, indexToMods); + if (index == lastSize) { + m_cache.emplace_back(edm::ParameterSetID{}, mod, mod, &pc, index); + } + } + } + for (auto const& md : iModDeps) { + for (auto const& dep : md.second) { + auto const lastSize = modsToIndex.size(); + auto index = indexForModule(dep, modsToIndex, indexToMods); + if (index == lastSize) { + m_cache.emplace_back(edm::ParameterSetID{}, dep, dep, &pc, index); + } + } + } + + if (not iPaths.empty()) { + auto indexForTriggerResults = indexForModule("TriggerResults", modsToIndex, indexToMods); + for (auto const& pToM : iPaths) { + auto index = indexForModule(pToM.first, modsToIndex, indexToMods); + m_cache.emplace_back(edm::ParameterSetID{}, "PathStatusInserter", pToM.first, &pc, index); + } + m_cache.emplace_back( + edm::ParameterSetID{}, "TriggerResultInserter", "TriggerResults", &pc, indexForTriggerResults); + } + + m_modules.reserve(m_cache.size()); + for (auto const& desc : m_cache) { + m_modules.push_back(&desc); + } + + //do consumes + edm::TypeID dummy; + m_moduleConsumesInfo.resize(m_modules.size()); + m_modulesWhoseProductsAreConsumedBy.resize(m_modules.size()); + for (auto const& md : iModDeps) { + auto moduleID = modsToIndex[md.first]; + auto& consumes = m_moduleConsumesInfo[moduleID]; + consumes.reserve(md.second.size()); + for (auto const& dep : md.second) { + consumes.emplace_back(dummy, dep.c_str(), "", "TEST", edm::InEvent, edm::PRODUCT_TYPE, true, false); + m_modulesWhoseProductsAreConsumedBy[moduleID].push_back(m_modules[modsToIndex[dep]]); + //m_modulesWhoseProductsAreConsumedBy[modsToIndex[dep]].push_back(m_modules[moduleID]); + } + } + + m_modulesOnPath.reserve(m_paths.size()); + for (auto const& pToM : iPaths) { + m_modulesOnPath.emplace_back(); + auto& newPath = m_modulesOnPath.back(); + newPath.reserve(pToM.second.size()); + for (auto const& mod : pToM.second) { + newPath.push_back(m_modules[modsToIndex[mod]]); + } + } + } +} // namespace + +class test_checkForModuleDependencyCorrectness : public CppUnit::TestFixture { + CPPUNIT_TEST_SUITE(test_checkForModuleDependencyCorrectness); CPPUNIT_TEST(onePathNoCycleTest); CPPUNIT_TEST(onePathHasCycleTest); CPPUNIT_TEST(twoPathsNoCycleTest); CPPUNIT_TEST(twoPathsWithCycleTest); + CPPUNIT_TEST(duplicateModuleOnPathTest); + CPPUNIT_TEST(selfCycleTest); CPPUNIT_TEST_SUITE_END(); @@ -35,122 +181,23 @@ class test_throwIfImproperDependencies : public CppUnit::TestFixture { void twoPathsNoCycleTest(); void twoPathsWithCycleTest(); - using ModuleDependsOnMap = std::map>; - using PathToModules = std::unordered_map>; + void selfCycleTest(); -private: - static unsigned int indexForModule(std::string const& iName, - std::unordered_map& modsToIndex, - std::unordered_map& indexToMods) { - auto found = modsToIndex.find(iName); - unsigned int fromIndex; - if (found == modsToIndex.end()) { - fromIndex = modsToIndex.size(); - modsToIndex.emplace(iName, fromIndex); - indexToMods.emplace(fromIndex, iName); - } else { - fromIndex = found->second; - } - return fromIndex; - } + void duplicateModuleOnPathTest(); +private: bool testCase(ModuleDependsOnMap const& iModDeps, PathToModules const& iPaths) const { - using namespace edm::graph; - - EdgeToPathMap edgeToPathMap; - - std::unordered_map modsToIndex; - std::unordered_map indexToMods; - - //We have an artificial case to be the root of the graph - const std::string kFinishedProcessing("FinishedProcessing"); - const unsigned int kFinishedProcessingIndex{0}; - modsToIndex.emplace(kFinishedProcessing, kFinishedProcessingIndex); - indexToMods.emplace(kFinishedProcessingIndex, kFinishedProcessing); - - //Setup the module to index map by using all module names used in both containers - //Start with keys from the module dependency to allow control of the numbering scheme - for (auto const& md : iModDeps) { - indexForModule(md.first, modsToIndex, indexToMods); - } - - std::vector> pathIndexToModuleIndexOrder(iPaths.size()); - - //Need to be able to quickly look up which paths a module appears on - std::unordered_map> moduleIndexToPathIndex; - - std::vector pathNames; - std::unordered_map pathToIndexMap; - for (auto const& path : iPaths) { - unsigned int lastModuleIndex = kInvalidIndex; - pathNames.push_back(path.first); - unsigned int pathIndex = pathToIndexMap.size(); - auto& pathOrder = pathIndexToModuleIndexOrder[pathIndex]; - pathToIndexMap.emplace(path.first, pathIndex); - for (auto const& mod : path.second) { - unsigned int index = indexForModule(mod, modsToIndex, indexToMods); - pathOrder.push_back(index); - moduleIndexToPathIndex[index].push_back(pathIndex); - - if (lastModuleIndex != kInvalidIndex) { - edgeToPathMap[std::make_pair(index, lastModuleIndex)].push_back(pathIndex); - } - lastModuleIndex = index; - } - pathOrder.push_back(kFinishedProcessingIndex); - if (lastModuleIndex != kInvalidIndex) { - edgeToPathMap[std::make_pair(kFinishedProcessingIndex, lastModuleIndex)].push_back(pathIndex); - } - } - - for (auto const& md : iModDeps) { - unsigned int fromIndex = indexForModule(md.first, modsToIndex, indexToMods); - for (auto const& dependsOn : md.second) { - unsigned int toIndex = indexForModule(dependsOn, modsToIndex, indexToMods); - - //see if all paths containing this module also contain the dependent module earlier in the path - // if it does, then treat this only as a path dependency and not a data dependency as this - // simplifies the circular dependency checking logic - - auto itPathsFound = moduleIndexToPathIndex.find(fromIndex); - bool keepDataDependency = true; - if (itPathsFound != moduleIndexToPathIndex.end() and - moduleIndexToPathIndex.find(toIndex) != moduleIndexToPathIndex.end()) { - keepDataDependency = false; - for (auto const pathIndex : itPathsFound->second) { - for (auto idToCheck : pathIndexToModuleIndexOrder[pathIndex]) { - if (idToCheck == toIndex) { - //found dependent module first so check next path - break; - } - if (idToCheck == fromIndex) { - //did not find dependent module earlier on path so - // must keep data dependency - keepDataDependency = true; - break; - } - } - if (keepDataDependency) { - break; - } - } - } - if (keepDataDependency) { - edgeToPathMap[std::make_pair(fromIndex, toIndex)].push_back(kDataDependencyIndex); - } - } - } - - throwIfImproperDependencies(edgeToPathMap, pathIndexToModuleIndexOrder, pathNames, indexToMods); + PathsAndConsumesOfModulesForTest pAndC(iModDeps, iPaths); + checkForModuleDependencyCorrectness(pAndC, false); return true; } }; ///registration of the test so that the runner can find it -CPPUNIT_TEST_SUITE_REGISTRATION(test_throwIfImproperDependencies); +CPPUNIT_TEST_SUITE_REGISTRATION(test_checkForModuleDependencyCorrectness); -void test_throwIfImproperDependencies::onePathNoCycleTest() { +void test_checkForModuleDependencyCorrectness::onePathNoCycleTest() { { ModuleDependsOnMap md = {{"C", {"B"}}, {"B", {"A"}}}; PathToModules paths = {{"p", {"A", "B", "C"}}}; @@ -177,7 +224,7 @@ void test_throwIfImproperDependencies::onePathNoCycleTest() { } } -void test_throwIfImproperDependencies::onePathHasCycleTest() { +void test_checkForModuleDependencyCorrectness::onePathHasCycleTest() { { ModuleDependsOnMap md = {{"C", {"B"}}, {"B", {"A"}}}; { @@ -222,7 +269,7 @@ void test_throwIfImproperDependencies::onePathHasCycleTest() { } } -void test_throwIfImproperDependencies::twoPathsNoCycleTest() { +void test_checkForModuleDependencyCorrectness::twoPathsNoCycleTest() { { ModuleDependsOnMap md = {{"C", {"B"}}}; @@ -472,9 +519,35 @@ void test_throwIfImproperDependencies::twoPathsNoCycleTest() { CPPUNIT_ASSERT(testCase(md, paths)); } + + { + //Have a module which can not be run initially be needed by two other modules + ModuleDependsOnMap md = {{"out", {"A", "B"}}, {"A", {"D"}}, {"B", {"D"}}}; + PathToModules paths = {{"p1", {"filter", "D"}}, {"p2", {"out"}}}; + CPPUNIT_ASSERT(testCase(md, paths)); + } + { + //like above, but with path names reversed + ModuleDependsOnMap md = {{"out", {"A", "B"}}, {"A", {"D"}}, {"B", {"D"}}}; + PathToModules paths = {{"p1", {"out"}}, {"p2", {"filter", "D"}}}; + CPPUNIT_ASSERT(testCase(md, paths)); + } + + { + //Have a module which can not be run initially be needed by two other modules + ModuleDependsOnMap md = {{"out", {"A", "B"}}, {"A", {"D"}}, {"B", {"D"}}, {"D", {"E"}}}; + PathToModules paths = {{"p1", {"filter", "E"}}, {"p2", {"out"}}}; + CPPUNIT_ASSERT(testCase(md, paths)); + } + { + //like above, but with path names reversed + ModuleDependsOnMap md = {{"out", {"A", "B"}}, {"A", {"D"}}, {"B", {"D"}}, {"D", {"E"}}}; + PathToModules paths = {{"p1", {"out"}}, {"p2", {"filter", "E"}}}; + CPPUNIT_ASSERT(testCase(md, paths)); + } } -void test_throwIfImproperDependencies::twoPathsWithCycleTest() { +void test_checkForModuleDependencyCorrectness::twoPathsWithCycleTest() { { ModuleDependsOnMap md = {{"C", {"B"}}, {"A", {"D"}}}; PathToModules paths = {{"p1", {"A", "B"}}, {"p2", {"C", "D"}}}; @@ -572,4 +645,35 @@ void test_throwIfImproperDependencies::twoPathsWithCycleTest() { CPPUNIT_ASSERT_THROW(testCase(md, paths), cms::Exception); } + + { + ModuleDependsOnMap md = {{"B", {"A"}}, {"C", {"B"}}, {"cFilter", {"C"}}}; + PathToModules paths = {{"p1", {"C", "cFilter", "D", "E", "F", "A", "B"}}, {"p2", {"oFilter", "D", "F", "B"}}}; + + CPPUNIT_ASSERT_THROW(testCase(md, paths), cms::Exception); + } +} + +void test_checkForModuleDependencyCorrectness::selfCycleTest() { + { + ModuleDependsOnMap md = {{"A", {"A"}}}; + PathToModules paths = {{"p", {"A"}}}; + + CPPUNIT_ASSERT_THROW(testCase(md, paths), cms::Exception); + } + { + ModuleDependsOnMap md = {{"A", {"A"}}, {"B", {"A"}}}; + PathToModules paths = {{"p", {"B"}}}; + + CPPUNIT_ASSERT_THROW(testCase(md, paths), cms::Exception); + } +} + +void test_checkForModuleDependencyCorrectness::duplicateModuleOnPathTest() { + { + ModuleDependsOnMap md = {{"C", {"B"}}, {"B", {"A"}}}; + PathToModules paths = {{"p", {"A", "B", "C", "A"}}}; + + CPPUNIT_ASSERT(testCase(md, paths)); + } } diff --git a/FWCore/Framework/test/test_bad_schedule_exception_message_cfg.py b/FWCore/Framework/test/test_bad_schedule_exception_message_cfg.py new file mode 100644 index 0000000000000..3ba21b8d91f5d --- /dev/null +++ b/FWCore/Framework/test/test_bad_schedule_exception_message_cfg.py @@ -0,0 +1,101 @@ +import FWCore.ParameterSet.Config as cms +import sys + +process = cms.Process("Test") + + +process.source = cms.Source("EmptySource") + + +mod = int(sys.argv[2]) + +process.load('FWCore.MessageService.MessageLogger_cfi') +process.MessageLogger.cerr.enable = False +process.MessageLogger.files.exceptionMessage = dict(filename="test_bad_schedule_{}".format(mod), + noTimeStamps= True) + +if mod == 0 : + #directly depends on + process.a = cms.EDProducer("AddIntsProducer", + labels=cms.VInputTag(cms.InputTag("a"))) + process.p = cms.Path(process.a) +elif mod == 1: + #cross path dependency + process.a = cms.EDProducer("AddIntsProducer", + labels=cms.VInputTag(cms.InputTag("b"))) + + process.b = cms.EDProducer("AddIntsProducer", + labels=cms.VInputTag(cms.InputTag("a"))) + + process.p = cms.Path(process.a) + process.p2 = cms.Path(process.b) + +elif mod == 2: + #path ordering + process.a = cms.EDProducer("AddIntsProducer", + labels=cms.VInputTag(cms.InputTag("b"))) + + process.b = cms.EDProducer("AddIntsProducer", + labels=cms.VInputTag(cms.InputTag(""))) + process.p = cms.Path(process.a + process.b) + +elif mod == 3: + #path ordering, extra path 1 + process.a = cms.EDProducer("AddIntsProducer", + labels=cms.VInputTag(cms.InputTag("b"))) + + process.b = cms.EDProducer("AddIntsProducer", + labels=cms.VInputTag(cms.InputTag(""))) + process.p = cms.Path(process.a + process.b) + process.p2 = cms.Path(process.a) + +elif mod == 4: + #path ordering, extra path 1 + process.a = cms.EDProducer("AddIntsProducer", + labels=cms.VInputTag(cms.InputTag("b"))) + + process.b = cms.EDProducer("AddIntsProducer", + labels=cms.VInputTag(cms.InputTag(""))) + process.p2 = cms.Path(process.a + process.b) + process.p = cms.Path(process.a) + +elif mod == 5: + #cycle with unscheduled + process.a = cms.EDProducer("AddIntsProducer", + labels=cms.VInputTag(cms.InputTag("b"))) + + process.b = cms.EDProducer("AddIntsProducer", + labels=cms.VInputTag(cms.InputTag("a"))) + + process.p = cms.Path(process.a, cms.Task(process.b)) + +elif mod == 6: + #multi-path cycle with unscheduled + process.a = cms.EDProducer("AddIntsProducer", + labels=cms.VInputTag(cms.InputTag("b"))) + + process.b = cms.EDProducer("AddIntsProducer", + labels=cms.VInputTag(cms.InputTag("c"))) + + process.c = cms.EDProducer("AddIntsProducer", + labels=cms.VInputTag(cms.InputTag("d"))) + process.d = cms.EDProducer("AddIntsProducer", + labels=cms.VInputTag(cms.InputTag("a"))) + + process.p1 = cms.Path(process.a, cms.Task(process.b)) + process.p2 = cms.Path(process.c, cms.Task(process.d)) + +elif mod == 7: + #cycle with unscheduled + process.a = cms.EDProducer("AddIntsProducer", + labels=cms.VInputTag(cms.InputTag("b"))) + + process.b = cms.EDProducer("AddIntsProducer", + labels=cms.VInputTag(cms.InputTag("c"))) + + process.c = cms.EDProducer("AddIntsProducer", + labels=cms.VInputTag(cms.InputTag("b"))) + + process.p1 = cms.Path(process.a, cms.Task(process.b, process.c)) + + diff --git a/FWCore/Framework/test/unit_test_outputs/test_bad_schedule_0.log b/FWCore/Framework/test/unit_test_outputs/test_bad_schedule_0.log new file mode 100644 index 0000000000000..5e079d1cb162f --- /dev/null +++ b/FWCore/Framework/test/unit_test_outputs/test_bad_schedule_0.log @@ -0,0 +1,6 @@ +An exception of category 'ScheduleExecutionFailure' occurred while + [0] Calling beginJob +Exception Message: +Unrunnable schedule +The Path/EndPath configuration could cause the job to deadlock + module 'a' is on path 'p' and depends on module 'a' diff --git a/FWCore/Framework/test/unit_test_outputs/test_bad_schedule_1.log b/FWCore/Framework/test/unit_test_outputs/test_bad_schedule_1.log new file mode 100644 index 0000000000000..90889024d6f71 --- /dev/null +++ b/FWCore/Framework/test/unit_test_outputs/test_bad_schedule_1.log @@ -0,0 +1,7 @@ +An exception of category 'ScheduleExecutionFailure' occurred while + [0] Calling beginJob +Exception Message: +Unrunnable schedule +The Path/EndPath configuration could cause the job to deadlock + module 'a' is on path 'p' and depends on module 'b' + module 'b' is on path 'p2' and depends on module 'a' diff --git a/FWCore/Framework/test/unit_test_outputs/test_bad_schedule_2.log b/FWCore/Framework/test/unit_test_outputs/test_bad_schedule_2.log new file mode 100644 index 0000000000000..f4fa8eb31e860 --- /dev/null +++ b/FWCore/Framework/test/unit_test_outputs/test_bad_schedule_2.log @@ -0,0 +1,7 @@ +An exception of category 'ScheduleExecutionFailure' occurred while + [0] Calling beginJob +Exception Message: +Unrunnable schedule +The Path/EndPath configuration could cause the job to deadlock + module 'a' is on path 'p' and depends on module 'b' + module 'b' is on path 'p' and follows module 'a' on the path diff --git a/FWCore/Framework/test/unit_test_outputs/test_bad_schedule_3.log b/FWCore/Framework/test/unit_test_outputs/test_bad_schedule_3.log new file mode 100644 index 0000000000000..f4fa8eb31e860 --- /dev/null +++ b/FWCore/Framework/test/unit_test_outputs/test_bad_schedule_3.log @@ -0,0 +1,7 @@ +An exception of category 'ScheduleExecutionFailure' occurred while + [0] Calling beginJob +Exception Message: +Unrunnable schedule +The Path/EndPath configuration could cause the job to deadlock + module 'a' is on path 'p' and depends on module 'b' + module 'b' is on path 'p' and follows module 'a' on the path diff --git a/FWCore/Framework/test/unit_test_outputs/test_bad_schedule_4.log b/FWCore/Framework/test/unit_test_outputs/test_bad_schedule_4.log new file mode 100644 index 0000000000000..6f4594ce95143 --- /dev/null +++ b/FWCore/Framework/test/unit_test_outputs/test_bad_schedule_4.log @@ -0,0 +1,7 @@ +An exception of category 'ScheduleExecutionFailure' occurred while + [0] Calling beginJob +Exception Message: +Unrunnable schedule +The Path/EndPath configuration could cause the job to deadlock + module 'a' is on path 'p2' and depends on module 'b' + module 'b' is on path 'p2' and follows module 'a' on the path diff --git a/FWCore/Framework/test/unit_test_outputs/test_bad_schedule_5.log b/FWCore/Framework/test/unit_test_outputs/test_bad_schedule_5.log new file mode 100644 index 0000000000000..3e642e23c9c63 --- /dev/null +++ b/FWCore/Framework/test/unit_test_outputs/test_bad_schedule_5.log @@ -0,0 +1,7 @@ +An exception of category 'ScheduleExecutionFailure' occurred while + [0] Calling beginJob +Exception Message: +Unrunnable schedule +The Path/EndPath configuration could cause the job to deadlock + module 'a' is on path 'p' and depends on module 'b' + module 'b' is in a task and depends on module 'a' diff --git a/FWCore/Framework/test/unit_test_outputs/test_bad_schedule_6.log b/FWCore/Framework/test/unit_test_outputs/test_bad_schedule_6.log new file mode 100644 index 0000000000000..fb9b905e8a38f --- /dev/null +++ b/FWCore/Framework/test/unit_test_outputs/test_bad_schedule_6.log @@ -0,0 +1,9 @@ +An exception of category 'ScheduleExecutionFailure' occurred while + [0] Calling beginJob +Exception Message: +Unrunnable schedule +The Path/EndPath configuration could cause the job to deadlock + module 'a' is on path 'p1' and depends on module 'b' + module 'b' is in a task and depends on module 'c' + module 'c' is on path 'p2' and depends on module 'd' + module 'd' is in a task and depends on module 'a' diff --git a/FWCore/Framework/test/unit_test_outputs/test_bad_schedule_7.log b/FWCore/Framework/test/unit_test_outputs/test_bad_schedule_7.log new file mode 100644 index 0000000000000..f1cc00d369fd2 --- /dev/null +++ b/FWCore/Framework/test/unit_test_outputs/test_bad_schedule_7.log @@ -0,0 +1,8 @@ +An exception of category 'ScheduleExecutionFailure' occurred while + [0] Calling beginJob +Exception Message: +Unrunnable schedule +Circular module dependency found in configuration + module 'a' depends on b + module 'b' depends on c + module 'c' depends on b