Issue764

Title Remove global variables for plan storage
Priority wish Status resolved
Superseder Nosy List florian, jendrik, malte
Assigned To florian Keywords
Optional summary

Created on 2018-03-15.12:17:22 by florian, last changed by florian.

Messages
msg7005 (view) Author: florian Date: 2018-04-04.16:10:03
Merged and pushed. Thanks Jendrik!
msg7004 (view) Author: jendrik Date: 2018-04-04.15:58:26
Florian addressed my comments. From my point of view this can be merged.
msg7003 (view) Author: jendrik Date: 2018-04-04.12:21:49
That makes sense. I have no further comments.
msg7001 (view) Author: malte Date: 2018-04-04.11:04:58
I think global objects are best avoided; they reduce flexibility. The
--internal-plan-file and --internal-previous-portfolio-plans options are obvious
hacks, too, and would more naturally be part of the search algorithm options in
the same way that the passed-in bounds are. One of the reasons they currently
aren't is that the option parser doesn't support string arguments.

If we ever later want to allow specifying multiple searches, it would make sense
for them to operate on different tasks and write to different sources. More
generally, I think the option syntax should eventually be unified further so
that we use the same sort of mechanism for everything. Currently we have a bit
of a mix between the component-based option parser and traditional --option
style options. I think it would make sense for the latter ones to eventually go
away, except perhaps for things like --help with are general conventions.
msg6999 (view) Author: jendrik Date: 2018-04-04.10:50:02
I left some minor comments. 

The current patch adds a plan manager to the search engine. This works since we 
currently only allow having a single search engine (which is sometimes an 
iterated search engine). I wonder whether we shouldn't have a single global plan 
manager. This would allow having multiple search engines later and is better 
aligned with the fact that we also only allow passing one value for --internal-
plan-file and --internal-previous-portfolio-plans. In addition, the PlanManager 
class would not need any "setter" methods, since we could construct the plan 
manager once all relevant attributes are known.
msg6998 (view) Author: jendrik Date: 2018-04-04.09:38:30
I'll have a look at the code.
msg6990 (view) Author: malte Date: 2018-04-04.00:08:27
Thanks! Can you find a reviewer? I don't see why experiments should be necessary.
msg6989 (view) Author: florian Date: 2018-04-04.00:06:08
https://bitbucket.org/FlorianPommerening/downward-issues/pull-requests/41

I created a pull request for this. Despite the large number of changed files,
this should be easy to review (most changes just remove unnecessary includes).

Do we need experiments for this? I tried around locally with different ways of
passing the parameters: using aliases, satisficing/optimal search, calling the
binary directly with the internal parameters, etc. All my tests were successful.
msg6856 (view) Author: florian Date: 2018-03-15.12:17:22
g_plan_filename, g_num_previously_generated_plans, and
g_is_part_of_anytime_portfolio can be collected in a class that handles the
creation and storage of plans.
History
Date User Action Args
2018-04-04 16:10:03floriansetstatus: reviewing -> resolved
messages: + msg7005
2018-04-04 15:58:26jendriksetmessages: + msg7004
2018-04-04 12:21:49jendriksetmessages: + msg7003
2018-04-04 11:04:58maltesetmessages: + msg7001
2018-04-04 10:50:02jendriksetmessages: + msg6999
2018-04-04 09:38:30jendriksetmessages: + msg6998
2018-04-04 00:08:27maltesetmessages: + msg6990
2018-04-04 00:06:09floriansetstatus: unread -> reviewing
messages: + msg6989
2018-03-15 12:17:22floriancreate