QC-1164 Modernize example code in the Skeleton module (#2300)

* QC-1164 Modernize example code in the Skeleton module

* the check should support the "example" histo from a task with any name

* use some default in aggregators

* use "data" everywhere as SkeletonTask input data binding

* one more

* use correct method to get a custom parameter

Framework/advanced-aggregator.json

@@ -131,7 +131,7 @@
       "id": "tst1",
       "active": "true",
       "machines": [],
-      "query" : "sum:TST/SUM/2;param:TST/PARAM/2",
+      "query" : "data:TST/SUM/2;param:TST/PARAM/2",
       "samplingConditions": [
         {
           "condition": "random",

Framework/advanced-external-histo.json

@@ -82,7 +82,7 @@
       "id": "tst-raw",
       "active": "true",
       "machines": [],
-      "query": "random:TST/RAWDATA/0",
+      "query": "data:TST/RAWDATA/0",
       "samplingConditions": [
         {
           "condition": "random",

Framework/advanced.json

@@ -82,7 +82,7 @@
       "id": "tst1",
       "active": "true",
       "machines": [],
-      "query" : "sum:TST/SUM/2;param:TST/PARAM/2",
+      "query" : "data:TST/SUM/2;param:TST/PARAM/2",
       "samplingConditions": [
         {
           "condition": "random",

Framework/basic-aggregator.json

@@ -97,7 +97,7 @@
       "id": "tst-raw",
       "active": "true",
       "machines": [],
-      "query": "random:TST/RAWDATA/0",
+      "query": "data:TST/RAWDATA/0",
       "samplingConditions": [
         {
           "condition": "random",

Framework/basic-functional.json.in

@@ -64,7 +64,7 @@
       "id": "tst-raw",
       "active": "true",
       "machines": [],
-      "query": "random:TST/RAWDATA/0",
+      "query": "data:TST/RAWDATA/0",
       "samplingConditions": [
         {
           "condition": "random",

Framework/basic-no-sampling.json

@@ -31,7 +31,7 @@
         "cycleDurationSeconds": "10",
         "dataSource": {
           "type": "direct",
-          "query" : "tst-rawdata:TST/RAWDATA/0"
+          "query" : "data:TST/RAWDATA/0"
         },
         "taskParameters": {
           "nothing": "rien"

Framework/basic.json

@@ -27,7 +27,7 @@
       },
       "infologger": {                     "": "Configuration of the Infologger (optional).",
         "filterDiscardDebug": "false",    "": "Set to true to discard debug and trace messages (default: false)",
-        "filterDiscardLevel": "11",       "": "Message at this level or above are discarded (default: 21 - Trace)",
+        "filterDiscardLevel": "12",       "": "Message at this level or above are discarded (default: 21 - Trace)",
         "filterDiscardFile": "/tmp/_ID_.txt", "": ["If set, the messages discarded because of filterDiscardLevel",
           "will go to this file (default: <none>); The keyword _ID_ is replaced by the device id. Discarded Debug ",
           "messages won't go there."]
@@ -67,7 +67,6 @@
             }
           }
         },
-        "location": "remote",
         "saveObjectsToFile": "",      "": "For debugging, path to the file where to save. If empty or missing it won't save."
       }
     },
@@ -98,7 +97,7 @@
       "id": "tst-raw",
       "active": "true",
       "machines": [],
-      "query": "random:TST/RAWDATA/0",
+      "query": "data:TST/RAWDATA/0",
       "samplingConditions": [
         {
           "condition": "random",

Framework/batch-test.json.in

@@ -66,7 +66,7 @@
       "id": "tst-raw",
       "active": "true",
       "machines": [],
-      "query": "random:TST/RAWDATA/0",
+      "query": "data:TST/RAWDATA/0",
       "samplingConditions": [],
       "blocking": "false"
     }

Framework/include/QualityControl/CheckInterface.h

@@ -45,7 +45,7 @@ class CheckInterface : public core::UserCodeInterface
 
   /// \brief Returns the quality associated with these objects.
   ///
-  /// @param moMap A map of the the MonitorObjects to check and their full names.
+  /// @param moMap A map of the the MonitorObjects to check and their full names (i.e. <task_name>/<mo name>) as keys.
   /// @return The quality associated with these objects.
   virtual core::Quality check(std::map<std::string, std::shared_ptr<core::MonitorObject>>* moMap) = 0;
 

Framework/multiNode.json

@@ -94,7 +94,7 @@
         "localnode1",
         "localnode2"
       ],
-      "query": "random:TST/RAWDATA",
+      "query": "data:TST/RAWDATA",
       "samplingConditions": [
         {
           "condition": "random",
@@ -111,7 +111,7 @@
         "localnode2"
       ],
       "port": "30333",
-      "query": "random:TST/RAWDATA",
+      "query": "data:TST/RAWDATA",
       "samplingConditions": [
         {
           "condition": "random",

Framework/multinode-test.json.in

@@ -97,7 +97,7 @@
       "machines": [
         "localhost"
       ],
-      "query": "random:TST/RAWDATA",
+      "query": "data:TST/RAWDATA",
       "samplingConditions": [
         {
           "condition": "random",
@@ -114,7 +114,7 @@
         "localhost"
       ],
       "port": "@UNIQUE_PORT_2@",
-      "query": "random0:TST/RAWDATA",
+      "query": "data:TST/RAWDATA",
       "samplingConditions": [
         {
           "condition": "random",

Framework/test/testAggregatorRunner.cxx

@@ -78,6 +78,7 @@ TEST_CASE("test_aggregator_runner")
 
   // check the reordering
   const std::vector<std::shared_ptr<Aggregator>> aggregators = aggregatorRunner.getAggregators();
+  REQUIRE(aggregators.size() == 4);
   CHECK((aggregators.at(0)->getName() == "MyAggregatorC" || aggregators.at(0)->getName() == "MyAggregatorB"));
   CHECK((aggregators.at(1)->getName() == "MyAggregatorC" || aggregators.at(1)->getName() == "MyAggregatorB"));
   CHECK(aggregators.at(2)->getName() == "MyAggregatorA");

Framework/test/testCheckWorkflow.json

@@ -134,7 +134,7 @@
       "id": "test-policy",
       "active": "true",
       "machines": [],
-      "query": "tst-data:TST/DATA/0",
+      "query": "data:TST/DATA/0",
       "samplingConditions": [],
       "blocking": "false"
     }

Framework/test/testSharedConfig.json

@@ -355,7 +355,7 @@
       "machines": [
         "o2flp1"
       ],
-      "query": "clusters:TPC/CLUSTERS/0",
+      "query": "data:TPC/CLUSTERS/0",
       "port": "30303",
       "samplingConditions": [
         {

Framework/test/testThrowNameClash.json

@@ -18,7 +18,7 @@
         "cycleDurationSeconds": "10",
         "dataSource": {
           "type": "direct",
-          "query" : "raw:TST/RAWDATA/0"
+          "query" : "data:TST/RAWDATA/0"
         },
         "location": "remote"
       }

Framework/test/testWorkflow.json

@@ -62,7 +62,7 @@
       "id": "test-policy",
       "active": "true",
       "machines": [],
-      "query": "tst-data:TST/DATA/0",
+      "query": "data:TST/DATA/0",
       "samplingConditions": [],
       "blocking": "false"
     }

Modules/Skeleton/include/Skeleton/SkeletonTask.h

@@ -18,6 +18,7 @@
 #define QC_MODULE_SKELETON_SKELETONTASK_H
 
 #include "QualityControl/TaskInterface.h"
+#include <memory>
 
 class TH1F;
 
@@ -26,7 +27,7 @@ using namespace o2::quality_control::core;
 namespace o2::quality_control_modules::skeleton
 {
 
-/// \brief Example Quality Control DPL Task
+/// \brief Example Quality Control Task
 /// \author My Name
 class SkeletonTask final : public TaskInterface
 {
@@ -46,7 +47,8 @@ class SkeletonTask final : public TaskInterface
   void reset() override;
 
  private:
-  TH1F* mHistogram = nullptr;
+  std::shared_ptr<TH1F> mHistogramA = nullptr;
+  std::shared_ptr<TH1F> mHistogramB = nullptr;
 };
 
 } // namespace o2::quality_control_modules::skeleton

Modules/Skeleton/src/SkeletonAggregator.cxx

@@ -23,10 +23,18 @@ using namespace o2::quality_control::core;
 namespace o2::quality_control_modules::skeleton
 {
 
-void SkeletonAggregator::configure() {}
+void SkeletonAggregator::configure()
+{
+  // THUS FUNCTION BODY IS AN EXAMPLE. PLEASE REMOVE EVERYTHING YOU DO NOT NEED.
+  // This method is called whenever CustomParameters are set.
+
+  // Example of retrieving a custom parameter
+  std::string parameter = mCustomParameters.atOrDefaultValue("myOwnKey", "fallback value");
+}
 
 std::map<std::string, Quality> SkeletonAggregator::aggregate(QualityObjectsMapType& qoMap)
 {
+  // THUS FUNCTION BODY IS AN EXAMPLE. PLEASE REMOVE EVERYTHING YOU DO NOT NEED.
   std::map<std::string, Quality> result;
 
   ILOG(Info, Devel) << "Entered SkeletonAggregator::aggregate" << ENDM;
@@ -37,7 +45,7 @@ std::map<std::string, Quality> SkeletonAggregator::aggregate(QualityObjectsMapTy
 
   // we return the worse quality of all the objects we receive
   Quality current = Quality::Good;
-  for (auto qo : qoMap) {
+  for (const auto& qo : qoMap) {
     if (qo.second->getQuality().isWorseThan(current)) {
       current = qo.second->getQuality();
     }

Modules/Skeleton/src/SkeletonCheck.cxx

@@ -30,51 +30,74 @@ using namespace o2::quality_control;
 namespace o2::quality_control_modules::skeleton
 {
 
-void SkeletonCheck::configure() {}
+void SkeletonCheck::configure()
+{
+  // THUS FUNCTION BODY IS AN EXAMPLE. PLEASE REMOVE EVERYTHING YOU DO NOT NEED.
+  // This method is called whenever CustomParameters are set.
+
+  // Example of retrieving a custom parameter
+  std::string parameter = mCustomParameters.atOrDefaultValue("myOwnKey1", "default");
+}
 
 Quality SkeletonCheck::check(std::map<std::string, std::shared_ptr<MonitorObject>>* moMap)
 {
+  // THUS FUNCTION BODY IS AN EXAMPLE. PLEASE REMOVE EVERYTHING YOU DO NOT NEED.
   Quality result = Quality::Null;
 
-  // you can get details about the activity via the object mActivity:
+  // You can get details about the activity via the object mActivity:
   ILOG(Debug, Devel) << "Run " << mActivity->mId << ", type: " << mActivity->mType << ", beam: " << mActivity->mBeamType << ENDM;
   // and you can get your custom parameters:
   ILOG(Debug, Devel) << "custom param physics.pp.myOwnKey1 : " << mCustomParameters.atOrDefaultValue("myOwnKey1", "default_value", "physics", "pp") << ENDM;
 
+  // This is an example of accessing the histogram 'example' created by SkeletonTask
   for (auto& [moName, mo] : *moMap) {
-
-    (void)moName;
     if (mo->getName() == "example") {
-      auto* h = dynamic_cast<TH1F*>(mo->getObject());
-
+      auto* h = dynamic_cast<TH1*>(mo->getObject());
+      if (h == nullptr) {
+        ILOG(Error, Support) << "Could not cast `example` to TH1*, skipping" << ENDM;
+        continue;
+      }
+      // unless we find issues, we assume the quality is good
       result = Quality::Good;
 
+      // an example of a naive quality check: we want bins 1-7 to be non-empty and bins 0 and >7 to be empty.
       for (int i = 0; i < h->GetNbinsX(); i++) {
         if (i > 0 && i < 8 && h->GetBinContent(i) == 0) {
           result = Quality::Bad;
-          result.addFlag(FlagTypeFactory::Unknown(),
-                         "It is bad because there is nothing in bin " + std::to_string(i));
+          // optionally, we can add flags indicating the effect on data and a comment explaining why it was assigned.
+          result.addFlag(FlagTypeFactory::BadPID(), "It is bad because there is nothing in bin " + std::to_string(i));
           break;
         } else if ((i == 0 || i > 7) && h->GetBinContent(i) > 0) {
           result = Quality::Medium;
-          result.addFlag(FlagTypeFactory::Unknown(),
-                         "It is medium because bin " + std::to_string(i) + " is not empty");
-          result.addFlag(FlagTypeFactory::BadTracking(),
-                         "This is to demonstrate that we can assign more than one Flag to a Quality");
+          // optionally, we can add flags indicating the effect on data and a comment explaining why it was assigned.
+          result.addFlag(FlagTypeFactory::Unknown(), "It is medium because bin " + std::to_string(i) + " is not empty");
+          result.addFlag(FlagTypeFactory::BadTracking(), "We can assign more than one Flag to a Quality");
         }
       }
+      // optionally, we can associate some custom metadata to a Quality
       result.addMetadata("mykey", "myvalue");
     }
   }
   return result;
 }
 
-std::string SkeletonCheck::getAcceptedType() { return "TH1"; }
+std::string SkeletonCheck::getAcceptedType()
+{
+  // This method is a remnant of early interface prototype and will be removed in the scope of ticket QC-373
+  return "TH1";
+}
 
 void SkeletonCheck::beautify(std::shared_ptr<MonitorObject> mo, Quality checkResult)
 {
+  // THUS FUNCTION BODY IS AN EXAMPLE. PLEASE REMOVE EVERYTHING YOU DO NOT NEED.
+
+  // This method lets you decorate the checked object according to the computed Quality
   if (mo->getName() == "example") {
     auto* h = dynamic_cast<TH1F*>(mo->getObject());
+    if (h == nullptr) {
+      ILOG(Error, Support) << "Could not cast `example` to TH1*, skipping" << ENDM;
+      return;
+    }
 
     if (checkResult == Quality::Good) {
       h->SetFillColor(kGreen);
@@ -91,17 +114,21 @@ void SkeletonCheck::beautify(std::shared_ptr<MonitorObject> mo, Quality checkRes
 
 void SkeletonCheck::reset()
 {
+  // THUS FUNCTION BODY IS AN EXAMPLE. PLEASE REMOVE EVERYTHING YOU DO NOT NEED.
   ILOG(Debug, Devel) << "SkeletonCheck::reset" << ENDM;
+  // please reset the state of the check here to allow for reuse between consecutive runs.
 }
 
 void SkeletonCheck::startOfActivity(const Activity& activity)
 {
+  // THUS FUNCTION BODY IS AN EXAMPLE. PLEASE REMOVE EVERYTHING YOU DO NOT NEED.
   ILOG(Debug, Devel) << "SkeletonCheck::start : " << activity.mId << ENDM;
   mActivity = make_shared<Activity>(activity);
 }
 
 void SkeletonCheck::endOfActivity(const Activity& activity)
 {
+  // THUS FUNCTION BODY IS AN EXAMPLE. PLEASE REMOVE EVERYTHING YOU DO NOT NEED.
   ILOG(Debug, Devel) << "SkeletonCheck::end : " << activity.mId << ENDM;
 }
 

Modules/Skeleton/src/SkeletonPostProcessing.cxx

@@ -31,6 +31,8 @@ SkeletonPostProcessing::~SkeletonPostProcessing()
 
 void SkeletonPostProcessing::configure(const boost::property_tree::ptree& config)
 {
+  // THUS FUNCTION BODY IS AN EXAMPLE. PLEASE REMOVE EVERYTHING YOU DO NOT NEED.
+
   // A histogram created here can be reused throughout the task lifetime
   mHistogramA = std::make_unique<TH1F>("exampleA", "exampleA", 20, 0, 30000);
   // We request that the histogram A is published until the end of the task lifetime
@@ -39,6 +41,8 @@ void SkeletonPostProcessing::configure(const boost::property_tree::ptree& config
 
 void SkeletonPostProcessing::initialize(Trigger, framework::ServiceRegistryRef)
 {
+  // THUS FUNCTION BODY IS AN EXAMPLE. PLEASE REMOVE EVERYTHING YOU DO NOT NEED.
+
   // Resetting the histogram A, so it is empty at each new START
   mHistogramA->Reset();
 
@@ -53,6 +57,8 @@ void SkeletonPostProcessing::initialize(Trigger, framework::ServiceRegistryRef)
 
 void SkeletonPostProcessing::update(Trigger t, framework::ServiceRegistryRef)
 {
+  // THUS FUNCTION BODY IS AN EXAMPLE. PLEASE REMOVE EVERYTHING YOU DO NOT NEED.
+
   ILOG(Info, Support) << "Trigger type is: " << t.triggerType << ", the timestamp is " << t.timestamp << ENDM;
 
   // Histogram C is recreated at each update and is expected to be published just once. We delete the previous