Issue586

Title make plug-in types fully pluggable
Priority wish Status resolved
Superseder Nosy List florian, jendrik, malte
Assigned To malte Keywords
Optional summary
1. ~~Get rid of the need to define TokenParser::parse in option_parser.h.~~
2. Get rid of manual calls to get_help_templ() in two places in
   option_parser.cc. => deferred; see issue588.
3. ~~Get rid of need to define a TypeNamer for the new type in
   option_parser_util.h.~~
4. ~~Get rid of the TypeDocumenter specializations in option_parser_util.h.~~

~~Before merging, grep the code for "issue586" to make sure all TODOs marked in
the code are dealt with.~~ => some TODOs remain that are now marked as
"post-issue586". These will be addressed in issue588.

Created on 2015-10-28.15:33:41 by malte, last changed by malte.

Summary
1. Get rid of the need to define TokenParser::parse in option_parser.h.
2. Get rid of manual calls to get_help_templ() in two places in
   option_parser.cc. => deferred; see issue588.
3. Get rid of need to define a TypeNamer for the new type in
   option_parser_util.h.
4. Get rid of the TypeDocumenter specializations in option_parser_util.h.

Before merging, grep the code for "issue586" to make sure all TODOs marked in
the code are dealt with. => some TODOs remain that are now marked as
"post-issue586". These will be addressed in issue588.
Messages
msg4756 (view) Author: malte Date: 2015-11-10.12:57:13
Yes, I will close issue286 and also issue266 referenced from there. Thanks for
the pointer!
msg4754 (view) Author: florian Date: 2015-11-10.11:49:29
Does this also resolve issue286?
msg4728 (view) Author: malte Date: 2015-11-01.12:01:06
> If you want to see the the wiki-formatted text, you could import and call
> build_planner() and get_pages_from_planner() from autodoc.py.

That's effectively what I did.
msg4727 (view) Author: florian Date: 2015-11-01.00:02:50
For the record: you can locally test the wiki output with the options "--help
--txt2tags" but this is unformatted txt2tags code. If you want to see the the
wiki-formatted text, you could import and call build_planner() and
get_pages_from_planner() from autodoc.py. Alternatively, we could add a dry-run
option to autodoc, that stops the scripts before uploading.
msg4725 (view) Author: malte Date: 2015-10-31.21:04:03
OK, it was indeed easy to fix. Marking as resolved.
msg4724 (view) Author: malte Date: 2015-10-31.18:47:50
Merged. The followup issue is issue588.

However, the buildbot does complain. At first glance, it looks like this is
because we now verify that we don't insert duplicate keys into plugin
registries, and apparently the open list registration code does introduce
duplicate entries. If this is as easy to fix as I hope, I'll do it directly in
the default branch.
msg4722 (view) Author: malte Date: 2015-10-31.18:30:52
Done, but the real test will be what happens to the auto-docs once this is
merged. The further changes after Florian's review were pretty much
straight-forward, so I hope it's OK if I merge this without further review
(assuming that Florian is currently travelling).

Working with the option parser code reminded me of some open issues there, and
it's perhaps a good idea to open a meta issue for that.
msg4721 (view) Author: malte Date: 2015-10-31.14:00:57
Thanks for the feedback, Florian!

The basic code for 4. is now also done, but it's a bit hard to test this because
the TypeDocumenter doesn't affect the regular --help output, only what is
printed in "wiki mode", and I have yet to find out how to test the wiki mode
output offline.

Still, this is mostly done. The main work that still remains to be done is
actually using the new plugin type plugins.
msg4716 (view) Author: malte Date: 2015-10-30.22:33:34
Here is a link to the pull request:
https://bitbucket.org/malte/downward/pull-requests/2/issue586/diff
msg4715 (view) Author: malte Date: 2015-10-30.22:30:56
I've pushed some changes that address 3 and made a pull request because now
might be a good opportunity for feedback. There are many TODOs in the code, but
all the main pieces for 1. and 3. are in place, and 4. should be mechanical given 3.

The main work that isn't done yet is actually introducing most of the
plugin-type plugins -- I wanted to wait with this until we're more certain of
the design.
msg4713 (view) Author: florian Date: 2015-10-30.18:54:40
Sounds good to me. I prefer having multiple small issues to one large one.
msg4710 (view) Author: malte Date: 2015-10-30.18:37:14
Having worked on this some more, 3. and 4. are reasonably easy, but 2. is
tricky. There is a lot of code triggered on that code path which currently
depends on compile-time knowledge of the types in question and would need to be
converted to something that only knows the types at runtime. This would require
more major restructuring of the option parser code than I expected.

Perhaps it would be better to first merge only the changes for 1., 3. and 4 and
leave 2. to a separate issue so that this one can be merged more quickly and is
more reviewable.
msg4707 (view) Author: malte Date: 2015-10-30.18:21:21
Looks like I accidentally classified this as a bug. Fixed.
msg4688 (view) Author: malte Date: 2015-10-28.18:50:24
Ah, there's also optional documentation for the plug-in type that we don't
always use but should definitely support (TypeDocumenter). I've added a TODO
list to the summary.
msg4687 (view) Author: malte Date: 2015-10-28.18:46:51
I've started working on the code for the non-predefinable case. There are
currently three things that need to be done when adding a new type to the option
parser.

1. Define TokenParser::parse for the new type in option_parser.h.
2. Call get_help_templ() for the new type in two places in option_parser.cc.
3. Define a TypeNamer for the new type in option_parser_util.h.

Step 1. is done in the issue branch
(https://bitbucket.org/malte/downward/branch/issue586). The other two steps are
only needed for the help output, but since this generates our wiki
documentation, this is of course important, too. For this we need a new kind of
plug-in. I think it won't be very complicated, but it may take me a while to
find time for it.
msg4686 (view) Author: malte Date: 2015-10-28.17:48:44
I looked into this a bit. Some things look easy, some not so easy. I'll try to
describe some of the issues involved.


Firstly, we have a distinction between "predefinable" plug-in types and ones
that are not predefinable.

Predefinable types can be "assigned to a variable" on the command line, as in
"--heuristic h=ff()". These can then be used in multiple places. For example, we
can use "h" both in a list of heuristics to evaluate and in a list of heuristics
to use for preferred operators. The only plug-in types that are currently
predefinable are heuristics (plus scalar evaluators, see below) and landmark
factories.

It is not complicated to make additional plug-in types predefinable, but our
code is not prepared for it because the internal state of the objects assumes
that it is used in only one context. For example, using the same merge strategy
object within two different merge-and-shrink heuristic objects is not going to
work well because it is likely to have internal state that is related to its
unique user. Given this, I'll start by only making the non-predefinable plugin
types fully pluggable. This is the simpler and more common case. We can later
make predefinable plugin types pluggable.


Secondly, inheritance complicates the plug-in code. In places where we currently
accept scalar evaluators, we must make sure to support both "ScalarEvaluator"
plugins and "Heuristic" plug-ins. We have no general mechanism for this: the
option parser code directly knows the necessary information about the
inheritance hierarchy and contains code to handle it correctly.

I think the cost of this feature is higher than its utility. Actually, I think
the utility is negative because exposing this inheritance relationship to the
user complicates the documentation etc. So my suggestion is to get rid of
inherited plugin types in the medium term. The alternative would be to
strengthen the support for inheritance between plugin types (which would
complicate this issue, but so be it).
msg4685 (view) Author: malte Date: 2015-10-28.15:33:41
"Plug-ins" are currently fully pluggable: for example, we can add a new
heuristic, shrink strategy or landmark generation method without touching any
code, only adding new source files.

"Plug-in types" are currently not pluggable: if we want to add a new *kind* of
plug-in, we need to touch various places in the option parser code. For example,
we had to do this when we added plug-ins for shrink strategies, merge
strategies, labels and (in issue527, not merged yet) constraint generators.

The suggestion is to change the option parser code so that plug-in types can be
added in a decentralized fashion.
History
Date User Action Args
2015-11-10 12:57:13maltesetmessages: + msg4756
2015-11-10 11:49:29floriansetmessages: + msg4754
2015-11-01 12:01:06maltesetmessages: + msg4728
2015-11-01 00:02:50floriansetmessages: + msg4727
2015-10-31 21:04:28maltesetstatus: chatting -> resolved
2015-10-31 21:04:03maltesetmessages: + msg4725
2015-10-31 18:47:50maltesetmessages: + msg4724
summary: 1. ~~Get rid of the need to define TokenParser::parse in option_parser.h.~~ 2. Get rid of manual calls to get_help_templ() in two places in option_parser.cc. => deferred to a later issue 3. ~~Get rid of need to define a TypeNamer for the new type in option_parser_util.h.~~ 4. ~~Get rid of the TypeDocumenter specializations in option_parser_util.h.~~ ~~Before merging, grep the code for "issue586" to make sure all TODOs marked in the code are dealt with.~~ => some TODOs remain that are now marked as "post-issue586" -> 1. ~~Get rid of the need to define TokenParser::parse in option_parser.h.~~ 2. Get rid of manual calls to get_help_templ() in two places in option_parser.cc. => deferred; see issue588. 3. ~~Get rid of need to define a TypeNamer for the new type in option_parser_util.h.~~ 4. ~~Get rid of the TypeDocumenter specializations in option_parser_util.h.~~ ~~Before merging, grep the code for "issue586" to make sure all TODOs marked in the code are dealt with.~~ => some TODOs remain that are now marked as "post-issue586". These will be addressed in issue588.
2015-10-31 18:30:52maltesetmessages: + msg4722
summary: 1. ~~Get rid of the need to define TokenParser::parse in option_parser.h.~~ 2. Get rid of manual calls to get_help_templ() in two places in option_parser.cc. 3. Get rid of need to define a TypeNamer for the new type in option_parser_util.h. 4. Get rid of the TypeDocumenter specializations in option_parser_util.h. Before merging, grep the code for "issue586" to make sure all TODOs marked in the code are dealt with. For now, we only cover plugin types that don't support predefinitions. Predefinitions will be handled later. -> 1. ~~Get rid of the need to define TokenParser::parse in option_parser.h.~~ 2. Get rid of manual calls to get_help_templ() in two places in option_parser.cc. => deferred to a later issue 3. ~~Get rid of need to define a TypeNamer for the new type in option_parser_util.h.~~ 4. ~~Get rid of the TypeDocumenter specializations in option_parser_util.h.~~ ~~Before merging, grep the code for "issue586" to make sure all TODOs marked in the code are dealt with.~~ => some TODOs remain that are now marked as "post-issue586"
2015-10-31 14:00:57maltesetmessages: + msg4721
2015-10-30 22:33:34maltesetmessages: + msg4716
2015-10-30 22:30:56maltesetmessages: + msg4715
summary: 1. ~~Get rid of the need to define TokenParser::parse in option_parser.h.~~ 2. Get rid of manual calls to get_help_templ() in two places in option_parser.cc. 3. Get rid of need to define a TypeNamer for the new type in option_parser_util.h. 4. Get rid of the TypeDocumenter specializations in option_parser_util.h. -> 1. ~~Get rid of the need to define TokenParser::parse in option_parser.h.~~ 2. Get rid of manual calls to get_help_templ() in two places in option_parser.cc. 3. Get rid of need to define a TypeNamer for the new type in option_parser_util.h. 4. Get rid of the TypeDocumenter specializations in option_parser_util.h. Before merging, grep the code for "issue586" to make sure all TODOs marked in the code are dealt with. For now, we only cover plugin types that don't support predefinitions. Predefinitions will be handled later.
2015-10-30 18:54:40floriansetmessages: + msg4713
2015-10-30 18:37:14maltesetmessages: + msg4710
2015-10-30 18:21:21maltesetpriority: bug -> wish
messages: + msg4707
2015-10-28 18:50:42maltesetsummary: 1. ~~Get rid of the need to define TokenParser::parse in option_parser.h.~~ 2. ~~Get rid of manual calls to get_help_templ() in two places in option_parser.cc.~~ 3. ~~Get rid of need to define a TypeNamer for the new type in option_parser_util.h.~~ 4. ~~Get rid of the TypeDocumenter specializations in option_parser_util.h.~~ -> 1. ~~Get rid of the need to define TokenParser::parse in option_parser.h.~~ 2. Get rid of manual calls to get_help_templ() in two places in option_parser.cc. 3. Get rid of need to define a TypeNamer for the new type in option_parser_util.h. 4. Get rid of the TypeDocumenter specializations in option_parser_util.h.
2015-10-28 18:50:24maltesetmessages: + msg4688
summary: 1. ~~Get rid of the need to define TokenParser::parse in option_parser.h.~~ 2. ~~Get rid of manual calls to get_help_templ() in two places in option_parser.cc.~~ 3. ~~Get rid of need to define a TypeNamer for the new type in option_parser_util.h.~~ 4. ~~Get rid of the TypeDocumenter specializations in option_parser_util.h.~~
2015-10-28 18:46:51maltesetmessages: + msg4687
2015-10-28 17:48:44maltesetmessages: + msg4686
2015-10-28 15:40:41jendriksetnosy: + jendrik
2015-10-28 15:34:46maltesetnosy: + florian
2015-10-28 15:33:41maltecreate