issue414

issue414

[malte.helmert, 2014-09-21 14:49:00]

First set of comments -- so far, I looked mainly at the integration into the
rest of the code and less at the actual portfolio code. Once we're agreed on the
integration part, I'll look a bit more closely at those parts of the actual
portfolio code that are important for this issue.

https://codereview.appspot.com/146850044/diff/1/src/driver/aliases.py
File src/driver/aliases.py (right):

https://codereview.appspot.com/146850044/diff/1/src/driver/aliases.py#newcode155
src/driver/aliases.py:155: 
In this issue, we want to get rid of all the bits in the search directory that
are not search code. The other aliases and script functionality from there have
moved into the driver program, so I think the portfolios should, too. I think we
whould also get rid of the glob-based naming convention and replace it with
something more based in normal Python namespaces.

Suggestion: move the portfolios to driver/portfolios. Use normal Python naming
conventions for the portfolio names, e.g. "downward-seq-opt-merge-and-shrink.py"
=> "portfolios/merge_and_shrink.py", or "portfolios/seq_opt_merge_and_shrink.py"
if we think this is a better name. Our code should translate between "_" and "-"
in the keys.

https://codereview.appspot.com/146850044/diff/1/src/driver/aliases.py#newcode179
src/driver/aliases.py:179: raise KeyError
The KeyError should include the key, and if this is the whole code, it's
probably cleaner to avoid the if/return pattern. Something like:


if alias_name in ALIASES:
    args.search_options = ALIASES[alias_name]
elif alias_name in PORTFOLIOS:
    args.portfolio = PORTFOLIOS[alias_name]
else:
    raise KeyError(alias_name)

https://codereview.appspot.com/146850044/diff/1/src/driver/main.py
File src/driver/main.py (left):

https://codereview.appspot.com/146850044/diff/1/src/driver/main.py#oldcode63
src/driver/main.py:63: main()
Should we really remove this? Having an entry point like this in suitable main
modules is common Python practice, I think. (The code can be run with "python
-m".)

https://codereview.appspot.com/146850044/diff/1/src/driver/main.py
File src/driver/main.py (right):

https://codereview.appspot.com/146850044/diff/1/src/driver/main.py#newcode52
src/driver/main.py:52: exitcode = run_components.run_search(args)
The loop should probably break if exitcode != 0, right?
(We probably shouldn't run the preprocessor/search after a failed translator
run, for example.)

Also, we probably have something like "else:
assert False" in other cases like this.

https://codereview.appspot.com/146850044/diff/1/src/driver/portfolio.py
File src/driver/portfolio.py (right):

https://codereview.appspot.com/146850044/diff/1/src/driver/portfolio.py#newcode4
src/driver/portfolio.py:4: # TODO: Rename portfolio.py to portfolios.py ?
Sounds good.

https://codereview.appspot.com/146850044/diff/1/src/driver/portfolio.py#newcode5
src/driver/portfolio.py:5: # TODO: Should we add *** in front of logging
messages?
No, the logging messages in the code I wrote so far are meant to be removed
before merging. We might consider adding proper logging messages, for which I
would be in favour of using the logging module to easily control the output
format and to set verbosity from the command line.

https://codereview.appspot.com/146850044/diff/1/src/driver/portfolio.py#newco...
src/driver/portfolio.py:19: # we reserve some memory for the Python process (see
also main.py).
The removed comment to the left suggests that this won't work.

https://codereview.appspot.com/146850044/diff/1/src/driver/portfolio.py#newco...
src/driver/portfolio.py:64: sys.stderr.write("Could not retrieve plan cost from
%s\n" % sas_plan_file)
I'd generally use "print" instead of sys.stderr.write. (Before we merge, we
probably want to be 2.7 and 3.x compatible, so I'd import print_function and use
print with the file argument.)

Also, I'm not sure if this should be reported on stderr. In a portfolio, it is
unlucky but completely possible for the planner to be terminated while writing
it's output file. Since this is part of expected behaviour, I say we should only
print a message on stdout and clean up in this case, i.e., delete the incomplete
plan file so that its number becomes unused.

The idea is that at the end of the portfolio run, plans should always have
sequential numbers, work, and have decreasing cost.

https://codereview.appspot.com/146850044/diff/1/src/driver/portfolio.py#newco...
src/driver/portfolio.py:68: plans = glob.glob("%s*" % plan_file)
This is not robust enough -- glob almost never is. ;-) For example, if we have
something like "sas_plan.old" lying around, perhaps a solution to a completely
different problem and hence perhaps with very low cost, it looks like this will
mess with things. I suggest looking for plans sequentially with only the correct
name. 

Also, it is a critical error (= planner bug or serious system screw-up) if we
cannot extract a plan cost from a fully written plan or if a given plan is *not*
cheaper than the previous one, so we should test for these things and abort with
a critical error if we notice them.

https://codereview.appspot.com/146850044/diff/1/src/driver/portfolio.py#newco...
src/driver/portfolio.py:286: #       use ArgumentParser.parse_known_args() to
parse it here?
parse_known_args is not robust. What is wrong with making --plan-file an option
of the driver script? We can pass it on to the search component. To safeguard
about users accidentally passing --plan-file as a search option, we could rename
the search option to something more scary such as "--plan-file-internal".

https://codereview.appspot.com/146850044/diff/1/src/driver/portfolio.py#newco...
src/driver/portfolio.py:295: #       for this problem.
I need to understand the intended behaviour here better to answer this. Let's
discuss this in person; please remind me. Generally speaking, I'd prefer to keep
input_analyzer deleted. The fewer different parsers (even partial ones) we need
to maintain, the better.

https://codereview.appspot.com/146850044/diff/1/src/driver/portfolio.py#newco...
src/driver/portfolio.py:333: remaining_time_at_start = float(timeout) -
sum(os.times()[:4])
Wasn't there an "elapsed_time" method somewhere that we can use instead of
os.times()? I remember seeing one. There are some intricacies here that it would
be good to encapsulate in one place, such as limited Windows support for
os.times().

https://codereview.appspot.com/146850044/diff/1/src/driver/run_components.py
File src/driver/run_components.py (right):

https://codereview.appspot.com/146850044/diff/1/src/driver/run_components.py#...
src/driver/run_components.py:31: return exitcode
I'm not sure if this is an improvement. Errors should never pass silently.
What's wrong with the previous implementation?

https://codereview.appspot.com/146850044/diff/1/src/search/downward-seq-sat-f...
File src/search/downward-seq-sat-fdss-1.py (right):

https://codereview.appspot.com/146850044/diff/1/src/search/downward-seq-sat-f...
src/search/downward-seq-sat-fdss-1.py:3: # NOTE: when using iterated search
included, we must include the option
First half of the sentence is ungrammatical?

https://codereview.appspot.com/146850044/diff/1/src/search/downward-seq-sat-f...
File src/search/downward-seq-sat-fdss-2.py (right):

https://codereview.appspot.com/146850044/diff/1/src/search/downward-seq-sat-f...
src/search/downward-seq-sat-fdss-2.py:4: #       "plan_counter=PLANCOUNTER"
etc.

[jendrikseipp, 2014-09-22 23:05:35]

https://codereview.appspot.com/146850044/diff/1/src/driver/aliases.py
File src/driver/aliases.py (right):

https://codereview.appspot.com/146850044/diff/1/src/driver/aliases.py#newcode155
src/driver/aliases.py:155: 
On 2014/09/21 14:48:59, malte.helmert wrote:
> In this issue, we want to get rid of all the bits in the search directory that
> are not search code. The other aliases and script functionality from there
have
> moved into the driver program, so I think the portfolios should, too. I think
we
> whould also get rid of the glob-based naming convention and replace it with
> something more based in normal Python namespaces.
> 
> Suggestion: move the portfolios to driver/portfolios. Use normal Python naming
> conventions for the portfolio names, e.g.
"downward-seq-opt-merge-and-shrink.py"
> => "portfolios/merge_and_shrink.py", or
"portfolios/seq_opt_merge_and_shrink.py"
> if we think this is a better name. Our code should translate between "_" and
"-"
> in the keys.

Done.

https://codereview.appspot.com/146850044/diff/1/src/driver/aliases.py#newcode179
src/driver/aliases.py:179: raise KeyError
On 2014/09/21 14:48:59, malte.helmert wrote:
> The KeyError should include the key, and if this is the whole code, it's
> probably cleaner to avoid the if/return pattern. Something like:
> 
> 
> if alias_name in ALIASES:
>     args.search_options = ALIASES[alias_name]
> elif alias_name in PORTFOLIOS:
>     args.portfolio = PORTFOLIOS[alias_name]
> else:
>     raise KeyError(alias_name)

Done.

https://codereview.appspot.com/146850044/diff/1/src/driver/main.py
File src/driver/main.py (left):

https://codereview.appspot.com/146850044/diff/1/src/driver/main.py#oldcode63
src/driver/main.py:63: main()
On 2014/09/21 14:48:59, malte.helmert wrote:
> Should we really remove this? Having an entry point like this in suitable main
> modules is common Python practice, I think. (The code can be run with "python
> -m".)

I thought this was just for testing. Added it back.

https://codereview.appspot.com/146850044/diff/1/src/driver/main.py
File src/driver/main.py (right):

https://codereview.appspot.com/146850044/diff/1/src/driver/main.py#newcode52
src/driver/main.py:52: exitcode = run_components.run_search(args)
On 2014/09/21 14:48:59, malte.helmert wrote:
> The loop should probably break if exitcode != 0, right?
> (We probably shouldn't run the preprocessor/search after a failed translator
> run, for example.)
> 
> Also, we probably have something like "else:
> assert False" in other cases like this.

Well, in case the exitcode is != 0, we will abort in run_components anyway. We
should probably discuss how to handle this in person.

https://codereview.appspot.com/146850044/diff/1/src/driver/portfolio.py
File src/driver/portfolio.py (right):

https://codereview.appspot.com/146850044/diff/1/src/driver/portfolio.py#newcode4
src/driver/portfolio.py:4: # TODO: Rename portfolio.py to portfolios.py ?
On 2014/09/21 14:48:59, malte.helmert wrote:
> Sounds good.

Now that we have the directory driver/portfolios/, I guess we should we
portfolio.py to driver/portfolios/, right? Do you have a good name for the
module? Or should it stay in driver/ and be called something like
portfolio_runner.py?

https://codereview.appspot.com/146850044/diff/1/src/driver/portfolio.py#newcode5
src/driver/portfolio.py:5: # TODO: Should we add *** in front of logging
messages?
On 2014/09/21 14:48:59, malte.helmert wrote:
> No, the logging messages in the code I wrote so far are meant to be removed
> before merging. We might consider adding proper logging messages, for which I
> would be in favour of using the logging module to easily control the output
> format and to set verbosity from the command line.

Acknowledged.

https://codereview.appspot.com/146850044/diff/1/src/driver/portfolio.py#newco...
src/driver/portfolio.py:19: # we reserve some memory for the Python process (see
also main.py).
On 2014/09/21 14:48:59, malte.helmert wrote:
> The removed comment to the left suggests that this won't work.

I thought set_memory_limit() in main.py magically took care of that problem, but
yes, I just tested it and it doesn't.

https://codereview.appspot.com/146850044/diff/1/src/driver/portfolio.py#newco...
src/driver/portfolio.py:64: sys.stderr.write("Could not retrieve plan cost from
%s\n" % sas_plan_file)
On 2014/09/21 14:48:59, malte.helmert wrote:
> I'd generally use "print" instead of sys.stderr.write. (Before we merge, we
> probably want to be 2.7 and 3.x compatible, so I'd import print_function and
use
> print with the file argument.)
> 
> Also, I'm not sure if this should be reported on stderr. In a portfolio, it is
> unlucky but completely possible for the planner to be terminated while writing
> it's output file. Since this is part of expected behaviour, I say we should
only
> print a message on stdout and clean up in this case, i.e., delete the
incomplete
> plan file so that its number becomes unused.
> 
> The idea is that at the end of the portfolio run, plans should always have
> sequential numbers, work, and have decreasing cost.

Done.

https://codereview.appspot.com/146850044/diff/1/src/driver/portfolio.py#newco...
src/driver/portfolio.py:68: plans = glob.glob("%s*" % plan_file)
On 2014/09/21 14:48:59, malte.helmert wrote:
> This is not robust enough -- glob almost never is. ;-) For example, if we have
> something like "sas_plan.old" lying around, perhaps a solution to a completely
> different problem and hence perhaps with very low cost, it looks like this
will
> mess with things. I suggest looking for plans sequentially with only the
correct
> name. 
> 
> Also, it is a critical error (= planner bug or serious system screw-up) if we
> cannot extract a plan cost from a fully written plan or if a given plan is
*not*
> cheaper than the previous one, so we should test for these things and abort
with
> a critical error if we notice them.

How do you want to check for the case when we "cannot extract a plan cost from a
fully written plan"? I thought the cost was our indicator for a fully written
plan.

https://codereview.appspot.com/146850044/diff/1/src/driver/portfolio.py#newco...
src/driver/portfolio.py:286: #       use ArgumentParser.parse_known_args() to
parse it here?
On 2014/09/21 14:48:59, malte.helmert wrote:
> parse_known_args is not robust. What is wrong with making --plan-file an
option
> of the driver script? We can pass it on to the search component. To safeguard
> about users accidentally passing --plan-file as a search option, we could
rename
> the search option to something more scary such as "--plan-file-internal".

Sounds good to me. I wasn't sure how you think about adding more commandline
options. Will implement this tomorrow.

https://codereview.appspot.com/146850044/diff/1/src/driver/portfolio.py#newco...
src/driver/portfolio.py:295: #       for this problem.
On 2014/09/21 14:48:59, malte.helmert wrote:
> I need to understand the intended behaviour here better to answer this. Let's
> discuss this in person; please remind me. Generally speaking, I'd prefer to
keep
> input_analyzer deleted. The fewer different parsers (even partial ones) we
need
> to maintain, the better.

Acknowledged.

https://codereview.appspot.com/146850044/diff/1/src/driver/portfolio.py#newco...
src/driver/portfolio.py:333: remaining_time_at_start = float(timeout) -
sum(os.times()[:4])
On 2014/09/21 14:48:59, malte.helmert wrote:
> Wasn't there an "elapsed_time" method somewhere that we can use instead of
> os.times()? I remember seeing one. There are some intricacies here that it
would
> be good to encapsulate in one place, such as limited Windows support for
> os.times().

Do you mean write_elapsed_time()? I added a get_elapsed_time() function and
added the comment about Windows support.

https://codereview.appspot.com/146850044/diff/1/src/driver/run_components.py
File src/driver/run_components.py (right):

https://codereview.appspot.com/146850044/diff/1/src/driver/run_components.py#...
src/driver/run_components.py:31: return exitcode
On 2014/09/21 14:48:59, malte.helmert wrote:
> I'm not sure if this is an improvement. Errors should never pass silently.
> What's wrong with the previous implementation?

The exitcode of the search component may be != 0, but we still want to continue
execution and exit with that exitcode instead of with 1, which is what happens
if subprocess.CalledProcessError occurs.

https://codereview.appspot.com/146850044/diff/1/src/search/downward-seq-sat-f...
File src/search/downward-seq-sat-fdss-1.py (right):

https://codereview.appspot.com/146850044/diff/1/src/search/downward-seq-sat-f...
src/search/downward-seq-sat-fdss-1.py:3: # NOTE: when using iterated search
included, we must include the option
On 2014/09/21 14:48:59, malte.helmert wrote:
> First half of the sentence is ungrammatical?

I think this comment can go away since portfolio.py will issue the content of
the comment as an error anyway if the option is missing in iterated search.

https://codereview.appspot.com/146850044/diff/1/src/search/downward-seq-sat-f...
File src/search/downward-seq-sat-fdss-2.py (right):

https://codereview.appspot.com/146850044/diff/1/src/search/downward-seq-sat-f...
src/search/downward-seq-sat-fdss-2.py:4: #       "plan_counter=PLANCOUNTER"
On 2014/09/21 14:48:59, malte.helmert wrote:
> etc.

Done.

[malte.helmert, 2014-09-22 23:29:30]

https://codereview.appspot.com/146850044/diff/1/src/driver/main.py
File src/driver/main.py (right):

https://codereview.appspot.com/146850044/diff/1/src/driver/main.py#newcode52
src/driver/main.py:52: exitcode = run_components.run_search(args)
Right, I saw this later, too. I would recommend letting the assertions from
run_components.xyz propagate and catching them in one place (perhaps here) to
terminate the program from there. This means that run_components and this
function should not return anything. But it's indeed probably best to discuss
this in person.

On 2014/09/22 23:05:35, jendrikseipp wrote:
> On 2014/09/21 14:48:59, malte.helmert wrote:
> > The loop should probably break if exitcode != 0, right?
> > (We probably shouldn't run the preprocessor/search after a failed translator
> > run, for example.)
> > 
> > Also, we probably have something like "else:
> > assert False" in other cases like this.
> 
> Well, in case the exitcode is != 0, we will abort in run_components anyway. We
> should probably discuss how to handle this in person.

https://codereview.appspot.com/146850044/diff/1/src/driver/portfolio.py
File src/driver/portfolio.py (right):

https://codereview.appspot.com/146850044/diff/1/src/driver/portfolio.py#newcode4
src/driver/portfolio.py:4: # TODO: Rename portfolio.py to portfolios.py ?
On 2014/09/22 23:05:35, jendrikseipp wrote:
> On 2014/09/21 14:48:59, malte.helmert wrote:
> > Sounds good.
> 
> Now that we have the directory driver/portfolios/, I guess we should we
> portfolio.py to driver/portfolios/, right? Do you have a good name for the
> module? Or should it stay in driver/ and be called something like
> portfolio_runner.py? 

I like portfolio_runner.py.

https://codereview.appspot.com/146850044/diff/1/src/driver/portfolio.py#newco...
src/driver/portfolio.py:19: # we reserve some memory for the Python process (see
also main.py).
On 2014/09/22 23:05:35, jendrikseipp wrote:
> On 2014/09/21 14:48:59, malte.helmert wrote:
> > The removed comment to the left suggests that this won't work.
> 
> I thought set_memory_limit() in main.py magically took care of that problem,
but
> yes, I just tested it and it doesn't.

Maybe keep the relevant parts of the old comment here as a TODO.

https://codereview.appspot.com/146850044/diff/1/src/driver/portfolio.py#newco...
src/driver/portfolio.py:68: plans = glob.glob("%s*" % plan_file)
> How do you want to check for the case when we "cannot extract a plan cost from
a
> fully written plan"? I thought the cost was our indicator for a fully written
> plan. 

You're right, the current code either succeeds in extracting a cost or treats
the plan as incomplete, which is fine. (I was think of things like: finding a
line that starts with "; cost = " and ends with \n, but then doesn't contain a
parseable int. This simply won't match with the current re solution. If I had
hacked it up, I'd probably be done things manually with split() etc., and then
there would have been a call to int which might have failed. But I think the re
solution is better.)

https://codereview.appspot.com/146850044/diff/1/src/driver/portfolio.py#newco...
src/driver/portfolio.py:286: #       use ArgumentParser.parse_known_args() to
parse it here?
> Sounds good to me. I wasn't sure how you think about adding more commandline
> options. Will implement this tomorrow.

Too many command-line options can be a problem, but being able to control the
output file name(s) feels like common functionality that one might expect from a
well-behaved command-line application. (For the same reason, the fixed
"output.sas" and "output" files are still a problem, but one thing at a time.)

https://codereview.appspot.com/146850044/diff/1/src/driver/portfolio.py#newco...
src/driver/portfolio.py:333: remaining_time_at_start = float(timeout) -
sum(os.times()[:4])
On 2014/09/22 23:05:35, jendrikseipp wrote:
> On 2014/09/21 14:48:59, malte.helmert wrote:
> > Wasn't there an "elapsed_time" method somewhere that we can use instead of
> > os.times()? I remember seeing one. There are some intricacies here that it
> would
> > be good to encapsulate in one place, such as limited Windows support for
> > os.times().
> 
> Do you mean write_elapsed_time()? I added a get_elapsed_time() function and
> added the comment about Windows support.

Acknowledged.

https://codereview.appspot.com/146850044/diff/1/src/driver/run_components.py
File src/driver/run_components.py (right):

https://codereview.appspot.com/146850044/diff/1/src/driver/run_components.py#...
src/driver/run_components.py:31: return exitcode
On 2014/09/22 23:05:35, jendrikseipp wrote:

> The exitcode of the search component may be != 0, but we still want to
continue
> execution and exit with that exitcode instead of with 1, which is what happens
> if subprocess.CalledProcessError occurs.

I would catch the CalledProcessError exception and terminate with its exit code,
which unless I misremember is an attribute of the exception object.

(The point being that this is a utility function which can fail, and it should
be up to the user to decide how to deal with the failure.)

https://codereview.appspot.com/146850044/diff/1/src/search/downward-seq-sat-f...
File src/search/downward-seq-sat-fdss-1.py (right):

https://codereview.appspot.com/146850044/diff/1/src/search/downward-seq-sat-f...
src/search/downward-seq-sat-fdss-1.py:3: # NOTE: when using iterated search
included, we must include the option
On 2014/09/22 23:05:35, jendrikseipp wrote:
> On 2014/09/21 14:48:59, malte.helmert wrote:
> > First half of the sentence is ungrammatical?
> 
> I think this comment can go away since portfolio.py will issue the content of
> the comment as an error anyway if the option is missing in iterated search.

Acknowledged.

[jendrikseipp, 2014-09-26 14:04:00]

I have made all the changes suggested here and created a pull request at
bitbucket that looks at the whole branch
(https://bitbucket.org/jendrikseipp/downward/pull-request/21/issue414-driver/diff).
If you'd rather discuss things at rietveld, that's fine with me too.

https://codereview.appspot.com/146850044/diff/1/src/driver/main.py
File src/driver/main.py (right):

https://codereview.appspot.com/146850044/diff/1/src/driver/main.py#newcode52
src/driver/main.py:52: exitcode = run_components.run_search(args)
On 2014/09/22 23:29:28, malte.helmert wrote:
> Right, I saw this later, too. I would recommend letting the assertions from
> run_components.xyz propagate and catching them in one place (perhaps here) to
> terminate the program from there. This means that run_components and this
> function should not return anything. But it's indeed probably best to discuss
> this in person.
> 
> On 2014/09/22 23:05:35, jendrikseipp wrote:
> > On 2014/09/21 14:48:59, malte.helmert wrote:
> > > The loop should probably break if exitcode != 0, right?
> > > (We probably shouldn't run the preprocessor/search after a failed
translator
> > > run, for example.)
> > > 
> > > Also, we probably have something like "else:
> > > assert False" in other cases like this.
> > 
> > Well, in case the exitcode is != 0, we will abort in run_components anyway.
We
> > should probably discuss how to handle this in person.
> 

Done.

https://codereview.appspot.com/146850044/diff/1/src/driver/portfolio.py
File src/driver/portfolio.py (right):

https://codereview.appspot.com/146850044/diff/1/src/driver/portfolio.py#newco...
src/driver/portfolio.py:19: # we reserve some memory for the Python process (see
also main.py).
On 2014/09/22 23:29:28, malte.helmert wrote:
> On 2014/09/22 23:05:35, jendrikseipp wrote:
> > On 2014/09/21 14:48:59, malte.helmert wrote:
> > > The removed comment to the left suggests that this won't work.
> > 
> > I thought set_memory_limit() in main.py magically took care of that problem,
> but
> > yes, I just tested it and it doesn't.
> 
> Maybe keep the relevant parts of the old comment here as a TODO.

Done.

https://codereview.appspot.com/146850044/diff/1/src/driver/portfolio.py#newco...
src/driver/portfolio.py:68: plans = glob.glob("%s*" % plan_file)
On 2014/09/22 23:29:28, malte.helmert wrote:
> > How do you want to check for the case when we "cannot extract a plan cost
from
> a
> > fully written plan"? I thought the cost was our indicator for a fully
written
> > plan. 
> 
> You're right, the current code either succeeds in extracting a cost or treats
> the plan as incomplete, which is fine. (I was think of things like: finding a
> line that starts with "; cost = " and ends with \n, but then doesn't contain a
> parseable int. This simply won't match with the current re solution. If I had
> hacked it up, I'd probably be done things manually with split() etc., and then
> there would have been a call to int which might have failed. But I think the
re
> solution is better.)

Acknowledged.

https://codereview.appspot.com/146850044/diff/1/src/driver/portfolio.py#newco...
src/driver/portfolio.py:286: #       use ArgumentParser.parse_known_args() to
parse it here?
On 2014/09/22 23:29:28, malte.helmert wrote:
> > Sounds good to me. I wasn't sure how you think about adding more commandline
> > options. Will implement this tomorrow.
> 
> Too many command-line options can be a problem, but being able to control the
> output file name(s) feels like common functionality that one might expect from
a
> well-behaved command-line application. (For the same reason, the fixed
> "output.sas" and "output" files are still a problem, but one thing at a time.)

Done.

https://codereview.appspot.com/146850044/diff/1/src/driver/run_components.py
File src/driver/run_components.py (right):

https://codereview.appspot.com/146850044/diff/1/src/driver/run_components.py#...
src/driver/run_components.py:31: return exitcode
On 2014/09/22 23:29:28, malte.helmert wrote:
> On 2014/09/22 23:05:35, jendrikseipp wrote:
> 
> > The exitcode of the search component may be != 0, but we still want to
> continue
> > execution and exit with that exitcode instead of with 1, which is what
happens
> > if subprocess.CalledProcessError occurs.
> 
> I would catch the CalledProcessError exception and terminate with its exit
code,
> which unless I misremember is an attribute of the exception object.
> 
> (The point being that this is a utility function which can fail, and it should
> be up to the user to decide how to deal with the failure.)

Done.