I reported the VAL issue at https://github.com/KCL-Planning/VAL/issues/10.

PS: changing VAL to 64 bits wouldn't really be a solution, as we cannot afford
to use more than 4 GB of memory anyway if we don't want to risk swapping on the
compute nodes.

The remaining TODOs, which are unrelated to the purpose of this one
(pluggability), have been moved to issue610, a new meta-issue for open list
refactoring. Closing this one.

Augmenting the logging output of the driver script is now issue609.

We have decided not to do anything (like send a bug report) about the few runs
where VAL ran out of memory. We currently don't limit VAL's memory, so if we run
out of memory, it's because we hit the 32-bit ceiling. If these cases happen
more frequently in the future, we might try to do something about it then.

These two items have been removed from the TODO list.

First TODO item removed; it is now issue605.

The TODO part about updating experiments/README is done, so I removed it.

OK, the buildbot is happy now. I've added the list of things that still need to
be done (in other issues) to the summary. I'll defer marking this "resolved"
until this TODOs are done and/or captured somewhere else on the tracker. But
apart from this, this issue seems to be done. Thanks to both of you for your help!

I don't think there is currently a way to remotely build on the Windows build
slave. I cloned on that machine and compiled locally in the past, but I don't
know a way to do this remotely (assuming you are not in Basel on Sunday).

issue565 is about looking at the ignored warnings in more detail.

Thanks! :-)

I can now build further; unfortunately, there are further warnings in other
files, but this is progress. Do we have a way of testing a build on Windows
without pushing to the default branch in the master repository?

On a different note, looking at the file you mentioned, I found this:
"# Disable warnings that currently trigger in the code until we fix them."

Is there an open issue for this?

Ahh, we were answering at the same time.

You can ignore warnings in src/cmake_modules/FastDownwardMacros.cmake (string
from line 40). The line probably is:


set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} /wd4505")

Sorry for producing conflicts, I hope the merge wasn't too bad.

I looked at the buildbot log and just wanted to mention 2 things: 

You can ignore all the warnings:
  "Unknown compiler version - please run the configure tests and report the results"
they come from the old boost version that we currently ship with our code (it
doesn't know about the new Visual Studio compilers yet).  We could fix this at
some point by using a newer version of those files, but the warning is not
problematic, Boost does the right thing even without recognizing the compiler.

The next error (C2220) is just the cause of the warning below. Its similar to
"-Werror" in gcc.

The actual problem is the warning C4505.

> Merged. Unfortunately, the Windows buildbot complains; I'll look into it.

Seems to be a Visual Studio bug:
http://stackoverflow.com/questions/3051992/compiler-warning-at-c-template-base-class

Do we currently already disable certain Visual Studio warnings? If yes, I
suggest adding C4505 to the list of ignored warnings. Do you know how to do this?

Merged. Unfortunately, the Windows buildbot complains; I'll look into it.

I now realize I didn't change the issue481-base tag after the last time I merged
from default, so the changes are confused with changes in default. Still, they
are so tiny that no further experiments are needed, I think.

Unfortunately I was 10 minutes to late in pushing this to default. :-)

issue585 that was just merged introduces non-trivial merge conflicts with this
one in four files (two of them are reasonably easy; the other two look more
complex), so this will need some more work before it can be merged.

Experiments are in:

   
http://ai.cs.unibas.ch/_tmp_files/helmert/issue481-v1-lama-issue481-base-issue481-v1-compare.html
   
http://ai.cs.unibas.ch/_tmp_files/helmert/issue481-v1-opt-test-issue481-base-issue481-v1-compare.html
   
http://ai.cs.unibas.ch/_tmp_files/helmert/issue481-v1-sat-test-issue481-base-issue481-v1-compare.html

I think all the differences in the results can be explained by random noise. The
changes in this issue should only affect the "plumbing" of the components during
initialization, so we would expect them to either affect the results
dramatically if the code is buggy or not affect them at all otherwise.

We wanted to get this merged before the sprint, so I'll merge it in the next
hours unless I hear from you. I don't think a code review would be very useful
at the moment since the open list code was and still is a mess, and more work on
it is planned. I'll write a list of open TODOs later.


One note: there were unexplained errors in the first experiment linked above. On
closer inspection, it seems that what happens is that VAL runs out of memory
trying to validate a plan for a large PSR task. VAL also claims that there is an
invalid plan, but I wouldn't trust it after it reported a bad_alloc exception --
most programs cannot recover cleanly from out-of-memory conditions. If you are
interested, check on maia:
/infai/helmert/downward/issue481/experiments/issue481/data/issue481-v1-lama/runs-01501-01600/01507/run.log.

The problematic tasks also have very long runtimes (more than 2000 seconds),
which I guess is OK despite a 30 minute time limit if the extra runtime is due
to VAL and/or large intermediary files. But I think we should make it easier to
discern these things from the log file.

Suggestion: change the logging format in the driver script so that it reports
various time-related statistics, such as
- current date and time
- elapsed clock time since program started
- elapsed CPU time in subprocesses so far

I think this only requires a suitable configuration call for Python's logging
module. What do you think?

> The only remaining "essential" items are re-enabling exit handlers (trivial)
> and adding factories for the remaining open lists (a few hours work).

Done; experiments queued.

Sounds good to me. If you create a new issue for the namespaces, can you update
the summary of issue64?

I've added namespaces to my TODO list for the issue.
cmake plugins were already on it.

Unfortunately, the list is still long, and adding more stuff to it won't help
get this merged faster. :-) I was actually thinking of doing the opposite of
adding more stuff to it, namely deferring all non-essential items on the
todo-list to a later issue so that we can merge. The only remaining "essential"
items are re-enabling exit handlers (trivial) and adding factories for the
remaining open lists (a few hours work).

What do you think?

I was just going to write that :-)

...but actually, we have the directory already anyway, right?

I think we should not move files to different directories at the same time that
we're modifying them heavily -- it makes the changes unreviewable.

Could you also add a namespace, directory and CMake plugin for open lists as
part of this issue, as we planned in issue64?

The pull request was broken (claiming there were no commits) after I stripped
some revisions on the issue branch, so I declined it, planning to create a new
one. After declining it, it seemed to work properly again, but since there is no
way to un-decline, I created a new one anyway:

https://bitbucket.org/malte/downward/pull-requests/4/new-pull-request-for-issue481/diff

This is now much closer to feature-completeness: all search algorithms work
again, and all "major" open lists do. Four open lists still need to be converted
(epsilon-greedy, pareto, single_buckets, type_based).

After that, there's still quite a bit of clean-up work to do. In particular, we
can finally get rid of the long-standing hacks regarding inclusion of header vs.
cc files in the open list code.

I've started working on this again and opened a pull request for future use,
adding both of you as reviews:

https://bitbucket.org/malte/downward/pull-requests/3/issue481-open-list-plugins/diff

This is not yet ready for review, though. I was in the middle of working on this
when I stopped for my holidays, and this is where things still stand. I've tried
to merge from default today and got the code to compile and run some simple
examples again, but this needs quite a bit more work.

I've thought about this for a bit. I think it's very difficult to find a good
solution which combines the following four properties:

A) Pluggable open lists: each open list is a plugin that can be added to the
planner by simply adding it to the Makefile without changing existing code.

For example, let's say that tomorrow we want to add a new open list that has an
upper size bound and randomly discards elements once this bound is reached.
Without changes to any existing code or recompilation, we add the new source
file bounded_size_open_list.cc, relink, and the new open list can be used by all
search algorithm.

B) Pluggable element types for open lists: each open list is templatized by an
arbitrary type, and we can add a plugin (e.g. a search algorithm) using a
previously unused element type at any time simply by adding it to the Makefile
without changing existing code.

For example, let's say that tomorrow we want to add a new search algorithm that
can take use arbitrary pluggable open lists whose entries are pairs of states.
We add it to the planner by adding the source file new_search_algorithm.cc,
relink, and the new search algorithm is available and can be used with all open
list plugins (including ones we add tomorrow).

C) The solution is reasonably simple in terms of code.

D) The solution is efficient.


Currently we have B) + C) + D), but we don't have A).

One way to get A) + B) + C) is to introduce a kind of "generic open list
element" (e.g. along the lines of boost::any). Essentially, there is only one
type of open list element, so open lists don't have to be templatized, but that
generic type can encapsulate arbitrary types as a payload. But that has a huge
overhead per open list element, so wouldn't give us D).

Starting from this generic open list element type solution, one can try to play
lots of tricks to make it more efficient, but I am quite convinced this would
lose C), giving us A) + B) + D).

The solution I prefer at the moment, then, is to go for A) + C) + D). Open lists
would be pluggable, but open list element types would not: if you want to add a
new element type for open lists, then all open lists would need to be
recompiled. At the moment, I think we only use three different element types for
open lists (one for eager search, one for lazy search and one for enforced
hill-climbing), and the third one is only "internal" in the sense that it is not
part of the option parsing code and only combined with fixed open list types,
which is not a pluggability problem.

We could therefore make open lists pluggable by adding a (not templatized)
abstract OpenListFactory class which has two methods; one for generating open
lists logically holding states (contain a StateID; could be used by eager
search) and one for generating open lists logically holding state transitions
(contain a StateID and a GlobalOperator *; could be used by lazy search). Then
the option parser code would work with such OpenListFactory instances, and the
search algorithms would ask them to produce the right kind of object for their
needs. If we ever want to support an additional element type for pluggable open
lists, we would have to modify OpenListFactory to support it and make some
simple changes to all open list classes to support it. (I think it would be
possible to only make these changes in one place with a bit of trickery, e.g.
using macros, but I don't think that would be the best solution in terms of
simplicity unless we end up adding new open list element types frequently.)

Ah, I see, this is why we have templates. No, using PerStateInformation won't
help here. The operator pointer is not used for storing applicable operators;
rather, it stores the operator that generates the state to be considered. That
state itself isn't materialized before the state is expanded: the StateID refers
to its parent. So the operator isn't a function of the StateID, and we need
multiple open list entries with the same ID.

The only way to change this would be to compute and store the state upon
generation (as in eager search) rather than upon expansion. This would lose one
of the big conceptual advantages of lazy search: not requiring storing a state
for each generated node. Maybe this isn't such a big deal with our more compact
state representation these days, and it would be interesting to experiment with
it. But it's probably not something we really want.

But even if open lists do remain templated, I don't think they need to remain
part of the core in the same way they currently are. We should be able to make
them as pluggable as everything else. I've changed the title to reflect this goal.

Currently, the open lists are templated on the type they contain. The type can either 
be StateID for eager search or pair<StateID, const GlobalOperator *> for lazy search. 
As discussed offline, we could probably use PerStateInformation to save the applicable 
operators for each expanded state and store only StateIDs in the open lists. This  
would remove the need for templated open lists.