part of issue757 and issue559
relates to issue1081

PR: https://github.com/aibasel/downward/pull/218

The issue changed the way the user communicates with the planner. 
Some strings that were accepted in older versions are no longer acepted from now on.
This affects only feature calls with positional arguments. We recommend to use keyword arguments instead of positional arguments.

Not only the parameter order changed but the string parameter *description* was added to all evaluators and searchalgorithms.

Listing the features that changed their signature:
- All Evaluatros
- MergeTree
	-linear merge tree
- OpenList
	- epsilon_greedy
	- pareto
	- tiebreaking
- PatternCollectionGenerator
	- disjoint_cegar
	- multiple_cegar
	- random_patterns
- PatternGenerator
	- cegar_pattern
	- random_pattern
- All SearchAlgorithms
- ShrinkStrategy
	- shrink_fh

I adjusted the PR to the review comments and merged it to main.

With that the issue is resolved \o/ many thanks to all contributors.

However, resolving one issue reveals two new potential issues:
One is about the parameter test. Florian suggested a less fragile approach in a 
review comment on GitHub
"In general, I think it is a bad idea to implement a parser for C++ code on your 
own (adding this as a comment here because there is no other specific point in 
the code for it). The parsing functions based on regular expressions are fragile 
and only work under a lot of assumptions of what the code looks like. A better 
solution would be to use a specialized C++ parser such as clang (which has a 
Python interface, see [1] and [2]). But since is just a test, I'm fine with 
leaving it as is as long as it works. I didn't go through the rest of the class 
in full detail.

[1] https://libclang.readthedocs.io/en/latest/
[2] https://eli.thegreenplace.net/2011/07/03/parsing-c-in-python-with-clang/"

The other is about the lazy_evaulator that is a parameter of the EagerSearch cpp 
class but not of the eager feature (but only for the astar feature).

On discord we discussed this and Malte said:
"The original reason for this is that lazy_eval evolved from the previous "mpd" 
option which would have crashed when used with general open lists.
Now that the mechanism has changed, the restriction might no longer be 
necessary, but that would require careful checking of the assumptions in this 
code."

During and after the January-February Sprint we worked on this issue. The communication happened mainly on the FastDownward Discord server in a 
private channel. I want to summarize what we discussed, to keep the issue tracker updated and have the information in a long term storage.

We had a meeting about the prototype of issue559 and decided that we are happy with the high level design of the prototype (ref msg11496).

On a second meeting we discussed the order of parameters (and if we want to specify them individually)
There, we decided on the following two things:
(1) We will specify the parameters individually (as we decided before the sprint, but we reopened the discussion following Alvaro's input).
(2) We also looked at the order in which the parameters are specified because this will be more work to change after we've done all the 
individual constructors. For example for evaluators, the verbosity parameter (and other "inherited" parameters") usually appear at the end of the 
parameter list, but sometimes also at the start and even sometimes in the middle. We decided that the convention we want to follow is to have 
them at the end. The main rationale for this is that in the cases where we want to use arguments positionally, they will generally be "the 
natural main parameter for the class", as in "landmark_heuristic(landmark_factory)" rather than the options that will end up being generic for 
all features (such as verbosity). This is also the convention we currently use in the overwhelming majority of cases Malte looked at, but we 
didn't look at all feature types.

More specifically, we agreed to have the parameter order in "blocks" like 
hm([hm parameters], [heuristic parameters], [evaluator parameters], [component parameters])

Instead of adding the parameter 'name' to the features we added the parameter 'description' to the features because the name would be used in 
cases where description is used already, and it would clash for example in the Pattern generators as they already use a variable 'name'.


With these decisions we (Claudia, Clemens, Florian, Tanja and me) shared the work to multiple people that worked in parallel according to this 
document:

= = = = =
Mechanical changes: <TODO issue1082 remove this file>

- Choose one class with ByOptions-constructor [e.g. HMHeuristic] and announce on discord your choice to block it for others.
- Check all classes that are higher up in the hierarchy [e.g. Heuristic, Evaluator] starting with the root [e.g. Evaluator]:

	- Each of these base classes keep their ByOptions-constructor but get a `// TODO issue1082 remove this` comment (until all its children 
are updated, then it can be removed).
    - If not there yet, add flat constructors.
      - The flat constructor is not `explicit` if it receives more than one parameter.
      - It expects the arguments in the same order as its base class, new introduced parameter are added to the front.
    The new introduced parameters are the ones that have a `feature.add_option<Foo>(
        "my_bar",
        "help txt",
        "my_default");` in the ComponentFeature class.
      The inherited parameters are the one added to the feature by a `add_xyz_options_to_feature`
      - If the base class has a `add_<base>_options_to_feature`, then add a `get_<baseComponent>_arguments_from_options` function that returns a 
tuple with the parameters to call the constructor of that component (in the fitting order). [cf. src/search/evaluator.cc]
      - If the constructor uses `opts.get_unparsed_configs`, then adjust the `add_<component>_options_to_feature` to expect a string parameter to 
forward the fitting default string. You might have to overload the function that eventually calls 'add_<baseComponent>_options_to_feature' with 
such an additional string parameter. If you do, then mark the old one with `// TODO issue1082 remove` [cf. add_evaluator_options_to_feature].


- For the chosen class and all its children:
  - Replace the ByOptions-constructor with a flat constructor.
If needed, use one of the overloaded `add_<whatever>_options` calls to forward the default description. It should be the same as the feature_key 
(the string used to call the TypedFeature constructor in the XyzFeature constructor).
  - If not already there, add a `create_component` function in the XyzFeature. The body is simply:
    `return plugins::make_shared_from_arg_tuples<Xyz>(
opts.get<int>("a"),
// ... further new introduced parameters extracted from options
opts.get<double>("z"),
get_<A>_arguments_from_options(opts),
// ... in the order as the add_<A-Z>_options_to_feature was called above.
get_<Z>_arguments_from_options(opts)
);`
[cf. src/search/heuristics/hm_heuristic.cc]
  - **ELSE** there is probably something acting on a copy of the options object.
  use that opts_copy to call `plugins::make_shared_from_args_tuple_and_args<Xyz>` like above but with `opts_copy`.
[cf. merge_and_shrink/merge_scoring_function_miasm.cc]


Confirm with `./builds/release/bin/downward --help --txt2tags <feature_key>` that the order and parameter names are the same as they would appear 
in the wiki as in the constructor. If they are not, then change the order of the 'add_options' calls and/or the parameters in the flat 
constructor.
DriveBy cleanup: Do we need the destructor? and is there a reason why we cannot use = default? Our convention is to leave it out if it is using 
its default.


Change constructor calls in other files.
If possible remove the plugins include as plugins::Options is not needed anymore.
[cf. src/search/cartesian_abstractions/utils.cc]
= = = = = 

The make_shared_from_arg_tuples<T> takes the arguments (which are either singletons or tuples), puts them into one big, flat tuple and uses the 
std::apply function with this tuple and make_shared<T>.

Afterwards, Tanja and I worked on an automated check to confirm that the parameters of the command line feature are the same as the parameters of 
the corresponding C++ constructor.

Florian and Malte helped me to correctly add this check to the GitHub actions.
It is now part of the Ubuntu 'Run driver, translator and search test'. The artifact that archives the required files for the tests now contains 
the src-files, too.

There are 3 kinds of exceptions for the parameter check:
(1) SHORT_HANDS such as 'astar' and 'ipdb'

(2) TEMPORARY_EXCEPTIONS
    "iterated" issue559 will take care of this
    "eager" the C++ constructor expects a lazy_evaluator which the feature does not
            the lazy_evaluator is used for astar.
    
    "sample_based_potentials",
    "initial_state_potential",
    "all_states_potential",
    "diverse_potentials" These 4 are of the same kind. 
    If they were not marked as exceptions the check would return this complaint:
    == FEATURE PARAMETERS 'all_states_potential'==
['max_potential,', 'lpsolver,', 'transform,', 'cache_estimates,', 'description,', 'verbosity,']
== CLASS PARAMETERS 'PotentialHeuristic'==
['function,', 'transform,', 'cache_estimates,', 'description,', 'verbosity,']


There is no parameter for 
function
 in the feature. Instead, the function is constructed inside the 
create_component
 call based on the feature parameters 
max_potential, lpsolver, transform
  and an optimization_function.

This looks like:

virtual shared_ptr<PotentialHeuristic> create_component(
        const plugins::Options &opts,
        const utils::Context &) const override {
        return make_shared<PotentialHeuristic>( 
            create_potential_function(
                opts.get<shared_ptr<AbstractTask>>("transform"),
                opts.get<lp::LPSolverType>("lpsolver"),
                opts.get<double>("max_potential"),
                OptimizeFor::ALL_STATES),
            opts.get<shared_ptr<AbstractTask>>("transform"),
            opts.get<bool>("cache_estimates"),
            opts.get<string>("description"),
            opts.get<utils::Verbosity>("verbosity")
            );
    }
I think it would be nicer to either have the function as separate feature (adjusting the feature to the CC-class) 
or
just forward (max_potential, lpsolver, transform)
 to the constructor and construct the function there (adjusting the CC-class to the feature)

For the second option there is a problem with the optimization_function. Either we change the constructor and feature to expect one or we make 
derived classes for PotentialHeuristics for the different features. 

These temporary exceptions are out of scope for this issue.

(3) PERMANENT_EXCEPTIONS 
    "adapt_costs"
The corresponding C++ constructor expects another task as parent. For this we use the g_root_task.

Additionally, I decided to do a further driveby-cleanup and split lines like
class ChildParentFeature : public plugins::TypedFeature<Parent, Child> {

to
class ChildParentFeature
    : public plugins::TypedFeature<Parent, Child> {

as these lines tend to be very long and exceed the websites style guide 
"Some points on which we differ from K&R or where K&R does not specify a rule:
- Line width is 72 characters if possible, 80 characters max."
(I found some with length>150)

I think with that the issue is ready for a review. 232 files were changed. So it would make sense to share the work for the review.
PR: https://github.com/aibasel/downward/pull/218

> In the long term we might want to combine the parameters 'name' and
> 'verbosity' to 'logger'.

Simon and I discussed a things related to this:

- Once we have a logger class, it seems natural to me to have name as the first attribute (because it's more fundamental) and the verbosity as the second. This is also how Python's logging.Logger does it (and has to do it because name is mandatory and verbosity/log level is optional).

- As long as we haven't consolidated this into a logger class yet, Simon has a preference of having verbosity first and name second. I have no strong preference.

- At the moment, where we don't have a logger class yet, our current implementation strategy is that the "name" attribute has a different default value for different classes, so we have an "inherited" attribute where the default value is not inherited but different in every derived class. This causes some code complexity, and it will become even more complex once the loggers are separated because it is less natural to have a separate logger object have a context-dependent default.

We might want to ask ourselves whether this complexity is warranted. An alternative is not to have these context-sensitive defaults. There are not so many scenarios where they provide significant value. In searches where we use multiple heuristics, they allow us to distinguish in the log which heuristic is which, but we might as well say "if you want this, assign names to the evaluators".

For the implementation strategy, we decided to first implement this for a subset of classes in the prototype for issue559 until we are happy with the prototype.

We discussed this live and talked about a couple of ways the name could be passed to the objects. There are essentially two kinds of names: one is the name of a component (similar to a __str__() method in Python) the other is the name of a logger object. Component names could used like this:
  f"new best heuristic value for heuristic {h.get_name()}: 42"
while logger names would be used like this:
  f"[{logger.get_name()}] new best heuristic value: 42"
The difference is that the logger name does not necessarily have to identify the object and it makes more sense to make it user-configurable, whereas a component name would not need to be configurable but should distinguish different objects of the same type.

While in theory we could have both component names and logger names, our long-term plan is to have configurable loggers, and we did not see an important use case for component names. As an intermediate step, we still want to introduce a configurable component name with the idea that this gets turned into a configurable logger name later on.

Short term, the syntax would look like this:
  ff(name="my_ff")
and long term, we want something like this:
  ff(logger=Logger(name="my_ff"))

In both cases, the names should have default values that identify the type of the component (not the identity of the object with all its parameters), so for example, any h^m heuristic would have a default name of "hm" (not "hm(m=2)").  

On a technical level, we can support the default values by passing them to the function add_evaluator_options_to_feature. We briefly talked about extracting the name from the feature that is passed to that function anyway but decided against it because it introduces additional coupling (the feature object is not fully constructed at that time, so virtual functions are problematic) and limits us to match the command-line name with the default value.

More generally, I think we have a misunderstanding regarding the goals of this parameter. I suggest we discuss this live between Simon, Florian and me. (If someone else wants to join, you are of course welcome -- please speak up here or on Discord.)

This assumes that the default name needs to be the command-line string, but that's currently only the case out of convenience. It would be perfectly fine for the default name to just be "hm".

Florian and I had a discussion about the name parameter. 
The constructor does not just need a default value for the name (like in the message I send before) but a sort of 'unparsing' 
step.

Something like:

HMHeuristic::HMHeuristic(string name, utils::Verbosity verbosity, const shared_ptr<AbstractTask> &transform, bool cache_estimates, 
int m=1)
        : Heuristic(name, verbosity, transform, cache_estimates),
          m(m),
      has_cond_effects(task_properties::has_conditional_effects(task_proxy)),
      goals(task_properties::get_fact_pairs(task_proxy.get_goals())) {
    if (name == ""){ 
      name = f"hm(m={m}, verbosity={verbosity}, transform={transform.name}, cache_estimates={cache_estimates})"; //unparsing
    }
    // ...
}

At the moment the constructors of Evaluator and SearchAlgorithm objects set their field 'description' with 
"description(opts.get_unparsed_config())".

We want the unpacked constructors to have the same 
parameters as in the wiki. (as said in issue559)
However, there is no parameter for the description.
I had a chat with Malte and we think changing the wiki the following way would be a good solution:

Example with the Blind Heuristic
From:
"blind(verbosity=normal, transform=no_transform(), cache_estimates=true)"
To:
"blind(name="blind", verbosity=normal, transform=no_transform(), cache_estimates=true)"


For this we would need string parameters. (issue1106: "Support string parameters in option parser")

In the long term we might want to combine the parameters 'name' and 'verbosity' to 'logger'.

This is also a relevant step for the component interaction problem issue559.

While discussing the Python interface, we decided that we want to construct our components (currently FFHeuristic, ..., later probably FFHeuristicBuilder) with constructors that mention all arguments explicitly, rather than passing in an Options objects and having the constructor look for the right arguments in it.

We want the boundary where the options are unpacked to be in the Feature class, which will mean that every feature will implement create_component to unpack the options and call the constructor in the correct way. This will add some code to each feature but that code would otherwise be in the component (looking up the right keywords in the options object).

As a side effect, this will make some template magic unnecessary that we currently have in features to avoid implementing create_component where a default implementation works.