From fff43f64292b815c4157cd5f02730f3d10dc96a3 Mon Sep 17 00:00:00 2001 From: "W. David Dagenhart" Date: Wed, 1 Mar 2017 09:55:22 -0600 Subject: [PATCH] Fix parentage provenance in corner case This fixes a bug that caused the parentage and things that depend on the parentage to be wrong in a corner case where a SubProcess is used and a module in the SubProcess causes unscheduled execution to run a module in an earlier SubProcess or top level Process and the parentage is accessed through the ProductProvenanceRetriever (not using the getProvenance function from the Event or the Provenance returned by a Handle). --- .../interface/ProductProvenanceRetriever.h | 3 + .../src/ProductProvenanceRetriever.cc | 11 ++ FWCore/Framework/interface/EventPrincipal.h | 3 +- FWCore/Framework/src/EventPrincipal.cc | 9 +- FWCore/Framework/src/SubProcess.cc | 4 +- FWCore/Integration/test/BuildFile.xml | 2 +- FWCore/Integration/test/TestParentage.cc | 113 ++++++++++++++++++ FWCore/Integration/test/run_SubProcess.sh | 3 + .../test/testSubProcessUnscheduledRead_cfg.py | 16 +++ .../test/testSubProcessUnscheduled_cfg.py | 15 +++ 10 files changed, 174 insertions(+), 5 deletions(-) create mode 100644 FWCore/Integration/test/TestParentage.cc create mode 100644 FWCore/Integration/test/testSubProcessUnscheduledRead_cfg.py diff --git a/DataFormats/Provenance/interface/ProductProvenanceRetriever.h b/DataFormats/Provenance/interface/ProductProvenanceRetriever.h index 30da74d560472..23471393201bf 100644 --- a/DataFormats/Provenance/interface/ProductProvenanceRetriever.h +++ b/DataFormats/Provenance/interface/ProductProvenanceRetriever.h @@ -67,6 +67,8 @@ namespace edm { void mergeProvenanceRetrievers(std::shared_ptr other); + void mergeParentProcessRetriever(ProductProvenanceRetriever const& provRetriever); + void deepCopy(ProductProvenanceRetriever const&); void reset(); @@ -82,6 +84,7 @@ namespace edm { mutable tbb::concurrent_unordered_set entryInfoSet_; mutable std::atomic*> readEntryInfoSet_; edm::propagate_const> nextRetriever_; + edm::propagate_const parentProcessRetriever_; std::shared_ptr provenanceReader_; unsigned int transitionIndex_; }; diff --git a/DataFormats/Provenance/src/ProductProvenanceRetriever.cc b/DataFormats/Provenance/src/ProductProvenanceRetriever.cc index c37999a1b35f3..b0f7b3647c9be 100644 --- a/DataFormats/Provenance/src/ProductProvenanceRetriever.cc +++ b/DataFormats/Provenance/src/ProductProvenanceRetriever.cc @@ -14,6 +14,7 @@ namespace edm { entryInfoSet_(), readEntryInfoSet_(), nextRetriever_(), + parentProcessRetriever_(nullptr), provenanceReader_(), transitionIndex_(iTransitionIndex){ } @@ -22,6 +23,7 @@ namespace edm { entryInfoSet_(), readEntryInfoSet_(), nextRetriever_(), + parentProcessRetriever_(nullptr), provenanceReader_(reader.release()), transitionIndex_(std::numeric_limits::max()) { @@ -86,6 +88,7 @@ namespace edm { if(nextRetriever_) { nextRetriever_->reset(); } + parentProcessRetriever_ = nullptr; } void @@ -102,11 +105,19 @@ namespace edm { nextRetriever_ = other; } + void + ProductProvenanceRetriever::mergeParentProcessRetriever(ProductProvenanceRetriever const& provRetriever) { + parentProcessRetriever_ = &provRetriever; + } + ProductProvenance const* ProductProvenanceRetriever::branchIDToProvenance(BranchID const& bid) const { ProductProvenance ei(bid); auto it = entryInfoSet_.find(ei); if(it == entryInfoSet_.end()) { + if (parentProcessRetriever_) { + return parentProcessRetriever_->branchIDToProvenance(bid); + } //check in source readProvenance(); auto ptr =readEntryInfoSet_.load(); diff --git a/FWCore/Framework/interface/EventPrincipal.h b/FWCore/Framework/interface/EventPrincipal.h index 6d16637bb7fb1..30a1a3627fc3a 100644 --- a/FWCore/Framework/interface/EventPrincipal.h +++ b/FWCore/Framework/interface/EventPrincipal.h @@ -75,7 +75,8 @@ namespace edm { EventSelectionIDVector&& eventSelectionIDs, BranchListIndexes&& branchListIndexes, ProductProvenanceRetriever const& provRetriever, - DelayedReader* reader = nullptr); + DelayedReader* reader = nullptr, + bool deepCopyRetriever = true); void clearEventPrincipal(); diff --git a/FWCore/Framework/src/EventPrincipal.cc b/FWCore/Framework/src/EventPrincipal.cc index 756c043e3fc5d..0398641888828 100644 --- a/FWCore/Framework/src/EventPrincipal.cc +++ b/FWCore/Framework/src/EventPrincipal.cc @@ -63,9 +63,14 @@ namespace edm { EventSelectionIDVector&& eventSelectionIDs, BranchListIndexes&& branchListIndexes, ProductProvenanceRetriever const& provRetriever, - DelayedReader* reader) { + DelayedReader* reader, + bool deepCopyRetriever) { eventSelectionIDs_ = eventSelectionIDs; - provRetrieverPtr_->deepCopy(provRetriever); + if (deepCopyRetriever) { + provRetrieverPtr_->deepCopy(provRetriever); + } else { + provRetrieverPtr_->mergeParentProcessRetriever(provRetriever); + } branchListIndexes_ = branchListIndexes; if(branchIDListHelper_->hasProducedProducts()) { // Add index into BranchIDListRegistry for products produced this process diff --git a/FWCore/Framework/src/SubProcess.cc b/FWCore/Framework/src/SubProcess.cc index 27083894cbda7..9f01dd30ea431 100644 --- a/FWCore/Framework/src/SubProcess.cc +++ b/FWCore/Framework/src/SubProcess.cc @@ -352,12 +352,14 @@ namespace edm { processHistoryRegistry.registerProcessHistory(principal.processHistory()); BranchListIndexes bli(principal.branchListIndexes()); branchIDListHelper_->fixBranchListIndexes(bli); + bool deepCopyRetriever = false; ep.fillEventPrincipal(aux, processHistoryRegistry, std::move(esids), std::move(bli), *(principal.productProvenanceRetrieverPtr()),//NOTE: this transfers the per product provenance - principal.reader()); + principal.reader(), + deepCopyRetriever); ep.setLuminosityBlockPrincipal(principalCache_.lumiPrincipalPtr()); propagateProducts(InEvent, principal, ep); diff --git a/FWCore/Integration/test/BuildFile.xml b/FWCore/Integration/test/BuildFile.xml index a7aa3672e88f5..5b1334fab750c 100644 --- a/FWCore/Integration/test/BuildFile.xml +++ b/FWCore/Integration/test/BuildFile.xml @@ -188,7 +188,7 @@ - + diff --git a/FWCore/Integration/test/TestParentage.cc b/FWCore/Integration/test/TestParentage.cc new file mode 100644 index 0000000000000..9129cba96ec4b --- /dev/null +++ b/FWCore/Integration/test/TestParentage.cc @@ -0,0 +1,113 @@ + +#include "DataFormats/Common/interface/Handle.h" +#include "DataFormats/Provenance/interface/BranchID.h" +#include "DataFormats/Provenance/interface/Parentage.h" +#include "DataFormats/Provenance/interface/ProductProvenance.h" +#include "DataFormats/Provenance/interface/ProductProvenanceRetriever.h" +#include "DataFormats/Provenance/interface/Provenance.h" +#include "DataFormats/TestObjects/interface/ToyProducts.h" +#include "FWCore/Framework/interface/EDAnalyzer.h" +#include "FWCore/Framework/interface/Event.h" +#include "FWCore/Framework/interface/MakerMacros.h" +#include "FWCore/ParameterSet/interface/ParameterSet.h" +#include "FWCore/Utilities/interface/InputTag.h" +#include "FWCore/Utilities/interface/EDGetToken.h" + +#include +#include +#include +#include + +namespace { + void getAncestors(edm::Event const& e, + edm::BranchID const& branchID, + std::set& ancestors) { + edm::Provenance prov = e.getProvenance(branchID); + for (auto const& parent : prov.productProvenance()->parentage().parents()) { + ancestors.insert(parent); + getAncestors(e, parent, ancestors); + } + } + + // Does the same thing as the previous function in a different + // way. The previous function goes through the links in the + // ProductsResolver which for SubProcesses could lead to a different + // retriever. In SubProcesses, the following function follows the + // links in the retrievers themselves. Both should give the same answer. + void getAncestorsFromRetriever(edm::ProductProvenanceRetriever const* retriever, + edm::BranchID const& branchID, + std::set& ancestors) { + edm::ProductProvenance const* productProvenance = retriever->branchIDToProvenance(branchID); + if (productProvenance) { + for (auto const& parent : productProvenance->parentage().parents()) { + ancestors.insert(parent); + getAncestorsFromRetriever(retriever, parent, ancestors); + } + } + } +} + +namespace edmtest { + + class TestParentage : public edm::EDAnalyzer { + public: + + explicit TestParentage(edm::ParameterSet const& pset); + virtual ~TestParentage(); + + virtual void analyze(edm::Event const& e, edm::EventSetup const& es) override; + + private: + + edm::InputTag inputTag_; + edm::EDGetTokenT token_; + std::vector expectedAncestors_; + }; + + TestParentage::TestParentage(edm::ParameterSet const& pset) : + inputTag_(pset.getParameter("inputTag")), + expectedAncestors_(pset.getParameter >("expectedAncestors")) { + + token_ = consumes(inputTag_); + } + + TestParentage::~TestParentage() {} + + void + TestParentage::analyze(edm::Event const& e, edm::EventSetup const&) { + + edm::Handle h; + e.getByToken(token_, h); + + edm::Provenance const* prov = h.provenance(); + std::set ancestors; + getAncestors(e, prov->branchID(), ancestors); + + std::set ancestorLabels; + for (edm::BranchID const& ancestor : ancestors) { + edm::Provenance ancestorProv = e.getProvenance(ancestor); + ancestorLabels.insert(ancestorProv.moduleLabel()); + } + std::set expectedAncestors(expectedAncestors_.begin(), expectedAncestors_.end()); + if (ancestorLabels != expectedAncestors) { + std::cerr << "TestParentage::analyze: ancestors do not match expected ancestors" << std::endl; + abort(); + } + edm::ProductProvenanceRetriever const* retriever = prov->store(); + std::set ancestorsFromRetriever; + getAncestorsFromRetriever(retriever, prov->branchID(), ancestorsFromRetriever); + + std::set ancestorLabels2; + for (edm::BranchID const& ancestor : ancestorsFromRetriever) { + edm::Provenance ancestorProv = e.getProvenance(ancestor); + ancestorLabels2.insert(ancestorProv.moduleLabel()); + } + if (ancestorLabels2 != expectedAncestors) { + std::cerr << "TestParentage::analyze: ancestors do not match expected ancestors (parentage from retriever)" << std::endl; + abort(); + } + } +} // namespace edmtest + +using edmtest::TestParentage; +DEFINE_FWK_MODULE(TestParentage); diff --git a/FWCore/Integration/test/run_SubProcess.sh b/FWCore/Integration/test/run_SubProcess.sh index 5e105098750a2..6569b7d05bb18 100755 --- a/FWCore/Integration/test/run_SubProcess.sh +++ b/FWCore/Integration/test/run_SubProcess.sh @@ -37,6 +37,9 @@ pushd ${LOCAL_TMP_DIR} echo cmsRun testSubProcessUnscheduled_cfg.py cmsRun -p ${LOCAL_TEST_DIR}/testSubProcessUnscheduled_cfg.py || die "cmsRun testSubProcessUnscheduled_cfg.py" $? + echo cmsRun testSubProcessUnscheduledRead_cfg.py + cmsRun -p ${LOCAL_TEST_DIR}/testSubProcessUnscheduledRead_cfg.py || die "cmsRun testSubProcessUnscheduledRead_cfg.py" $? + popd exit 0 diff --git a/FWCore/Integration/test/testSubProcessUnscheduledRead_cfg.py b/FWCore/Integration/test/testSubProcessUnscheduledRead_cfg.py new file mode 100644 index 0000000000000..0e30c86731e24 --- /dev/null +++ b/FWCore/Integration/test/testSubProcessUnscheduledRead_cfg.py @@ -0,0 +1,16 @@ +import FWCore.ParameterSet.Config as cms + +process = cms.Process( "TEST2" ) + +process.source = cms.Source("PoolSource", + fileNames = cms.untracked.vstring( + 'file:testSubProcessUnscheduled.root' + ) +) + +process.test = cms.EDAnalyzer("TestParentage", + inputTag = cms.InputTag("final"), + expectedAncestors = cms.vstring("two", "ten", "adder") +) +process.o = cms.EndPath(process.test) + diff --git a/FWCore/Integration/test/testSubProcessUnscheduled_cfg.py b/FWCore/Integration/test/testSubProcessUnscheduled_cfg.py index 1f47913f037d3..dfbad2e586afa 100644 --- a/FWCore/Integration/test/testSubProcessUnscheduled_cfg.py +++ b/FWCore/Integration/test/testSubProcessUnscheduled_cfg.py @@ -36,3 +36,18 @@ subprocess.final = cms.EDProducer("AddIntsProducer", labels = cms.vstring('adder')) subprocess.subpath = cms.Path( subprocess.final ) + +subprocess.test = cms.EDAnalyzer("TestParentage", + inputTag = cms.InputTag("final"), + expectedAncestors = cms.vstring("two", "ten", "adder") +) + +subprocess.out = cms.OutputModule("PoolOutputModule", + fileName = cms.untracked.string('testSubProcessUnscheduled.root'), + outputCommands = cms.untracked.vstring( + 'drop *', + 'keep *_final_*_*', + ) +) +subprocess.o = cms.EndPath(subprocess.test * subprocess.out) +