Use "best matching" EDAlias in module dependence checks when placing …

…ConditionalTask modules in Path If the EDAlias has wildcards for one module, and more specific aliases for another module, we should use the one that has the most constrained match. There is one ambiguous case left, for which an exception is thrown. I hope we won't encounter that situation in practice. The problem was encountered with a SwitchProducer having the EDAlias case, but I think the problem could happen also without SwitchProducer.
cms-sw · Sep 16, 2022 · cf5e630 · cf5e630
1 parent 30a3bf4
commit cf5e630
Show file tree

Hide file tree

Showing 3 changed files with 166 additions and 32 deletions.
diff --git a/FWCore/Framework/src/StreamSchedule.cc b/FWCore/Framework/src/StreamSchedule.cc
@@ -158,6 +158,68 @@ namespace edm {
       }
       return std::pair(beg + 1, std::prev(modnames.end()));
     }
+
+    std::optional<std::string> findBestMatchingAlias(
+        std::unordered_multimap<std::string, edm::BranchDescription const*> const& conditionalModuleBranches,
+        std::unordered_multimap<std::string, StreamSchedule::AliasInfo> const& aliasMap,
+        std::string const& productModuleLabel,
+        ConsumesInfo const& consumesInfo) {
+      std::optional<std::string> best;
+      int wildcardsInBest;
+
+      auto updateBest = [&best, &wildcardsInBest, &consumesInfo, &productModuleLabel](
+                            std::string const& label, bool instanceIsWildcard, bool typeIsWildcard) {
+        int const wildcards = static_cast<int>(instanceIsWildcard) + static_cast<int>(typeIsWildcard);
+        if (wildcards == 0) {
+          return true;
+        }
+        if (not best or wildcards < wildcardsInBest) {
+          best = label;
+          wildcardsInBest = wildcards;
+        } else if (best and *best != label and wildcardsInBest == wildcards) {
+          throw Exception(errors::UnimplementedFeature)
+              << "Encountered ambiguity when trying to find a best-matching alias for\n"
+              << " friendly class name " << consumesInfo.type().friendlyClassName() << "\n"
+              << " module label " << productModuleLabel << "\n"
+              << " product instance name " << consumesInfo.instance() << "\n"
+              << "when processing EDAliases for modules in ConditionalTasks. Two aliases have the same number of "
+                 "wildcards ("
+              << wildcards << ")";
+        }
+        return false;
+      };
+
+      auto findAlias = aliasMap.equal_range(productModuleLabel);
+      for (auto it = findAlias.first; it != findAlias.second; ++it) {
+        std::string const& aliasInstanceLabel =
+            it->second.instanceLabel != "*" ? it->second.instanceLabel : it->second.originalInstanceLabel;
+        bool const instanceIsWildcard = (aliasInstanceLabel == "*");
+        if (instanceIsWildcard or consumesInfo.instance() == aliasInstanceLabel) {
+          bool const typeIsWildcard = it->second.friendlyClassName == "*";
+          if (typeIsWildcard or (consumesInfo.type().friendlyClassName() == it->second.friendlyClassName)) {
+            if (updateBest(it->second.originalModuleLabel, instanceIsWildcard, typeIsWildcard)) {
+              return it->second.originalModuleLabel;
+            }
+          } else if (consumesInfo.kindOfType() == ELEMENT_TYPE) {
+            //consume is a View so need to do more intrusive search
+            //find matching branches in module
+            auto branches = conditionalModuleBranches.equal_range(productModuleLabel);
+            for (auto itBranch = branches.first; itBranch != branches.second; ++it) {
+              if (typeIsWildcard or itBranch->second->productInstanceName() == it->second.originalInstanceLabel) {
+                if (productholderindexhelper::typeIsViewCompatible(consumesInfo.type(),
+                                                                   TypeID(itBranch->second->wrappedType().typeInfo()),
+                                                                   itBranch->second->className())) {
+                  if (updateBest(it->second.originalModuleLabel, instanceIsWildcard, typeIsWildcard)) {
+                    return it->second.originalModuleLabel;
+                  }
+                }
+              }
+            }
+          }
+        }
+      }
+      return best;
+    }
   }  // namespace
 
   // -----------------------------
@@ -589,38 +651,11 @@ namespace edm {
           auto itFound = conditionalModules.find(productModuleLabel);
           if (itFound == conditionalModules.end()) {
             //Check to see if this was an alias
-            auto findAlias = aliasMap.equal_range(productModuleLabel);
-            for (auto it = findAlias.first; it != findAlias.second; ++it) {
-              //this was previously filtered so only the conditional modules remain
-              productModuleLabel = it->second.originalModuleLabel;
-              if (it->second.instanceLabel == "*" or ci.instance() == it->second.instanceLabel) {
-                if (it->second.friendlyClassName == "*" or
-                    (ci.type().friendlyClassName() == it->second.friendlyClassName)) {
-                  productFromConditionalModule = true;
-                  //need to check the rest of the data product info
-                  break;
-                } else if (ci.kindOfType() == ELEMENT_TYPE) {
-                  //consume is a View so need to do more intrusive search
-                  //find matching branches in module
-                  auto branches = conditionalModuleBranches.equal_range(productModuleLabel);
-                  for (auto itBranch = branches.first; itBranch != branches.second; ++it) {
-                    if (it->second.originalInstanceLabel == "*" or
-                        itBranch->second->productInstanceName() == it->second.originalInstanceLabel) {
-                      if (typeIsViewCompatible(ci.type(),
-                                               TypeID(itBranch->second->wrappedType().typeInfo()),
-                                               itBranch->second->className())) {
-                        productFromConditionalModule = true;
-                        break;
-                      }
-                    }
-                  }
-                  if (productFromConditionalModule) {
-                    break;
-                  }
-                }
-              }
-            }
-            if (productFromConditionalModule) {
+            //note that aliasMap was previously filtered so only the conditional modules remain there
+            auto foundAlias = findBestMatchingAlias(conditionalModuleBranches, aliasMap, productModuleLabel, ci);
+            if (foundAlias) {
+              productModuleLabel = *foundAlias;
+              productFromConditionalModule = true;
               itFound = conditionalModules.find(productModuleLabel);
               //check that the alias-for conditional module has not been used
               if (itFound == conditionalModules.end()) {

diff --git a/FWCore/Integration/test/run_TestSwitchProducer.sh b/FWCore/Integration/test/run_TestSwitchProducer.sh
@@ -79,6 +79,19 @@ pushd ${LOCAL_TMP_DIR}
   echo "SwitchProducer in a ConditionalTask, test EDAlias with all cases being explicitly consumed, case test2 disabled"
   cmsRun -n ${NUMTHREADS} ${LOCAL_TEST_DIR}/${test}ConditionalTaskEDAliasConsumeAllCases_cfg.py disableTest2 || die "cmsRun ${test}ConditionalTaskEDAliasConsumeAllCases_cfg.py 2" $?
 
+  echo "*************************************************"
+  echo "SwitchProducer in a ConditionalTask, test EDAlias with a wildcard"
+  cmsRun -n ${NUMTHREADS} ${LOCAL_TEST_DIR}/${test}ConditionalTaskEDAliasWildcard_cfg.py || die "cmsRun ${test}ConditionalTaskEDAliasWildcard_cfg.py" $?
+
+  echo "*************************************************"
+  echo "SwitchProducer in a ConditionalTask, test EDAlias with a wildcard pattern, other module having the wildcard"
+  cmsRun -n ${NUMTHREADS} ${LOCAL_TEST_DIR}/${test}ConditionalTaskEDAliasWildcard_cfg.py -- --patternOnOtherModule || die "cmsRun ${test}ConditionalTaskEDAliasWildcard_cfg.py --patternOnOtherModule" $?
+
+  echo "*************************************************"
+  echo "SwitchProducer in a ConditionalTask, test EDAlias with an a wildcard, case test2 disabled"
+  cmsRun -n ${NUMTHREADS} ${LOCAL_TEST_DIR}/${test}ConditionalTaskEDAliasWildcard_cfg.py -- --disableTest2 || die "cmsRun ${test}ConditionalTaskEDAliasWildcard_cfg.py --disableTest2" $?
+
+
 
   echo "*************************************************"
   echo "SwitchProducer in a Path"

diff --git a/FWCore/Integration/test/testSwitchProducerConditionalTaskEDAliasWildcard_cfg.py b/FWCore/Integration/test/testSwitchProducerConditionalTaskEDAliasWildcard_cfg.py
@@ -0,0 +1,86 @@
+import FWCore.ParameterSet.Config as cms
+import sys
+import argparse
+
+parser = argparse.ArgumentParser(prog=sys.argv[0], description="Test SwitchProducer, that has an EDAlias with '*' wildcard, in a ConditionalTask")
+
+parser.add_argument("--disableTest2", help="Disable 'test2' case of the SwitchProducerTest", action="store_true")
+parser.add_argument("--wildcardOnOtherModule", help="Use the wildcard for alias from another module", action="store_true")
+
+argv = sys.argv[:]
+if '--' in argv:
+    argv.remove("--")
+args, unknown = parser.parse_known_args(argv)
+
+enableTest2 = not args.disableTest2
+class SwitchProducerTest(cms.SwitchProducer):
+    def __init__(self, **kargs):
+        super(SwitchProducerTest,self).__init__(
+            dict(
+                test1 = lambda accelerators: (True, -10),
+                test2 = lambda accelerators: (enableTest2, -9)
+            ), **kargs)
+
+process = cms.Process("PROD1")
+
+process.source = cms.Source("EmptySource")
+
+process.maxEvents.input = 3
+
+process.intProducer1 = cms.EDProducer(
+    "ManyIntProducer",
+    ivalue = cms.int32(1),
+    values = cms.VPSet(
+    )
+)
+process.intProducer2 = cms.EDProducer(
+    "ManyIntProducer",
+    ivalue = cms.int32(11),
+    values = cms.VPSet(
+        cms.PSet(instance=cms.string("bar"), value=cms.int32(12))
+    )
+)
+process.intProducer3 = cms.EDProducer(
+    "ManyIntProducer",
+    ivalue = cms.int32(21)
+)
+if args.wildcardOnOtherModule:
+    process.intProducer1.values.append(cms.PSet(instance=cms.string("bar"), value=cms.int32(2)))
+    process.intProducer2.values = []
+
+process.intProducer4 = cms.EDProducer(
+    "ManyIntProducer",
+    ivalue = cms.int32(31),
+    values = cms.VPSet(
+        cms.PSet(instance=cms.string("foo"), value=cms.int32(32)),
+        cms.PSet(instance=cms.string("bar"), value=cms.int32(33)),
+        cms.PSet(instance=cms.string("xyzzy"), value=cms.int32(34)),
+   )
+)
+
+process.intProducer = SwitchProducerTest(
+    test1 = cms.EDAlias(
+        intProducer4 = cms.EDAlias.allProducts()
+    ),
+    test2 = cms.EDAlias()
+)
+allMatchName = "intProducer1"
+otherName ="intProducer2"
+if args.wildcardOnOtherModule:
+    (allMatchName, otherName) = (otherName, allMatchName)
+setattr(process.intProducer.test2, allMatchName, cms.EDAlias.allProducts())
+setattr(process.intProducer.test2, otherName, cms.VPSet(
+    cms.PSet(type = cms.string("*"), fromProductInstance = cms.string(""), toProductInstance = cms.string("foo")),
+    cms.PSet(type = cms.string("*"), fromProductInstance = cms.string("bar"), toProductInstance = cms.string("*"))
+))
+process.intProducer.test2.intProducer3 = cms.VPSet(
+    cms.PSet(type = cms.string("edmtestIntProduct"), toProductInstance = cms.string("xyzzy"))
+)
+
+process.intConsumer = cms.EDAnalyzer("edmtest::GenericIntsAnalyzer", srcEvent = cms.untracked.VInputTag("intProducer"))
+process.intConsumer2 = cms.EDAnalyzer("edmtest::GenericIntsAnalyzer", srcEvent = cms.untracked.VInputTag("intProducer", "intProducer2", "intProducer3"))
+
+process.ct = cms.ConditionalTask(process.intProducer1, process.intProducer2, process.intProducer3, process.intProducer4, process.intProducer)
+
+process.p1 = cms.Path(process.intConsumer, process.ct)
+process.p2 = cms.Path(process.intConsumer2, process.ct)