From 56b38d043123d40e75e01d112712b3657f977b33 Mon Sep 17 00:00:00 2001 From: Anton Alkin Date: Thu, 1 Oct 2026 10:32:58 +0200 Subject: [PATCH 1/7] Fix leaks and self-assignment in Array2D The copy and move assignment operators overwrote the data pointer without releasing the previous buffer, leaking it on every assignment to a non-empty array Copy self-assignment also allocated the new buffer before reading from the source, so the array was filled from its own uninitialised memory. Now guards against self-assignment and releases the old buffer. --- Framework/Core/include/Framework/Array2D.h | 22 +++++--- Framework/Core/test/test_Variants.cxx | 58 ++++++++++++++++++++++ 2 files changed, 74 insertions(+), 6 deletions(-) diff --git a/Framework/Core/include/Framework/Array2D.h b/Framework/Core/include/Framework/Array2D.h index 857e4b3c89f29..de877797105c9 100644 --- a/Framework/Core/include/Framework/Array2D.h +++ b/Framework/Core/include/Framework/Array2D.h @@ -79,19 +79,29 @@ struct Array2D { Array2D& operator=(Array2D const& other) { - this->rows = other.rows; - this->cols = other.cols; - data = new T[rows * cols]; - for (auto i = 0U; i < rows; ++i) { - for (auto j = 0U; j < cols; ++j) { - data[i * cols + j] = *(other.data + (i * cols + j)); + if (this == &other) { + return *this; + } + // Copy into a new buffer first, so that a throwing copy leaves this array untouched + auto* newData = new T[other.rows * other.cols]; + for (auto i = 0U; i < other.rows; ++i) { + for (auto j = 0U; j < other.cols; ++j) { + newData[i * other.cols + j] = other.data[i * other.cols + j]; } } + delete[] data; + data = newData; + this->rows = other.rows; + this->cols = other.cols; return *this; } Array2D& operator=(Array2D&& other) { + if (this == &other) { + return *this; + } + delete[] data; this->rows = other.rows; this->cols = other.cols; data = other.data; diff --git a/Framework/Core/test/test_Variants.cxx b/Framework/Core/test/test_Variants.cxx index da1f39f241e96..c47a54eb68e62 100644 --- a/Framework/Core/test/test_Variants.cxx +++ b/Framework/Core/test/test_Variants.cxx @@ -229,6 +229,64 @@ TEST_CASE("Array2DTest") } } +TEST_CASE("Array2DAssignmentTest") +{ + std::vector v = {1, 2, 3, 4, 5, 6}; + Array2D source(v, 2, 3); + + // copy assignment to an array of different shape makes an independent deep copy + Array2D copied(std::vector{7}, 1, 1); + copied = source; + REQUIRE(copied.rows == 2); + REQUIRE(copied.cols == 3); + REQUIRE(copied.data != source.data); + source[0][0] = 42; + REQUIRE(copied(0, 0) == 1); + for (auto i = 1U; i < 6; ++i) { + REQUIRE(copied(i / 3, i % 3) == v[i]); + } + + // self copy assignment keeps the contents + auto& self = copied; + copied = self; + REQUIRE(copied.rows == 2); + REQUIRE(copied.cols == 3); + REQUIRE(copied(1, 2) == 6); + + // move assignment takes over the buffer and empties the source + auto* buffer = copied.data; + Array2D moved(std::vector{7, 8}, 1, 2); + moved = std::move(copied); + REQUIRE(moved.data == buffer); + REQUIRE(moved.rows == 2); + REQUIRE(moved.cols == 3); + REQUIRE(moved(1, 2) == 6); + REQUIRE(copied.data == nullptr); + REQUIRE(copied.rows == 0); + REQUIRE(copied.cols == 0); + + // self move assignment keeps the contents + auto& selfMoved = moved; + moved = std::move(selfMoved); + REQUIRE(moved.data == buffer); + REQUIRE(moved(1, 2) == 6); + + // assigning an empty array releases the old buffer and leaves an empty array + moved = Array2D{}; + REQUIRE(moved.data == nullptr); + REQUIRE(moved.rows == 0); + REQUIRE(moved.cols == 0); + + // strings are copied element by element + std::vector s = {"one", "two"}; + Array2D ms(s, 2, 1); + Array2D msc; + msc = ms; + ms[0][0] = "changed"; + REQUIRE(msc(0, 0) == "one"); + REQUIRE(msc(1, 0) == "two"); +} + TEST_CASE("LabeledArrayTest") { float m[3][4] = {{0.1, 0.2, 0.3, 0.4}, {0.5, 0.6, 0.7, 0.8}, {0.9, 1.0, 1.1, 1.2}}; From eff3ac14c96017910fcfb6aab74f4ce9dd4ddbbd Mon Sep 17 00:00:00 2001 From: Anton Alkin Date: Thu, 1 Oct 2026 10:44:39 +0200 Subject: [PATCH 2/7] Fix ownership of the content of Variant Copy and move of Variants holding Array2D, LabeledArray or string array values copied the storage byte by byte, so copies shared the same heap buffers, and the destructor never destroyed these objects, leaking them. Assignments never released the previous content, move assignment of strings and arrays duplicated the buffer and dropped the original one, and move assignment of string arrays corrupted the source vector. --- Framework/Core/include/Framework/Variant.h | 42 ++-- Framework/Core/src/Variant.cxx | 232 ++++++++++++--------- Framework/Core/test/test_Variants.cxx | 168 ++++++++++++++- 3 files changed, 319 insertions(+), 123 deletions(-) diff --git a/Framework/Core/include/Framework/Variant.h b/Framework/Core/include/Framework/Variant.h index 7121c5fad0669..3611fad438e0a 100644 --- a/Framework/Core/include/Framework/Variant.h +++ b/Framework/Core/include/Framework/Variant.h @@ -13,6 +13,7 @@ #include "Framework/RuntimeError.h" #include "Framework/Array2D.h" +#include "Framework/Traits.h" #include #include #include @@ -261,9 +262,9 @@ struct variant_helper { new (reinterpret_cast(store)) T{}; *(reinterpret_cast(store)) = value; } - static void set(void* store, T values, size_t size) + static void set(void* store, std::remove_pointer_t const* values, size_t size) { - *reinterpret_cast(store) = reinterpret_cast(std::memcpy(std::malloc(size * sizeof(std::remove_pointer_t)), reinterpret_cast(values), size * sizeof(std::remove_pointer_t))); + *reinterpret_cast(store) = reinterpret_cast(std::memcpy(std::malloc(size * sizeof(std::remove_pointer_t)), reinterpret_cast(values), size * sizeof(std::remove_pointer_t))); } static T get(const void* store) { return *(reinterpret_cast(store)); } @@ -317,9 +318,10 @@ struct variant_helper { class Variant { public: - Variant(VariantType type = VariantType::Unknown) : mType{type} {} + Variant(VariantType type = VariantType::Unknown); template + requires(!is_specialization_v) Variant(T value) : mType{variant_trait_v} { variant_helper::set(&mStore, value); @@ -331,17 +333,23 @@ class Variant variant_helper::set(&mStore, values, mSize); } + // A Variant owns what it stores: the content of vectors is copied template - Variant(std::vector& values) : mType{variant_trait_v}, mSize{values.size()} + Variant(std::vector const& values) : mType{variant_trait_v}, mSize{values.size()} { variant_helper::set(&mStore, values.data(), mSize); } - Variant(std::vector& values) : mType{VariantType::ArrayString}, mSize{values.size()} + Variant(std::vector const& values) : mType{VariantType::ArrayString}, mSize{values.size()} { variant_helper>::set(&mStore, values); } + // A temporary vector cannot hand over its buffer to a Variant, + // so creating one only to copy it is an error + template + Variant(std::vector&&) = delete; + template Variant(std::initializer_list) { @@ -355,6 +363,8 @@ class Variant ~Variant(); Variant& operator=(const Variant& other); Variant& operator=(Variant&& other) noexcept; + // Assignment from a temporary vector has to be allowed because it is used, but copies + // to make sure Variant owns its content template Variant& operator=(std::vector&& other) noexcept { @@ -381,31 +391,25 @@ class Variant return variant_helper::get(&mStore); } + // The setters replace the current content, releasing it first. template void set(T value) { - return variant_helper::set(&mStore, value); + *this = Variant(value); } template void set(T value, size_t size) { - mSize = size; - return variant_helper::set(&mStore, value, mSize); - } - - template - void set(std::vector& values) - requires(std::is_pod_v) - { - return variant_helper::set(&mStore, values.data(), values.size()); + *this = Variant(value, size); } + /// FIXME: set for vector of strings is not used anywhere, why? template void set(std::vector& values) - requires(std::is_same_v) + requires(std::is_pod_v || std::is_same_v) { - return variant_helper::set(&mStore, values); + *this = Variant(values); } [[nodiscard]] VariantType type() const { return mType; } @@ -414,6 +418,10 @@ class Variant private: friend std::ostream& operator<<(std::ostream& oss, Variant const& val); + // Helpers to manage the store depending on the actual content + void copyStore(Variant const& other); + void moveStore(Variant& other) noexcept; + void destroyStore() noexcept; using storage_t = std::aligned_union<8, int, int8_t, int16_t, int64_t, uint8_t, uint16_t, uint32_t, uint64_t, const char*, float, double, bool, diff --git a/Framework/Core/src/Variant.cxx b/Framework/Core/src/Variant.cxx index e54a973bd4413..f780a1a952f1b 100644 --- a/Framework/Core/src/Variant.cxx +++ b/Framework/Core/src/Variant.cxx @@ -11,7 +11,10 @@ #include "Framework/Variant.h" #include "Framework/VariantPropertyTreeHelpers.h" #include "Framework/VariantJSONHelpers.h" +#include +#include #include +#include #include namespace o2::framework @@ -84,145 +87,166 @@ std::string Variant::asString() const return ss.str(); } -Variant::Variant(const Variant& other) : mType(other.mType) +namespace { - // In case this is an array we need to duplicate it to avoid - // double deletion. - switch (mType) { - case VariantType::String: - mSize = other.mSize; - variant_helper::set(&mStore, other.get()); - return; - case VariantType::ArrayInt: - mSize = other.mSize; - variant_helper::set(&mStore, other.get(), mSize); - return; - case VariantType::ArrayFloat: - mSize = other.mSize; - variant_helper::set(&mStore, other.get(), mSize); - return; - case VariantType::ArrayDouble: - mSize = other.mSize; - variant_helper::set(&mStore, other.get(), mSize); - return; - case VariantType::ArrayBool: - mSize = other.mSize; - variant_helper::set(&mStore, other.get(), mSize); - return; +/// Helper visitor for Variant +template +bool visitStoredObject(VariantType type, F&& f) +{ + switch (type) { case VariantType::ArrayString: - mSize = other.mSize; - variant_helper>::set(&mStore, other.get>()); - return; + f.template operator()>(); + return true; + case VariantType::Array2DInt: + f.template operator()>(); + return true; + case VariantType::Array2DFloat: + f.template operator()>(); + return true; + case VariantType::Array2DDouble: + f.template operator()>(); + return true; + case VariantType::LabeledArrayInt: + f.template operator()>(); + return true; + case VariantType::LabeledArrayFloat: + f.template operator()>(); + return true; + case VariantType::LabeledArrayDouble: + f.template operator()>(); + return true; + case VariantType::LabeledArrayString: + f.template operator()>(); + return true; default: - mStore = other.mStore; - mSize = other.mSize; + return false; } } -Variant::Variant(Variant&& other) noexcept : mType(other.mType) +/// Types for which the storage keeps a pointer to a manually allocated buffer +bool holdsMallocedPointer(VariantType type) { - mStore = other.mStore; - mSize = other.mSize; - switch (mType) { + switch (type) { case VariantType::String: case VariantType::ArrayInt: case VariantType::ArrayFloat: case VariantType::ArrayDouble: case VariantType::ArrayBool: - case VariantType::ArrayString: - *reinterpret_cast(&(other.mStore)) = nullptr; - return; + return true; default: - return; + return false; } } -Variant::~Variant() +template +T* copyBuffer(T const* values, size_t size) +{ + if (values == nullptr) { + return nullptr; + } + return reinterpret_cast(std::memcpy(std::malloc(size * sizeof(T)), values, size * sizeof(T))); +} +} // namespace + +Variant::Variant(VariantType type) : mType{type} +{ + // Make sure that destroying a Variant created without a value is always safe + // by creating a default stored object upfront + if (!visitStoredObject(mType, [this]() { new (&mStore) T{}; })) { + std::memset(&mStore, 0, sizeof(mStore)); + } +} + +void Variant::copyStore(Variant const& other) { - // In case we allocated an array, we - // should delete it. + // Proper objects are simply copied + if (visitStoredObject(mType, [this, &other]() { new (&mStore) T(*reinterpret_cast(&other.mStore)); })) { + return; + } + // Manually allocated buffers have to be managed switch (mType) { - case VariantType::String: + case VariantType::String: { + auto const* value = *reinterpret_cast(&other.mStore); + *reinterpret_cast(&mStore) = value != nullptr ? strdup(value) : nullptr; + return; + } case VariantType::ArrayInt: + *reinterpret_cast(&mStore) = copyBuffer(*reinterpret_cast(&other.mStore), mSize); + return; case VariantType::ArrayFloat: + *reinterpret_cast(&mStore) = copyBuffer(*reinterpret_cast(&other.mStore), mSize); + return; case VariantType::ArrayDouble: - case VariantType::ArrayBool: { - if (reinterpret_cast(&mStore) != nullptr) { - free(*reinterpret_cast(&mStore)); - } + *reinterpret_cast(&mStore) = copyBuffer(*reinterpret_cast(&other.mStore), mSize); return; - } - case VariantType::ArrayString: { - // Allocated with placement new. Nothing to delete. + case VariantType::ArrayBool: + *reinterpret_cast(&mStore) = copyBuffer(*reinterpret_cast(&other.mStore), mSize); return; - } default: - return; + // Trivially copyable content + mStore = other.mStore; + } +} + +void Variant::moveStore(Variant& other) noexcept +{ + // Correct move for objects, leaving proper "moved from" state + if (visitStoredObject(mType, [this, &other]() { new (&mStore) T(std::move(*reinterpret_cast(&other.mStore))); })) { + return; + } + mStore = other.mStore; + // Buffers have to change their owner + if (holdsMallocedPointer(mType)) { + *reinterpret_cast(&other.mStore) = nullptr; + } +} + +void Variant::destroyStore() noexcept +{ + // destroy objects + if (visitStoredObject(mType, [this]() { std::destroy_at(reinterpret_cast(&mStore)); })) { + return; + } + // deallocate buffers + if (holdsMallocedPointer(mType)) { + free(*reinterpret_cast(&mStore)); } } +Variant::Variant(const Variant& other) : mType(other.mType), mSize(other.mSize) +{ + copyStore(other); +} + +Variant::Variant(Variant&& other) noexcept : mType(other.mType), mSize(other.mSize) +{ + moveStore(other); +} + +Variant::~Variant() +{ + destroyStore(); +} + Variant& Variant::operator=(const Variant& other) { - mSize = other.mSize; - mType = other.mType; - switch (mType) { - case VariantType::String: - variant_helper::set(&mStore, other.get()); - return *this; - case VariantType::ArrayInt: - variant_helper::set(&mStore, other.get(), mSize); - return *this; - case VariantType::ArrayFloat: - variant_helper::set(&mStore, other.get(), mSize); - return *this; - case VariantType::ArrayDouble: - variant_helper::set(&mStore, other.get(), mSize); - return *this; - case VariantType::ArrayBool: - variant_helper::set(&mStore, other.get(), mSize); - return *this; - case VariantType::ArrayString: - variant_helper>::set(&mStore, other.get>()); - return *this; - default: - mStore = other.mStore; - return *this; + if (this != &other) { + // Copy first, so that a throwing copy leaves this Variant untouched + Variant copy(other); + *this = std::move(copy); } + return *this; } Variant& Variant::operator=(Variant&& other) noexcept { - mSize = other.mSize; - mType = other.mType; - switch (mType) { - case VariantType::String: - variant_helper::set(&mStore, other.get()); - *reinterpret_cast(&(other.mStore)) = nullptr; - return *this; - case VariantType::ArrayInt: - variant_helper::set(&mStore, other.get(), mSize); - *reinterpret_cast(&(other.mStore)) = nullptr; - return *this; - case VariantType::ArrayFloat: - variant_helper::set(&mStore, other.get(), mSize); - *reinterpret_cast(&(other.mStore)) = nullptr; - return *this; - case VariantType::ArrayDouble: - variant_helper::set(&mStore, other.get(), mSize); - *reinterpret_cast(&(other.mStore)) = nullptr; - return *this; - case VariantType::ArrayBool: - variant_helper::set(&mStore, other.get(), mSize); - *reinterpret_cast(&(other.mStore)) = nullptr; - return *this; - case VariantType::ArrayString: - variant_helper>::set(&mStore, other.get>()); - *reinterpret_cast**>(&(other.mStore)) = nullptr; - return *this; - default: - mStore = other.mStore; - return *this; + if (this != &other) { + destroyStore(); + mType = other.mType; + mSize = other.mSize; + moveStore(other); } + return *this; } std::pair, std::vector> extractLabels(boost::property_tree::ptree const& tree) diff --git a/Framework/Core/test/test_Variants.cxx b/Framework/Core/test/test_Variants.cxx index c47a54eb68e62..244dd3c9359c5 100644 --- a/Framework/Core/test/test_Variants.cxx +++ b/Framework/Core/test/test_Variants.cxx @@ -287,6 +287,88 @@ TEST_CASE("Array2DAssignmentTest") REQUIRE(msc(1, 0) == "two"); } +TEST_CASE("VariantLifecycleTest") +{ + float m[2][3] = {{1, 2, 3}, {4, 5, 6}}; + LabeledArray laf{&m[0][0], 2, 3, {"r1", "r2"}, {"c1", "c2", "c3"}}; + std::vector vs{"s1", "s2", "s3"}; + auto checkLabeled = [&](Variant const& v) { + REQUIRE(v.type() == VariantType::LabeledArrayFloat); + auto la = v.get>(); + REQUIRE(la.rows() == 2); + REQUIRE(la.cols() == 3); + REQUIRE(la.get("r2", "c3") == 6); + REQUIRE(la.getLabelsRows() == std::vector{"r1", "r2"}); + }; + + Variant vl(laf); + // copies are independent and survive the destruction of the original + auto* copy = new Variant(vl); + Variant moved(std::move(*copy)); + delete copy; + checkLabeled(vl); + checkLabeled(moved); + + // assignment across types releases the previous content + Variant vstr("a string"); + vstr = vl; + checkLabeled(vstr); + vstr = Variant("back to a string"); + REQUIRE(vstr.type() == VariantType::String); + REQUIRE(std::string(vstr.get()) == "back to a string"); + vstr = std::move(moved); + checkLabeled(vstr); + Variant vvs(vs); + vstr = vvs; + REQUIRE(vstr.get>() == vs); + + // self assignment keeps the content + auto& self = vl; + vl = self; + checkLabeled(vl); + vl = std::move(self); + checkLabeled(vl); + + // moving a string array leaves a valid moved-from Variant behind + Variant vvsMoved(std::move(vvs)); + REQUIRE(vvsMoved.get>() == vs); + vvs = vvsMoved; + REQUIRE(vvs.get>() == vs); + + // reallocation of a container moves the Variants around + std::vector collection; + for (auto i = 0; i < 20; ++i) { + collection.emplace_back(laf); + collection.emplace_back(Array2D{&m[0][0], 2, 3}); + collection.emplace_back(vs); + collection.emplace_back("a string"); + } + std::vector collectionCopy = collection; + collection.clear(); + for (auto i = 0U; i < collectionCopy.size(); i += 4) { + checkLabeled(collectionCopy[i]); + REQUIRE(collectionCopy[i + 1].get>()(1, 2) == 6); + REQUIRE(collectionCopy[i + 2].get>() == vs); + REQUIRE(std::string(collectionCopy[i + 3].get()) == "a string"); + } + + // a Variant created with only a type can be copied and destroyed + Variant typed(VariantType::LabeledArrayFloat); + Variant typedCopy(typed); + REQUIRE(typedCopy.get>().rows() == 0); + Variant typedString(VariantType::String); + Variant typedStringCopy(typedString); + REQUIRE(typedStringCopy.get() == nullptr); + + // set replaces the content and the type + Variant vset(1); + vset.set(laf); + checkLabeled(vset); + vset.set(vs); + REQUIRE(vset.type() == VariantType::ArrayString); + REQUIRE(vset.get>() == vs); +} + TEST_CASE("LabeledArrayTest") { float m[3][4] = {{0.1, 0.2, 0.3, 0.4}, {0.5, 0.6, 0.7, 0.8}, {0.9, 1.0, 1.1, 1.2}}; @@ -311,11 +393,14 @@ TEST_CASE("LabeledArrayTest") TEST_CASE("VariantTreeConversionsTest") { std::vector vstrings{"0 1", "0 2", "0 3"}; - Variant vvstr(std::move(vstrings)); + Variant vvstr(vstrings); auto tree = vectorToBranch(vvstr.get(), vvstr.size()); - auto v = Variant(vectorFromBranch(tree)); + auto fromTree = vectorFromBranch(tree); + auto v = Variant(fromTree); + REQUIRE(vvstr.size() == vstrings.size()); + REQUIRE(v.size() == vstrings.size()); for (auto i = 0U; i < vvstr.size(); ++i) { REQUIRE(vvstr.get()[i] == v.get()[i]); } @@ -410,3 +495,82 @@ TEST_CASE("VariantThrowing") REQUIRE(error.what == std::string("Variant::get: Mismatch between types 4 0.")); } } + +// A Variant can be created from a vector only by copying its content +static_assert(!std::is_constructible_v>); +static_assert(!std::is_constructible_v>); +static_assert(!std::is_constructible_v>); +static_assert(!std::is_constructible_v>); +static_assert(!std::is_constructible_v>); +// assignment from a temporary vector copies it +static_assert(std::is_assignable_v>); +static_assert(std::is_assignable_v>); +static_assert(std::is_constructible_v&>); +static_assert(std::is_constructible_v const&>); +static_assert(std::is_constructible_v const&>); +static_assert(std::is_assignable_v const&>); + +namespace +{ +template +void checkVectorCopy(VariantType type, std::vector source) +{ + auto check = [&](Variant const& v, std::vector const& from) { + REQUIRE(v.type() == type); + REQUIRE(v.size() == from.size()); + auto const* stored = v.get(); + REQUIRE(stored != from.data()); + for (auto i = 0U; i < from.size(); ++i) { + REQUIRE(stored[i] == from[i]); + } + }; + std::vector const constSource = source; + Variant fromConst(constSource); + check(fromConst, constSource); + + Variant fromMutable(source); + check(fromMutable, source); + // the Variant owns a copy, unaffected by later changes of the source + auto const original = source; + source[0] = source.back(); + source.push_back(source[0]); + check(fromMutable, original); +} +} // namespace + +TEST_CASE("VariantFromVectorTest") +{ + checkVectorCopy(VariantType::ArrayInt, {1, 2, 3, 4}); + checkVectorCopy(VariantType::ArrayFloat, {0.5f, 1.5f, 2.5f}); + checkVectorCopy(VariantType::ArrayDouble, {1e-3, 1e3}); + checkVectorCopy(VariantType::ArrayString, {"a", "bb", "ccc"}); + + // assignment from a vector copies as well + std::vector vi{7, 8, 9}; + Variant v(1); + v = vi; + REQUIRE(v.type() == VariantType::ArrayInt); + REQUIRE(v.size() == 3); + REQUIRE(v.get() != vi.data()); + REQUIRE(v.get()[2] == 9); + // also from a temporary + auto makeStrings = []() { return std::vector{"x", "y"}; }; + v = makeStrings(); + REQUIRE(v.type() == VariantType::ArrayString); + REQUIRE(v.size() == 2); + REQUIRE(v.get()[1] == "y"); + v = std::vector{1.5, 2.5, 3.5}; + REQUIRE(v.type() == VariantType::ArrayDouble); + REQUIRE(v.size() == 3); + REQUIRE(v.get()[2] == 3.5); + + // a Variant created from a const vector round-trips through JSON + std::vector const vd{0.25, 0.5, 0.75}; + Variant vdv(vd); + std::stringstream is(vdv.asString()); + auto read = VariantJSONHelpers::read(is); + REQUIRE(read.size() == vd.size()); + for (auto i = 0U; i < vd.size(); ++i) { + REQUIRE(read.get()[i] == vd[i]); + } +} From a544cafff2aab8ace3bd7a05f72ad2a03648553a Mon Sep 17 00:00:00 2001 From: Anton Alkin Date: Thu, 1 Oct 2026 12:35:52 +0200 Subject: [PATCH 3/7] Allow passing LabeledArrays as commandline options * Protect from long default values in `--help` output by adding a size limit and manually adding reduced string representation * Use pre-existing to/from JSON conversions to dump and retrieve labeled arrays in command line * Allow passing only values if the labels do not change from defaults * Test options retrieval for the driver and devices --- .../include/Framework/ConfigParamsHelper.h | 24 +++- .../Framework/VariantPropertyTreeHelpers.h | 1 - Framework/Core/src/BoostOptionsRetriever.cxx | 6 +- Framework/Core/src/ConfigParamsHelper.cxx | 24 +++- Framework/Core/src/PropertyTreeHelpers.cxx | 69 +++++---- Framework/Core/src/Variant.cxx | 4 + .../Core/test/test_BoostOptionsRetriever.cxx | 135 ++++++++++++++++++ .../Core/test/test_DeviceSpecHelpers.cxx | 18 ++- .../Core/test/test_FairMQOptionsRetriever.cxx | 43 ++++++ Framework/Core/test/test_Variants.cxx | 44 ++++++ .../Core/test/test_WorkflowSerialization.cxx | 28 +++- 11 files changed, 349 insertions(+), 47 deletions(-) diff --git a/Framework/Core/include/Framework/ConfigParamsHelper.h b/Framework/Core/include/Framework/ConfigParamsHelper.h index 3783cdb374bd1..ceff4f47c4ec1 100644 --- a/Framework/Core/include/Framework/ConfigParamsHelper.h +++ b/Framework/Core/include/Framework/ConfigParamsHelper.h @@ -37,6 +37,14 @@ struct ConfigParamsHelper { options_description& options, boost::program_options::options_description const& vetos = options_description()); + /// Maximum length of a default value shown in the help, so that + /// an option and its description fit into a typical terminal line + static constexpr size_t MaxHelpDefaultLength = 40; + + /// Text to show in the help for the default @a value: values longer than + /// MaxHelpDefaultLength are reduced to their beginning and end, separated by "..." + static std::string defaultValueForHelp(std::string const& value); + /// Check if option is defined static bool hasOption(const std::vector& specs, const std::string& optName); @@ -112,7 +120,12 @@ struct ConfigParamsHelper { using Type = typename variant_type::type; using BoostType = typename std::conditional::type; auto value = boost::program_options::value(); - value = value->default_value(spec.defaultValue.get()); + if constexpr (V == VariantType::String) { + auto defaultValue = spec.defaultValue.get(); + value = value->default_value(defaultValue, defaultValueForHelp(defaultValue)); + } else { + value = value->default_value(spec.defaultValue.get()); + } if constexpr (V == VariantType::Bool) { // for bool values we also support the zero_token option to make // the option usable as a single switch @@ -121,11 +134,18 @@ struct ConfigParamsHelper { options.add_options()(name, value, help); } else if constexpr (isArray() || isArray2D()) { auto value = boost::program_options::value(); - value = value->default_value(spec.defaultValue.asString()); + auto defaultValue = spec.defaultValue.asString(); + value = value->default_value(defaultValue, defaultValueForHelp(defaultValue)); if constexpr (V != VariantType::String) { value = value->multitoken(); } options.add_options()(name, value, help); + } else if constexpr (isLabeledArray()) { + // the JSON representation is passed as a single argument + auto value = boost::program_options::value(); + auto defaultValue = spec.defaultValue.asString(); + value = value->default_value(defaultValue, defaultValueForHelp(defaultValue)); + options.add_options()(name, value, help); } else { using Type = typename variant_type::type; using BoostType = typename std::conditional::type; diff --git a/Framework/Core/include/Framework/VariantPropertyTreeHelpers.h b/Framework/Core/include/Framework/VariantPropertyTreeHelpers.h index 05ab71d39b0e4..afc4a7f989802 100644 --- a/Framework/Core/include/Framework/VariantPropertyTreeHelpers.h +++ b/Framework/Core/include/Framework/VariantPropertyTreeHelpers.h @@ -12,7 +12,6 @@ #define FRAMEWORK_VARIANTPTREEHELPERS_H #include "Array2D.h" -#include "Framework/Variant.h" #include namespace o2::framework diff --git a/Framework/Core/src/BoostOptionsRetriever.cxx b/Framework/Core/src/BoostOptionsRetriever.cxx index fbf56a312cf66..ee559801fdb6e 100644 --- a/Framework/Core/src/BoostOptionsRetriever.cxx +++ b/Framework/Core/src/BoostOptionsRetriever.cxx @@ -91,13 +91,13 @@ void BoostOptionsRetriever::update(std::vector const& specs, case VariantType::Array2DInt: case VariantType::Array2DFloat: case VariantType::Array2DDouble: - options = options(name, bpo::value()->default_value(spec.defaultValue.asString()), help); - break; - case VariantType::Dict: case VariantType::LabeledArrayInt: case VariantType::LabeledArrayFloat: case VariantType::LabeledArrayDouble: case VariantType::LabeledArrayString: + options = options(name, bpo::value()->default_value(spec.defaultValue.asString()), help); + break; + case VariantType::Dict: case VariantType::Unknown: case VariantType::Empty: break; diff --git a/Framework/Core/src/ConfigParamsHelper.cxx b/Framework/Core/src/ConfigParamsHelper.cxx index fc59710fd71a0..77532a3796468 100644 --- a/Framework/Core/src/ConfigParamsHelper.cxx +++ b/Framework/Core/src/ConfigParamsHelper.cxx @@ -13,6 +13,7 @@ #include #include +#include #include #include @@ -99,9 +100,17 @@ void ConfigParamsHelper::populateBoostProgramOptions( addConfigSpecOption(spec, options); break; case VariantType::LabeledArrayInt: + addConfigSpecOption(spec, options); + break; case VariantType::LabeledArrayFloat: + addConfigSpecOption(spec, options); + break; case VariantType::LabeledArrayDouble: + addConfigSpecOption(spec, options); + break; case VariantType::LabeledArrayString: + addConfigSpecOption(spec, options); + break; // FIXME: for Dict we should probably allow parsing stuff // provided on the command line case VariantType::Dict: @@ -112,6 +121,19 @@ void ConfigParamsHelper::populateBoostProgramOptions( } } +std::string ConfigParamsHelper::defaultValueForHelp(std::string const& value) +{ + constexpr std::string_view ellipsis = "..."; + if (value.size() <= MaxHelpDefaultLength) { + return value; + } + // Keep more of the beginning, which identifies the value, and the end, which closes it + constexpr size_t kept = MaxHelpDefaultLength - ellipsis.size(); + constexpr size_t tail = kept / 3; + constexpr size_t head = kept - tail; + return value.substr(0, head) + std::string{ellipsis} + value.substr(value.size() - tail); +} + /// Check if option is defined bool ConfigParamsHelper::hasOption(const std::vector& specs, const std::string& optName) { @@ -158,7 +180,7 @@ bool ConfigParamsHelper::dpl2BoostOptions(const std::vector& sp if (configSpec.type != VariantType::Bool) { if (configSpec.defaultValue.type() != VariantType::Empty) { options.add_options()(configSpec.name.c_str(), - bpo::value()->default_value(defaultValue.str()), + bpo::value()->default_value(defaultValue.str(), defaultValueForHelp(defaultValue.str())), configSpec.help.c_str()); } else { options.add_options()(configSpec.name.c_str(), diff --git a/Framework/Core/src/PropertyTreeHelpers.cxx b/Framework/Core/src/PropertyTreeHelpers.cxx index 18055edb635b4..dc7e6c2130e96 100644 --- a/Framework/Core/src/PropertyTreeHelpers.cxx +++ b/Framework/Core/src/PropertyTreeHelpers.cxx @@ -35,6 +35,32 @@ inline Variant fromString(std::string const& str) ss.str(str); return VariantJSONHelpers::read(ss); } + +template +auto replaceLabels(LabeledArray& input, LabeledArray&& spec) +{ + if (!input.getLabelsCols().empty() || !input.getLabelsRows().empty()) { + return false; + } + if (spec.getLabelsCols().empty() == false) { + input.replaceLabelsCols(spec.getLabelsCols()); + } + if (spec.getLabelsRows().empty() == false) { + input.replaceLabelsRows(spec.getLabelsRows()); + } + return true; +} + +/// Special case for a labeled array to restore default labels that were omitted +template +boost::property_tree::ptree labeledArrayFromStringToBranch(std::string const& str, Variant const& defaultValue) +{ + auto v = fromString(str).template get>(); + if (defaultValue.type() == V) { + replaceLabels(v, defaultValue.get>()); + } + return labeledArrayToBranch(std::move(v)); +} } // namespace void PropertyTreeHelpers::populateDefaults(std::vector const& schema, boost::property_tree::ptree& pt, @@ -233,22 +259,18 @@ void PropertyTreeHelpers::populate(std::vector const& schema, auto v = fromString(vmap[key].as()); pt.put_child(key, array2DToBranch(v.get>())); } break; - case VariantType::LabeledArrayInt: { - auto v = fromString(vmap[key].as()); - pt.put_child(key, labeledArrayToBranch(v.get>())); - } break; - case VariantType::LabeledArrayFloat: { - auto v = fromString(vmap[key].as()); - pt.put_child(key, labeledArrayToBranch(v.get>())); - } break; - case VariantType::LabeledArrayDouble: { - auto v = fromString(vmap[key].as()); - pt.put_child(key, labeledArrayToBranch(v.get>())); - } break; - case VariantType::LabeledArrayString: { - auto v = fromString(vmap[key].as()); - pt.put_child(key, labeledArrayToBranch(v.get>())); - } break; + case VariantType::LabeledArrayInt: + pt.put_child(key, labeledArrayFromStringToBranch(vmap[key].as(), spec.defaultValue)); + break; + case VariantType::LabeledArrayFloat: + pt.put_child(key, labeledArrayFromStringToBranch(vmap[key].as(), spec.defaultValue)); + break; + case VariantType::LabeledArrayDouble: + pt.put_child(key, labeledArrayFromStringToBranch(vmap[key].as(), spec.defaultValue)); + break; + case VariantType::LabeledArrayString: + pt.put_child(key, labeledArrayFromStringToBranch(vmap[key].as(), spec.defaultValue)); + break; case VariantType::Dict: pt.put_child(key, vmap[key].as()); break; @@ -272,21 +294,6 @@ void PropertyTreeHelpers::populate(std::vector const& schema, O2_SIGNPOST_END(configuration, cid, "populate", "Done"); } -template -auto replaceLabels(LabeledArray& input, LabeledArray&& spec) -{ - if (!input.getLabelsCols().empty() || !input.getLabelsRows().empty()) { - return false; - } - if (spec.getLabelsCols().empty() == false) { - input.replaceLabelsCols(spec.getLabelsCols()); - } - if (spec.getLabelsRows().empty() == false) { - input.replaceLabelsRows(spec.getLabelsRows()); - } - return true; -} - void PropertyTreeHelpers::populate(std::vector const& schema, boost::property_tree::ptree& pt, boost::property_tree::ptree const& in, diff --git a/Framework/Core/src/Variant.cxx b/Framework/Core/src/Variant.cxx index f780a1a952f1b..b982ffd22c99e 100644 --- a/Framework/Core/src/Variant.cxx +++ b/Framework/Core/src/Variant.cxx @@ -66,6 +66,10 @@ std::ostream& operator<<(std::ostream& oss, Variant const& val) case VariantType::Array2DInt: case VariantType::Array2DFloat: case VariantType::Array2DDouble: + case VariantType::LabeledArrayInt: + case VariantType::LabeledArrayFloat: + case VariantType::LabeledArrayDouble: + case VariantType::LabeledArrayString: VariantJSONHelpers::write(oss, val); break; case VariantType::Empty: diff --git a/Framework/Core/test/test_BoostOptionsRetriever.cxx b/Framework/Core/test/test_BoostOptionsRetriever.cxx index d2a40ea36b4bd..33869f00d3088 100644 --- a/Framework/Core/test/test_BoostOptionsRetriever.cxx +++ b/Framework/Core/test/test_BoostOptionsRetriever.cxx @@ -11,9 +11,14 @@ #include "Framework/ConfigParamsHelper.h" #include "Framework/ConfigParamSpec.h" +#include "Framework/BoostOptionsRetriever.h" +#include "Framework/ConfigParamRegistry.h" +#include "Framework/ConfigParamStore.h" +#include "Framework/DataProcessorSpec.h" #include #include #include +#include #include using namespace o2::framework; @@ -70,3 +75,133 @@ TEST_CASE("TrivialBoostOptionsRetrieverTest") REQUIRE(vm["someFloat"].as() == 0.5); REQUIRE(vm["someDouble"].as() == 0.5); } + +namespace +{ +constexpr char const* fullLabeledJSON = R"({"labels_rows":["a","b"],"labels_cols":["x","y"],"values":[[5,6],[7,8]]})"; + +LabeledArray defaultLabeledArray() +{ + int m[2][2] = {{1, 2}, {3, 4}}; + return LabeledArray{&m[0][0], 2, 2, {"r1", "r2"}, {"c1", "c2"}}; +} + +std::unique_ptr registryFromCommandLine(std::vector const& specs, std::vector args) +{ + args.insert(args.begin(), "test"); + std::vector> retrievers; + retrievers.emplace_back(std::make_unique(true, args.size(), const_cast(args.data()))); + auto store = std::make_unique(specs, std::move(retrievers)); + store->preload(); + store->activate(); + return std::make_unique(std::move(store)); +} +} // namespace + +TEST_CASE("LabeledArrayBoostOptionsTest") +{ + namespace bpo = boost::program_options; + auto specs = std::vector{ + {"someLabeledArray", VariantType::LabeledArrayInt, defaultLabeledArray(), {"some labeled array option"}}}; + + // the option is registered as a string holding the JSON representation + bpo::options_description opts; + ConfigParamsHelper::populateBoostProgramOptions(opts, specs); + const char* args[] = {"test", "--someLabeledArray", fullLabeledJSON}; + bpo::variables_map vm; + bpo::store(parse_command_line(sizeof(args) / sizeof(char*), args, opts), vm); + bpo::notify(vm); + REQUIRE(vm["someLabeledArray"].as() == fullLabeledJSON); + + // the default is the JSON representation of the default value + bpo::variables_map vmDefault; + const char* noArgs[] = {"test"}; + bpo::store(parse_command_line(1, noArgs, opts), vmDefault); + REQUIRE(vmDefault["someLabeledArray"].as() == specs[0].defaultValue.asString()); +} + +TEST_CASE("LabeledArrayBoostOptionsRetrieverTest") +{ + auto specs = std::vector{ + {"someLabeledArray", VariantType::LabeledArrayInt, defaultLabeledArray(), {"some labeled array option"}}}; + + // full JSON on the command line, its labels are kept + auto registry = registryFromCommandLine(specs, {"--someLabeledArray", fullLabeledJSON}); + auto la = registry->get>("someLabeledArray"); + REQUIRE(la.getLabelsRows() == std::vector{"a", "b"}); + REQUIRE(la.getLabelsCols() == std::vector{"x", "y"}); + REQUIRE(la.get("b", "y") == 8); + + // absent from the command line: the default value + registry = registryFromCommandLine(specs, {}); + la = registry->get>("someLabeledArray"); + REQUIRE(la.get("r2", "c2") == 4); + + // only values on the command line: the default labels are used + registry = registryFromCommandLine(specs, {"--someLabeledArray", R"({"values":[[5,6],[7,8]]})"}); + la = registry->get>("someLabeledArray"); + REQUIRE(la.getLabelsRows() == std::vector{"r1", "r2"}); + REQUIRE(la.getLabelsCols() == std::vector{"c1", "c2"}); + REQUIRE(la.get("r2", "c1") == 7); + + // values which do not match the default labels + REQUIRE_THROWS(registryFromCommandLine(specs, {"--someLabeledArray", R"({"values":[[5,6,7]]})"})); +} + +TEST_CASE("DefaultValueForHelpTest") +{ + auto const max = ConfigParamsHelper::MaxHelpDefaultLength; + REQUIRE(ConfigParamsHelper::defaultValueForHelp("") == ""); + REQUIRE(ConfigParamsHelper::defaultValueForHelp("short") == "short"); + std::string atLimit(max, 'x'); + REQUIRE(ConfigParamsHelper::defaultValueForHelp(atLimit) == atLimit); + + std::string longValue = std::string("BEGIN") + std::string(100, '-') + "END"; + auto shown = ConfigParamsHelper::defaultValueForHelp(longValue); + REQUIRE(shown.size() == max); + REQUIRE(shown.starts_with("BEGIN")); + REQUIRE(shown.ends_with("END")); + REQUIRE(shown.find("...") != std::string::npos); + REQUIRE(ConfigParamsHelper::defaultValueForHelp(std::string(max + 1, 'x')).size() == max); +} + +TEST_CASE("LongDefaultInHelpTest") +{ + namespace bpo = boost::program_options; + std::vector values(30, 12345); + std::vector rows{"row"}; + std::vector cols; + for (auto i = 0U; i < values.size(); ++i) { + cols.push_back("column" + std::to_string(i)); + } + LabeledArray la{values.data(), 1, static_cast(values.size()), rows, cols}; + auto workflowOptions = std::vector{ + {"longLabeledArray", VariantType::LabeledArrayInt, la, {"a labeled array option with a long default"}}}; + auto full = workflowOptions[0].defaultValue.asString(); + REQUIRE(full.size() > ConfigParamsHelper::MaxHelpDefaultLength); + + // the help shows the abbreviated default only + auto description = ConfigParamsHelper::prepareOptionDescriptions(std::vector{}, workflowOptions); + std::stringstream help; + help << description; + REQUIRE(help.str().find(ConfigParamsHelper::defaultValueForHelp(full)) != std::string::npos); + REQUIRE(help.str().find(full) == std::string::npos); + + // the default value used when parsing is still the full one + bpo::options_description forwarded; + ConfigParamsHelper::dpl2BoostOptions(workflowOptions, forwarded); + const char* noArgs[] = {"test"}; + bpo::variables_map vm; + bpo::store(parse_command_line(1, noArgs, forwarded), vm); + REQUIRE(vm["longLabeledArray"].as() == full); + + // also for the options registered for the devices + bpo::options_description deviceOptions; + ConfigParamsHelper::populateBoostProgramOptions(deviceOptions, workflowOptions); + std::stringstream deviceHelp; + deviceHelp << deviceOptions; + REQUIRE(deviceHelp.str().find(full) == std::string::npos); + bpo::variables_map deviceVm; + bpo::store(parse_command_line(1, noArgs, deviceOptions), deviceVm); + REQUIRE(deviceVm["longLabeledArray"].as() == full); +} diff --git a/Framework/Core/test/test_DeviceSpecHelpers.cxx b/Framework/Core/test/test_DeviceSpecHelpers.cxx index 4184be848c5ec..d689e7a2765c4 100644 --- a/Framework/Core/test/test_DeviceSpecHelpers.cxx +++ b/Framework/Core/test/test_DeviceSpecHelpers.cxx @@ -101,12 +101,16 @@ void check(const std::vector& arguments, TEST_CASE("test_prepareArguments") { + int lm[1][2] = {{1, 2}}; std::vector workflowOptions{ - {"foo", VariantType::String, "bar", {"the famous foo option"}}, - {"depth", VariantType::Int, 1, {"number of processors"}}, + {"foo", VariantType::String, "bar", {"charlie foxtrot"}}, + {"depth", VariantType::Int, 1, {"height"}}, + {"labeled", VariantType::LabeledArrayInt, LabeledArray{&lm[0][0], 1, 2, {"r"}, {"c1", "c2"}}, {"a labeled array"}}, }; + std::string labeledDefault = R"({"labels_rows":["r"],"labels_cols":["c1","c2"],"values":[[1,2]]})"; + std::string labeledOther = R"({"labels_rows":["r"],"labels_cols":["c1","c2"],"values":[[3,4]]})"; - auto algorithm = [](ProcessingContext& ctx) {}; + auto algorithm = [](ProcessingContext& /*ctx*/) {}; WorkflowSpec workflow{ {"processor0", @@ -177,6 +181,14 @@ TEST_CASE("test_prepareArguments") matrix["processor0"] = {{"--depth", "2"}, {"--foo", "bar"}, {"--mode", "silly"}}; matrix["processor1"] = {{"--depth", "2"}, {"--foo", "bar"}, {"--mode", "default"}}; check({"--depth", "2", "--processor0", "--mode silly"}, workflowOptions, deviceSpecs, matrix); + + // a LabeledArray workflow option is forwarded as a single JSON argument, defaulted or set + matrix["processor0"] = {{"--depth", "1"}, {"--labeled", labeledDefault}}; + matrix["processor1"] = matrix["processor0"]; + check({}, workflowOptions, deviceSpecs, matrix); + matrix["processor0"] = {{"--depth", "1"}, {"--labeled", labeledOther}}; + matrix["processor1"] = matrix["processor0"]; + check({"--labeled", labeledOther}, workflowOptions, deviceSpecs, matrix); } TEST_CASE("CheckOptionReworking") diff --git a/Framework/Core/test/test_FairMQOptionsRetriever.cxx b/Framework/Core/test/test_FairMQOptionsRetriever.cxx index 11f8acd7534fa..8cf6870e469b5 100644 --- a/Framework/Core/test/test_FairMQOptionsRetriever.cxx +++ b/Framework/Core/test/test_FairMQOptionsRetriever.cxx @@ -13,6 +13,8 @@ #include #include "Framework/FairOptionsRetriever.h" #include "Framework/ConfigParamStore.h" +#include "Framework/ConfigParamsHelper.h" +#include "Framework/VariantPropertyTreeHelpers.h" #include #include @@ -130,3 +132,44 @@ TEST_CASE("TestOptionsDefaults") REQUIRE(store.provenance("aNested.float") == "fairmq"); REQUIRE(store.provenance("aNested.double") == "default"); } + +TEST_CASE("TestFairMQOptionsRetrieverLabeledArray") +{ + int m[2][2] = {{1, 2}, {3, 4}}; + std::vector specs{ + ConfigParamSpec{"aLabeledArray", VariantType::LabeledArrayInt, LabeledArray{&m[0][0], 2, 2, {"r1", "r2"}, {"c1", "c2"}}, {"a labeled array option"}}, + }; + // options registered as for a device + bpo::options_description testOptions("Test options"); + ConfigParamsHelper::populateBoostProgramOptions(testOptions, specs); + + auto storeFromCommandLine = [&](std::vector const& args) { + auto options = std::make_unique(); + options->AddToCmdLineOptions(testOptions); + options->ParseAll(args, true); + std::vector> retrievers; + retrievers.emplace_back(std::make_unique(options.get())); + auto store = std::make_unique(specs, std::move(retrievers)); + store->preload(); + store->activate(); + return labeledArrayFromBranch(store->store().get_child("aLabeledArray")); + }; + + // the full JSON, as forwarded by the driver + auto la = storeFromCommandLine({"cmd", "--aLabeledArray", R"({"labels_rows":["a","b"],"labels_cols":["x","y"],"values":[[5,6],[7,8]]})"}); + REQUIRE(la.getLabelsRows() == std::vector{"a", "b"}); + REQUIRE(la.getLabelsCols() == std::vector{"x", "y"}); + REQUIRE(la.get("a", "y") == 6); + + // no argument: the default + la = storeFromCommandLine({"cmd"}); + REQUIRE(la.get("r1", "c2") == 2); + + // only values: the default labels are used + la = storeFromCommandLine({"cmd", "--aLabeledArray", R"({"values":[[5,6],[7,8]]})"}); + REQUIRE(la.getLabelsRows() == std::vector{"r1", "r2"}); + REQUIRE(la.get("r2", "c2") == 8); + + // values which do not match the default labels + REQUIRE_THROWS(storeFromCommandLine({"cmd", "--aLabeledArray", R"({"values":[[5,6,7]]})"})); +} diff --git a/Framework/Core/test/test_Variants.cxx b/Framework/Core/test/test_Variants.cxx index 244dd3c9359c5..77f6a7d2918e3 100644 --- a/Framework/Core/test/test_Variants.cxx +++ b/Framework/Core/test/test_Variants.cxx @@ -574,3 +574,47 @@ TEST_CASE("VariantFromVectorTest") REQUIRE(read.get()[i] == vd[i]); } } + +namespace +{ +template +void checkLabeledArrayStringRoundTrip(LabeledArray const& la) +{ + Variant v(la); + auto str = v.asString(); + REQUIRE(str.find("undefined") == std::string::npos); + std::stringstream is(str); + auto vr = VariantJSONHelpers::read(is); + REQUIRE(vr.type() == V); + auto lar = vr.template get>(); + REQUIRE(lar.rows() == la.rows()); + REQUIRE(lar.cols() == la.cols()); + REQUIRE(lar.getLabelsRows() == la.getLabelsRows()); + REQUIRE(lar.getLabelsCols() == la.getLabelsCols()); + for (auto i = 0U; i < la.rows(); ++i) { + for (auto j = 0U; j < la.cols(); ++j) { + REQUIRE(lar.get(i, j) == la.get(i, j)); + } + } +} +} // namespace + +TEST_CASE("LabeledArrayAsStringTest") +{ + int mi[2][2] = {{1, 2}, {3, 4}}; + float mf[2][3] = {{0.5, 0.25, 0.125}, {1, 2, 3}}; + double md[1][2] = {{1e-3, 1e10}}; + std::string ms[2][1] = {{"a"}, {"b c"}}; + + checkLabeledArrayStringRoundTrip(LabeledArray{&mi[0][0], 2, 2, {"r1", "r2"}, {"c1", "c2"}}); + checkLabeledArrayStringRoundTrip(LabeledArray{&mf[0][0], 2, 3, {"r1", "r2"}, {"c1", "c2", "c3"}}); + checkLabeledArrayStringRoundTrip(LabeledArray{&md[0][0], 1, 2, {"r1"}, {"c1", "c2"}}); + checkLabeledArrayStringRoundTrip(LabeledArray{&ms[0][0], 2, 1, {"r1", "r2"}, {"c1"}}); + // without labels + checkLabeledArrayStringRoundTrip(LabeledArray{&mi[0][0], 2, 2}); + // labels which need escaping in JSON + checkLabeledArrayStringRoundTrip(LabeledArray{&mf[0][0], 2, 3, {"row \"one\"", "row\\two"}, {"c 1", "c,2", "c:3"}}); + + Variant v(LabeledArray{&mi[0][0], 2, 2, {"r1", "r2"}, {"c1", "c2"}}); + REQUIRE(v.asString() == R"({"labels_rows":["r1","r2"],"labels_cols":["c1","c2"],"values":[[1,2],[3,4]]})"); +} diff --git a/Framework/Core/test/test_WorkflowSerialization.cxx b/Framework/Core/test/test_WorkflowSerialization.cxx index 791c40c326733..638d4e023e83a 100644 --- a/Framework/Core/test/test_WorkflowSerialization.cxx +++ b/Framework/Core/test/test_WorkflowSerialization.cxx @@ -18,6 +18,8 @@ using namespace o2::framework; TEST_CASE("TestVerifyWorkflowSerialization") { using namespace o2::framework; + int lm[2][2] = {{1, 2}, {3, 4}}; + LabeledArray labeled{&lm[0][0], 2, 2, {"r1", "r2"}, {"c1", "c2"}}; WorkflowSpec w0{ // DataProcessorSpec{"A", // {InputSpec{"foo", "A", "COLLISIONCONTEXT", 1, Lifetime::Condition, { @@ -25,34 +27,35 @@ TEST_CASE("TestVerifyWorkflowSerialization") ConfigParamSpec{"bUrl", VariantType::String, "foo/foo", {"Another InputSpec option"}}, // }}}, // {OutputSpec{{"bar"}, "C", "D", 2, Lifetime::Timeframe}}, // - AlgorithmSpec{[](ProcessingContext& ctx) {}}, // + AlgorithmSpec{[](ProcessingContext& /*ctx*/) {}}, // { // ConfigParamSpec{"aInt", VariantType::Int, 0, {"An Int"}}, // ConfigParamSpec{"aFloat", VariantType::Float, 1.3, {"A Float"}}, // ConfigParamSpec{"aBool", VariantType::Bool, true, {"A Bool"}}, // - ConfigParamSpec{"aString", VariantType::String, "some string", {"A String"}}}}, // // + ConfigParamSpec{"aString", VariantType::String, "some string", {"A String"}}, // + ConfigParamSpec{"aLabeledArray", VariantType::LabeledArrayInt, labeled, {"A LabeledArray"}}}}, // // DataProcessorSpec{"B", // {InputSpec{"foo", "C", "D"}}, // { // OutputSpec{{"bar1"}, "E", "F", 0}, // OutputSpec{{"bar2"}, "E", "F", 1}}, // - AlgorithmSpec{[](ProcessingContext& ctx) {}}, // + AlgorithmSpec{[](ProcessingContext& /*ctx*/) {}}, // {}}, // DataProcessorSpec{"C", {}, // { // OutputSpec{{"bar"}, "G", "H"}}, // - AlgorithmSpec{[](ProcessingContext& ctx) {}}, // + AlgorithmSpec{[](ProcessingContext& /*ctx*/) {}}, // {}}, // DataProcessorSpec{"D", {InputSpec{"foo", {"C", "D"}}}, // {OutputSpec{{"bar"}, {"I", "L"}}}, // - AlgorithmSpec{[](ProcessingContext& ctx) {}}, // + AlgorithmSpec{[](ProcessingContext& /*ctx*/) {}}, // {}, // CommonServices::defaultServices(), // {{"label a"}, {"label \"b\""}}, {{"key1", "v\"al'1"}, {"", "val2"}, {"key3", ""}, {"", ""}}}}; std::vector dataProcessorInfoOut{ - {.name = "A", .executable = "test_Framework_test_SerializationWorkflow", .cmdLineArgs = {"foo"}, .workflowOptions = {ConfigParamSpec{"aBool", VariantType::Bool, true, {"A Bool"}}}}, + {.name = "A", .executable = "test_Framework_test_SerializationWorkflow", .cmdLineArgs = {"foo"}, .workflowOptions = {ConfigParamSpec{"aBool", VariantType::Bool, true, {"A Bool"}}, ConfigParamSpec{"aLabeledArray", VariantType::LabeledArrayInt, labeled, {"A LabeledArray"}}}}, {.name = "B", .executable = "test_Framework_test_SerializationWorkflow", .cmdLineArgs = {"b-bar", "bfoof", "fbdbfaso"}}, {.name = "C", .executable = "test_Framework_test_SerializationWorkflow"}, {.name = "D", .executable = "test_Framework_test_SerializationWorkflow"}, @@ -80,6 +83,19 @@ TEST_CASE("TestVerifyWorkflowSerialization") // also check if the conversion to ConcreteDataMatcher is working at import REQUIRE(std::get_if(&w1[0].inputs[0].matcher) != nullptr); + + // LabeledArray options survive the round trip, both as processor and as workflow options + auto checkLabeled = [&](ConfigParamSpec const& spec) { + REQUIRE(spec.name == "aLabeledArray"); + REQUIRE(spec.type == VariantType::LabeledArrayInt); + auto la = spec.defaultValue.get>(); + REQUIRE(la.getLabelsRows() == labeled.getLabelsRows()); + REQUIRE(la.getLabelsCols() == labeled.getLabelsCols()); + REQUIRE(la.get("r2", "c1") == 3); + }; + checkLabeled(w1[0].options.back()); + REQUIRE(dataProcessorInfoIn[0].workflowOptions.size() == 2); + checkLabeled(dataProcessorInfoIn[0].workflowOptions.back()); } /// Test a workflow with a single data processor with a single input From 208e07f0007e309000c4d2c3ef0e34ef47feec98 Mon Sep 17 00:00:00 2001 From: Anton Alkin Date: Thu, 1 Oct 2026 13:40:59 +0200 Subject: [PATCH 4/7] Allow overriding vectors in ConfigParamRegistry by replacing a subtree * Add a code path that uses put_child rather than put for vectors that are stored as subtrees * Add override tests --- Framework/Core/src/ConfigParamRegistry.cxx | 39 ++++++---- .../Core/test/test_ConfigParamRegistry.cxx | 77 +++++++++++++++++++ 2 files changed, 103 insertions(+), 13 deletions(-) diff --git a/Framework/Core/src/ConfigParamRegistry.cxx b/Framework/Core/src/ConfigParamRegistry.cxx index 0e895bc38e063..8c0068a14ca01 100644 --- a/Framework/Core/src/ConfigParamRegistry.cxx +++ b/Framework/Core/src/ConfigParamRegistry.cxx @@ -12,6 +12,8 @@ #include "Framework/VariantPropertyTreeHelpers.h" #include "Framework/Array2D.h" +#include + namespace o2::framework { @@ -83,8 +85,19 @@ T ConfigParamRegistry::get(const char* key) const void ConfigParamRegistry::override(const char* key, ConfigValueType auto const& val) const { + using T = std::decay_t; try { - mStore->store().put(key, val); + auto& store = mStore->store(); + if constexpr (SimpleConfigValueType || StringConfigValueType) { + store.put(key, val); + } else if constexpr (VectorConfigValueType) { + // arrays are stored as subtrees, which have to be replaced as a whole + store.put_child(key, vectorToBranch(val.data(), val.size())); + } else if constexpr (Array2DLike) { + store.put_child(key, array2DToBranch(T(val))); + } else if constexpr (LabeledArrayLike) { + store.put_child(key, labeledArrayToBranch(T(val))); + } } catch (std::exception& e) { throw std::invalid_argument(std::string("failed to store an option: ") + key + " (" + e.what() + ")"); } catch (...) { @@ -143,16 +156,16 @@ template void ConfigParamRegistry::override(const char* key, double const&) cons template void ConfigParamRegistry::override(const char* key, std::string const&) const; template void ConfigParamRegistry::override(const char* key, bool const&) const; -// template void ConfigParamRegistry::override(char const* key, LabeledArray const&) const; -// template void ConfigParamRegistry::override(char const* key, LabeledArray const&) const; -// template void ConfigParamRegistry::override(char const* key, LabeledArray const&) const; -// template void ConfigParamRegistry::override(char const* key, LabeledArray const&) const; -// template void ConfigParamRegistry::override(char const* key, Array2D const&) const; -// template void ConfigParamRegistry::override(char const* key, Array2D const&) const; -// template void ConfigParamRegistry::override(char const* key, Array2D const&) const; -// template void ConfigParamRegistry::override(char const* key, Array2D const&) const; -// template void ConfigParamRegistry::override(char const* key, std::vector const&) const; -// template void ConfigParamRegistry::override(char const* key, std::vector const&) const; -// template void ConfigParamRegistry::override(char const* key, std::vector const&) const; -// template void ConfigParamRegistry::override(char const* key, std::vector const&) const; +template void ConfigParamRegistry::override(char const* key, LabeledArray const&) const; +template void ConfigParamRegistry::override(char const* key, LabeledArray const&) const; +template void ConfigParamRegistry::override(char const* key, LabeledArray const&) const; +template void ConfigParamRegistry::override(char const* key, LabeledArray const&) const; +template void ConfigParamRegistry::override(char const* key, Array2D const&) const; +template void ConfigParamRegistry::override(char const* key, Array2D const&) const; +template void ConfigParamRegistry::override(char const* key, Array2D const&) const; +template void ConfigParamRegistry::override(char const* key, Array2D const&) const; +template void ConfigParamRegistry::override(char const* key, std::vector const&) const; +template void ConfigParamRegistry::override(char const* key, std::vector const&) const; +template void ConfigParamRegistry::override(char const* key, std::vector const&) const; +template void ConfigParamRegistry::override(char const* key, std::vector const&) const; } // namespace o2::framework diff --git a/Framework/Core/test/test_ConfigParamRegistry.cxx b/Framework/Core/test/test_ConfigParamRegistry.cxx index ed3dd95ad2786..0abfc87b23f1f 100644 --- a/Framework/Core/test/test_ConfigParamRegistry.cxx +++ b/Framework/Core/test/test_ConfigParamRegistry.cxx @@ -12,6 +12,8 @@ #include #include "Framework/FairOptionsRetriever.h" #include "Framework/ConfigParamRegistry.h" +#include "Framework/ConfigParamStore.h" +#include "Framework/Array2D.h" #include #include @@ -119,3 +121,78 @@ TEST_CASE("TestConfigParamRegistry") REQUIRE(obj.x == 1); REQUIRE(obj.y == 2.f); } + +TEST_CASE("TestConfigParamRegistryOverride") +{ + float m[3][4] = {{1, 2, 3, 4}, {5, 6, 7, 8}, {9, 10, 11, 12}}; + int l[2][2] = {{1, 2}, {3, 4}}; + // a Variant cannot be created from a temporary vector, so the defaults are named + std::vector defaultVector{1, 2, 3, 4, 5}; + std::vector defaultStringVector{"a", "b", "c"}; + std::vector defaultNestedVector{0.5, 1.5}; + // the store keeps a reference to the specs, which have to outlive it + std::vector specs{ + ConfigParamSpec{"anInt", VariantType::Int, 1, {"an int option"}}, + ConfigParamSpec{"aString", VariantType::String, "foo", {"a string option"}}, + ConfigParamSpec{"aVector", VariantType::ArrayInt, defaultVector, {"an int array option"}}, + ConfigParamSpec{"aStringVector", VariantType::ArrayString, defaultStringVector, {"a string array option"}}, + ConfigParamSpec{"anArray2D", VariantType::Array2DFloat, Array2D{&m[0][0], 3, 4}, {"a 2D array option"}}, + ConfigParamSpec{"aLabeledArray", VariantType::LabeledArrayInt, LabeledArray{&l[0][0], 2, 2, {"r1", "r2"}, {"c1", "c2"}}, {"a labeled array option"}}, + ConfigParamSpec{"aNested.x", VariantType::Int, 2, {"an int option, nested in an object"}}, + ConfigParamSpec{"aNested.vec", VariantType::ArrayDouble, defaultNestedVector, {"a double array option, nested in an object"}}, + }; + auto store = std::make_unique(specs, std::vector>{}); + store->preload(); + store->activate(); + ConfigParamRegistry registry(std::move(store)); + REQUIRE(registry.get>("aVector").size() == 5); + + // leaf values keep working + registry.override("anInt", 10); + REQUIRE(registry.get("anInt") == 10); + + // a shorter vector replaces the whole subtree, no stale elements are left + registry.override("aVector", std::vector{7, 8}); + REQUIRE(registry.get>("aVector") == std::vector{7, 8}); + registry.override("aStringVector", std::vector{"x"}); + REQUIRE(registry.get>("aStringVector") == std::vector{"x"}); + + // a smaller 2D array replaces the whole subtree + float m2[1][2] = {{0.25, 0.75}}; + registry.override("anArray2D", Array2D{&m2[0][0], 1, 2}); + auto a2d = registry.get>("anArray2D"); + REQUIRE(a2d.rows == 1); + REQUIRE(a2d.cols == 2); + REQUIRE(a2d(0, 1) == 0.75f); + + // labels are replaced together with the values + int l2[1][3] = {{5, 6, 7}}; + registry.override("aLabeledArray", LabeledArray{&l2[0][0], 1, 3, {"row"}, {"x", "y", "z"}}); + auto la = registry.get>("aLabeledArray"); + REQUIRE(la.getLabelsRows() == std::vector{"row"}); + REQUIRE(la.getLabelsCols() == std::vector{"x", "y", "z"}); + REQUIRE(la.get("row", "z") == 7); + // an unlabeled value is stored as it is, without merging the previous labels + registry.override("aLabeledArray", LabeledArray{&l[0][0], 2, 2}); + la = registry.get>("aLabeledArray"); + REQUIRE(la.getLabelsRows().empty()); + REQUIRE(la.getLabelsCols().empty()); + REQUIRE(la.get(1U, 0U) == 3); + + // nested subtrees are replaced without touching their siblings + registry.override("aNested.vec", std::vector{2.5}); + REQUIRE(registry.get>("aNested.vec") == std::vector{2.5}); + REQUIRE(registry.get("aNested.x") == 2); + REQUIRE(registry.get("aString") == "foo"); + REQUIRE(registry.get>("aVector") == std::vector{7, 8}); + + // a key which does not exist yet is created + REQUIRE(registry.hasOption("aNewVector") == false); + registry.override("aNewVector", std::vector{1.f, 2.f}); + REQUIRE(registry.hasOption("aNewVector")); + REQUIRE(registry.get>("aNewVector") == std::vector{1.f, 2.f}); + + // a leaf value can be replaced by a subtree + registry.override("aString", std::vector{"now", "an", "array"}); + REQUIRE(registry.get>("aString") == std::vector{"now", "an", "array"}); +} From b8e644177cc1d07468a9f8cd26b1e09ec8247ce6 Mon Sep 17 00:00:00 2001 From: Anton Alkin Date: Fri, 2 Oct 2026 10:50:45 +0200 Subject: [PATCH 5/7] Allow erasing parameter subtrees in ConfigParamRegistry --- .../include/Framework/ConfigParamRegistry.h | 3 + Framework/Core/src/ConfigParamRegistry.cxx | 20 ++++++ .../Core/test/test_ConfigParamRegistry.cxx | 61 +++++++++++++++++++ 3 files changed, 84 insertions(+) diff --git a/Framework/Core/include/Framework/ConfigParamRegistry.h b/Framework/Core/include/Framework/ConfigParamRegistry.h index b6902a2316997..42bde713039e3 100644 --- a/Framework/Core/include/Framework/ConfigParamRegistry.h +++ b/Framework/Core/include/Framework/ConfigParamRegistry.h @@ -87,6 +87,9 @@ class ConfigParamRegistry void override(const char* key, ConfigValueType auto const& val) const; + // Remove an option (a leaf or a whole subtree) from the active store, no-op if it is missing + void erase(const char* key) const; + // Load extra parameters discovered while we process data void loadExtra(std::vector& extras); diff --git a/Framework/Core/src/ConfigParamRegistry.cxx b/Framework/Core/src/ConfigParamRegistry.cxx index 8c0068a14ca01..4521bd693fe57 100644 --- a/Framework/Core/src/ConfigParamRegistry.cxx +++ b/Framework/Core/src/ConfigParamRegistry.cxx @@ -12,6 +12,7 @@ #include "Framework/VariantPropertyTreeHelpers.h" #include "Framework/Array2D.h" +#include #include namespace o2::framework @@ -69,6 +70,19 @@ auto getImpl(boost::property_tree::ptree& tree, const char* key) { return labeledArrayFromBranch(tree.get_child(key)); } + +// ptree::erase only removes direct children, so the parent of a nested key has to be looked up first +void eraseImpl(boost::property_tree::ptree& tree, std::string_view key) +{ + auto pos = key.rfind('.'); + if (pos == std::string_view::npos) { + tree.erase(std::string(key)); + return; + } + if (auto parent = tree.get_child_optional(std::string(key.substr(0, pos)))) { + parent->erase(std::string(key.substr(pos + 1))); + } +} } // namespace template @@ -105,6 +119,12 @@ void ConfigParamRegistry::override(const char* key, ConfigValueType auto const& } } +void ConfigParamRegistry::erase(const char* key) const +{ + eraseImpl(mStore->store(), key); + eraseImpl(mStore->provenanceTree(), key); +} + // Load extra parameters discovered while we process data void ConfigParamRegistry::loadExtra(std::vector& extras) { diff --git a/Framework/Core/test/test_ConfigParamRegistry.cxx b/Framework/Core/test/test_ConfigParamRegistry.cxx index 0abfc87b23f1f..346ebdb1b3e8d 100644 --- a/Framework/Core/test/test_ConfigParamRegistry.cxx +++ b/Framework/Core/test/test_ConfigParamRegistry.cxx @@ -196,3 +196,64 @@ TEST_CASE("TestConfigParamRegistryOverride") registry.override("aString", std::vector{"now", "an", "array"}); REQUIRE(registry.get>("aString") == std::vector{"now", "an", "array"}); } + +TEST_CASE("TestConfigParamRegistryErase") +{ + int l[2][2] = {{1, 2}, {3, 4}}; + // a Variant cannot be created from a temporary vector, so the defaults are named + std::vector defaultVector{1, 2, 3, 4, 5}; + std::vector defaultStringVector{"a", "b", "c"}; + std::vector defaultNestedVector{0.5, 1.5}; + // the store keeps a reference to the specs, which have to outlive it + std::vector specs{ + ConfigParamSpec{"anInt", VariantType::Int, 1, {"an int option"}}, + ConfigParamSpec{"aString", VariantType::String, "foo", {"a string option"}}, + ConfigParamSpec{"aVector", VariantType::ArrayInt, defaultVector, {"an int array option"}}, + ConfigParamSpec{"aStringVector", VariantType::ArrayString, defaultStringVector, {"a string array option"}}, + ConfigParamSpec{"aLabeledArray", VariantType::LabeledArrayInt, LabeledArray{&l[0][0], 2, 2, {"r1", "r2"}, {"c1", "c2"}}, {"a labeled array option"}}, + ConfigParamSpec{"aNested.x", VariantType::Int, 2, {"an int option, nested in an object"}}, + ConfigParamSpec{"aNested.vec", VariantType::ArrayDouble, defaultNestedVector, {"a double array option, nested in an object"}}, + }; + auto store = std::make_unique(specs, std::vector>{}); + store->preload(); + store->activate(); + ConfigParamRegistry registry(std::move(store)); + + // a leaf value is removed + REQUIRE(registry.hasOption("anInt")); + registry.erase("anInt"); + REQUIRE(registry.hasOption("anInt") == false); + REQUIRE(registry.isSet("anInt") == false); + REQUIRE(registry.isDefault("anInt") == false); + REQUIRE_THROWS_AS(registry.get("anInt"), std::invalid_argument); + + // arrays are removed as whole subtrees + registry.erase("aVector"); + REQUIRE(registry.hasOption("aVector") == false); + REQUIRE(registry.isSet("aVector") == false); + registry.erase("aLabeledArray"); + REQUIRE(registry.hasOption("aLabeledArray") == false); + REQUIRE(registry.isSet("aLabeledArray") == false); + + // a nested key is removed without touching its siblings + registry.erase("aNested.vec"); + REQUIRE(registry.hasOption("aNested.vec") == false); + REQUIRE(registry.get("aNested.x") == 2); + + // other options are untouched + REQUIRE(registry.get("aString") == "foo"); + REQUIRE(registry.get>("aStringVector") == std::vector{"a", "b", "c"}); + + // missing keys are ignored + REQUIRE_NOTHROW(registry.erase("anInt")); + REQUIRE_NOTHROW(registry.erase("noSuchKey")); + REQUIRE_NOTHROW(registry.erase("noSuchParent.x")); + REQUIRE_NOTHROW(registry.erase("aNested.noSuchKey")); + REQUIRE(registry.get("aNested.x") == 2); + + // an erased key can be stored again + registry.override("anInt", 3); + REQUIRE(registry.get("anInt") == 3); + registry.override("aVector", std::vector{7, 8}); + REQUIRE(registry.get>("aVector") == std::vector{7, 8}); +} From cd319b04ee09d82dd3c6637e25e2e2b9720a221d Mon Sep 17 00:00:00 2001 From: Anton Alkin Date: Sat, 10 Oct 2026 22:01:30 +0200 Subject: [PATCH 6/7] working state --- .../src/AODJAlienReaderHelpers.cxx | 26 +- Framework/AnalysisSupport/src/Plugin.cxx | 233 +++++++++++++----- Framework/Core/CMakeLists.txt | 1 + Framework/Core/include/Framework/Array2D.h | 7 + .../include/Framework/ConfigParamDiscovery.h | 13 + Framework/Core/src/ConfigParamDiscovery.cxx | 89 +++++++ Framework/Core/src/Plugin.cxx | 13 +- .../Core/test/test_DeviceSpecHelpers.cxx | 5 +- Framework/Core/test/test_Variants.cxx | 4 + 9 files changed, 311 insertions(+), 80 deletions(-) diff --git a/Framework/AnalysisSupport/src/AODJAlienReaderHelpers.cxx b/Framework/AnalysisSupport/src/AODJAlienReaderHelpers.cxx index e101288bf5b83..93f6edbb24a3d 100644 --- a/Framework/AnalysisSupport/src/AODJAlienReaderHelpers.cxx +++ b/Framework/AnalysisSupport/src/AODJAlienReaderHelpers.cxx @@ -11,12 +11,10 @@ #include "AODJAlienReaderHelpers.h" #include -#include #include #include #include #include -#include #include #include #include @@ -27,6 +25,7 @@ #include "Framework/AlgorithmSpec.h" #include "Framework/ArrowContext.h" #include "Framework/ConfigParamRegistry.h" +#include "Framework/Array2D.h" #include "Framework/ControlService.h" #include "Framework/CallbackService.h" #include "Framework/EndOfStreamContext.h" @@ -155,24 +154,11 @@ AlgorithmSpec AODJAlienReaderHelpers::rootFileReaderCallback(ConfigContext const parentAccessLevel = ctx.options().get("aod-parent-access-level"); } std::vector> originLevelMapping; - if (ctx.options().isSet("aod-origin-level-mapping")) { - auto originLevelMappingStr = ctx.options().get("aod-origin-level-mapping"); - for (auto pairRange : originLevelMappingStr | std::views::split(',')) { - std::string_view pair{pairRange.begin(), pairRange.end()}; - auto colonPos = pair.find(':'); - if (colonPos == std::string_view::npos) { - LOGP(fatal, "Badly formatted aod-origin-level-mapping entry: \"{}\"", pair); - continue; - } - std::string key(pair.substr(0, colonPos)); - std::string_view valueStr = pair.substr(colonPos + 1); - int value{}; - auto [ptr, ec] = std::from_chars(valueStr.data(), valueStr.data() + valueStr.size(), value); - if (ec == std::errc{}) { - originLevelMapping.emplace_back(std::move(key), value); - } else { - LOGP(fatal, "Unable to parse level in aod-origin-level-mapping entry: \"{}\"", pair); - } + if (ctx.options().hasOption("aod-origin-level-map")) { + auto originLevelMap = ctx.options().get>("aod-origin-level-map"); + auto origins = originLevelMap.getLabelsRows(); + for (auto i = 0U; i < originLevelMap.rows(); ++i) { + originLevelMapping.emplace_back(origins[i], originLevelMap.get(i, 0U)); } } auto callback = AlgorithmSpec{adaptStateful([parentFileReplacement, parentAccessLevel, originLevelMapping](ConfigParamRegistry const& options, diff --git a/Framework/AnalysisSupport/src/Plugin.cxx b/Framework/AnalysisSupport/src/Plugin.cxx index 5f61a236cbd58..03b3a9b627db3 100644 --- a/Framework/AnalysisSupport/src/Plugin.cxx +++ b/Framework/AnalysisSupport/src/Plugin.cxx @@ -23,6 +23,8 @@ #include #include #include +#include +#include #include O2_DECLARE_DYNAMIC_LOG(analysis_support); @@ -105,7 +107,7 @@ struct RunSummary : o2::framework::ServicePlugin { } }; -std::vector getListOfTables(std::unique_ptr& f) +std::vector getListOfTables(TFile* f) { std::vector r; TList* keyList = f->GetListOfKeys(); @@ -155,17 +157,17 @@ std::vector getListOfTables(std::unique_ptr& f) return r; } -auto readMetadata(std::unique_ptr& currentFile) -> std::vector +// Ordered key / value pairs of the metadata, empty if there is none +auto readMetadata(TFile* currentFile) -> std::vector> { // Get the metadata, if any auto m = (TMap*)currentFile->Get("metaData"); if (!m) { return {}; } - std::vector results; + std::vector> results; auto it = m->MakeIterator(); - // Serialise metadata into a ; separated string with : separating key and value bool first = true; while (auto obj = it->Next()) { if (first) { @@ -174,14 +176,90 @@ auto readMetadata(std::unique_ptr& currentFile) -> std::vectorGetValue(obj); LOGP(info, "- {}: {}", obj->GetName(), objString->String().Data()); - std::string key = "aod-metadata-" + std::string(obj->GetName()); - char const* value = strdup(objString->String()); - results.push_back(ConfigParamSpec{key, VariantType::String, value, {"Metadata in AOD"}}); + results.emplace_back(obj->GetName(), objString->String().Data()); } return results; } +auto metadataToSpecs(std::vector> const& metadata) -> std::vector +{ + std::vector results; + for (auto const& [key, value] : metadata) { + results.push_back(ConfigParamSpec{"aod-metadata-" + key, VariantType::String, value, {"Metadata in AOD"}}); + } + return results; +} + +auto metadataKeys(std::vector> const& metadata) -> std::vector +{ + std::vector keys; + for (auto const& [key, value] : metadata) { + keys.push_back(key); + } + return keys; +} + +struct FileAtLevel { + std::unique_ptr owned; // empty for level 0, which is the file we started from + TFile* file = nullptr; // nullptr if the chain of parents is shorter than the level + std::string name; +}; + +// Follow the first parent file `level` times, starting from `file` +FileAtLevel openAtLevel(TFile* file, std::string const& filename, int level, ConfigParamRegistry& registry) +{ + FileAtLevel result{nullptr, file, filename}; + for (auto l = 0; l < level; ++l) { + auto parentFiles = (TMap*)result.file->Get("parentFiles"); + if (!parentFiles || parentFiles->GetSize() == 0) { + LOGP(info, "No parent file found for \"{}\"", result.name); + return {nullptr, nullptr, result.name}; + } + std::string parentFilename; + for (auto* p : *parentFiles) { + parentFilename = ((TPair*)p)->Value()->GetName(); + break; + } + // Do the replacement. Notice this will require changing aod-parent-base-path-replacement to be + // a workflow option (because the metadata itself is potentially changing the topology). + if (registry.isSet("aod-parent-base-path-replacement")) { + auto parentFileReplacement = registry.get("aod-parent-base-path-replacement"); + auto pos = parentFileReplacement.find(';'); + if (pos == std::string::npos) { + throw std::runtime_error(fmt::format("Invalid syntax in aod-parent-base-path-replacement: \"{}\"", parentFileReplacement.c_str())); + } + auto from = parentFileReplacement.substr(0, pos); + auto to = parentFileReplacement.substr(pos + 1); + pos = parentFilename.find(from); + if (pos != std::string::npos) { + parentFilename.replace(pos, from.length(), to); + } + } + + if (parentFilename.starts_with("alien://") && !gGrid) { + TGrid::Connect("alien://"); + } + + std::unique_ptr parentFile{TFile::Open(parentFilename.c_str())}; + if (parentFile.get() == nullptr) { + LOGP(fatal, "Couldn't open derived file \"{}\"!", parentFilename); + } + result.file = parentFile.get(); + result.owned = std::move(parentFile); + result.name = parentFilename; + } + return result; +} + +// What was found at a given parent level +struct LevelMetadata { + bool found = false; // the file at this level exists + std::string source; + std::vector> metadata; + std::vector tables; +}; + struct DiscoverMetadataInAOD : o2::framework::ConfigDiscoveryPlugin { ConfigDiscovery* create() override { @@ -210,73 +288,112 @@ struct DiscoverMetadataInAOD : o2::framework::ConfigDiscoveryPlugin { if (currentFile.get() == nullptr) { LOGP(fatal, "Couldn't open file \"{}\"!", filename); } - std::vector results = readMetadata(currentFile); - const bool metaDataEmpty = results.empty(); - auto tables = getListOfTables(currentFile); - if (tables.empty() == false) { - results.push_back(ConfigParamSpec{"aod-metadata-tables", VariantType::ArrayString, tables, {"Tables in first AOD"}}); - } - // Found metadata already in the main file. - if (!metaDataEmpty) { - results.push_back(ConfigParamSpec{"aod-metadata-source", VariantType::String, filename, {"File from which the metadata was extracted."}}); - return results; + // Each level is read only once, even if several origins map to it + std::map levels; + auto readLevel = [&](int level) -> LevelMetadata const& { + if (auto it = levels.find(level); it != levels.end()) { + return it->second; + } + LevelMetadata info; + auto atLevel = openAtLevel(currentFile.get(), filename, level, registry); + if (atLevel.file) { + info.found = true; + info.source = atLevel.name; + info.metadata = readMetadata(atLevel.file); + info.tables = getListOfTables(atLevel.file); + } + return levels.emplace(level, std::move(info)).first->second; + }; + int parentAccessLevel = 0; + if (registry.isSet("aod-parent-access-level")) { + parentAccessLevel = registry.get("aod-parent-access-level"); } - if (!registry.isSet("aod-parent-access-level") || registry.get("aod-parent-access-level") == 0) { + // The plain aod-metadata-* options, from the file itself or from its first parent + auto const& top = readLevel(0); + std::vector results = metadataToSpecs(top.metadata); + std::vector plainKeys; + if (!top.tables.empty()) { + results.push_back(ConfigParamSpec{"aod-metadata-tables", VariantType::ArrayString, top.tables, {"Tables in first AOD"}}); + } + if (!top.metadata.empty()) { + // Found metadata already in the main file. + results.push_back(ConfigParamSpec{"aod-metadata-source", VariantType::String, filename, {"File from which the metadata was extracted."}}); + plainKeys = metadataKeys(top.metadata); + } else if (parentAccessLevel == 0) { LOGP(info, "No metadata found in file \"{}\" and parent level 0 prevents further lookup.", filename); results.push_back(ConfigParamSpec{"aod-metadata-disable", VariantType::String, "1", {"Metadata not found in AOD"}}); - return results; + } else { + // Lets try in parent file. + LOGP(info, "No metadata found in file \"{}\", checking in its parents.", filename); + auto const& parent = readLevel(1); + if (!parent.found) { + LOGP(info, "No metadata found in file \"{}\"", filename); + } else { + results = metadataToSpecs(parent.metadata); + if (!parent.metadata.empty()) { + if (!parent.tables.empty()) { + results.push_back(ConfigParamSpec{"aod-metadata-tables", VariantType::ArrayString, parent.tables, {"Tables in first AOD"}}); + } + results.push_back(ConfigParamSpec{"aod-metadata-source", VariantType::String, filename, {"File from which the metadata was extracted."}}); + plainKeys = metadataKeys(parent.metadata); + } else { + LOGP(info, "No metadata found in file \"{}\" nor in its parent file \"{}\"", filename, parent.source); + } + } + if (plainKeys.empty()) { + results.push_back(ConfigParamSpec{"aod-metadata-disable", VariantType::String, "1", {"Metadata not found in AOD"}}); + } } - // Lets try in parent file. - auto parentFiles = (TMap*)currentFile->Get("parentFiles"); - if (!parentFiles) { - LOGP(info, "No metadata found in file \"{}\"", filename); - results.push_back(ConfigParamSpec{"aod-metadata-disable", VariantType::String, "1", {"Metadata not found in AOD"}}); + if (!registry.hasOption("aod-origin-level-map")) { return results; } - LOGP(info, "No metadata found in file \"{}\", checking in its parents.", filename); - for (auto* p : *parentFiles) { - std::string parentFilename = ((TPair*)p)->Value()->GetName(); - // Do the replacement. Notice this will require changing aod-parent-base-path-replacement to be - // a workflow option (because the metadata itself is potentially changing the topology). - if (registry.isSet("aod-parent-base-path-replacement")) { - auto parentFileReplacement = registry.get("aod-parent-base-path-replacement"); - auto pos = parentFileReplacement.find(';'); - if (pos == std::string::npos) { - throw std::runtime_error(fmt::format("Invalid syntax in aod-parent-base-path-replacement: \"{}\"", parentFileReplacement.c_str())); - } - auto from = parentFileReplacement.substr(0, pos); - auto to = parentFileReplacement.substr(pos + 1); - pos = parentFilename.find(from); - if (pos != std::string::npos) { - parentFilename.replace(pos, from.length(), to); + + // The metadata of the level each origin reads, one row per origin. The columns are + // all the keys found, including the plain ones, so that an empty cell always means + // that the key is missing for that origin. + auto levelMap = registry.get>("aod-origin-level-map"); + auto origins = levelMap.getLabelsRows(); + std::vector keys = plainKeys; + for (auto i = 0U; i < levelMap.rows(); ++i) { + auto level = levelMap.get(i, 0U); + if (level > parentAccessLevel) { + LOGP(fatal, "Origin {} is mapped to parent level {}, but aod-parent-access-level is {}", origins[i], level, parentAccessLevel); + } + for (auto const& [key, value] : readLevel(level).metadata) { + if (std::ranges::find(keys, key) == keys.end()) { + keys.push_back(key); } } + } + auto sourceColumn = keys.size(); + keys.emplace_back("source"); + auto disableColumn = keys.size(); + keys.emplace_back("disable"); - if (parentFilename.starts_with("alien://") && !gGrid) { - TGrid::Connect("alien://"); + std::vector values(origins.size() * keys.size()); + for (auto i = 0U; i < origins.size(); ++i) { + auto const& info = readLevel(levelMap.get(i, 0U)); + auto* row = values.data() + i * keys.size(); + for (auto const& [key, value] : info.metadata) { + row[std::ranges::find(keys, key) - keys.begin()] = value; } - - std::unique_ptr parentFile{TFile::Open(parentFilename.c_str())}; - if (parentFile.get() == nullptr) { - LOGP(fatal, "Couldn't open derived file \"{}\"!", parentFilename); + row[sourceColumn] = info.source; + if (info.metadata.empty()) { + LOGP(info, "Metadata info missing for origin {}", origins[i]); + row[disableColumn] = "1"; } - results = readMetadata(parentFile); - // Found metadata already in the main file. - if (!results.empty()) { - auto tables = getListOfTables(parentFile); - if (tables.empty() == false) { - results.push_back(ConfigParamSpec{"aod-metadata-tables", VariantType::ArrayString, tables, {"Tables in first AOD"}}); - } - results.push_back(ConfigParamSpec{"aod-metadata-source", VariantType::String, filename, {"File from which the metadata was extracted."}}); - return results; + } + LabeledArray originMetadata{values.data(), static_cast(origins.size()), static_cast(keys.size()), origins, keys}; + results.push_back(ConfigParamSpec{"aod-metadata-origins", VariantType::LabeledArrayString, originMetadata, {"Metadata in AOD for each origin: one row per origin, one column per key"}}); + for (auto i = 0U; i < origins.size(); ++i) { + auto const& tables = readLevel(levelMap.get(i, 0U)).tables; + if (!tables.empty()) { + results.push_back(ConfigParamSpec{"aod-metadata-origin-" + origins[i] + "-tables", VariantType::ArrayString, tables, {"Tables in the AOD read for this origin"}}); } - LOGP(info, "No metadata found in file \"{}\" nor in its parent file \"{}\"", filename, parentFilename); - break; } - results.push_back(ConfigParamSpec{"aod-metadata-disable", VariantType::String, "1", {"Metadata not found in AOD"}}); return results; }}; } diff --git a/Framework/Core/CMakeLists.txt b/Framework/Core/CMakeLists.txt index 437dc5ee6478c..aa2bd5b450d98 100644 --- a/Framework/Core/CMakeLists.txt +++ b/Framework/Core/CMakeLists.txt @@ -217,6 +217,7 @@ add_executable(o2-test-framework-core test/test_ControlServiceHelpers.cxx test/test_ConfigParamStore.cxx test/test_ConfigParamRegistry.cxx + test/test_ConfigParamDiscovery.cxx test/test_DataDescriptorMatcher.cxx test/test_DataDescriptorQueryBuilder.cxx test/test_DataProcessingStats.cxx diff --git a/Framework/Core/include/Framework/Array2D.h b/Framework/Core/include/Framework/Array2D.h index de877797105c9..4b21ac2ba5b32 100644 --- a/Framework/Core/include/Framework/Array2D.h +++ b/Framework/Core/include/Framework/Array2D.h @@ -241,6 +241,13 @@ class LabeledArray : public LabelMap return values[y]; } + // nullptr if there is no row with this label + T* getRow(const char* y) const + { + auto it = rowmap.find(y); + return it == rowmap.end() ? nullptr : values[it->second]; + } + T* operator[](uint32_t y) const { return values[y]; diff --git a/Framework/Core/include/Framework/ConfigParamDiscovery.h b/Framework/Core/include/Framework/ConfigParamDiscovery.h index 3d13d3662ceb1..c64c4c8255cb9 100644 --- a/Framework/Core/include/Framework/ConfigParamDiscovery.h +++ b/Framework/Core/include/Framework/ConfigParamDiscovery.h @@ -13,6 +13,9 @@ #define O2_FRAMEWORK_CONFIGPARAMDISCOVERY_H_ #include "Framework/ConfigParamRegistry.h" +#include "Framework/Array2D.h" + +#include namespace o2::framework { @@ -20,6 +23,16 @@ namespace o2::framework // options. struct ConfigParamDiscovery { static std::vector discover(ConfigParamRegistry&, int, char**); + + /// Parse the value of aod-origin-level-mapping, "ORIGIN:LEVEL[,ORIGIN2:LEVEL2,...]", + /// into a LabeledArray with one row per origin and a single "level" column. + /// Throws std::runtime_error if the mapping is malformed. + static LabeledArray parseOriginLevelMapping(std::string_view mapping); + + /// Replace the plain aod-metadata-* options with the ones in the row of + /// aod-metadata-origins which corresponds to the origin this process reads + /// (aod-origin-replace, or AOD). Does nothing if there is no such row. + static void applyOriginMetadata(ConfigParamRegistry& registry); }; struct ConfigDiscovery { diff --git a/Framework/Core/src/ConfigParamDiscovery.cxx b/Framework/Core/src/ConfigParamDiscovery.cxx index fc8d6f2600bb4..76448ea8222db 100644 --- a/Framework/Core/src/ConfigParamDiscovery.cxx +++ b/Framework/Core/src/ConfigParamDiscovery.cxx @@ -16,6 +16,12 @@ #include #include #include +#include + +#include +#include +#include +#include O2_DECLARE_DYNAMIC_LOG(config_parameter_discovery); @@ -77,7 +83,90 @@ std::vector ConfigParamDiscovery::discover(ConfigParamRegistry& } registry.loadExtra(extras); } + applyOriginMetadata(registry); return result; } +LabeledArray ConfigParamDiscovery::parseOriginLevelMapping(std::string_view mapping) +{ + std::vector origins; + std::vector levels; + for (auto pairRange : mapping | std::views::split(',')) { + std::string_view pair{pairRange.begin(), pairRange.end()}; + auto colonPos = pair.find(':'); + if (colonPos == std::string_view::npos) { + throw std::runtime_error(fmt::format("Badly formatted aod-origin-level-mapping entry: \"{}\"", pair)); + } + std::string origin{pair.substr(0, colonPos)}; + if (origin.empty() || origin.size() > 4) { + throw std::runtime_error(fmt::format("Origin in aod-origin-level-mapping entry must have 1 to 4 characters: \"{}\"", pair)); + } + if (std::ranges::find(origins, origin) != origins.end()) { + throw std::runtime_error(fmt::format("Duplicate origin in aod-origin-level-mapping: \"{}\"", origin)); + } + auto levelStr = pair.substr(colonPos + 1); + int level{}; + auto [ptr, ec] = std::from_chars(levelStr.data(), levelStr.data() + levelStr.size(), level); + if (levelStr.empty() || ec != std::errc{} || ptr != levelStr.data() + levelStr.size() || level < 0) { + throw std::runtime_error(fmt::format("Unable to parse level in aod-origin-level-mapping entry: \"{}\"", pair)); + } + origins.push_back(std::move(origin)); + levels.push_back(level); + } + if (origins.empty()) { + throw std::runtime_error("Empty aod-origin-level-mapping"); + } + return LabeledArray{levels.data(), static_cast(levels.size()), 1, origins, {"level"}}; +} + +void ConfigParamDiscovery::applyOriginMetadata(ConfigParamRegistry& registry) +{ + if (!registry.hasOption("aod-metadata-origins")) { + return; + } + std::string origin = "AOD"; + if (registry.hasOption("aod-origin-replace")) { + auto replace = registry.get("aod-origin-replace"); + if (!replace.empty()) { + origin = replace; + } + } + auto metadata = registry.get>("aod-metadata-origins"); + auto* row = metadata.getRow(origin.c_str()); + if (row == nullptr) { + return; + } + O2_SIGNPOST_ID_GENERATE(sid, config_parameter_discovery); + auto const& keys = metadata.getLabelsCols(); + auto disable = metadata.colmap.find("disable"); + if (disable != metadata.colmap.end() && row[disable->second] == "1") { + O2_SIGNPOST_EVENT_EMIT(config_parameter_discovery, sid, "origin metadata", "No metadata for origin %{public}s", origin.c_str()); + for (auto const& key : keys) { + registry.erase(("aod-metadata-" + key).c_str()); + } + registry.erase("aod-metadata-tables"); + registry.override("aod-metadata-disable", std::string{"1"}); + return; + } + O2_SIGNPOST_EVENT_EMIT(config_parameter_discovery, sid, "origin metadata", "Using metadata for origin %{public}s", origin.c_str()); + for (auto j = 0U; j < metadata.cols(); ++j) { + if (keys[j] == "disable") { + continue; + } + auto key = "aod-metadata-" + keys[j]; + if (row[j].empty()) { + registry.erase(key.c_str()); + } else { + registry.override(key.c_str(), row[j]); + } + } + registry.erase("aod-metadata-disable"); + auto tablesKey = "aod-metadata-origin-" + origin + "-tables"; + if (registry.hasOption(tablesKey.c_str())) { + registry.override("aod-metadata-tables", registry.get>(tablesKey.c_str())); + } else { + registry.erase("aod-metadata-tables"); + } +} + } // namespace o2::framework diff --git a/Framework/Core/src/Plugin.cxx b/Framework/Core/src/Plugin.cxx index d1abb44c3cc80..ffe4ed34d40ff 100644 --- a/Framework/Core/src/Plugin.cxx +++ b/Framework/Core/src/Plugin.cxx @@ -129,10 +129,14 @@ struct DiscoverMetadataInCommandLine : o2::framework::ConfigDiscoveryPlugin { std::string value = argv[i + 1]; O2_SIGNPOST_EVENT_EMIT(capabilities, sid, "DiscoverMetadataInCommandLine", "Found %{public}s with value %{public}s.", key.c_str(), value.c_str()); - if (key == "aod-metadata-tables") { + if (key == "aod-metadata-tables" || (key.starts_with("aod-metadata-origin-") && key.ends_with("-tables"))) { std::stringstream is(value); auto arrayValue = VariantJSONHelpers::read(is); results.push_back(ConfigParamSpec{key, VariantType::ArrayString, arrayValue, {"Metadata in command line"}}); + } else if (key == "aod-metadata-origins") { + std::stringstream is(value); + auto arrayValue = VariantJSONHelpers::read(is); + results.push_back(ConfigParamSpec{key, VariantType::LabeledArrayString, arrayValue, {"Metadata in command line"}}); } else { results.push_back(ConfigParamSpec{key, VariantType::String, value, {"Metadata in command line"}}); } @@ -175,6 +179,13 @@ struct DiscoverAODOptionsInCommandLine : o2::framework::ConfigDiscoveryPlugin { } if (key == "aod-origin-level-mapping") { results.push_back(ConfigParamSpec{"aod-origin-level-mapping", VariantType::String, value, {"Map origin to parent level for AOD reading. Syntax: ORIGIN:LEVEL[,ORIGIN2:LEVEL2,...]. E.g. \"DYN:1\"."}}); + LabeledArray levelMap; + try { + levelMap = ConfigParamDiscovery::parseOriginLevelMapping(value); + } catch (std::runtime_error const& e) { + LOGP(fatal, "{}", e.what()); + } + results.push_back(ConfigParamSpec{"aod-origin-level-map", VariantType::LabeledArrayInt, levelMap, {"Parsed aod-origin-level-mapping: one row per origin, with its parent level"}}); } } if (injectOption) { diff --git a/Framework/Core/test/test_DeviceSpecHelpers.cxx b/Framework/Core/test/test_DeviceSpecHelpers.cxx index d689e7a2765c4..bf163f919c795 100644 --- a/Framework/Core/test/test_DeviceSpecHelpers.cxx +++ b/Framework/Core/test/test_DeviceSpecHelpers.cxx @@ -102,12 +102,15 @@ void check(const std::vector& arguments, TEST_CASE("test_prepareArguments") { int lm[1][2] = {{1, 2}}; + std::string om[2][3] = {{"MC", "/path/L0.root", ""}, {"", "/path/L1.root", "1"}}; std::vector workflowOptions{ {"foo", VariantType::String, "bar", {"charlie foxtrot"}}, {"depth", VariantType::Int, 1, {"height"}}, {"labeled", VariantType::LabeledArrayInt, LabeledArray{&lm[0][0], 1, 2, {"r"}, {"c1", "c2"}}, {"a labeled array"}}, + {"aod-metadata-origins", VariantType::LabeledArrayString, LabeledArray{&om[0][0], 2, 3, {"EMB", "AOD"}, {"DataType", "source", "disable"}}, {"metadata per origin"}}, }; std::string labeledDefault = R"({"labels_rows":["r"],"labels_cols":["c1","c2"],"values":[[1,2]]})"; + std::string originsDefault = R"({"labels_rows":["EMB","AOD"],"labels_cols":["DataType","source","disable"],"values":[["MC","/path/L0.root",""],["","/path/L1.root","1"]]})"; std::string labeledOther = R"({"labels_rows":["r"],"labels_cols":["c1","c2"],"values":[[3,4]]})"; auto algorithm = [](ProcessingContext& /*ctx*/) {}; @@ -183,7 +186,7 @@ TEST_CASE("test_prepareArguments") check({"--depth", "2", "--processor0", "--mode silly"}, workflowOptions, deviceSpecs, matrix); // a LabeledArray workflow option is forwarded as a single JSON argument, defaulted or set - matrix["processor0"] = {{"--depth", "1"}, {"--labeled", labeledDefault}}; + matrix["processor0"] = {{"--depth", "1"}, {"--labeled", labeledDefault}, {"--aod-metadata-origins", originsDefault}}; matrix["processor1"] = matrix["processor0"]; check({}, workflowOptions, deviceSpecs, matrix); matrix["processor0"] = {{"--depth", "1"}, {"--labeled", labeledOther}}; diff --git a/Framework/Core/test/test_Variants.cxx b/Framework/Core/test/test_Variants.cxx index 77f6a7d2918e3..bbdb0fff45e52 100644 --- a/Framework/Core/test/test_Variants.cxx +++ b/Framework/Core/test/test_Variants.cxx @@ -387,7 +387,11 @@ TEST_CASE("LabeledArrayTest") REQUIRE(las.get(i, xl[j].c_str()) == las.get(i, j)); REQUIRE(las.get(yl[i].c_str(), j) == las.get(i, j)); } + REQUIRE(laf.getRow(yl[i].c_str()) == laf.getRow(i)); + REQUIRE(las.getRow(yl[i].c_str()) == las.getRow(i)); } + REQUIRE(laf.getRow("r4") == nullptr); + REQUIRE(las.getRow("r4") == nullptr); } TEST_CASE("VariantTreeConversionsTest") From 35fa690a25385cdb4ad8e7ec51d0c3974c074088 Mon Sep 17 00:00:00 2001 From: Anton Alkin Date: Sun, 11 Oct 2026 10:04:01 +0200 Subject: [PATCH 7/7] leftover --- Framework/Core/CMakeLists.txt | 1 - 1 file changed, 1 deletion(-) diff --git a/Framework/Core/CMakeLists.txt b/Framework/Core/CMakeLists.txt index aa2bd5b450d98..437dc5ee6478c 100644 --- a/Framework/Core/CMakeLists.txt +++ b/Framework/Core/CMakeLists.txt @@ -217,7 +217,6 @@ add_executable(o2-test-framework-core test/test_ControlServiceHelpers.cxx test/test_ConfigParamStore.cxx test/test_ConfigParamRegistry.cxx - test/test_ConfigParamDiscovery.cxx test/test_DataDescriptorMatcher.cxx test/test_DataDescriptorQueryBuilder.cxx test/test_DataProcessingStats.cxx