Skip to content

Commit d03d660

Browse files
aalkinaalkin
authored
DPL: Allow overriding array values in ConfigParamRegistry (#15927)
1 parent 946c509 commit d03d660

4 files changed

Lines changed: 188 additions & 14 deletions

File tree

‎Framework/Core/include/Framework/ConfigParamRegistry.h‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -87,6 +87,9 @@ class ConfigParamRegistry
8787

8888
void override(const char* key, ConfigValueType auto const& val) const;
8989

90+
// Remove an option (a leaf or a whole subtree) from the active store, no-op if it is missing
91+
void erase(const char* key) const;
92+
9093
// Load extra parameters discovered while we process data
9194
void loadExtra(std::vector<ConfigParamSpec>& extras);
9295

‎Framework/Core/src/ConfigParamRegistry.cxx‎

Lines changed: 46 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,9 @@
1212
#include "Framework/VariantPropertyTreeHelpers.h"
1313
#include "Framework/Array2D.h"
1414

15+
#include <string_view>
16+
#include <type_traits>
17+
1518
namespace o2::framework
1619
{
1720

@@ -67,6 +70,19 @@ auto getImpl(boost::property_tree::ptree& tree, const char* key)
6770
{
6871
return labeledArrayFromBranch<typename T::element_t>(tree.get_child(key));
6972
}
73+
74+
// ptree::erase only removes direct children, so the parent of a nested key has to be looked up first
75+
void eraseImpl(boost::property_tree::ptree& tree, std::string_view key)
76+
{
77+
auto pos = key.rfind('.');
78+
if (pos == std::string_view::npos) {
79+
tree.erase(std::string(key));
80+
return;
81+
}
82+
if (auto parent = tree.get_child_optional(std::string(key.substr(0, pos)))) {
83+
parent->erase(std::string(key.substr(pos + 1)));
84+
}
85+
}
7086
} // namespace
7187

7288
template <ConfigValueType T>
@@ -83,15 +99,32 @@ T ConfigParamRegistry::get(const char* key) const
8399

84100
void ConfigParamRegistry::override(const char* key, ConfigValueType auto const& val) const
85101
{
102+
using T = std::decay_t<decltype(val)>;
86103
try {
87-
mStore->store().put(key, val);
104+
auto& store = mStore->store();
105+
if constexpr (SimpleConfigValueType<T> || StringConfigValueType<T>) {
106+
store.put(key, val);
107+
} else if constexpr (VectorConfigValueType<T>) {
108+
// arrays are stored as subtrees, which have to be replaced as a whole
109+
store.put_child(key, vectorToBranch(val.data(), val.size()));
110+
} else if constexpr (Array2DLike<T>) {
111+
store.put_child(key, array2DToBranch(T(val)));
112+
} else if constexpr (LabeledArrayLike<T>) {
113+
store.put_child(key, labeledArrayToBranch(T(val)));
114+
}
88115
} catch (std::exception& e) {
89116
throw std::invalid_argument(std::string("failed to store an option: ") + key + " (" + e.what() + ")");
90117
} catch (...) {
91118
throw std::invalid_argument(std::string("failed to store an option: ") + key);
92119
}
93120
}
94121

122+
void ConfigParamRegistry::erase(const char* key) const
123+
{
124+
eraseImpl(mStore->store(), key);
125+
eraseImpl(mStore->provenanceTree(), key);
126+
}
127+
95128
// Load extra parameters discovered while we process data
96129
void ConfigParamRegistry::loadExtra(std::vector<ConfigParamSpec>& extras)
97130
{
@@ -143,16 +176,16 @@ template void ConfigParamRegistry::override(const char* key, double const&) cons
143176
template void ConfigParamRegistry::override(const char* key, std::string const&) const;
144177
template void ConfigParamRegistry::override(const char* key, bool const&) const;
145178

146-
// template void ConfigParamRegistry::override(char const* key, LabeledArray<std::string> const&) const;
147-
// template void ConfigParamRegistry::override(char const* key, LabeledArray<double> const&) const;
148-
// template void ConfigParamRegistry::override(char const* key, LabeledArray<float> const&) const;
149-
// template void ConfigParamRegistry::override(char const* key, LabeledArray<int> const&) const;
150-
// template void ConfigParamRegistry::override(char const* key, Array2D<std::string> const&) const;
151-
// template void ConfigParamRegistry::override(char const* key, Array2D<double> const&) const;
152-
// template void ConfigParamRegistry::override(char const* key, Array2D<float> const&) const;
153-
// template void ConfigParamRegistry::override(char const* key, Array2D<int> const&) const;
154-
// template void ConfigParamRegistry::override(char const* key, std::vector<std::string> const&) const;
155-
// template void ConfigParamRegistry::override(char const* key, std::vector<double> const&) const;
156-
// template void ConfigParamRegistry::override(char const* key, std::vector<float> const&) const;
157-
// template void ConfigParamRegistry::override(char const* key, std::vector<int> const&) const;
179+
template void ConfigParamRegistry::override(char const* key, LabeledArray<std::string> const&) const;
180+
template void ConfigParamRegistry::override(char const* key, LabeledArray<double> const&) const;
181+
template void ConfigParamRegistry::override(char const* key, LabeledArray<float> const&) const;
182+
template void ConfigParamRegistry::override(char const* key, LabeledArray<int> const&) const;
183+
template void ConfigParamRegistry::override(char const* key, Array2D<std::string> const&) const;
184+
template void ConfigParamRegistry::override(char const* key, Array2D<double> const&) const;
185+
template void ConfigParamRegistry::override(char const* key, Array2D<float> const&) const;
186+
template void ConfigParamRegistry::override(char const* key, Array2D<int> const&) const;
187+
template void ConfigParamRegistry::override(char const* key, std::vector<std::string> const&) const;
188+
template void ConfigParamRegistry::override(char const* key, std::vector<double> const&) const;
189+
template void ConfigParamRegistry::override(char const* key, std::vector<float> const&) const;
190+
template void ConfigParamRegistry::override(char const* key, std::vector<int> const&) const;
158191
} // namespace o2::framework

‎Framework/Core/test/test_ConfigParamRegistry.cxx‎

Lines changed: 138 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,8 @@
1212
#include <catch_amalgamated.hpp>
1313
#include "Framework/FairOptionsRetriever.h"
1414
#include "Framework/ConfigParamRegistry.h"
15+
#include "Framework/ConfigParamStore.h"
16+
#include "Framework/Array2D.h"
1517

1618
#include <fairmq/ProgOptions.h>
1719
#include <boost/program_options.hpp>
@@ -119,3 +121,139 @@ TEST_CASE("TestConfigParamRegistry")
119121
REQUIRE(obj.x == 1);
120122
REQUIRE(obj.y == 2.f);
121123
}
124+
125+
TEST_CASE("TestConfigParamRegistryOverride")
126+
{
127+
float m[3][4] = {{1, 2, 3, 4}, {5, 6, 7, 8}, {9, 10, 11, 12}};
128+
int l[2][2] = {{1, 2}, {3, 4}};
129+
// a Variant cannot be created from a temporary vector, so the defaults are named
130+
std::vector<int> defaultVector{1, 2, 3, 4, 5};
131+
std::vector<std::string> defaultStringVector{"a", "b", "c"};
132+
std::vector<double> defaultNestedVector{0.5, 1.5};
133+
// the store keeps a reference to the specs, which have to outlive it
134+
std::vector<ConfigParamSpec> specs{
135+
ConfigParamSpec{"anInt", VariantType::Int, 1, {"an int option"}},
136+
ConfigParamSpec{"aString", VariantType::String, "foo", {"a string option"}},
137+
ConfigParamSpec{"aVector", VariantType::ArrayInt, defaultVector, {"an int array option"}},
138+
ConfigParamSpec{"aStringVector", VariantType::ArrayString, defaultStringVector, {"a string array option"}},
139+
ConfigParamSpec{"anArray2D", VariantType::Array2DFloat, Array2D<float>{&m[0][0], 3, 4}, {"a 2D array option"}},
140+
ConfigParamSpec{"aLabeledArray", VariantType::LabeledArrayInt, LabeledArray<int>{&l[0][0], 2, 2, {"r1", "r2"}, {"c1", "c2"}}, {"a labeled array option"}},
141+
ConfigParamSpec{"aNested.x", VariantType::Int, 2, {"an int option, nested in an object"}},
142+
ConfigParamSpec{"aNested.vec", VariantType::ArrayDouble, defaultNestedVector, {"a double array option, nested in an object"}},
143+
};
144+
auto store = std::make_unique<ConfigParamStore>(specs, std::vector<std::unique_ptr<ParamRetriever>>{});
145+
store->preload();
146+
store->activate();
147+
ConfigParamRegistry registry(std::move(store));
148+
REQUIRE(registry.get<std::vector<int>>("aVector").size() == 5);
149+
150+
// leaf values keep working
151+
registry.override("anInt", 10);
152+
REQUIRE(registry.get<int>("anInt") == 10);
153+
154+
// a shorter vector replaces the whole subtree, no stale elements are left
155+
registry.override("aVector", std::vector<int>{7, 8});
156+
REQUIRE(registry.get<std::vector<int>>("aVector") == std::vector<int>{7, 8});
157+
registry.override("aStringVector", std::vector<std::string>{"x"});
158+
REQUIRE(registry.get<std::vector<std::string>>("aStringVector") == std::vector<std::string>{"x"});
159+
160+
// a smaller 2D array replaces the whole subtree
161+
float m2[1][2] = {{0.25, 0.75}};
162+
registry.override("anArray2D", Array2D<float>{&m2[0][0], 1, 2});
163+
auto a2d = registry.get<Array2D<float>>("anArray2D");
164+
REQUIRE(a2d.rows == 1);
165+
REQUIRE(a2d.cols == 2);
166+
REQUIRE(a2d(0, 1) == 0.75f);
167+
168+
// labels are replaced together with the values
169+
int l2[1][3] = {{5, 6, 7}};
170+
registry.override("aLabeledArray", LabeledArray<int>{&l2[0][0], 1, 3, {"row"}, {"x", "y", "z"}});
171+
auto la = registry.get<LabeledArray<int>>("aLabeledArray");
172+
REQUIRE(la.getLabelsRows() == std::vector<std::string>{"row"});
173+
REQUIRE(la.getLabelsCols() == std::vector<std::string>{"x", "y", "z"});
174+
REQUIRE(la.get("row", "z") == 7);
175+
// an unlabeled value is stored as it is, without merging the previous labels
176+
registry.override("aLabeledArray", LabeledArray<int>{&l[0][0], 2, 2});
177+
la = registry.get<LabeledArray<int>>("aLabeledArray");
178+
REQUIRE(la.getLabelsRows().empty());
179+
REQUIRE(la.getLabelsCols().empty());
180+
REQUIRE(la.get(1U, 0U) == 3);
181+
182+
// nested subtrees are replaced without touching their siblings
183+
registry.override("aNested.vec", std::vector<double>{2.5});
184+
REQUIRE(registry.get<std::vector<double>>("aNested.vec") == std::vector<double>{2.5});
185+
REQUIRE(registry.get<int>("aNested.x") == 2);
186+
REQUIRE(registry.get<std::string>("aString") == "foo");
187+
REQUIRE(registry.get<std::vector<int>>("aVector") == std::vector<int>{7, 8});
188+
189+
// a key which does not exist yet is created
190+
REQUIRE(registry.hasOption("aNewVector") == false);
191+
registry.override("aNewVector", std::vector<float>{1.f, 2.f});
192+
REQUIRE(registry.hasOption("aNewVector"));
193+
REQUIRE(registry.get<std::vector<float>>("aNewVector") == std::vector<float>{1.f, 2.f});
194+
195+
// a leaf value can be replaced by a subtree
196+
registry.override("aString", std::vector<std::string>{"now", "an", "array"});
197+
REQUIRE(registry.get<std::vector<std::string>>("aString") == std::vector<std::string>{"now", "an", "array"});
198+
}
199+
200+
TEST_CASE("TestConfigParamRegistryErase")
201+
{
202+
int l[2][2] = {{1, 2}, {3, 4}};
203+
// a Variant cannot be created from a temporary vector, so the defaults are named
204+
std::vector<int> defaultVector{1, 2, 3, 4, 5};
205+
std::vector<std::string> defaultStringVector{"a", "b", "c"};
206+
std::vector<double> defaultNestedVector{0.5, 1.5};
207+
// the store keeps a reference to the specs, which have to outlive it
208+
std::vector<ConfigParamSpec> specs{
209+
ConfigParamSpec{"anInt", VariantType::Int, 1, {"an int option"}},
210+
ConfigParamSpec{"aString", VariantType::String, "foo", {"a string option"}},
211+
ConfigParamSpec{"aVector", VariantType::ArrayInt, defaultVector, {"an int array option"}},
212+
ConfigParamSpec{"aStringVector", VariantType::ArrayString, defaultStringVector, {"a string array option"}},
213+
ConfigParamSpec{"aLabeledArray", VariantType::LabeledArrayInt, LabeledArray<int>{&l[0][0], 2, 2, {"r1", "r2"}, {"c1", "c2"}}, {"a labeled array option"}},
214+
ConfigParamSpec{"aNested.x", VariantType::Int, 2, {"an int option, nested in an object"}},
215+
ConfigParamSpec{"aNested.vec", VariantType::ArrayDouble, defaultNestedVector, {"a double array option, nested in an object"}},
216+
};
217+
auto store = std::make_unique<ConfigParamStore>(specs, std::vector<std::unique_ptr<ParamRetriever>>{});
218+
store->preload();
219+
store->activate();
220+
ConfigParamRegistry registry(std::move(store));
221+
222+
// a leaf value is removed
223+
REQUIRE(registry.hasOption("anInt"));
224+
registry.erase("anInt");
225+
REQUIRE(registry.hasOption("anInt") == false);
226+
REQUIRE(registry.isSet("anInt") == false);
227+
REQUIRE(registry.isDefault("anInt") == false);
228+
REQUIRE_THROWS_AS(registry.get<int>("anInt"), std::invalid_argument);
229+
230+
// arrays are removed as whole subtrees
231+
registry.erase("aVector");
232+
REQUIRE(registry.hasOption("aVector") == false);
233+
REQUIRE(registry.isSet("aVector") == false);
234+
registry.erase("aLabeledArray");
235+
REQUIRE(registry.hasOption("aLabeledArray") == false);
236+
REQUIRE(registry.isSet("aLabeledArray") == false);
237+
238+
// a nested key is removed without touching its siblings
239+
registry.erase("aNested.vec");
240+
REQUIRE(registry.hasOption("aNested.vec") == false);
241+
REQUIRE(registry.get<int>("aNested.x") == 2);
242+
243+
// other options are untouched
244+
REQUIRE(registry.get<std::string>("aString") == "foo");
245+
REQUIRE(registry.get<std::vector<std::string>>("aStringVector") == std::vector<std::string>{"a", "b", "c"});
246+
247+
// missing keys are ignored
248+
REQUIRE_NOTHROW(registry.erase("anInt"));
249+
REQUIRE_NOTHROW(registry.erase("noSuchKey"));
250+
REQUIRE_NOTHROW(registry.erase("noSuchParent.x"));
251+
REQUIRE_NOTHROW(registry.erase("aNested.noSuchKey"));
252+
REQUIRE(registry.get<int>("aNested.x") == 2);
253+
254+
// an erased key can be stored again
255+
registry.override("anInt", 3);
256+
REQUIRE(registry.get<int>("anInt") == 3);
257+
registry.override("aVector", std::vector<int>{7, 8});
258+
REQUIRE(registry.get<std::vector<int>>("aVector") == std::vector<int>{7, 8});
259+
}

‎Framework/Core/test/test_WorkflowSerialization.cxx‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,7 @@ TEST_CASE("TestVerifyWorkflowSerialization")
3333
ConfigParamSpec{"aFloat", VariantType::Float, 1.3, {"A Float"}}, //
3434
ConfigParamSpec{"aBool", VariantType::Bool, true, {"A Bool"}}, //
3535
ConfigParamSpec{"aString", VariantType::String, "some string", {"A String"}}, //
36-
ConfigParamSpec{"aLabeledArray", VariantType::LabeledArrayInt, labeled, {"A LabeledArray"}}}}, // //
36+
ConfigParamSpec{"aLabeledArray", VariantType::LabeledArrayInt, labeled, {"A LabeledArray"}}}}, //
3737
DataProcessorSpec{"B", //
3838
{InputSpec{"foo", "C", "D"}}, //
3939
{ //

0 commit comments

Comments
 (0)