Feature/config params #5

antons-it · 2025-02-18T08:14:33Z

Enable plugins to request parameters from config file
Note: Do not merge yet, PR is currently for discussion only

…le in the data

FussyDuck · 2025-02-18T08:14:38Z

All committers have signed the CLA.

codecov-commenter · 2025-02-18T08:22:47Z

Codecov Report

Attention: Patch coverage is 66.85393% with 118 lines in your changes missing coverage. Please review.

Project coverage is 40.76%. Comparing base (0355966) to head (22565c6).

Files with missing lines	Patch %	Lines
examples/example3.F90	0.00%	38 Missing ⚠️
examples/example2.cc	0.00%	21 Missing ⚠️
examples/example1.F90	0.00%	9 Missing ⚠️
examples/example1.cc	0.00%	9 Missing ⚠️
examples/example3.cc	0.00%	9 Missing ⚠️
src/plume/data/ModelData.cc	0.00%	8 Missing ⚠️
examples/example2.F90	0.00%	7 Missing ⚠️
examples/plugin_bar.cc	0.00%	6 Missing ⚠️
examples/plugin_foo.cc	0.00%	3 Missing ⚠️
src/plume/Configurable.cc	85.71%	2 Missing ⚠️
... and 4 more

Additional details and impacted files

@@             Coverage Diff             @@
##           develop       #5      +/-   ##
===========================================
+ Coverage    36.59%   40.76%   +4.17%     
===========================================
  Files           44       53       +9     
  Lines         1585     1820     +235     
  Branches        67       93      +26     
===========================================
+ Hits           580      742     +162     
- Misses        1005     1078      +73

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

cducher · 2025-03-07T08:55:41Z

src/plume/ManagerConfig.h

+    // 1) in this case, we can check that each contained plugin has a valid configuration
+    if (this->config().has("plugins")) {
+        try {
+            auto pconfigs = this->config().getSubConfigurations("plugins");


There are lookups methods on data types now in eckit, which could be used to avoid the try/catch group (see here) but I don't think there is a release that contains these changes at the moment. I am quite heavily using them as well, do you think we could ask for a release ? This would change to something like:

if (!this->config().isSubConfigurationList("plugins")) { throw eckit::BadValue("ManagerConfig: plugins must be a list of configurations", Here()); }

This way we can safely make the call to getSubConfigurations in the for loop. What do you think?

Are you referring to the changes that introduce reflection, @cducher ? I thought they have been merged/released already...

cducher · 2025-03-07T09:12:39Z

src/plume/Negotiator.cc

+            if (group_satisfied) {
+                eckit::Log::info() << " ---> Parameter Group Accepted!" << std::endl;
+                for (const auto& param : params) {
+                    if (std::find_if(allRequestedParams.begin(), allRequestedParams.end(), 


Maybe that's where using a set or unordered_set would help keep things a bit simpler and more efficient since you're not interested in duplicates. Then you can recast it as a vector in your return. Since you're also not interested in the result of the insertion, this for loop can become:

for (const auto& param: params) { allRequestedParams.insert(param.getString("name")); }

geier1993 · 2025-03-07T09:50:09Z

src/plume/Configurable.cc

@@ -50,11 +54,14 @@ CheckedConfigurable::~CheckedConfigurable() {

 }

-bool CheckedConfigurable::isValid(const eckit::Configuration& config, const std::vector<std::string> essentialKeys, const eckit::Configuration& options) {
+bool CheckedConfigurable::isValid(const eckit::Configuration& config,
+                                  const std::vector<std::string> essentialKeys,


I don't know where these keys are coming from, but we may use a std::unordered_set<std::string> (https://en.cppreference.com/w/cpp/container/unordered_set) in general???

Yeah, std::set (or std::unordered_set) seem to make more sense. Also, pass by const ref.

dsarmany

Thank you for this. It is thorough work. Many thanks especially for the tests and examples; they will be very useful.

See my comments. They are mostly about style, clarity and simplification.

dsarmany · 2025-02-27T15:31:24Z

src/plume/Negotiator.h

+class Negotiator {
+public:
+
+    Negotiator();


Define this as default or not at all (it will be synthetised).

dsarmany · 2025-02-27T15:33:48Z

src/plume/Negotiator.h

+     * @param config_params 
+     * @return PluginDecision 
+     */
+    PluginDecision negotiate(const Protocol& offers, const Protocol& requires, const std::vector<eckit::LocalConfiguration>& config_params);


Why not use a single constructor with a default third parameter?

dsarmany · 2025-02-27T15:35:25Z

src/plume/Negotiator.cc

+    // 1) Check requested parameters (from the plugin)
+    // if ANY one of these parameters is not offered, the plugin is rejected
+    eckit::Log::info() << "Requesting Parameters: [";
+    for (int i = 0; i < requested_params.size(); i++) {


You can use range-based for loops or even an algorithm.

Or you can keep the current way and do the printing a bit more elegantly by looping only up to size() -1 and print the last element in the same statement as the "]". That way, the ugly 'if' in the body of the for loop can be avoided.

dsarmany · 2025-03-12T10:28:14Z

src/plume/Configurable.cc

-CheckedConfigurable::CheckedConfigurable(const eckit::Configuration& config, const std::vector<std::string> essentialKeys, const eckit::Configuration& options) : Configurable{config} {
-    if (!isValid(config, essentialKeys, options)) {
+CheckedConfigurable::CheckedConfigurable(const eckit::Configuration& config,
+                                         const std::vector<std::string> essentialKeys,


We probably want to pass these by const ref.

dsarmany · 2025-03-12T10:29:31Z

src/plume/Configurable.cc

@@ -50,11 +54,14 @@ CheckedConfigurable::~CheckedConfigurable() {

 }

-bool CheckedConfigurable::isValid(const eckit::Configuration& config, const std::vector<std::string> essentialKeys, const eckit::Configuration& options) {
+bool CheckedConfigurable::isValid(const eckit::Configuration& config,
+                                  const std::vector<std::string> essentialKeys,


Yeah, std::set (or std::unordered_set) seem to make more sense. Also, pass by const ref.

src/plume/PluginConfig.h

dsarmany · 2025-03-12T11:53:52Z

src/plume/PluginHandler.h

 private:

    // internal Plugin ptr
    Plugin* pluginPtr_;

+    // store the plugin configuration
+    PluginConfig config_;
+
    // internal PluginCore ptr
    PluginCore* plugincorePtr_;


Could this be a std::unique_ptr? Who owns this object?

src/plume/Plugin_decision.h

dsarmany · 2025-03-12T11:57:07Z

tests/core/test_configs.cc

+
+CASE("test_manager_configuration") {
+
+    const char* mgr_configstr_valid = R"YAML({


I would define this as an std::string here already, but no big deal obviously...

dsarmany · 2025-03-12T11:59:50Z

examples/plugin_bar.cc

@@ -35,6 +35,20 @@ PluginCoreBar::PluginCoreBar(const eckit::Configuration& conf) : PluginCore(conf
 PluginCoreBar::~PluginCoreBar() {}


See other comment about destructors declaration/definition.

antons-it added 3 commits February 4, 2025 12:42

plugins can request parameters from config

1f53bfd

updated examples with params from config

58f6cc5

added method to check which parameters of a specific type are availab…

e7704f7

…le in the data

antons-it requested review from cducher and dsarmany February 18, 2025 08:14

antons-it marked this pull request as draft February 23, 2025 16:27

plugin parameters from config can now be defined in groups

3909779

antons-it marked this pull request as ready for review February 28, 2025 07:58

antons-it self-assigned this Feb 28, 2025

cducher reviewed Mar 7, 2025

View reviewed changes

geier1993 reviewed Mar 7, 2025

View reviewed changes

dsarmany requested changes Mar 12, 2025

View reviewed changes

antons-it added 15 commits March 17, 2025 15:19

use unordered_set (and add missing const ref)

35a12df

config checks in separate functions

635dc85

use default destructor

2e00b19

added final

63bd0b1

minor

37c174e

use default destructor

9958262

print statement

abeebcb

remove redundant destructor declarations/definitions

f235fac

use eckit config introspection methods

6e12d49

use algorithm instead of explicit loop

d717b72

use set instead of vector

fadc88a

removed redundant empty constructor

a2b4ea4

replaced overloaded function with default param

1cee58b

improved loop

b7fe465

use eckit config introspection methods

df9061a

use std::string

22565c6

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/config params #5

Feature/config params #5

antons-it commented Feb 18, 2025

FussyDuck commented Feb 18, 2025 •

edited

Loading

codecov-commenter commented Feb 18, 2025 •

edited

Loading

cducher Mar 7, 2025 •

edited

Loading

dsarmany Mar 12, 2025

cducher Mar 7, 2025

geier1993 Mar 7, 2025

dsarmany Mar 12, 2025

dsarmany left a comment

dsarmany Feb 27, 2025

dsarmany Feb 27, 2025

dsarmany Feb 27, 2025

dsarmany Mar 12, 2025

dsarmany Mar 12, 2025

dsarmany Mar 12, 2025

dsarmany Mar 12, 2025

dsarmany Mar 12, 2025

dsarmany Mar 12, 2025


		CASE("test_manager_configuration") {

		const char* mgr_configstr_valid = R"YAML({

		@@ -35,6 +35,20 @@ PluginCoreBar::PluginCoreBar(const eckit::Configuration& conf) : PluginCore(conf
		PluginCoreBar::~PluginCoreBar() {}

Feature/config params #5

Are you sure you want to change the base?

Feature/config params #5

Conversation

antons-it commented Feb 18, 2025

FussyDuck commented Feb 18, 2025 • edited Loading

codecov-commenter commented Feb 18, 2025 • edited Loading

Codecov Report

cducher Mar 7, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dsarmany left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FussyDuck commented Feb 18, 2025 •

edited

Loading

codecov-commenter commented Feb 18, 2025 •

edited

Loading

cducher Mar 7, 2025 •

edited

Loading