Skip to content

Commit

Permalink
Allow SwitchProducer cases to declare different transient products
Browse files Browse the repository at this point in the history
  • Loading branch information
@wddgit
wddgit committed Nov 17, 2022
1 parent 7f1707e commit 8de4a67
Show file tree
Hide file tree
Showing 3 changed files with 75 additions and 27 deletions.
61 changes: 34 additions & 27 deletions FWCore/Framework/src/Schedule.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -187,7 +187,9 @@ namespace edm {
branchKey.moduleLabel() == prod.second.switchAliasModuleLabel() and
branchKey.productInstanceName() == prod.second.productInstanceName()) {
prod.second.setSwitchAliasForBranch(desc);
it->second.chosenBranches.push_back(prod.first); // with moduleLabel of the Switch
if (!prod.second.transient()) {
it->second.chosenBranches.push_back(prod.first); // with moduleLabel of the Switch
}
found = true;
}
}
Expand Down Expand Up @@ -233,7 +235,7 @@ namespace edm {
}
};

// Check that non-chosen cases declare exactly the same branches
// Check that non-chosen cases declare exactly the same non-transient branches
// Also set the alias-for branches to transient
std::vector<bool> foundBranches;
for (auto const& switchItem : switchMap) {
Expand All @@ -246,6 +248,32 @@ namespace edm {
for (auto& nonConstItem : preg.productListUpdator()) {
auto const& item = nonConstItem;
if (item.first.moduleLabel() == caseLabel and item.first.processName() == processName) {
// Check that products which are not transient in the dictionary are consistent between
// all the cases of a SwitchProducer.
if (!item.second.transient()) {
auto range = std::equal_range(chosenBranches.begin(),
chosenBranches.end(),
BranchKey(item.first.friendlyClassName(),
switchLabel,
item.first.productInstanceName(),
item.first.processName()));
if (range.first == range.second) {
Exception ex(errors::Configuration);
ex << "SwitchProducer " << switchLabel << " has a case " << caseLabel << " with a product "
<< item.first << " that is not produced by the chosen case "
<< proc_pset.getParameter<edm::ParameterSet>(switchLabel)
.getUntrackedParameter<std::string>("@chosen_case")
<< " and that product is not transient. "
<< "If the intention is to produce only a subset of the non-transient products listed below, each "
"case with more non-transient products needs to be replaced with an EDAlias to only the "
"necessary products, and the EDProducer itself needs to be moved to a Task.\n\n";
addProductsToException(caseLabels, ex);
throw ex;
}
assert(std::distance(range.first, range.second) == 1);
foundBranches[std::distance(chosenBranches.begin(), range.first)] = true;
}

// Set the alias-for branch as transient so it gets fully ignored in output.
// I tried first to implicitly drop all branches with
// '@' in ProductSelector, but that gave problems on
Expand All @@ -256,27 +284,6 @@ namespace edm {
// SwitchProducer branches are not alias branches)
nonConstItem.second.setTransient(true);

auto range = std::equal_range(chosenBranches.begin(),
chosenBranches.end(),
BranchKey(item.first.friendlyClassName(),
switchLabel,
item.first.productInstanceName(),
item.first.processName()));
if (range.first == range.second) {
Exception ex(errors::Configuration);
ex << "SwitchProducer " << switchLabel << " has a case " << caseLabel << " with a product "
<< item.first << " that is not produced by the chosen case "
<< proc_pset.getParameter<edm::ParameterSet>(switchLabel)
.getUntrackedParameter<std::string>("@chosen_case")
<< ". If the intention is to produce only a subset of the products listed below, each case with "
"more products needs to be replaced with an EDAlias to only the necessary products, and the "
"EDProducer itself needs to be moved to a Task.\n\n";
addProductsToException(caseLabels, ex);
throw ex;
}
assert(std::distance(range.first, range.second) == 1);
foundBranches[std::distance(chosenBranches.begin(), range.first)] = true;

// Check that there are no BranchAliases for any of the cases
auto const& bd = item.second;
if (not bd.branchAliases().empty()) {
Expand All @@ -299,10 +306,10 @@ namespace edm {
Exception ex(errors::Configuration);
ex << "SwitchProducer " << switchLabel << " has a case " << caseLabel
<< " that does not produce a product " << chosenBranches[i] << " that is produced by the chosen case "
<< chosenLabel
<< ". If the intention is to produce only a subset of the products listed below, each case with more "
"products needs to be replaced with an EDAlias to only the necessary products, and the "
"EDProducer itself needs to be moved to a Task.\n\n";
<< chosenLabel << " and that product is not transient. "
<< "If the intention is to produce only a subset of the non-transient products listed below, each "
"case with more non-transient products needs to be replaced with an EDAlias to only the "
"necessary products, and the EDProducer itself needs to be moved to a Task.\n\n";
addProductsToException(caseLabels, ex);
throw ex;
}
Expand Down
23 changes: 23 additions & 0 deletions FWCore/Framework/test/stubs/ToyIntProducers.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -528,6 +528,18 @@ namespace edmtest {
return TokenValue{produces<IntProduct>(pset.getParameter<std::string>("instance")),
pset.getParameter<int>("value")};
})},
transientTokenValues_{vector_transform(
p.getParameter<std::vector<edm::ParameterSet>>("transientValues"),
[this](edm::ParameterSet const& pset) {
auto const& branchAlias = pset.getParameter<std::string>("branchAlias");
if (not branchAlias.empty()) {
return TransientTokenValue{produces<TransientIntProduct>(pset.getParameter<std::string>("instance"))
.setBranchAlias(branchAlias),
pset.getParameter<int>("value")};
}
return TransientTokenValue{produces<TransientIntProduct>(pset.getParameter<std::string>("instance")),
pset.getParameter<int>("value")};
})},
throw_{p.getUntrackedParameter<bool>("throw")} {
tokenValues_.push_back(TokenValue{produces<IntProduct>(), p.getParameter<int>("ivalue")});
}
Expand All @@ -543,6 +555,7 @@ namespace edmtest {
pset.add<int>("value");
pset.add<std::string>("branchAlias", "");
desc.addVPSet("values", pset, std::vector<edm::ParameterSet>{});
desc.addVPSet("transientValues", pset, std::vector<edm::ParameterSet>{});
}

descriptions.addDefault(desc);
Expand All @@ -556,6 +569,13 @@ namespace edmtest {
int value;
};
std::vector<TokenValue> tokenValues_;

struct TransientTokenValue {
edm::EDPutTokenT<TransientIntProduct> token;
int value;
};
std::vector<TransientTokenValue> transientTokenValues_;

bool throw_;
};

Expand All @@ -568,6 +588,9 @@ namespace edmtest {
for (auto const& tv : tokenValues_) {
e.emplace(tv.token, tv.value);
}
for (auto const& tv : transientTokenValues_) {
e.emplace(tv.token, tv.value);
}
}

//
Expand Down
18 changes: 18 additions & 0 deletions FWCore/Integration/test/SwitchProducer_t.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -247,6 +247,24 @@ TEST_CASE("Configuration with different branches", s_tag) {
}
}

TEST_CASE("Configuration with different transient branches", s_tag) {
const std::string test1{"cms.EDProducer('ManyIntProducer', ivalue = cms.int32(1), values = cms.VPSet())"};
const std::string test2{
"cms.EDProducer('ManyIntProducer', ivalue = cms.int32(2), transientValues = "
"cms.VPSet(cms.PSet(instance=cms.string('foo'),value=cms.int32(3))))"};

const std::string baseConfig1 = makeConfig(true, test1, test2);
const std::string baseConfig2 = makeConfig(false, test1, test2);

SECTION("Different transient branches are allowed") {
edm::test::TestProcessor::Config config1{baseConfig1};
edm::test::TestProcessor testProcessor1{config1};

edm::test::TestProcessor::Config config2{baseConfig2};
edm::test::TestProcessor testProcessor2{config2};
}
}

TEST_CASE("Configuration with different branches with EDAlias", s_tag) {
const std::string otherprod{
"cms.EDProducer('ManyIntProducer', ivalue = cms.int32(2), values = "
Expand Down

0 comments on commit 8de4a67

Please sign in to comment.