[QC-1079] Run type as a string (new attempt...) (#2282)

* [QC-1079] Run type as a string (#2077) * [QC-1079] run type as string * doc * config files * format * clearer method name * 1. extract isOnlyDigits to stringUtils 2. handle the case when the run type is a number and convert it to the string representation 3. tests * fix broken test * Test the translation of an integer run type into a string run type. Also warn the user of the translation. * format * forgot the test * fix (one line went missing0 * format * rebase master unify isOnlyDigits and is_unsigned_integer into isUnsignedInteger * use snake-case properties names to ahve it work on both FLPs and EPNs. * fix a mistake
AliceO2Group · May 8, 2024 · f77c1ef · f77c1ef
1 parent e853762
commit f77c1ef
Show file tree

Hide file tree

Showing 144 changed files with 403 additions and 265 deletions.
diff --git a/Framework/CMakeLists.txt b/Framework/CMakeLists.txt
@@ -1,5 +1,5 @@
 # Produce the final Version.h using template Version.h.in and substituting
-# variables. We don't want to polute our source tree with it, thus putting it in
+# variables. We don't want to pollute our source tree with it, thus putting it in
 # binary tree.
 configure_file("include/QualityControl/Version.h.in"
                "${CMAKE_CURRENT_BINARY_DIR}/include/QualityControl/Version.h"
@@ -280,7 +280,9 @@ set(TEST_SRCS
     test/testRepoPathUtils.cxx
     test/testQualitiesToFlagCollectionConverter.cxx
     test/testUserCodeInterface.cxx
-  )
+    test/testStringUtils.cxx
+    test/testRunnerUtils.cxx
+)
 
 set(TEST_ARGS
     ""
@@ -300,6 +302,8 @@ set(TEST_ARGS
     ""
     ""
     ""
+    ""
+    ""
   )
 
 list(LENGTH TEST_SRCS count)

diff --git a/Framework/advanced-aggregator.json b/Framework/advanced-aggregator.json
@@ -10,7 +10,7 @@
       },
       "Activity": {
         "number": "42",
-        "type": "2"
+        "type": "NONE"
       },
       "monitoring": {
         "url": "infologger:///debug?qc"

diff --git a/Framework/advanced-external-histo.json b/Framework/advanced-external-histo.json
@@ -10,7 +10,7 @@
       },
       "Activity": {
         "number": "42",
-        "type": "2"
+        "type": "NONE"
       },
       "monitoring": {
         "url": "infologger:///debug?qc"

diff --git a/Framework/advanced.json b/Framework/advanced.json
@@ -10,7 +10,7 @@
       },
       "Activity": {
         "number": "42",
-        "type": "2"
+        "type": "NONE"
       },
       "monitoring": {
         "url": "infologger:///debug?qc"

diff --git a/Framework/basic-aggregator.json b/Framework/basic-aggregator.json
@@ -10,7 +10,7 @@
       },
       "Activity": {
         "number": "42",
-        "type": "2"
+        "type": "NONE"
       },
       "monitoring": {
         "url": "infologger:///debug?qc"

diff --git a/Framework/basic-external-histo.json b/Framework/basic-external-histo.json
@@ -10,7 +10,7 @@
       },
       "Activity": {
         "number": "42",
-        "type": "2"
+        "type": "NONE"
       },
       "monitoring": {
         "url": "infologger:///debug?qc"

diff --git a/Framework/basic-functional.json.in b/Framework/basic-functional.json.in
@@ -10,7 +10,7 @@
       },
       "Activity": {
         "number": "42",
-        "type": "2",
+        "type": "NONE",
         "start": "8000000",
         "end": "9000000",
         "periodName": "LHC9000x",

diff --git a/Framework/basic-no-sampling.json b/Framework/basic-no-sampling.json
@@ -10,7 +10,7 @@
       },
       "Activity": {
         "number": "42",
-        "type": "2"
+        "type": "NONE"
       },
       "monitoring": {
         "url": "infologger:///debug?qc"

diff --git a/Framework/basic.json b/Framework/basic.json
@@ -11,7 +11,7 @@
       },
       "Activity": {
         "number": "42",
-        "type": "2",
+        "type": "NONE",
         "periodName": "",           "": "Period name - e.g. LHC22c, LHC22c1b_test",
         "passName": "",             "": "Pass type - e.g. spass, cpass1",
         "provenance": "qc",         "": "Provenance - qc or qc_mc depending whether it is normal data or monte carlo data"

diff --git a/Framework/batch-test.json.in b/Framework/batch-test.json.in
@@ -10,7 +10,7 @@
       },
       "Activity": {
         "number": "42",
-        "type": "2",
+        "type": "NONE",
         "start": "8000000",
         "end": "9000000",
         "periodName": "LHC9000x",

diff --git a/Framework/example-default.json b/Framework/example-default.json
@@ -10,7 +10,7 @@
       },
       "Activity": {
         "number": "42",
-        "type": "2"
+        "type": "NONE"
       },
       "monitoring": {
         "url": "infologger:///debug?qc"

diff --git a/Framework/include/QualityControl/Activity.h b/Framework/include/QualityControl/Activity.h
@@ -32,7 +32,7 @@ class Activity
  public:
   Activity() = default;
   Activity(int id,
-           int type,
+           const std::string& type,
            const std::string& periodName = "",
            const std::string& passName = "",
            const std::string& provenance = "qc",
@@ -72,7 +72,7 @@ class Activity
   virtual ~Activity() = default;
 
   int mId{ 0 };
-  int mType{ 0 };
+  std::string mType{ "NONE" };
   std::string mPeriodName{};
   std::string mPassName{};
   std::string mProvenance{ "qc" };

diff --git a/Framework/include/QualityControl/CommonSpec.h b/Framework/include/QualityControl/CommonSpec.h
@@ -29,7 +29,7 @@ struct CommonSpec {
 
   std::unordered_map<std::string, std::string> database;
   int activityNumber{};
-  int activityType{};
+  std::string activityType = "NONE";
   std::string activityPeriodName;
   std::string activityPassName;
   std::string activityProvenance = "qc";

diff --git a/Framework/include/QualityControl/runnerUtils.h b/Framework/include/QualityControl/runnerUtils.h
@@ -17,9 +17,11 @@
 #ifndef QUALITYCONTROL_RUNNERUTILS_H
 #define QUALITYCONTROL_RUNNERUTILS_H
 
+#include <boost/lexical_cast/bad_lexical_cast.hpp>
 #include <string>
 #include <Framework/ServiceRegistryRef.h>
 #include <Framework/RawDeviceService.h>
+#include <ProgOptions.h>
 #include <fairmq/Device.h>
 #include "QualityControl/QcInfoLogger.h"
 #include "QualityControl/Activity.h"
@@ -38,8 +40,8 @@ std::string getFirstTaskName(const std::string& configurationSource);
 std::string getFirstCheckName(const std::string& configurationSource);
 bool hasChecks(const std::string& configSource);
 
-template <typename T> // TODO we should probably limit T to numbers somehow
-T computeActivityField(framework::ServiceRegistryRef services, const std::string& name, T fallbackNumber = 0)
+template <typename T, typename = typename std::enable_if<std::is_arithmetic<T>::value, T>::type>
+T computeNumericalActivityField(framework::ServiceRegistryRef services, const std::string& name, T fallbackNumber = 0)
 {
   T result = 0;
 
@@ -59,6 +61,7 @@ T computeActivityField(framework::ServiceRegistryRef services, const std::string
   return result;
 }
 
+std::string_view translateIntegerRunType(const std::string& runType);
 Activity computeActivity(framework::ServiceRegistryRef services, const Activity& fallbackActivity);
 
 std::string indentTree(int level);

diff --git a/Framework/include/QualityControl/stringUtils.h b/Framework/include/QualityControl/stringUtils.h
@@ -40,6 +40,11 @@ bool decodeBool(const std::string& value);
 /// @throw std::runtime_error the value is not a bool
 bool parseBoolParam(const CustomParameters& customParameters, const std::string& name, const std::string& runType = "default", const std::string& beamType = "default");
 
+/**
+ * Check if the string contains only digits.
+ */
+bool isUnsignedInteger(const std::string& s);
+
 } // namespace o2::quality_control::core
 
 #endif // QC_STRING_UTILS_H
diff --git a/Framework/multiNode.json b/Framework/multiNode.json
@@ -10,7 +10,7 @@
       },
       "Activity": {
         "number": "42",
-        "type": "2"
+        "type": "NONE"
       },
       "monitoring": {
         "url": "infologger:///debug?qc"

diff --git a/Framework/multinode-test.json.in b/Framework/multinode-test.json.in
@@ -10,7 +10,7 @@
       },
       "Activity": {
         "number": "42",
-        "type": "2",
+        "type": "NONE",
         "start": "8000000",
         "end": "9000000"
       },

diff --git a/Framework/postprocessing.json b/Framework/postprocessing.json
@@ -10,7 +10,7 @@
       },
       "Activity": {
         "number": "42",
-        "type": "2"
+        "type": "NONE"
       },
       "monitoring": {
         "url": "infologger:///debug?qc"

diff --git a/Framework/readout-no-sampling.json b/Framework/readout-no-sampling.json
@@ -10,7 +10,7 @@
       },
       "Activity": {
         "number": "42",
-        "type": "2"
+        "type": "NONE"
       },
       "monitoring": {
         "url": "infologger:///debug?qc"

diff --git a/Framework/readout.json b/Framework/readout.json
@@ -10,7 +10,7 @@
       },
       "Activity": {
         "number": "42",
-        "type": "2"
+        "type": "NONE"
       },
       "monitoring": {
 	    "url": "infologger:///debug?qc"

diff --git a/Framework/script/o2-qc-batch-test.sh b/Framework/script/o2-qc-batch-test.sh
@@ -69,7 +69,7 @@ o2-qc --config json:/${JSON_DIR}/batch-test.json --remote-batch /tmp/batch_test_
 code=$(curl -L ccdb-test.cern.ch:8080/qc/TST/MO/BatchTestTask${UNIQUE_ID}/example/8000000/PeriodName=LHC9000x/PassName=apass500 --write-out %{http_code} --silent --output /tmp/batch_test_obj${UNIQUE_ID}.root)
 if (( $code != 200 )); then
   echo "Error, monitor object of the QC Task could not be found."
-  delete_data
+#  delete_data
   exit 3
 fi
 # try to check that we got a valid root object
@@ -94,22 +94,22 @@ code=$(curl -L ccdb-test.cern.ch:8080/qc/TST/MO/BatchTestTask${UNIQUE_ID}/mw/exa
 if (( $code != 200 )); then
   echo "Error, monitor object of the QC Task could not be found."
   delete_data
-  exit 3
+  exit 6
 fi
 # try to check that we got a valid root object
 root -b -l -q -e 'TFile f("/tmp/batch_test_obj_mw${UNIQUE_ID}.root"); f.Print();'
 if (( $? != 0 )); then
   echo "Error, monitor object of the QC Task is invalid."
   delete_data
-  exit 4
+  exit 7
 fi
 # try if it is a non empty histogram
 entries=`root -b -l -q -e 'TFile f("/tmp/batch_test_obj_mw${UNIQUE_ID}.root"); TH1F *h = (TH1F*)f.Get("ccdb_object"); cout << h->GetEntries() << endl;' | tail -n 1`
 if [ $entries -lt 225 ] 2>/dev/null
 then
   echo "The histogram of the QC Task has less than 225 (75%) of expected samples."
   delete_data
-  exit 5
+  exit 8
 fi
 
 # check QualityObject
@@ -118,7 +118,7 @@ code=$(curl -L ccdb-test.cern.ch:8080/qc/TST/QO/BatchTestCheck${UNIQUE_ID}/80000
 if (( $code != 200 )); then
   echo "Error, quality object of the QC Task could not be found."
   delete_data
-  exit 6
+  exit 9
 fi
 
 echo "Batch test passed."

diff --git a/Framework/src/Activity.cxx b/Framework/src/Activity.cxx
@@ -46,7 +46,7 @@ bool Activity::matches(const Activity& other) const
   // is included in this validity. If checked for any overlaps, we would match with all past Activities, which is not
   // what we want e.g. in Post-processing triggers. Once we indicate the correct validity, we can change this behaviour.
   return (mId == 0 || mId == other.mId) &&
-         (mType == 0 || mType == other.mType) &&
+         (mType == "NONE" || mType == other.mType) &&
          (mPeriodName.empty() || mPeriodName == other.mPeriodName) &&
          (mPassName.empty() || mPassName == other.mPassName) &&
          (mProvenance == other.mProvenance) && // provenance has to match!

diff --git a/Framework/src/ActivityHelpers.cxx b/Framework/src/ActivityHelpers.cxx
@@ -20,6 +20,10 @@
 #include <CCDB/BasicCCDBManager.h>
 #include "QualityControl/ObjectMetadataKeys.h"
 
+#include <DataFormatsParameters/ECSDataAdapters.h>
+#include <QualityControl/stringUtils.h>
+#include <QualityControl/runnerUtils.h>
+
 using namespace o2::quality_control::repository;
 
 namespace o2::quality_control::core::activity_helpers
@@ -28,10 +32,8 @@ namespace o2::quality_control::core::activity_helpers
 std::map<std::string, std::string> asDatabaseMetadata(const core::Activity& activity, bool putDefault)
 {
   std::map<std::string, std::string> metadata;
-  if (putDefault || activity.mType != 0) {
-    // TODO should we really treat 0 as none?
-    //  we could consider making Activity use std::optional to be clear about this
-    metadata[metadata_keys::runType] = std::to_string(activity.mType);
+  if (putDefault || activity.mType != "NONE") {
+    metadata[metadata_keys::runType] = activity.mType;
   }
   if (putDefault || activity.mId != 0) {
     metadata[metadata_keys::runNumber] = std::to_string(activity.mId);
@@ -49,7 +51,12 @@ core::Activity asActivity(const std::map<std::string, std::string>& metadata, co
 {
   core::Activity activity;
   if (auto runType = metadata.find(metadata_keys::runType); runType != metadata.end()) {
-    activity.mType = std::strtol(runType->second.c_str(), nullptr, 10);
+    if (isUnsignedInteger(runType->second)) {
+      // we probably got the former representation of run types, i.e. an integer. We convert it as best as we can.
+      activity.mType = translateIntegerRunType(runType->second);
+    } else {
+      activity.mType = runType->second;
+    }
   }
   if (auto runNumber = metadata.find(metadata_keys::runNumber); runNumber != metadata.end()) {
     activity.mId = std::strtol(runNumber->second.c_str(), nullptr, 10);
@@ -73,8 +80,14 @@ core::Activity asActivity(const std::map<std::string, std::string>& metadata, co
 core::Activity asActivity(const boost::property_tree::ptree& tree, const std::string& provenance)
 {
   core::Activity activity;
-  if (auto runType = tree.get_optional<int>(metadata_keys::runType); runType.has_value()) {
-    activity.mType = runType.value();
+  if (auto runType = tree.get_optional<std::string>(metadata_keys::runType); runType.has_value()) {
+    if (isUnsignedInteger(runType.value())) {
+      // we probably got the former representation of run types, i.e. an integer. We convert it as best
+      // as we can using O2's ECSDataAdapter
+      activity.mType = parameters::GRPECS::RunTypeNames[std::stoi(runType.value())];
+    } else {
+      activity.mType = runType.value();
+    }
   }
   if (auto runNumber = tree.get_optional<int>(metadata_keys::runNumber); runNumber.has_value()) {
     activity.mId = runNumber.value();

diff --git a/Framework/src/CustomParameters.cxx b/Framework/src/CustomParameters.cxx
@@ -71,13 +71,7 @@ std::string CustomParameters::at(const std::string& key, const std::string& runT
 
 std::string CustomParameters::at(const std::string& key, const Activity& activity) const
 {
-  // Get the proper parameter for the given activity
-  const int runType = activity.mType; // get the type for this run
-  // convert it to a string (via a string_view as this is what we get from O2)
-  const std::string_view runTypeStringView = o2::parameters::GRPECS::RunTypeNames[runType];
-  const std::string runTypeString{ runTypeStringView };
-  // get the param
-  return at(key, runTypeString, activity.mBeamType);
+  return at(key, activity.mType, activity.mBeamType);
 }
 
 std::optional<std::string> CustomParameters::atOptional(const std::string& key, const std::string& runType, const std::string& beamType) const
@@ -99,13 +93,7 @@ std::optional<std::string> CustomParameters::atOptional(const std::string& key,
 
 std::optional<std::string> CustomParameters::atOptional(const std::string& key, const Activity& activity) const
 {
-  // Get the proper parameter for the given activity
-  const int runType = activity.mType; // get the type for this run
-  // convert it to a string (via a string_view as this is what we get from O2)
-  const std::string_view runTypeStringView = o2::parameters::GRPECS::RunTypeNames[runType];
-  const std::string runTypeString{ runTypeStringView };
-  // get the param
-  return atOptional(key, runTypeString, activity.mBeamType);
+  return atOptional(key, activity.mType, activity.mBeamType);
 }
 
 std::string CustomParameters::atOrDefaultValue(const std::string& key, std::string defaultValue, const std::string& runType, const std::string& beamType)
@@ -120,12 +108,7 @@ std::string CustomParameters::atOrDefaultValue(const std::string& key, std::stri
 std::string CustomParameters::atOrDefaultValue(const std::string& key, std::string defaultValue, const Activity& activity) const
 {
   try {
-    // Get the proper parameter for the given activity
-    const int runType = activity.mType; // get the type for this run as an int
-    // convert it to a string (via a string_view as this is what we get from O2)
-    const std::string_view runTypeStringView = o2::parameters::GRPECS::RunTypeNames[runType];
-    const std::string runTypeString{ runTypeStringView };
-    return mCustomParameters.at(runTypeString).at(activity.mBeamType).at(key);
+    return mCustomParameters.at(activity.mType).at(activity.mBeamType).at(key);
   } catch (const std::out_of_range& exc) {
     return defaultValue;
   }

diff --git a/Framework/src/InfrastructureSpecReader.cxx b/Framework/src/InfrastructureSpecReader.cxx
@@ -59,7 +59,7 @@ CommonSpec InfrastructureSpecReader::readSpecEntry<CommonSpec>(const std::string
     spec.database.emplace(key, value.get_value<std::string>());
   }
   spec.activityNumber = commonTree.get<int>("Activity.number", spec.activityNumber);
-  spec.activityType = commonTree.get<int>("Activity.type", spec.activityType);
+  spec.activityType = commonTree.get<std::string>("Activity.type", spec.activityType);
   spec.activityPassName = commonTree.get<std::string>("Activity.passName", spec.activityPassName);
   spec.activityPeriodName = commonTree.get<std::string>("Activity.periodName", spec.activityPeriodName);
   spec.activityProvenance = commonTree.get<std::string>("Activity.provenance", spec.activityProvenance);