Title add ExplicitTask class
Priority feature Status resolved
Superseder Nosy List florian, jendrik, malte, mkatz, patfer
Assigned To florian Keywords
Optional summary
Part of issue509.

Created on 2016-12-20.17:59:52 by jendrik, last changed by florian.

Part of issue509.
msg6986 (view) Author: florian Date: 2018-04-03.19:52:31
Thanks, everyone.

I merged this but had to do a little adjustment in to avoid computing
is_unit_cost in help mode where no task is available. The diff for that commit
is at

(The method is_unit_cost is deprecated and should be replaced by the task
interface eventually).
msg6983 (view) Author: malte Date: 2018-04-03.16:19:14
msg6982 (view) Author: florian Date: 2018-04-03.16:15:31
The diff between the two versions looks fine to me:

I guess that means that we changed something in on of the last merged issues
that makes the impact of this issue a bit stronger. From my point of view the
difference is small enough that we can merge either version and I'd prefer the
one with the class file in the .cc file.
msg6977 (view) Author: florian Date: 2018-04-03.11:05:50
Yes, currently v1 and v2 are based on different revisions on the default branch.
I'll create a new version that has a clean diff to v2 and run an experiment with
just h^2.
msg6976 (view) Author: malte Date: 2018-04-03.10:56:30
This is strange: if all methods are virtual and no implementation is provided in
the header file, there should be no performance difference. Can we get a clean
experiment that only compares this change? The way I understand it, this is not
what we currently have?
msg6975 (view) Author: florian Date: 2018-04-03.10:47:16
Yes, the diff is just moving the class declaration from the .h to the .cc file:
msg6974 (view) Author: malte Date: 2018-04-03.10:42:46
Based on h^2 runtime, my instinct is to rather go with the faster version. Is it
possible to see a code diff between the slower and faster version?
msg6972 (view) Author: jendrik Date: 2018-04-03.10:31:36
Could you please produce a report that directly compares v1 and v2? Ah, I see 
that v2 contains additional changes from the default branch. Well, never mind. 

I don't know if the impact of moving the class to the .cc file is big enough to 
influence our design. We could just choose the code version that we like 
better. In my opinion encapsulating the class in the .cc file is a good idea 
and clarifies the interface.
msg6970 (view) Author: florian Date: 2018-04-03.09:17:05
I tried profiling this and saw that most of the time was spent creating tuples
of facts. Interestingly, the only method of RootTask that showed up in the
profile (getting the effect of an operator) was faster in the new version.

Looking at the code again, I noticed that we could move the whole class to the
cc file to reduce the size of the interface. I tried this but unfortunately, it
made things worse:

My suggestion is to roll back the last diff and merge the previous version.
msg6966 (view) Author: jendrik Date: 2018-04-01.11:10:05
I think the experimental results look good. The only configuration where 
total_time visibly increases is astar(hm(2)). However, the increase is never 
greater than 6%. You could profile one of these runs and try to find out where 
the additional runtime comes from, but I'd be fine with merging this in any case.
msg6963 (view) Author: malte Date: 2018-03-31.16:08:17
I don't have time to look into this, but if the others are happy with the
experimental results, I'm happy to see it merged. (The code was already reviewed.)
msg6945 (view) Author: florian Date: 2018-03-18.10:19:07
The repeated experiment of EHC/FF also looks fine:
msg6944 (view) Author: florian Date: 2018-03-18.10:15:45
The results for ipdb and h^2 are in. The error in slurm.err looks like it
happened while trying to publish the report.

In h^2 the total time noticeably decreases but the decrease is within 5%.

In iPDB there are changes within +-10% but no clear trend one way or the other.
msg6877 (view) Author: florian Date: 2018-03-16.08:57:46
Maybe it is the validator that is running out of memory. The error happens on 5
PSR-large tasks in all four configurations ({lama,lama-first} x {base,v1}). All
run and driver logs for these runs are empty even though sas_plan files exist
and in the slurm log there are 20 lines saying that a memory limit was exceeded
which probably correspond to those tasks. Either way, I don't think this
influences this experiment because the planner behaves the same before and after
the patch.
msg6876 (view) Author: malte Date: 2018-03-16.08:22:07
> The remaining errors all look like either warnings for >1MB logs in lama or
> like the translator running out of memory on very large PSR tasks.

That would be strange. For example, on my machine translating p34 takes 20
seconds and has a peak memory of roughly 600 MiB. It's not very large either; we
could already solve all tasks up to p35 back in 2004.
msg6875 (view) Author: florian Date: 2018-03-16.07:55:23
I updated the report to exclude EHC/FF. The remaining errors all look like
either warnings for >1MB logs in lama or like the translator running out of
memory on very large PSR tasks.

I'll run the new experiments when the grid is back online.
msg6859 (view) Author: florian Date: 2018-03-15.12:24:53
I added items 2, 4, and 5 from msg6840 to issue509 and created issue764 for item 3.
msg6850 (view) Author: malte Date: 2018-03-15.11:26:50
I would like to ignore the satisficing experiment until we have a reasonable
number of unexplained errors in the report. I think it's too easy to miss
something when there are hundreds of unexplained errors. (If it's easy to
produce a report that excludes the EHC configuration, I'm happy to look at it.)

Regarding the other experiment, it looks like the things in "unexplained errors"
aren't errors but warnings, and perhaps it would be nice to be able to make this
distinction in the reports. Of course this is unrelated to this issue.

The blind search and LM-Cut results look good.

I'd be interested in experimental results for h^2 and iPDB because these
previously stuck out as making a lot of global state/operator accesses and had
some performance issues related to these.

One the satisficing results are updated with fewer unexplained error, I'd like
to see what happens with lama-first in Airport, Satellite and PSR. This doesn't
require a special report. It's mainly a note to self.
msg6847 (view) Author: malte Date: 2018-03-15.11:06:03
Regarding msg6840:

I think that items 2, 4 and 5 are conceptually part of "the task interface", so
they should be tackled as part of the surrounding meta-issue for this. For me it
would be sufficient at the moment to include them in the TODO list for the
meta-issue. Concrete issues for them can be added now or later.

Item 3 should be addressed at some point, but doesn't look very related to this
meta-issue. I think it is somewhat related to the option handling, so if we have
a meta-issue for that, I'd add it there. But I'd also be OK with ignoring it for

I agree on item 1.
msg6844 (view) Author: florian Date: 2018-03-15.08:15:12
The experiments with satisficing configurations contained an error but results
for lama-first are ok: coverage goes down by 2 tasks (one because of memory, one
because of time) but looking at the plots, I don't see a systematic bias.
Overall, the coverage of LAMA is not affected. The hiking domain sticks out of
the memory plot, I don't know why.

I'll rerun EHC/FF. Should I add any other configurations?
msg6843 (view) Author: florian Date: 2018-03-15.07:51:14
Experiments on the optimal suite with A*/LM-cut and blind search look fine to
me: there is no change in cost or expansions, LM-cut shows a slight trend to
faster runtimes and better memory usage. For blind search the variance of
runtimes a bit higher than usual but most changes are within +/- ~10% and I
don't see a trend in one direction or the other. Memory usage doesn't change
much, except for getting better in very small instances.
msg6840 (view) Author: florian Date: 2018-03-14.23:31:37
Thanks for the review. Experiments are on their way.

I completely agree with handling the other global variables later.
Coincidentally, the example you picked (g_variable_domain) is the last
occurrence of that variable. It was already unused and I removed it now.

Out of interest, I had a look at the other g_* variables that are left:

1) g_log and g_timer should probably stay.

2) g_root_task could be replaced by a local variable eventually.

3) g_plan_filename, g_num_previously_generated_plans, and
g_is_part_of_anytime_portfolio could be collected in a class that handles the
creation and storage of plans eventually. 

4) g_axiom_evaluator, g_state_packer, g_successor_generator,
g_initial_state_data should all be accessed through the task interface eventually.

5) g_use_metric is currently only used for debug output in dump_everything(). We
could remove it in this issue or enable access to it through the task interface

Should I create issues for any of these?
msg6839 (view) Author: malte Date: 2018-03-14.19:01:54
PS: To emphasize, if we close this issue, it doesn't mean this is all already in
the shape that we would ideally want to have at the end. But I think any further
changes should rather be part of separate issues that can be individually vetted
(and experimented with).
msg6838 (view) Author: malte Date: 2018-03-14.18:59:20
Code looks good to me. There are still some task-related global variables in that should eventually go (like g_variable_domain), and if we don't
have an issue for that yet, we should probably create one (and mention it in the
task class meta-issue, issue509). But I don't see this as part of this issue.

We need experiments for this now, exercising a variety of configurations (at
least: blind search and a decent collection of optimal configs; some satisficing
configs including some good ones).
msg6674 (view) Author: jendrik Date: 2017-12-01.11:14:37
This is now waiting for a review by Malte in the review queue.
msg6666 (view) Author: jendrik Date: 2017-11-30.22:38:35
I'm done with this round of comments on bitbucket.
msg6638 (view) Author: florian Date: 2017-11-29.23:09:20
With issue748 merged, this looks easier now. I tried to prepare a pull request
for this:

There are still a lot of rough corners but I suggest we don't try to fix all of
them in this issue. The patch is already kind of big, so for example, i wouldn't
want to start changing the successor generator or axiom evaluator at the same time.
msg6618 (view) Author: florian Date: 2017-11-27.23:29:06
We may be able to tackle this earlier by finishing issue747 and issue748.
Issue724 is not really necessary for this.
msg6612 (view) Author: florian Date: 2017-11-27.18:43:19
Issue726 is merged now but the code still uses g_operators (for g and real_g).
To get rid of g_operators completely, we also have to wait for issue724, which
will change the way path-dependent heuristics work, so evaluators can keep track
of the g values and we no longer need to store them in the search node.
msg6326 (view) Author: florian Date: 2017-05-02.19:22:37
Switching get_successor_state to OperatorProxy is issue726.
msg6323 (view) Author: florian Date: 2017-05-02.19:12:45
I updated the pull request with the new code. There are still two global
variables that make the code for this issue awkward: g_operators and
g_initial_state_data. The latter has a circular dependency with this issue but
the workaround for this issue wouldn't be so bad, I think.

We already have an issue in the pipeline (issue725) that will start to simplify
things for g_operators. In the next offline meeting we also want to discuss how
to deal with "g" and "real_g", another place that relies on g_operators. If we
also switch get_successor_state to OperatorProxy then we can remove g_operators
and this issue will be a much cleaner patch. So I suggest to wait until then.
msg6320 (view) Author: florian Date: 2017-05-02.16:35:40
Issue629 and issue688 are now merged.
msg6015 (view) Author: florian Date: 2017-01-05.18:13:56
From some offline discussion: For now, we want to simplify things by
implementing the class we called ExplicitTask so far as RootTask instead of
deriving ExplicitTask from DelegatingTask and then RootTask from ExplicitTask.
This will then only work for the root task, not for TNF tasks as we originally
planned in issue576. Once it is merged, we can think about how to best implement
TNF task.
msg5960 (view) Author: florian Date: 2016-12-21.15:19:36
I started working on this and put up a pull request, but I suggest we wait with
this until at least issue629 and and issue688 are merged.
msg5944 (view) Author: jendrik Date: 2016-12-20.17:59:51
Split off from issue576.

We need ExplicitTask to store the data for the RootTask interface.
Date User Action Args
2018-04-03 19:52:31floriansetstatus: reviewing -> resolved
messages: + msg6986
2018-04-03 16:19:14maltesetmessages: + msg6983
2018-04-03 16:15:31floriansetmessages: + msg6982
2018-04-03 11:05:50floriansetmessages: + msg6977
2018-04-03 10:56:30maltesetmessages: + msg6976
2018-04-03 10:47:16floriansetmessages: + msg6975
2018-04-03 10:42:46maltesetmessages: + msg6974
2018-04-03 10:31:37jendriksetmessages: + msg6972
2018-04-03 09:17:05floriansetmessages: + msg6970
2018-04-01 11:10:05jendriksetmessages: + msg6966
2018-03-31 16:08:17maltesetmessages: + msg6963
2018-03-21 10:49:23patfersetnosy: + patfer
2018-03-18 10:19:07floriansetmessages: + msg6945
2018-03-18 10:15:45floriansetmessages: + msg6944
2018-03-16 08:57:46floriansetmessages: + msg6877
2018-03-16 08:22:07maltesetmessages: + msg6876
2018-03-16 07:55:23floriansetmessages: + msg6875
2018-03-15 12:24:53floriansetmessages: + msg6859
2018-03-15 11:26:50maltesetmessages: + msg6850
2018-03-15 11:06:03maltesetmessages: + msg6847
2018-03-15 08:15:13floriansetmessages: + msg6844
2018-03-15 07:51:14floriansetmessages: + msg6843
2018-03-14 23:31:37floriansetmessages: + msg6840
2018-03-14 19:01:54maltesetmessages: + msg6839
2018-03-14 18:59:20maltesetmessages: + msg6838
2017-12-01 11:14:37jendriksetmessages: + msg6674
2017-11-30 22:38:35jendriksetstatus: chatting -> reviewing
messages: + msg6666
2017-11-29 23:09:20floriansetmessages: + msg6638
summary: Part of issue509. Waiting for (issue724), issue725, issue726, issue747, issue748 and other issues that make the search independent of g_operators. -> Part of issue509.
2017-11-27 23:29:06floriansetmessages: + msg6618
summary: Part of issue509. Waiting for issue724, issue725, issue726, and other issues that make the search independent of g_operators. -> Part of issue509. Waiting for (issue724), issue725, issue726, issue747, issue748 and other issues that make the search independent of g_operators.
2017-11-27 18:43:19floriansetassignedto: florian
messages: + msg6612
summary: Part of issue509. Waiting for issue725, issue726, and other issues that make the search independent of g_operators. -> Part of issue509. Waiting for issue724, issue725, issue726, and other issues that make the search independent of g_operators.
2017-05-16 10:22:26mkatzsetnosy: + mkatz
2017-05-02 19:22:37floriansetmessages: + msg6326
summary: Part of issue509. Waiting for issue725 and other issues that make the search independent of g_operators. -> Part of issue509. Waiting for issue725, issue726, and other issues that make the search independent of g_operators.
2017-05-02 19:12:45floriansetmessages: + msg6323
summary: Part of issue509. Waiting for issue725 and other issues that make the search independent of g_operators.
2017-05-02 16:35:40floriansetmessages: + msg6320
summary: Wait for issue629 and and issue688. -> (no value)
2017-01-05 18:13:56floriansetmessages: + msg6015
2016-12-21 15:19:36floriansetstatus: unread -> chatting
messages: + msg5960
summary: Wait for issue629 and and issue688.
2016-12-20 17:59:52jendrikcreate