Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow SwitchProducer cases to declare different transient products #40104

Merged
merged 2 commits into from
Nov 25, 2022
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
61 changes: 34 additions & 27 deletions FWCore/Framework/src/Schedule.cc
Original file line number Diff line number Diff line change
@@ -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;
}
}
@@ -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) {
@@ -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
@@ -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()) {
@@ -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;
}
23 changes: 23 additions & 0 deletions FWCore/Framework/test/stubs/ToyIntProducers.cc
Original file line number Diff line number Diff line change
@@ -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")});
}
@@ -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);
@@ -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_;
};

@@ -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);
}
}

//
32 changes: 32 additions & 0 deletions FWCore/Integration/test/SwitchProducer_t.cpp
Original file line number Diff line number Diff line change
@@ -247,6 +247,38 @@ 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};
auto event1 = testProcessor1.test();
REQUIRE(event1.get<edmtest::IntProduct>()->value == 2);

// It would be better if the next line of executable code would
// throw an exception, but that is not the current expected behavior.
// It would be nice to have all cases of a SwitchProducer fail if
// any case fails on a "get" (but it is intentional and desirable
// that it does not fail only because a case declares it produces
// different transient products). We decided it was not worth the
// effort and complexity to implement this behavior (at least not yet).
REQUIRE(event1.get<edmtest::TransientIntProduct>("foo")->value == 3);

edm::test::TestProcessor::Config config2{baseConfig2};
edm::test::TestProcessor testProcessor2{config2};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about adding here tester.test() call and checks for the product values? The necessary code gets exercised (for now) via TestProcessor constructor, but I think it would be good to ensure that the processor itself runs and gives correct values.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. A good suggestion. Thanks.

auto event2 = testProcessor2.test();
REQUIRE(event2.get<edmtest::IntProduct>()->value == 1);
REQUIRE_THROWS(event2.get<edmtest::TransientIntProduct>()->value == 3);
}
}

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