From 5b46812c8d27acec8c18a4516ac3219aadce0ff3 Mon Sep 17 00:00:00 2001 From: Matti Kortelainen Date: Wed, 14 Sep 2022 21:23:41 +0200 Subject: [PATCH] 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. --- FWCore/Framework/src/StreamSchedule.cc | 106 ++++++++++++------ .../test/run_TestSwitchProducer.sh | 13 +++ ...ducerConditionalTaskEDAliasWildcard_cfg.py | 86 ++++++++++++++ 3 files changed, 173 insertions(+), 32 deletions(-) create mode 100644 FWCore/Integration/test/testSwitchProducerConditionalTaskEDAliasWildcard_cfg.py diff --git a/FWCore/Framework/src/StreamSchedule.cc b/FWCore/Framework/src/StreamSchedule.cc index 473d05147d7d0..9a719399a6903 100644 --- a/FWCore/Framework/src/StreamSchedule.cc +++ b/FWCore/Framework/src/StreamSchedule.cc @@ -35,6 +35,7 @@ #include #include #include +#include #include #include #include @@ -159,6 +160,74 @@ namespace edm { } return std::pair(beg + 1, std::prev(modnames.end())); } + + std::optional findBestMatchingAlias( + std::unordered_multimap const& conditionalModuleBranches, + std::unordered_multimap const& aliasMap, + std::string const& productModuleLabel, + ConsumesInfo const& consumesInfo) { + std::optional best; + int wildcardsInBest = std::numeric_limits::max(); + bool bestIsAmbiguous = false; + + auto updateBest = [&best, &wildcardsInBest, &bestIsAmbiguous]( + std::string const& label, bool instanceIsWildcard, bool typeIsWildcard) { + int const wildcards = static_cast(instanceIsWildcard) + static_cast(typeIsWildcard); + if (wildcards == 0) { + bestIsAmbiguous = false; + return true; + } + if (not best or wildcards < wildcardsInBest) { + best = label; + wildcardsInBest = wildcards; + bestIsAmbiguous = false; + } else if (best and *best != label and wildcardsInBest == wildcards) { + bestIsAmbiguous = true; + } + 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; + } + } + } + } + } + } + } + if (bestIsAmbiguous) { + 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 (" + << wildcardsInBest << ")"; + } + return best; + } } // namespace // ----------------------------- @@ -658,38 +727,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 index f50836f58c4fc..55e78493a7d2f 100755 --- 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 new file mode 100644 index 0000000000000..df4665c623edb --- /dev/null +++ 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)