From 8437380ba2024b99cf57dcb375baa11a55036c39 Mon Sep 17 00:00:00 2001 From: Anton Alkin Date: Thu, 1 Oct 2026 10:44:39 +0200 Subject: [PATCH 1/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 | 1 + 1 file changed, 1 insertion(+) diff --git a/Framework/Core/include/Framework/Variant.h b/Framework/Core/include/Framework/Variant.h index c56c0c01ef9fc..e0bbccf560fad 100644 --- a/Framework/Core/include/Framework/Variant.h +++ b/Framework/Core/include/Framework/Variant.h @@ -403,6 +403,7 @@ class Variant *this = Variant(value, size); } + /// FIXME: set for vector of strings is not used anywhere, why? template void set(std::vector& values) requires(std::is_pod_v || std::is_same_v) From d5f1a773a8112a7b8e57c50052c76a5eb7029cc4 Mon Sep 17 00:00:00 2001 From: Anton Alkin Date: Thu, 1 Oct 2026 12:35:52 +0200 Subject: [PATCH 2/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 35d20f85ba919..ad89e22a3bf39 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 d5744baf130aca160b5129e3af59d7bb0bab0b4c Mon Sep 17 00:00:00 2001 From: Anton Alkin Date: Wed, 14 Oct 2026 09:34:42 +0200 Subject: [PATCH 3/7] format --- Framework/Core/test/test_WorkflowSerialization.cxx | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/Framework/Core/test/test_WorkflowSerialization.cxx b/Framework/Core/test/test_WorkflowSerialization.cxx index 638d4e023e83a..5ffbbcda3df57 100644 --- a/Framework/Core/test/test_WorkflowSerialization.cxx +++ b/Framework/Core/test/test_WorkflowSerialization.cxx @@ -27,28 +27,28 @@ 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{"aLabeledArray", VariantType::LabeledArrayInt, labeled, {"A LabeledArray"}}}}, // // + 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\""}}, From b96faebab96afb03ee70e507d9434e5fa1670095 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 99187505698f82e7774689ee5fdd79bfc138f7df 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 0290670517253e52127b7c057d72288fa7521769 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 d11b171f9dcc7..05b4b2890ecfe 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 ae5ce005fdbd4b2718cdcc108d5ea1e45e51e269 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