search

LeelaChessZero / lc0

The rewritten engine, originally for tensorflow. Now all other backends have been ported here.
GNU General Public License v3.0
2.45k stars 530 forks source link

Improve error checking in time manager options #1480

Open borg323 opened 3 years ago

borg323 commented 3 years ago

Following a question on discord, I checked and --time-manager=legacy(steepness4.0) is accepted but does nothing.

KarlKfoury commented 2 months ago

Following a question on discord, I checked and --time-manager=legacy(steepness4.0) is accepted but does nothing.

In this case, were both legacy and steepness set through the UCI? Or some other protocol

borg323 commented 2 months ago

This is a command line switch, the corresponding uci option is TimeManager. The issue is that there should be a = like this, --time-manager=legacy(steepness=4.0) but there is no error check.

KarlKfoury commented 2 months ago

This is a command line switch, the corresponding uci option is TimeManager. The issue is that there should be a = like this, --time-manager=legacy(steepness=4.0) but there is no error check.

There seems to be no error checks concerning whether valid value was provided to the flag in the value parsing and setting process (Option::ProcessLongFlag, Option::SetVal, optionsdict::Set ). So any non-valid value can be provided like --time-manager=NonExistentTimeManager and it would still be accepted with no error. Wouldn't it be better to make a list for all acceptable value values for all flags, and compare the user input to it in one of these steps, raising an error if no match is found?

borg323 commented 2 months ago

We don't check the validity of options early (except basic range/value checks). This is the same for filenames, the final checks are when starting a search. So if there is an invalid time manager like in your example, it will generate an error message. Similarly, spelling mistakes or unknown time manager options will be caught, but only if there is an = sign in the text. If the = is missing the value is ignored, which is the issue here. So --time-manager=legacy(foo=bar) is giving an error but --time-manager=legacy(foobar) is not. I think what is happening is the token is treated as a new subdict during the call to AddSubdictFromString() call, which should be easy to check in this case but I'm not sure whether a more general solution is possible.

mooskagh commented 2 months ago

I remember intending to allow expressions like --time-manager=legacy(foobar), e.g. for something like --backend-opts=multiplexing(one,two,thre), but I don't know whether it's used.

Possibly we need to redesign that config line language.