Issue288

Title option parser accepts incorrect configurations
Priority bug Status resolved
Superseder Nosy List jendrik, malte, moritz, silvan
Assigned To Keywords
Optional summary

Created on 2011-10-17.21:09:15 by jendrik, last changed by malte.

Messages
msg11177 (view) Author: malte Date: 2023-07-25.10:49:33
We now have a reimplemented option parser, and from what I can tell this issue is resolved.
msg6203 (view) Author: jendrik Date: 2017-04-21.18:56:35
We discussed this today and concluded that the way to fix most of the issues below 
is to write a proper parser (along the lines of msg3149) checking for both 
syntactic and semantic (e.g., double arguments, non-existent arguments) errors in 
configurations.
msg6185 (view) Author: silvan Date: 2017-04-04.10:54:23
The option parser crashes with the following arguments:
--heuristic "hipdb=" --search "astar(hipdb)"
I think it should show throw an error if after "=" nothing valid can be parsed
(and empty is something invalid).
msg5568 (view) Author: jendrik Date: 2016-08-19.12:13:52
Another option parser curiosity is that it allows passing parameters multiple 
times. I think this should be an error.
msg4184 (view) Author: jendrik Date: 2015-05-11.15:58:05
Moritz, could you please send us your patch? It doesn't matter at all in which 
state it is. We just want to avoid repeating the work that you have already done.
msg4090 (view) Author: silvan Date: 2015-03-18.17:27:34
There is another example of the option parser should complain: it currently
accepts arbitrarily many commas as parameters:

astar(merge_and_shrink(,))
astar(merge_and_shrink(,,,,))
astar(merge_and_shrink(,shrink_strategy=shrink_bisimulation,,merge_strategy=merge_dfp))

all work correctly.
msg4077 (view) Author: jendrik Date: 2015-03-17.15:11:32
Hi again, Moritz! We currently have a HiWi student who could integrate your patch 
into the current Fast Downward version. Do you think you could post the patch 
somewhere?
msg3997 (view) Author: jendrik Date: 2015-01-09.09:34:37
In principle you could explain the changes to me, but I'll be away until the 
beginning of February as well, and understanding the changes might be hard 
since I'm not familiar with the current option parser code. 

Maybe you can upload the code somewhere, give us access and we'll continue the 
discussion in February?
msg3996 (view) Author: malte Date: 2015-01-09.00:10:53
Sounds excellent to me! With the caveat that I'll be unavailable until mid
February (but I don't have to be part of the code review).
msg3995 (view) Author: moritz Date: 2015-01-08.23:25:44
Hi, sorry for disappearing. I started a full time job shortly after my last
message. Which has taken up all my focus (and still does, mostly).

My suggestion is as follows:
1) I upload the current version of the code (have to unstash my old computer
first). All mentioned issues should be fixed, but there are still a few things
to adapt, since the parse tree structure changed. 
2) We do a code review via TeamViewer/Skype, where I explain to Jendrik or Malte
what I changed, and what still needs to be done. 
3) Then one of you finishes it.
The review might also make it easier for you to extend, modify or replace the
parser in the future.

Ok?
msg3993 (view) Author: jendrik Date: 2015-01-04.17:58:35
You're right. I created issue507 for this.
msg3991 (view) Author: malte Date: 2015-01-04.14:54:05
If we want to borrow additional ideas from Python, Python has "keyword-only"
arguments, which also solve the problem of not changing meaning if additional
options are added. For the large majority of options, we by convention always
use the keyword form anyway. Depending on the type of object, we usually use the
non-keyword positional form only for 0-1 arguments. Maybe keyword-only should be
the default and possibly positional arguments should be the special case. But
either way, this is probably a separate issue, right?
msg3990 (view) Author: jendrik Date: 2015-01-03.19:18:12
Patrick just stumbled over a parser subtlety. I'll use a config that is present 
in the master branch to demonstrate:

When a user wants to run lazy_greedy with ff() and cea() he might forget the 
square brackets:

./downward-release --search "lazy_greedy(ff(), cea())"

The parser happily accepts this and cea() is used to calculate preferred 
operators. We should maybe think about ways to prevent such subtle usage 
errors.
msg3894 (view) Author: jendrik Date: 2014-11-10.14:22:30
I found another oddity concerning the parser. Currently, the parser accepts 
arguments of type double with arbitrary text appended to it, e.g. astar(lmcut(), 
max_time=1aha).
msg3171 (view) Author: jendrik Date: 2014-05-19.17:23:27
Sure, I can run some experiments.

At the moment we don't allow C++11 functions in the code. I would suggest adding 
comments where their usage would benefit the code (maybe only a single 
explanatory comment).
msg3170 (view) Author: moritz Date: 2014-05-19.16:55:52
Used the suggested changes, with some modification to the numbers (allow initial
signs, and a leading dot).

Nearly done. Looks like I can get rid of the external dependency tree.hh.

Jendrik, could you run a few performance regression tests for me if I push the
code to some repository?

Btw, what is the current policy on the 'move' keyword? There are a few places in
parse tree transformation where this would be useful (but of course not necessary).
msg3152 (view) Author: malte Date: 2014-04-30.00:44:18
As in most programming language grammars, I would suggest to separate the
"AlphaNumeric" token into two kinds of tokens: pure numbers vs. identifiers. So
for example "3=foo()" should not be a valid "KeyValue" because 3 is not a valid
identifier, and similarly 22(foo) would not be a call because 22 is not an
identifier.

Identifier would be the regex [a-zA-Z_][a-zA-Z0-9_]* (as e.g. in C).

Number would be [0-9][0-9]* (for integers) or [0-9][0-9]*.[0-9][0-9]* (for
floating-point numbers).

I think the grammar would use Identifier in all places where it currently uses
AlphaNumeric, except in RHSParam, where both Identified and Number would be allowed.
msg3151 (view) Author: jendrik Date: 2014-04-29.18:50:31
Looks good to me. Maybe we should come up with a better name for "Call", but I 
can't think of one now.
msg3150 (view) Author: moritz Date: 2014-04-29.18:41:21
Correction, should be: Call -> AlphaNumeric(ParamListHead)
msg3149 (view) Author: moritz Date: 2014-04-29.18:14:09
Found some time, nearly complete now.

Here's how I formalized FDs configuration language (ignoring predefinitons):
Call -> AlphaNumeric(ParamList)
AlphaNumeric -> any alphanumeric sequence (including '_')
ParamListHead -> ParamList | ""
ParamList -> Param, ParamList | Param
Param -> KeyValue | RHSParam
KeyValue -> AlphaNumeric=RHSParam
RHSParam -> Call | List | AlphaNumeric   //parameters that are assignable to a key
List -> [ParamListHead]

Jendrik, could you take a quick look at these rules and let me know if you see
any flaws, or if something is unclear?
msg3038 (view) Author: moritz Date: 2014-03-07.17:36:25
Hello Jendrik,

I'm still reading the mails.

The current parser is a refactoring of the original parser, sharing the ad-hoc
core parsing mechanism (which transforms the config string into a tree). While I
can fix the issues you mentioned by hacky workarounds, this mechanism should
better be replaced by something like a top-down parser. I wanted to to do this
after issue232 was merged, to avoid conflicts. Was traveling for the last few
months though.

Anyway, I can take a look at the code soon, within 4 weeks max.
msg3037 (view) Author: jendrik Date: 2014-03-06.21:29:52
Another parser issue: The following config produces a segfault:

--landmarks 
"lmg=lm_hm(reasonable_orders=false,only_causal_landmarks=false,disjunctive_landmarks=true,conjunctive_landmarks=false,no_orders=
true,m=1,cost_type=0)" --heuristic "hLM=lmcount(lmg,admissible=true,pref=false,cost_type=0)"  --search 
"lazy(single(sum(g(),weight(hLM, 3))),preferred=[],reopen_closed=false,cost_type=0)"
msg3018 (view) Author: jendrik Date: 2014-03-03.19:50:51
I would like to revive efforts for fixing this issue. Moritz, are you still 
reading the tracker emails? Could you point us in the right direction here? What 
exactly do you mean in msg2336: do you already have fixes for some of the issues? 
What would need to be changed to "fix everything"?
msg2337 (view) Author: malte Date: 2012-09-26.00:39:05
If it helps, I can look into issue232 this weekend.
msg2336 (view) Author: moritz Date: 2012-09-25.23:05:26
There are two possibilities how to go on with this issue:

1: I can fix (or already fixed) most of what Jendrik said with minor changes to
the code.

2: to fix everything, I'd need to change a bigger part of the code. This is
because the parser, at the lowest level, copies the functionality of the
original parser (which accepted the same input). See also msg2007. I can do this
too, but we'd have to decide if we want to integrate issue232 first.
msg2258 (view) Author: moritz Date: 2012-06-12.17:00:03
Hi,
since I'm currently writing my diploma thesis and my contract for 
FD-work expired, I don't actively work on FD anymore. But I've fixed 
most of the stuff Jendrik mentioned already anyway, so I plan to take 
care of this issue, sometime in the next weeks.

On 06/12/2012 03:51 PM, Malte Helmert wrote:
> Malte Helmert<malte.helmert@unibas.ch>  added the comment:
>
> I think it should be an error rather than a warning (but definitely no segfault
> ;-)).
>
> _______________________________________________________
> Fast Downward issue tracker<downward.issues@gmail.com>
> <http://issues.fast-downward.org/issue288>
> _______________________________________________________
>
msg2257 (view) Author: malte Date: 2012-06-12.15:51:03
I think it should be an error rather than a warning (but definitely no segfault
;-)).
msg2256 (view) Author: jendrik Date: 2012-06-12.15:37:53
Another request: 

I think it would be nice to get a warning instead of a segmentation fault when 
e.g. the parentheses don't match in the command line string:

./downward-1 --search "(astar(blind())" < output
Simplifying transitions... done!
Peak memory: 3592 KB
caught signal 11 -- exiting
Speicherzugriffsfehler (Speicherabzug geschrieben)
msg2068 (view) Author: malte Date: 2012-02-20.18:22:30
Sorry for the prolonged silence. Yes, we will integrate that one.
I really need an eighth day of the week.
msg2041 (view) Author: moritz Date: 2012-02-04.12:42:46
Are there still plans to integrate issue232? I've been waiting for the code
review on that one before working on the issue here.
msg2011 (view) Author: moritz Date: 2011-12-31.11:25:03
We did use optional, no-default parameters for M&S (lines 4.* here: http://hg.fast-downward.org/rev/d2e6a00486fd ). Must have been reverted during M&S-Refactoring.
msg2010 (view) Author: malte Date: 2011-12-31.01:25:05
Optional parameters without a "regular" default value are already implemented,
and this was one of the motivating examples. See issue236. I guess we're not
using it in M&S yet; patches welcome.
msg2009 (view) Author: jendrik Date: 2011-12-31.00:26:13
Something else: I think that assigning default values to int parameters when "-1" 
is specified is very error prone. I think raising an error is the better 
behaviour. If the user wants the default parameter the option should be left out 
or we should allow specifying "default".

$ ./downward --heuristic 
"hmas=merge_and_shrink(shrink_strategy=shrink_fh(max_states=-1))" --search 
"astar(hmas)" < output | grep "Abstraction size limit"
Abstraction size limit: 50000
Abstraction size limit right before merge: 50000

Probably this is restricted to shrink strategies. This behaviour has tricked at 
least Chris and me already.
msg2008 (view) Author: jendrik Date: 2011-12-30.12:20:03
Exactly.
msg2007 (view) Author: moritz Date: 2011-12-30.11:50:03
Using the default value here will cause a Parse Error ("invalid enum argument not_greedy for option greedy at: 
shrink_strategy = shrink_bisimulation"). So I guess you mean an unconditional check for validity of the default value every time add_enum_option() is called?
msg2006 (view) Author: jendrik Date: 2011-12-29.16:49:59
Another thing: Shouldn't parser.add_enum_option() check that the default value 
is part of the enum? This is not done e.g. in shrink_bisimulation.cc line 439:

parser.add_enum_option(
        "greedy", greediness, "NOT_GREEDY",
        "use exact, somewhat greedy or greedy bisimulation");
msg1837 (view) Author: moritz Date: 2011-10-25.10:23:24
Although I fixed what Jendrik mentioned, the parser still accepts many malformed
configs. This is because of my very ad-hoc approach for parse-tree generation.
I'll replace this with a recursive descent parser soon.
msg1836 (view) Author: moritz Date: 2011-10-24.09:40:03
> Shouldn't Parse Errors be reported on stderr to be consistent with the general 
> behaviour?

Yes, right.

> I found the following on stdout while testing shell escaping:
> 
> Parse Error: 
> heuristic       hcea not found at: 
> preferred =     hcea
> 

The parser currently only ignores spaces and newlines. I'll change it to
ignore all whitespace.
msg1835 (view) Author: jendrik Date: 2011-10-23.23:56:46
While you're at it, you might have a look at the following:

Shouldn't Parse Errors be reported on stderr to be consistent with the general 
behaviour?

I found the following on stdout while testing shell escaping:

Parse Error: 
heuristic       hcea not found at: 
preferred =     hcea
msg1819 (view) Author: moritz Date: 2011-10-18.09:07:17
Fixed, but I'll test some other configurations later before submitting.
msg1812 (view) Author: malte Date: 2011-10-17.21:24:45
Bug.
msg1810 (view) Author: jendrik Date: 2011-10-17.21:09:15
Fast Downward currently accepts "cost_type=" without a valid cost type. Is this 
a bug or a feature?

./downward-2 --heuristic "hAdd=add(cost_type=)" --search 
"lazy(alt([single(sum([g(),weight(hAdd, 7)])),single(sum([g(),weight(hAdd, 
7)]),pref_only=true)], boost=0),preferred=[hAdd],reopen_closed=true,cost_type=)" 
< ../../new-scripts/preprocessed-tasks/WORK-WORK/gripper/prob01.pddl/output
Simplifying transitions... done!
Conducting lazy best first search, (real) bound = 2147483647
Initializing additive heuristic...
Simplifying 66 unary operators... done! [66 unary operators]
Best heuristic value: 12 [g=0, 1 evaluated, 0 expanded, t=0s]
Best heuristic value: 11 [g=1, 3 evaluated, 2 expanded, t=0s]
Best heuristic value: 10 [g=2, 4 evaluated, 3 expanded, t=0s]
Best heuristic value: 9 [g=3, 5 evaluated, 4 expanded, t=0s]
Best heuristic value: 8 [g=5, 8 evaluated, 7 expanded, t=0s]
Best heuristic value: 7 [g=6, 9 evaluated, 8 expanded, t=0s]
Best heuristic value: 6 [g=7, 10 evaluated, 9 expanded, t=0s]
Best heuristic value: 5 [g=9, 13 evaluated, 12 expanded, t=0s]
Best heuristic value: 4 [g=10, 14 evaluated, 13 expanded, t=0s]
Best heuristic value: 3 [g=11, 15 evaluated, 14 expanded, t=0s]
Best heuristic value: 2 [g=13, 19 evaluated, 18 expanded, t=0s]
Best heuristic value: 1 [g=14, 20 evaluated, 19 expanded, t=0s]
Solution found!
Actual search time: 0s [t=0s]
pick ball4 rooma left (1)
move rooma roomb (1)
drop ball4 roomb left (1)
move roomb rooma (1)
pick ball3 rooma right (1)
move rooma roomb (1)
drop ball3 roomb right (1)
move roomb rooma (1)
pick ball1 rooma right (1)
move rooma roomb (1)
drop ball1 roomb right (1)
move roomb rooma (1)
pick ball2 rooma right (1)
move rooma roomb (1)
drop ball2 roomb right (1)
Plan length: 15 step(s).
Plan cost: 15
Initial state h value: 12.
Expanded 20 state(s).
Reopened 0 state(s).
Evaluated 21 state(s).
Evaluations: 21
Generated 81 state(s).
Dead ends: 0 state(s).
Search time: 0s
Total time: 0s
Peak memory: 2656 KB
History
Date User Action Args
2023-07-25 10:49:33maltesetstatus: chatting -> resolved
messages: + msg11177
2017-04-21 18:56:35jendriksetstatus: in-progress -> chatting
assignedto: moritz ->
messages: + msg6203
title: OptionParser accepts incomplete parameters -> option parser accepts incorrect configurations
2017-04-04 10:54:24silvansetmessages: + msg6185
2016-08-19 12:13:52jendriksetmessages: + msg5568
2015-05-11 15:58:05jendriksetmessages: + msg4184
2015-03-18 17:27:34silvansetnosy: + silvan
messages: + msg4090
2015-03-17 15:11:32jendriksetmessages: + msg4077
2015-01-09 09:34:37jendriksetmessages: + msg3997
2015-01-09 00:10:53maltesetmessages: + msg3996
2015-01-08 23:25:44moritzsetmessages: + msg3995
2015-01-04 17:58:35jendriksetmessages: + msg3993
2015-01-04 14:54:05maltesetmessages: + msg3991
2015-01-03 19:18:12jendriksetmessages: + msg3990
2014-11-10 14:22:30jendriksetmessages: + msg3894
2014-05-19 17:23:27jendriksetmessages: + msg3171
2014-05-19 16:55:52moritzsetmessages: + msg3170
2014-04-30 00:44:18maltesetmessages: + msg3152
2014-04-29 18:50:31jendriksetmessages: + msg3151
2014-04-29 18:41:21moritzsetmessages: + msg3150
2014-04-29 18:14:09moritzsetmessages: + msg3149
2014-04-14 17:21:42moritzsetstatus: testing -> in-progress
2014-03-07 17:36:25moritzsetmessages: + msg3038
2014-03-06 21:29:52jendriksetmessages: + msg3037
2014-03-03 19:50:51jendriksetmessages: + msg3018
2012-09-26 00:39:05maltesetmessages: + msg2337
2012-09-25 23:05:26moritzsetmessages: + msg2336
2012-06-12 17:00:03moritzsetmessages: + msg2258
2012-06-12 15:51:03maltesetmessages: + msg2257
2012-06-12 15:37:53jendriksetmessages: + msg2256
2012-02-20 18:22:31maltesetmessages: + msg2068
2012-02-04 12:42:47moritzsetmessages: + msg2041
2011-12-31 11:25:03moritzsetmessages: + msg2011
2011-12-31 01:25:06maltesetmessages: + msg2010
2011-12-31 00:26:14jendriksetmessages: + msg2009
2011-12-30 12:20:03jendriksetmessages: + msg2008
2011-12-30 11:50:03moritzsetmessages: + msg2007
2011-12-29 16:50:00jendriksetmessages: + msg2006
2011-10-25 10:23:25moritzsetmessages: + msg1837
2011-10-24 09:40:03moritzsetmessages: + msg1836
2011-10-23 23:56:46jendriksetmessages: + msg1835
2011-10-18 09:07:18moritzsetstatus: chatting -> testing
assignedto: moritz
messages: + msg1819
2011-10-17 21:24:45maltesetstatus: unread -> chatting
messages: + msg1812
2011-10-17 21:09:15jendrikcreate