From c0144f29ad5860ab6eef8d62fe71e7b15fa0967d Mon Sep 17 00:00:00 2001 From: Matti Kortelainen <matti.kortelainen@cern.ch> Date: Tue, 25 Aug 2020 19:11:48 +0200 Subject: [PATCH 1/3] Add support for wildcards in EDAlias type field Also add a shorthand EDAlias.allProducts() --- FWCore/Framework/src/Schedule.cc | 55 +++++- FWCore/Integration/test/BuildFile.xml | 1 + FWCore/Integration/test/EDAlias_t.cpp | 239 ++++++++++++++++++++++++++ FWCore/ParameterSet/python/Types.py | 25 +++ 4 files changed, 319 insertions(+), 1 deletion(-) diff --git a/FWCore/Framework/src/Schedule.cc b/FWCore/Framework/src/Schedule.cc index cb17d567b9dc0..bb8973a70b401 100644 --- a/FWCore/Framework/src/Schedule.cc +++ b/FWCore/Framework/src/Schedule.cc @@ -232,6 +232,12 @@ namespace edm { std::map<BranchKey, BranchKey> aliasKeys; // Used to search for duplicates or clashes. + // Auxiliary search structure to support wildcard for friendlyClassName + std::multimap<std::string, BranchKey> moduleLabelToBranches; + for (auto const& prod : preg.productList()) { + moduleLabelToBranches.emplace(prod.first.moduleLabel(), prod.first); + } + // Now, loop over the alias information and store it in aliasMap. for (std::string const& alias : aliases) { ParameterSet const& aliasPSet = proc_pset.getParameterSet(alias); @@ -243,7 +249,54 @@ namespace edm { std::string friendlyClassName = pset.getParameter<std::string>("type"); std::string productInstanceName = pset.getParameter<std::string>("fromProductInstance"); std::string instanceAlias = pset.getParameter<std::string>("toProductInstance"); - if (productInstanceName == star) { + + if (friendlyClassName == star) { + bool match = false; + for (auto it = moduleLabelToBranches.lower_bound(moduleLabel); + it != moduleLabelToBranches.end() && it->first == moduleLabel; + ++it) { + if (processName != it->second.processName()) { + continue; + } + if (productInstanceName != star and productInstanceName != it->second.productInstanceName()) { + continue; + } + match = true; + + checkAndInsertAlias(it->second.friendlyClassName(), + moduleLabel, + it->second.productInstanceName(), + processName, + alias, + instanceAlias, + preg, + aliasMap, + aliasKeys); + } + if (not match) { + // No product was found matching the alias. + // We throw an exception only if a module with the specified module label was created in this process. + for (auto const& product : preg.productList()) { + if (moduleLabel == product.first.moduleLabel() && processName == product.first.processName()) { + if (productInstanceName == star) { + // This one doesn't really throw currently, + // because there is no way a module would + // produce something but would not be found with + // wildcards for both the type and the instance + // name. I'm leaving the exception here though + // in case the condition above to throw an + // exception gets changed. + throw Exception(errors::Configuration, "EDAlias parameter set mismatch\n") + << "There are no products with module label '" << moduleLabel << "'.\n"; + } else { + throw Exception(errors::Configuration, "EDAlias parameter set mismatch\n") + << "There are no products with module label '" << moduleLabel + << "' and product instance name '" << productInstanceName << "'.\n"; + } + } + } + } + } else if (productInstanceName == star) { bool match = false; BranchKey lowerBound(friendlyClassName, moduleLabel, empty, empty); for (ProductRegistry::ProductList::const_iterator it = preg.productList().lower_bound(lowerBound); diff --git a/FWCore/Integration/test/BuildFile.xml b/FWCore/Integration/test/BuildFile.xml index 726c4c63bb3f7..cc73b2efb269d 100644 --- a/FWCore/Integration/test/BuildFile.xml +++ b/FWCore/Integration/test/BuildFile.xml @@ -189,6 +189,7 @@ <use name="FWCore/TestProcessor"/> <use name="DataFormats/Provenance"/> <use name="catch2"/> + <use name="fmt"/> </bin> <library file="TestPSetAnalyzer.cc" name="TestPSetAnalyzer"> diff --git a/FWCore/Integration/test/EDAlias_t.cpp b/FWCore/Integration/test/EDAlias_t.cpp index d1d2c63ed52de..af65577b2901e 100644 --- a/FWCore/Integration/test/EDAlias_t.cpp +++ b/FWCore/Integration/test/EDAlias_t.cpp @@ -6,6 +6,7 @@ #include "FWCore/TestProcessor/interface/TestProcessor.h" #include <iostream> +#include <fmt/format.h> static constexpr auto s_tag = "[EDAlias]"; @@ -116,3 +117,241 @@ process.moduleToTest(process.test, cms.Task(process.intprod)) Catch::Contains("One has module label") && Catch::Contains("the other has module label")); } } + +//////////////////////////////////////// +TEST_CASE("Configuration with all products of a module", s_tag) { + const std::string baseConfig{ + R"_(from FWCore.TestProcessor.TestProcess import * +import FWCore.ParameterSet.Config as cms + +process = TestProcess() + +process.intprod = cms.EDProducer('ManyIntProducer', ivalue = cms.int32(2), + values = cms.VPSet( + cms.PSet(instance=cms.string('foo'),value=cms.int32(3)), + cms.PSet(instance=cms.string('another'),value=cms.int32(4)), + ) +) + +process.intalias = cms.EDAlias(intprod = cms.EDAlias.allProducts()) + +# Module to be tested can not be an EDAlias +process.test = cms.EDProducer('AddIntsProducer', + labels = cms.VInputTag('intalias', ('intalias', 'foo'), ('intalias', 'another')) +) + +process.moduleToTest(process.test, cms.Task(process.intprod)) +)_"}; + + edm::test::TestProcessor::Config config{baseConfig}; + + SECTION("Base configuration is OK") { REQUIRE_NOTHROW(edm::test::TestProcessor(config)); } + + SECTION("No event data") { + edm::test::TestProcessor tester(config); + REQUIRE_NOTHROW(tester.test()); + } + + SECTION("beginJob and endJob only") { + edm::test::TestProcessor tester(config); + REQUIRE_NOTHROW(tester.testBeginAndEndJobOnly()); + } + + SECTION("Run with no LuminosityBlocks") { + edm::test::TestProcessor tester(config); + REQUIRE_NOTHROW(tester.testRunWithNoLuminosityBlocks()); + } + + SECTION("LuminosityBlock with no Events") { + edm::test::TestProcessor tester(config); + REQUIRE_NOTHROW(tester.testLuminosityBlockWithNoEvents()); + } + + SECTION("Getting value") { + edm::test::TestProcessor tester(config); + auto event = tester.test(); + REQUIRE(event.get<edmtest::IntProduct>()->value == 9); + } +} + +TEST_CASE("Configuration with all products of a module with a given product instance name", s_tag) { + const std::string baseConfig{ + R"_(from FWCore.TestProcessor.TestProcess import * +import FWCore.ParameterSet.Config as cms + +process = TestProcess() + +process.intprod = cms.EDProducer('ManyIntProducer', ivalue = cms.int32(2), + values = cms.VPSet( + cms.PSet(instance=cms.string('foo'),value=cms.int32(3)), + cms.PSet(instance=cms.string('another'),value=cms.int32(4)), + ) +) + +process.intalias = cms.EDAlias(intprod = cms.VPSet(cms.PSet(type = cms.string('*'), fromProductInstance = cms.string('another')))) + +# Module to be tested can not be an EDAlias +process.test = cms.EDProducer('AddIntsProducer', + labels = cms.VInputTag({}) +) + +process.moduleToTest(process.test, cms.Task(process.intprod)) +)_"}; + + edm::test::TestProcessor::Config config{fmt::format(baseConfig, "'intalias:another'")}; + + SECTION("Base configuration is OK") { REQUIRE_NOTHROW(edm::test::TestProcessor(config)); } + + SECTION("No event data") { + edm::test::TestProcessor tester(config); + REQUIRE_NOTHROW(tester.test()); + } + + SECTION("beginJob and endJob only") { + edm::test::TestProcessor tester(config); + REQUIRE_NOTHROW(tester.testBeginAndEndJobOnly()); + } + + SECTION("Run with no LuminosityBlocks") { + edm::test::TestProcessor tester(config); + REQUIRE_NOTHROW(tester.testRunWithNoLuminosityBlocks()); + } + + SECTION("LuminosityBlock with no Events") { + edm::test::TestProcessor tester(config); + REQUIRE_NOTHROW(tester.testLuminosityBlockWithNoEvents()); + } + + SECTION("Getting value") { + edm::test::TestProcessor tester(config); + auto event = tester.test(); + REQUIRE(event.get<edmtest::IntProduct>()->value == 4); + } + + SECTION("Other product instances are not aliased") { + { + edm::test::TestProcessor::Config config{fmt::format(baseConfig, "'intalias:foo'")}; + edm::test::TestProcessor tester(config); + REQUIRE_THROWS_WITH(tester.test(), Catch::Contains("ProductNotFound")); + } + { + edm::test::TestProcessor::Config config{fmt::format(baseConfig, "'intalias'")}; + edm::test::TestProcessor tester(config); + REQUIRE_THROWS_WITH(tester.test(), Catch::Contains("ProductNotFound")); + } + } +} + +//////////////////////////////////////// +TEST_CASE("Configuration with all products of two modules", s_tag) { + const std::string baseConfig{ + R"_(from FWCore.TestProcessor.TestProcess import * +import FWCore.ParameterSet.Config as cms + +process = TestProcess() + +process.intprod = cms.EDProducer('ManyIntProducer', ivalue = cms.int32(2), + values = cms.VPSet( + cms.PSet(instance=cms.string('foo'),value=cms.int32(3)), + cms.PSet(instance=cms.string('another'),value=cms.int32(4)), + ) +) +process.intprod2 = cms.EDProducer('ManyIntProducer', ivalue = cms.int32(20), + values = cms.VPSet( + cms.PSet(instance=cms.string('foo2'),value=cms.int32(30)), + cms.PSet(instance=cms.string('another2'),value=cms.int32(40)), + ) +) + +process.intalias = cms.EDAlias( + intprod = cms.EDAlias.allProducts(), + # can't use allProducts() because the product instance '' would lead to duplicate brances to be aliased + intprod2 = cms.VPSet( + cms.PSet(type = cms.string('*'), fromProductInstance = cms.string('foo2')), + cms.PSet(type = cms.string('*'), fromProductInstance = cms.string('another2')), + ) +) + +# Module to be tested can not be an EDAlias +process.test = cms.EDProducer('AddIntsProducer', + labels = cms.VInputTag('intalias', ('intalias', 'foo'), ('intalias', 'another'), ('intalias', 'foo2'), ('intalias', 'another2')) +) + +process.moduleToTest(process.test, cms.Task(process.intprod, process.intprod2)) +)_"}; + + edm::test::TestProcessor::Config config{baseConfig}; + + SECTION("Base configuration is OK") { REQUIRE_NOTHROW(edm::test::TestProcessor(config)); } + + SECTION("No event data") { + edm::test::TestProcessor tester(config); + REQUIRE_NOTHROW(tester.test()); + } + + SECTION("beginJob and endJob only") { + edm::test::TestProcessor tester(config); + REQUIRE_NOTHROW(tester.testBeginAndEndJobOnly()); + } + + SECTION("Run with no LuminosityBlocks") { + edm::test::TestProcessor tester(config); + REQUIRE_NOTHROW(tester.testRunWithNoLuminosityBlocks()); + } + + SECTION("LuminosityBlock with no Events") { + edm::test::TestProcessor tester(config); + REQUIRE_NOTHROW(tester.testLuminosityBlockWithNoEvents()); + } + + SECTION("Getting value") { + edm::test::TestProcessor tester(config); + auto event = tester.test(); + REQUIRE(event.get<edmtest::IntProduct>()->value == 79); + } +} + +//////////////////////////////////////// +TEST_CASE("No products found with wildcards", s_tag) { + const std::string baseConfig{ + R"_(from FWCore.TestProcessor.TestProcess import * +import FWCore.ParameterSet.Config as cms + +process = TestProcess() + +process.intprod = cms.EDProducer('ManyIntProducer', ivalue = cms.int32(2), + values = cms.VPSet( + cms.PSet(instance=cms.string('foo'),value=cms.int32(3)), + cms.PSet(instance=cms.string('another'),value=cms.int32(4)), + ) +) + +process.intalias = cms.EDAlias({}) + +# Module to be tested can not be an EDAlias +process.test = cms.EDProducer('AddIntsProducer', + labels = cms.VInputTag('intalias') +) + +process.moduleToTest(process.test, cms.Task(process.intprod)) +)_"}; + + SECTION("Type wildcard") { + edm::test::TestProcessor::Config config{fmt::format( + baseConfig, + "intprod = cms.VPSet(cms.PSet(type = cms.string('*'), fromProductInstance = cms.string('nonexistent')))")}; + + REQUIRE_THROWS_WITH( + edm::test::TestProcessor(config), + Catch::Contains("There are no products with module label 'intprod' and product instance name 'nonexistent'")); + } + + SECTION("Instance wildcard") { + edm::test::TestProcessor::Config config{ + fmt::format(baseConfig, "intprod = cms.VPSet(cms.PSet(type = cms.string('nonexistentType')))")}; + + REQUIRE_THROWS_WITH(edm::test::TestProcessor(config), + Catch::Contains("There are no products of type 'nonexistentType'") && + Catch::Contains("with module label 'intprod'")); + } +} diff --git a/FWCore/ParameterSet/python/Types.py b/FWCore/ParameterSet/python/Types.py index c7714120794be..6b9c9b3156d52 100644 --- a/FWCore/ParameterSet/python/Types.py +++ b/FWCore/ParameterSet/python/Types.py @@ -1362,6 +1362,15 @@ class EDAlias(_ConfigureComponent,_Labelable,_Parameterizable): def __init__(self,*arg,**kargs): super(EDAlias,self).__init__(**kargs) + @staticmethod + def allProducts(): + """A helper to specify that all products of a module are to be aliased for. Example usage: + process.someAlias = cms.EDAlias( + aliasForModuleLabel = cms.EDAlias.allProducts() + ) + """ + return VPSet(PSet(type = string('*'))) + def clone(self, *args, **params): returnValue = EDAlias.__new__(type(self)) myparams = self.parameters_() @@ -1974,6 +1983,22 @@ def testEDAlias(self): self.assertTrue(hasattr(aliasfoo4, "foo3")) self.assertEqual(aliasfoo4.foo3[0].type, "Foo3") + aliasfoo5 = EDAlias(foo5 = EDAlias.allProducts()) + self.assertEqual(len(aliasfoo5.foo5), 1) + self.assertEqual(aliasfoo5.foo5[0].type.value(), "*") + self.assertFalse(hasattr(aliasfoo5.foo5[0], "fromProductInstance")) + self.assertFalse(hasattr(aliasfoo5.foo5[0], "toProductInstance")) + + aliasfoo6 = aliasfoo5.clone(foo5 = None, foo6 = EDAlias.allProducts()) + self.assertFalse(hasattr(aliasfoo6, "foo5")) + self.assertTrue(hasattr(aliasfoo6, "foo6")) + self.assertEqual(len(aliasfoo6.foo6), 1) + self.assertEqual(aliasfoo6.foo6[0].type.value(), "*") + + aliasfoo7 = EDAlias(foo5 = EDAlias.allProducts(), foo6 = EDAlias.allProducts()) + self.assertEqual(len(aliasfoo7.foo5), 1) + self.assertEqual(len(aliasfoo7.foo6), 1) + def testFileInPath(self): f = FileInPath("FWCore/ParameterSet/python/Types.py") self.assertEqual(f.configValue(), "'FWCore/ParameterSet/python/Types.py'") From c5a284d8d75cf2a43d01e11249c822a04e35e5c2 Mon Sep 17 00:00:00 2001 From: Matti Kortelainen <matti.kortelainen@cern.ch> Date: Sat, 29 Aug 2020 03:26:12 +0200 Subject: [PATCH 2/3] Check process name earlier --- FWCore/Framework/src/Schedule.cc | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/FWCore/Framework/src/Schedule.cc b/FWCore/Framework/src/Schedule.cc index bb8973a70b401..5928654b28694 100644 --- a/FWCore/Framework/src/Schedule.cc +++ b/FWCore/Framework/src/Schedule.cc @@ -235,7 +235,9 @@ namespace edm { // Auxiliary search structure to support wildcard for friendlyClassName std::multimap<std::string, BranchKey> moduleLabelToBranches; for (auto const& prod : preg.productList()) { - moduleLabelToBranches.emplace(prod.first.moduleLabel(), prod.first); + if (processName == prod.second.processName()) { + moduleLabelToBranches.emplace(prod.first.moduleLabel(), prod.first); + } } // Now, loop over the alias information and store it in aliasMap. @@ -255,9 +257,6 @@ namespace edm { for (auto it = moduleLabelToBranches.lower_bound(moduleLabel); it != moduleLabelToBranches.end() && it->first == moduleLabel; ++it) { - if (processName != it->second.processName()) { - continue; - } if (productInstanceName != star and productInstanceName != it->second.productInstanceName()) { continue; } From 5340da8bd955298346232f1d166b3e976eb96863 Mon Sep 17 00:00:00 2001 From: Matti Kortelainen <matti.kortelainen@cern.ch> Date: Wed, 2 Sep 2020 17:08:25 +0200 Subject: [PATCH 3/3] Simplify check of module label existing in the same process, and an exception if no aliased-for products are found --- FWCore/Framework/src/Schedule.cc | 28 ++++++++-------------------- 1 file changed, 8 insertions(+), 20 deletions(-) diff --git a/FWCore/Framework/src/Schedule.cc b/FWCore/Framework/src/Schedule.cc index 5928654b28694..47eaa78865da8 100644 --- a/FWCore/Framework/src/Schedule.cc +++ b/FWCore/Framework/src/Schedule.cc @@ -253,10 +253,12 @@ namespace edm { std::string instanceAlias = pset.getParameter<std::string>("toProductInstance"); if (friendlyClassName == star) { + bool processHasLabel = false; bool match = false; for (auto it = moduleLabelToBranches.lower_bound(moduleLabel); it != moduleLabelToBranches.end() && it->first == moduleLabel; ++it) { + processHasLabel = true; if (productInstanceName != star and productInstanceName != it->second.productInstanceName()) { continue; } @@ -272,28 +274,14 @@ namespace edm { aliasMap, aliasKeys); } - if (not match) { + if (not match and processHasLabel) { // No product was found matching the alias. // We throw an exception only if a module with the specified module label was created in this process. - for (auto const& product : preg.productList()) { - if (moduleLabel == product.first.moduleLabel() && processName == product.first.processName()) { - if (productInstanceName == star) { - // This one doesn't really throw currently, - // because there is no way a module would - // produce something but would not be found with - // wildcards for both the type and the instance - // name. I'm leaving the exception here though - // in case the condition above to throw an - // exception gets changed. - throw Exception(errors::Configuration, "EDAlias parameter set mismatch\n") - << "There are no products with module label '" << moduleLabel << "'.\n"; - } else { - throw Exception(errors::Configuration, "EDAlias parameter set mismatch\n") - << "There are no products with module label '" << moduleLabel - << "' and product instance name '" << productInstanceName << "'.\n"; - } - } - } + // Note that if that condition is ever relatex, it might be best to throw an exception with different + // message (omitting productInstanceName) in case 'productInstanceName == start' + throw Exception(errors::Configuration, "EDAlias parameter set mismatch\n") + << "There are no products with module label '" << moduleLabel << "' and product instance name '" + << productInstanceName << "'.\n"; } } else if (productInstanceName == star) { bool match = false;