From 33138876c626c119a5830f961b6612da14bbf342 Mon Sep 17 00:00:00 2001 From: Thomas Madlener Date: Tue, 10 Dec 2024 17:24:32 +0100 Subject: [PATCH 1/6] Make sure that spuriously missing ParticleID info fails CI --- k4MarlinWrapper/examples/clicRec_e4h_input.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/k4MarlinWrapper/examples/clicRec_e4h_input.py b/k4MarlinWrapper/examples/clicRec_e4h_input.py index ef6eb7e3..ee802f4e 100644 --- a/k4MarlinWrapper/examples/clicRec_e4h_input.py +++ b/k4MarlinWrapper/examples/clicRec_e4h_input.py @@ -2433,7 +2433,7 @@ from Configurables import PodioOutput out = PodioOutput("PodioOutput", filename="my_output.root") -out.outputCommands = ["keep *"] +out.outputCommands = ["keep *", "drop RefinedVertexJets_PID_RefinedVertex"] algList.append(inp) From 3d8b6e6a5b84ed8e287e98629507779191919f3c Mon Sep 17 00:00:00 2001 From: Thomas Madlener Date: Tue, 10 Dec 2024 14:27:04 +0100 Subject: [PATCH 2/6] Make the LcioEventAlgo stop the run before running out of events Effectively stop the run when processing the last event even if it is at the beginning of the event loop. Also make it throw a proper exception to immediately abort when the reading fails. --- .../k4MarlinWrapper/LcioEventAlgo.h | 2 + .../src/components/LcioEventAlgo.cpp | 73 ++++++++----------- 2 files changed, 32 insertions(+), 43 deletions(-) diff --git a/k4MarlinWrapper/k4MarlinWrapper/LcioEventAlgo.h b/k4MarlinWrapper/k4MarlinWrapper/LcioEventAlgo.h index 7ed13f75..c60db683 100644 --- a/k4MarlinWrapper/k4MarlinWrapper/LcioEventAlgo.h +++ b/k4MarlinWrapper/k4MarlinWrapper/LcioEventAlgo.h @@ -41,6 +41,8 @@ class LcioEvent : public Gaudi::Algorithm { private: Gaudi::Property> m_fileNames{this, "Files", {}}; MT::LCReader* m_reader = nullptr; + int m_numberOfEvents{}; + mutable int m_currentEvent{}; bool isReEntrant() const final { return false; } }; diff --git a/k4MarlinWrapper/src/components/LcioEventAlgo.cpp b/k4MarlinWrapper/src/components/LcioEventAlgo.cpp index 5394effa..7649d004 100644 --- a/k4MarlinWrapper/src/components/LcioEventAlgo.cpp +++ b/k4MarlinWrapper/src/components/LcioEventAlgo.cpp @@ -16,11 +16,8 @@ * See the License for the specific language governing permissions and * limitations under the License. */ - -#include - -#include "k4MarlinWrapper/LCEventWrapper.h" #include "k4MarlinWrapper/LcioEventAlgo.h" +#include "k4MarlinWrapper/LCEventWrapper.h" #include "marlin/Global.h" #include "marlin/StringParameters.h" @@ -28,7 +25,9 @@ #include #include -#include +#include + +#include #include DECLARE_COMPONENT(LcioEvent) @@ -44,54 +43,42 @@ StatusCode LcioEvent::initialize() { m_reader = new MT::LCReader(0); m_reader->open(m_fileNames); - info() << "Initialized the LcioEvent Algo: " << m_fileNames[0] << endmsg; + m_numberOfEvents = m_reader->getNumberOfEvents(); + info() << "Initialized the LcioEvent Algo. Reading from " << m_fileNames.size() << " with " << m_numberOfEvents + << " events in total." << endmsg; return StatusCode::SUCCESS; } StatusCode LcioEvent::execute(const EventContext&) const { + debug() << "Reading event " << ++m_currentEvent << " / " << m_numberOfEvents << endmsg; auto theEvent = m_reader->readNextEvent(EVENT::LCIO::UPDATE); - if (!theEvent) { - // Store flag to indicate there was NOT a LCEvent - auto pStatus = std::make_unique(false); - const StatusCode scStatus = eventSvc()->registerObject("/Event/LCEventStatus", pStatus.release()); - if (scStatus.isFailure()) { - error() << "Failed to store flag for underlying LCEvent: MarlinProcessorWrapper may try to run over non existing " - "event" - << endmsg; - return scStatus; - } + if (theEvent == nullptr) { + fatal() << "Failed to read event " << m_currentEvent << endmsg; + throw std::runtime_error("LCEvent could not be read"); + } - auto svc = service("ApplicationMgr"); - if (svc) { - svc->stopRun().ignore(); - svc->release(); - } else { - abort(); + // Since this is presumably the first algorithm to run we have to check here + // to see if we need to stop the run. Events in flight will still be + // processed, so technically this signals that we want to end the run after + // all of the algorithms for this event have finished. + if (m_currentEvent >= m_numberOfEvents) { + info() << "This is the last event in the input files. Stopping the run" << endmsg; + auto evtProcService = service("ApplicationMgr", false); + if (!evtProcService) { + fatal() << "Could not get the ApplicationMgr for stopping the run" << endmsg; } - } else { - // pass theEvent to the DataStore, so we can access them in our processor - // wrappers - info() << "Reading from file: " << m_fileNames[0] << endmsg; - - auto myEvWr = new LCEventWrapper(std::move(theEvent)); - const StatusCode sc = eventSvc()->registerObject("/Event/LCEvent", myEvWr); - if (sc.isFailure()) { - error() << "Failed to store the LCEvent" << endmsg; - return sc; - } else { - // Store flag to indicate there was a LCEvent - auto pStatus = std::make_unique(true); - std::cout << "Saving status: " << pStatus->hasLCEvent << std::endl; - const StatusCode scStatus = eventSvc()->registerObject("/Event/LCEventStatus", pStatus.release()); - if (scStatus.isFailure()) { - error() << "Failed to store flag for underlying LCEvent: MarlinProcessorWrapper may try to run over non " - "existing event" - << endmsg; - return scStatus; - } + if (evtProcService->stopRun().isFailure()) { + error() << "Out of events, but could not signal to stop the run" << endmsg; } } + auto myEvWr = new LCEventWrapper(std::move(theEvent)); + const StatusCode sc = eventSvc()->registerObject("/Event/LCEvent", myEvWr); + if (sc.isFailure()) { + error() << "Failed to store the LCEvent" << endmsg; + return sc; + } + return StatusCode::SUCCESS; } From f27b41e0d53d57f0297d335a0813e19d446ebeca Mon Sep 17 00:00:00 2001 From: Thomas Madlener Date: Tue, 10 Dec 2024 14:29:02 +0100 Subject: [PATCH 3/6] Add a warning output for potential issues --- k4MarlinWrapper/src/components/MarlinProcessorWrapper.cpp | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/k4MarlinWrapper/src/components/MarlinProcessorWrapper.cpp b/k4MarlinWrapper/src/components/MarlinProcessorWrapper.cpp index e2ed6f4a..d020fb2a 100644 --- a/k4MarlinWrapper/src/components/MarlinProcessorWrapper.cpp +++ b/k4MarlinWrapper/src/components/MarlinProcessorWrapper.cpp @@ -214,7 +214,7 @@ StatusCode MarlinProcessorWrapper::initialize() { } StatusCode MarlinProcessorWrapper::execute(const EventContext&) const { - // Get flag to know if there was an underlying LCEvent + // Get flag to check if this processor should be skipped or not DataObject* pStatus = nullptr; StatusCode scStatus = eventSvc()->retrieveObject("/Event/LCEventStatus", pStatus); if (scStatus.isSuccess()) { @@ -279,6 +279,9 @@ StatusCode MarlinProcessorWrapper::execute(const EventContext&) const { // Handle exceptions that may come from Marlin catch (marlin::SkipEventException& e) { + warning() << "Caught marlin::SkipEventException. Skipping the wrapped Processors, but Gaudi Algorithms will still " + "execute and may fail" + << endmsg; // Store flag to prevent the rest of the event from processing auto upStatus = std::make_unique(false); const StatusCode code = eventSvc()->registerObject("/Event/LCEventStatus", upStatus.release()); From dd77752f6e9e475b64f6babae3162c6cbfbdd4cc Mon Sep 17 00:00:00 2001 From: Thomas Madlener Date: Tue, 10 Dec 2024 16:23:01 +0100 Subject: [PATCH 4/6] Add some documentation about the current limitation --- doc/MarlinWrapperIntroduction.md | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/doc/MarlinWrapperIntroduction.md b/doc/MarlinWrapperIntroduction.md index f0d7a3e1..150df306 100644 --- a/doc/MarlinWrapperIntroduction.md +++ b/doc/MarlinWrapperIntroduction.md @@ -220,3 +220,25 @@ collection in the process, you would configure the tool like so lcio2edm4hepConv = Lcio2EDM4hepTool("Lcio2EDM4hep") lcio2edm4hepConv.collNameMapping = {"MCParticle": "MCParticles"} ``` + +## Potential pitfals when using other Gaudi Algorithms + +Although mixing wrapped Marlin Processors with other Gaudi Algorithms is working +for most cases, there are a few conceptual differences that have not yet been +completely mapped. This might lead to unexpected or different results when +running in Gaudi vs. running the same processors via Marlin (as far as +possible). This list aims to collect them and where possible also tries to point +out ways to work around them. + +- In Marlin processors can use a `marlin::SkipEventException` to skip the + processing of the rest of the event. +- In Marlin a `marlin::StopProcessingException` will immediately stop the + processing of the event loop. However, in Gaudi the current event will still + finish processing. + +For the `MarlinProcessorWrapper` algorithm we have put in checks and effectively +skip the execution of the actual wrapped processor in these cases. However, +other Gaudi algorithms will not have this check. Hence, execution chains that +**only consist of wrapped Marlin processors will work as expected** here. +However, **mixed chains will most likely not work as expected** (e.g. algorithms +will not find expected inputs, etc.). From b2e19a4ac03f6908e570399365e1063c427719f6 Mon Sep 17 00:00:00 2001 From: Thomas Madlener Date: Fri, 13 Dec 2024 13:27:36 +0100 Subject: [PATCH 5/6] Fix typo in documentation Co-authored-by: Mateusz Jakub Fila <37295697+m-fila@users.noreply.github.com> --- doc/MarlinWrapperIntroduction.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/MarlinWrapperIntroduction.md b/doc/MarlinWrapperIntroduction.md index 150df306..a793a00c 100644 --- a/doc/MarlinWrapperIntroduction.md +++ b/doc/MarlinWrapperIntroduction.md @@ -221,7 +221,7 @@ lcio2edm4hepConv = Lcio2EDM4hepTool("Lcio2EDM4hep") lcio2edm4hepConv.collNameMapping = {"MCParticle": "MCParticles"} ``` -## Potential pitfals when using other Gaudi Algorithms +## Potential pitfalls when using other Gaudi Algorithms Although mixing wrapped Marlin Processors with other Gaudi Algorithms is working for most cases, there are a few conceptual differences that have not yet been From a3be2effd547e521f2ec7aed7e4f17ad9d8304ab Mon Sep 17 00:00:00 2001 From: Thomas Madlener Date: Sun, 15 Dec 2024 21:01:55 +0100 Subject: [PATCH 6/6] Update k4MarlinWrapper/k4MarlinWrapper/LcioEventAlgo.h --- k4MarlinWrapper/k4MarlinWrapper/LcioEventAlgo.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/k4MarlinWrapper/k4MarlinWrapper/LcioEventAlgo.h b/k4MarlinWrapper/k4MarlinWrapper/LcioEventAlgo.h index c60db683..db006d47 100644 --- a/k4MarlinWrapper/k4MarlinWrapper/LcioEventAlgo.h +++ b/k4MarlinWrapper/k4MarlinWrapper/LcioEventAlgo.h @@ -42,7 +42,7 @@ class LcioEvent : public Gaudi::Algorithm { Gaudi::Property> m_fileNames{this, "Files", {}}; MT::LCReader* m_reader = nullptr; int m_numberOfEvents{}; - mutable int m_currentEvent{}; + mutable int m_currentEvent{}; // No atomicity necessary since not re-entrant bool isReEntrant() const final { return false; } };