[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
AliceO2Group · May 2, 2024 · 7191a3e · 7191a3e
1 parent cdcc035
commit 7191a3e
Show file tree

Hide file tree

Showing 143 changed files with 394 additions and 254 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/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);

diff --git a/Framework/src/MonitorObject.cxx b/Framework/src/MonitorObject.cxx
@@ -42,7 +42,7 @@ MonitorObject::MonitorObject(TObject* object, const std::string& taskName, const
     mTaskName(taskName),
     mTaskClass(taskClass),
     mDetectorName(detectorName),
-    mActivity(runNumber, 0, periodName, passName, provenance, gInvalidValidityInterval),
+    mActivity(runNumber, "NONE", periodName, passName, provenance, gInvalidValidityInterval),
     mIsOwner(true)
 {
 }

diff --git a/Framework/src/PostProcessingConfig.cxx b/Framework/src/PostProcessingConfig.cxx
@@ -31,7 +31,7 @@ PostProcessingConfig::PostProcessingConfig(const std::string& id, const boost::p
     ccdbUrl(config.get<std::string>("qc.config.conditionDB.url", "")),
     consulUrl(config.get<std::string>("qc.config.consul.url", "")),
     activity(config.get<int>("qc.config.Activity.number", 0),
-             config.get<int>("qc.config.Activity.type", 0),
+             config.get<std::string>("qc.config.Activity.type", "NONE"),
              config.get<std::string>("qc.config.Activity.periodName", ""),
              config.get<std::string>("qc.config.Activity.passName", ""),
              config.get<std::string>("qc.config.Activity.provenance", "qc"),

diff --git a/Framework/src/QualityObject.cxx b/Framework/src/QualityObject.cxx
@@ -35,7 +35,7 @@ QualityObject::QualityObject(
     mPolicyName{ std::move(policyName) },
     mInputs{ std::move(inputs) },
     mMonitorObjectsNames{ std::move(monitorObjectsNames) },
-    mActivity(runNumber, 0, "", "", "qc", gInvalidValidityInterval)
+    mActivity(runNumber, "NONE", "", "", "qc", gInvalidValidityInterval)
 {
   mQuality.addMetadata(std::move(metadata));
 }