From 0e2d5c339006e8c9f1122e4997834862816a11ca Mon Sep 17 00:00:00 2001 From: Barthelemy Date: Mon, 24 Jan 2022 16:19:31 +0100 Subject: [PATCH 1/6] QC-310: reconfigure checkrunner --- .../include/QualityControl/CheckRunner.h | 2 +- Framework/src/CheckRunner.cxx | 43 ++++++++++--------- 2 files changed, 23 insertions(+), 22 deletions(-) diff --git a/Framework/include/QualityControl/CheckRunner.h b/Framework/include/QualityControl/CheckRunner.h index 1ebe863e06..7f3678a9eb 100644 --- a/Framework/include/QualityControl/CheckRunner.h +++ b/Framework/include/QualityControl/CheckRunner.h @@ -209,7 +209,7 @@ class CheckRunner : public framework::Task // General state std::string mDeviceName; - std::vector mChecks; + std::map mChecks; std::string mDetectorName; Activity mActivity; CheckRunnerConfig mConfig; diff --git a/Framework/src/CheckRunner.cxx b/Framework/src/CheckRunner.cxx index 5c02fa6cb9..5f98d52d5a 100644 --- a/Framework/src/CheckRunner.cxx +++ b/Framework/src/CheckRunner.cxx @@ -139,13 +139,12 @@ CheckRunner::CheckRunner(CheckRunnerConfig checkRunnerConfig, const std::vector< mTotalQOSent(0) { for (const auto& checkConfig : checkConfigs) { - mChecks.emplace_back(checkConfig); + mChecks[checkConfig.name] = checkConfig; } } CheckRunner::CheckRunner(CheckRunnerConfig checkRunnerConfig, InputSpec input) : mDeviceName(createSinkCheckRunnerName(input)), - mChecks{}, mConfig(std::move(checkRunnerConfig)), mInputs{ input }, mOutputs{}, @@ -181,11 +180,17 @@ void CheckRunner::refreshConfig(InitContext& iCtx) // prepare the information we need auto infrastructureSpec = InfrastructureSpecReader::readInfrastructureSpec(updatedTree); - // TODO: use the config to reconfigure the check runner. - // TODO: in particular, reset mChecks and update it. - // TODO: Problem is that a lot of the logic is in the infrastructure generator. - // TODO: we should probably just preserve the checks list and update their state. - // TODO: also see if we should update the detector name + // Use the config to reconfigure the check runner. + // The configs for the checks we find in the config and in our map are updated. + // Topology changes are ignored: New checks are ignored. Removed checks are ignored. + for (const auto& checkSpec : infrastructureSpec.checks) { + // search if we have this check in this runner and replace it + if ( mChecks.find(checkSpec.checkName) != mChecks.end() ) { + auto checkConfig = Check::extractConfig(infrastructureSpec.common, checkSpec); + mChecks[checkSpec.checkName] = checkConfig; + ILOG(Debug, Devel) << "Check "<< checkSpec.checkName << " has been updated" << ENDM; + } + } } } catch (std::invalid_argument& error) { // ignore the error, we just skip the update of the config file. It can be legit, e.g. in command line mode @@ -207,9 +212,9 @@ void CheckRunner::init(framework::InitContext& iCtx) iCtx.services().get().set(CallbackService::Id::Reset, [this]() { reset(); }); iCtx.services().get().set(CallbackService::Id::Stop, [this]() { stop(); }); - for (auto& check : mChecks) { - check.init(); - updatePolicyManager.addPolicy(check.getName(), check.getUpdatePolicyType(), check.getObjectsNames(), check.getAllObjectsOption(), false); + for (auto& tuple : mChecks) { + tuple.second.init(); + updatePolicyManager.addPolicy(tuple.second.getName(), tuple.second.getUpdatePolicyType(), tuple.second.getObjectsNames(), tuple.second.getAllObjectsOption(), false); } } catch (...) { // catch the exceptions and print it (the ultimate caller might not know how to display it) @@ -314,18 +319,18 @@ QualityObjectsType CheckRunner::check() << ENDM; QualityObjectsType allQOs; - for (auto& check : mChecks) { - if (updatePolicyManager.isReady(check.getName())) { - auto newQOs = check.check(mMonitorObjects); + for (auto& tuple : mChecks) { + if (updatePolicyManager.isReady(tuple.second.getName())) { + auto newQOs = tuple.second.check(mMonitorObjects); mTotalNumberCheckExecuted += newQOs.size(); allQOs.insert(allQOs.end(), std::make_move_iterator(newQOs.begin()), std::make_move_iterator(newQOs.end())); newQOs.clear(); // Was checked, update latest revision - updatePolicyManager.updateActorRevision(check.getName()); + updatePolicyManager.updateActorRevision(tuple.first); } else { - ILOG(Info, Support) << "Monitor Objects for the check '" << check.getName() << "' are not ready, ignoring" << ENDM; + ILOG(Info, Support) << "Monitor Objects for the check '" << tuple.first << "' are not ready, ignoring" << ENDM; } } return allQOs; @@ -366,12 +371,8 @@ void CheckRunner::send(QualityObjectsType& qualityObjects, framework::DataAlloca ILOG(Info, Support) << "Sending " << qualityObjects.size() << " quality objects" << ENDM; for (const auto& qo : qualityObjects) { - - const auto& correspondingCheck = std::find_if(mChecks.begin(), mChecks.end(), [checkName = qo->getCheckName()](const auto& check) { - return check.getName() == checkName; - }); - - auto outputSpec = correspondingCheck->getOutputSpec(); + const auto& correspondingCheck = mChecks.at(qo->getCheckName()); + auto outputSpec = correspondingCheck.getOutputSpec(); auto concreteOutput = framework::DataSpecUtils::asConcreteDataMatcher(outputSpec); allocator.snapshot( framework::Output{ concreteOutput.origin, concreteOutput.description, concreteOutput.subSpec, outputSpec.lifetime }, *qo); From f7032303f143f03a4943b7eead03a52bff03351d Mon Sep 17 00:00:00 2001 From: Barthelemy Date: Tue, 25 Jan 2022 08:42:19 +0100 Subject: [PATCH 2/6] pr comment --- Framework/src/CheckRunner.cxx | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/Framework/src/CheckRunner.cxx b/Framework/src/CheckRunner.cxx index 5f98d52d5a..206e99b345 100644 --- a/Framework/src/CheckRunner.cxx +++ b/Framework/src/CheckRunner.cxx @@ -212,9 +212,9 @@ void CheckRunner::init(framework::InitContext& iCtx) iCtx.services().get().set(CallbackService::Id::Reset, [this]() { reset(); }); iCtx.services().get().set(CallbackService::Id::Stop, [this]() { stop(); }); - for (auto& tuple : mChecks) { - tuple.second.init(); - updatePolicyManager.addPolicy(tuple.second.getName(), tuple.second.getUpdatePolicyType(), tuple.second.getObjectsNames(), tuple.second.getAllObjectsOption(), false); + for (auto& [checkName, check] : mChecks) { + check.init(); + updatePolicyManager.addPolicy(check.getName(), check.getUpdatePolicyType(), check.getObjectsNames(), check.getAllObjectsOption(), false); } } catch (...) { // catch the exceptions and print it (the ultimate caller might not know how to display it) @@ -319,18 +319,18 @@ QualityObjectsType CheckRunner::check() << ENDM; QualityObjectsType allQOs; - for (auto& tuple : mChecks) { - if (updatePolicyManager.isReady(tuple.second.getName())) { - auto newQOs = tuple.second.check(mMonitorObjects); + for (auto& [checkName, check] : mChecks) { + if (updatePolicyManager.isReady(check.getName())) { + auto newQOs = check.check(mMonitorObjects); mTotalNumberCheckExecuted += newQOs.size(); allQOs.insert(allQOs.end(), std::make_move_iterator(newQOs.begin()), std::make_move_iterator(newQOs.end())); newQOs.clear(); // Was checked, update latest revision - updatePolicyManager.updateActorRevision(tuple.first); + updatePolicyManager.updateActorRevision(checkName); } else { - ILOG(Info, Support) << "Monitor Objects for the check '" << tuple.first << "' are not ready, ignoring" << ENDM; + ILOG(Info, Support) << "Monitor Objects for the check '" << checkName << "' are not ready, ignoring" << ENDM; } } return allQOs; From 833fbf5f70e6c3e467a16b2e3631a289ff439681 Mon Sep 17 00:00:00 2001 From: Barthelemy Date: Tue, 25 Jan 2022 08:49:30 +0100 Subject: [PATCH 3/6] reset the policies in init. --- Framework/include/QualityControl/UpdatePolicyManager.h | 6 ++++++ Framework/src/CheckRunner.cxx | 1 + Framework/src/UpdatePolicyManager.cxx | 7 +++++++ 3 files changed, 14 insertions(+) diff --git a/Framework/include/QualityControl/UpdatePolicyManager.h b/Framework/include/QualityControl/UpdatePolicyManager.h index ddf0d9b131..74dc239e6a 100644 --- a/Framework/include/QualityControl/UpdatePolicyManager.h +++ b/Framework/include/QualityControl/UpdatePolicyManager.h @@ -122,6 +122,12 @@ class UpdatePolicyManager * @param policyHelper */ void addPolicy(std::string actorName, UpdatePolicyType policyType, std::vector objectNames, bool allObjects, bool policyHelper); + + /** + * Remove all policies and reset revisions. + */ + void reset(); + /** * Checks whether the given actor is ready or not. * @param actorName diff --git a/Framework/src/CheckRunner.cxx b/Framework/src/CheckRunner.cxx index 206e99b345..b607fd3e8c 100644 --- a/Framework/src/CheckRunner.cxx +++ b/Framework/src/CheckRunner.cxx @@ -212,6 +212,7 @@ void CheckRunner::init(framework::InitContext& iCtx) iCtx.services().get().set(CallbackService::Id::Reset, [this]() { reset(); }); iCtx.services().get().set(CallbackService::Id::Stop, [this]() { stop(); }); + updatePolicyManager.reset(); for (auto& [checkName, check] : mChecks) { check.init(); updatePolicyManager.addPolicy(check.getName(), check.getUpdatePolicyType(), check.getObjectsNames(), check.getAllObjectsOption(), false); diff --git a/Framework/src/UpdatePolicyManager.cxx b/Framework/src/UpdatePolicyManager.cxx index ab773347d8..cfa86300c6 100644 --- a/Framework/src/UpdatePolicyManager.cxx +++ b/Framework/src/UpdatePolicyManager.cxx @@ -183,4 +183,11 @@ std::ostream& operator<<(std::ostream& out, const UpdatePolicy& updatePolicy) // return out; } +void UpdatePolicyManager::reset() +{ + mPoliciesByActor.clear(); + mObjectsRevision.clear(); + mGlobalRevision = 1; +} + } // namespace o2::quality_control::checker \ No newline at end of file From ac0642802960e6715d5cab1d390d4f030cea3a29 Mon Sep 17 00:00:00 2001 From: Barthelemy Date: Tue, 25 Jan 2022 09:04:57 +0100 Subject: [PATCH 4/6] Make the insertion into the map of checks more explicit. --- Framework/src/CheckRunner.cxx | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/Framework/src/CheckRunner.cxx b/Framework/src/CheckRunner.cxx index b607fd3e8c..44b0893bf1 100644 --- a/Framework/src/CheckRunner.cxx +++ b/Framework/src/CheckRunner.cxx @@ -138,8 +138,8 @@ CheckRunner::CheckRunner(CheckRunnerConfig checkRunnerConfig, const std::vector< mTotalNumberMOStored(0), mTotalQOSent(0) { - for (const auto& checkConfig : checkConfigs) { - mChecks[checkConfig.name] = checkConfig; + for (auto& checkConfig : checkConfigs) { + mChecks.emplace(checkConfig.name, checkConfig); } } @@ -187,7 +187,7 @@ void CheckRunner::refreshConfig(InitContext& iCtx) // search if we have this check in this runner and replace it if ( mChecks.find(checkSpec.checkName) != mChecks.end() ) { auto checkConfig = Check::extractConfig(infrastructureSpec.common, checkSpec); - mChecks[checkSpec.checkName] = checkConfig; + mChecks.emplace(checkConfig.name, checkConfig); ILOG(Debug, Devel) << "Check "<< checkSpec.checkName << " has been updated" << ENDM; } } From 1225d674f440afcec2c4e39d1615b3e7a5c1a9a4 Mon Sep 17 00:00:00 2001 From: Barthelemy Date: Tue, 25 Jan 2022 09:26:48 +0100 Subject: [PATCH 5/6] format --- Framework/src/CheckRunner.cxx | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Framework/src/CheckRunner.cxx b/Framework/src/CheckRunner.cxx index 44b0893bf1..2182468821 100644 --- a/Framework/src/CheckRunner.cxx +++ b/Framework/src/CheckRunner.cxx @@ -185,10 +185,10 @@ void CheckRunner::refreshConfig(InitContext& iCtx) // Topology changes are ignored: New checks are ignored. Removed checks are ignored. for (const auto& checkSpec : infrastructureSpec.checks) { // search if we have this check in this runner and replace it - if ( mChecks.find(checkSpec.checkName) != mChecks.end() ) { + if (mChecks.find(checkSpec.checkName) != mChecks.end()) { auto checkConfig = Check::extractConfig(infrastructureSpec.common, checkSpec); mChecks.emplace(checkConfig.name, checkConfig); - ILOG(Debug, Devel) << "Check "<< checkSpec.checkName << " has been updated" << ENDM; + ILOG(Debug, Devel) << "Check " << checkSpec.checkName << " has been updated" << ENDM; } } } From 28d21e85b1f9581d1fdb4de4218a68e6b2526ff5 Mon Sep 17 00:00:00 2001 From: Barthelemy Date: Tue, 25 Jan 2022 16:12:54 +0100 Subject: [PATCH 6/6] erase the element before emplacing a new one --- Framework/src/CheckRunner.cxx | 1 + 1 file changed, 1 insertion(+) diff --git a/Framework/src/CheckRunner.cxx b/Framework/src/CheckRunner.cxx index 2182468821..58332b1dca 100644 --- a/Framework/src/CheckRunner.cxx +++ b/Framework/src/CheckRunner.cxx @@ -187,6 +187,7 @@ void CheckRunner::refreshConfig(InitContext& iCtx) // search if we have this check in this runner and replace it if (mChecks.find(checkSpec.checkName) != mChecks.end()) { auto checkConfig = Check::extractConfig(infrastructureSpec.common, checkSpec); + mChecks.erase(checkConfig.name); mChecks.emplace(checkConfig.name, checkConfig); ILOG(Debug, Devel) << "Check " << checkSpec.checkName << " has been updated" << ENDM; }