search

LeelaChessZero / lc0

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

Confirm parameter setting with `info string` uci response #1598

Open mooskagh opened 3 years ago

mooskagh commented 3 years ago

Currently we don't return anything on successful setparam set, and return error non-standart response in error.

It would be more visible in GUI if we used info string <text> in both cases.

ErdoganSeref commented 2 years ago

Is setparam a method in a source file? What <text> should we return?

mooskagh commented 2 years ago

Error is output in this line: https://github.com/LeelaChessZero/lc0/blob/ba241420ced7bf40695f98f684e7deabaa3893da/src/chess/uciloop.cc#L143

The setparam is handled here: https://github.com/LeelaChessZero/lc0/blob/86288ac9ce1655355fa1c88cfd99505345411d24/src/engine.cc#L350

ErdoganSeref commented 2 years ago

Fixed part of the issue with #1755.

rooklift commented 2 years ago

Is this necessary? I don't think other engines do this.

mooskagh commented 2 years ago

Other engines probably don't do that indeed, but I believe it makes sense to make errors more visible.

Most of GUIs will show info string messages to the user somehow instead of ignoring, and I think it good and I don't even see drawbacks. Do you see why it could be bad?

rooklift commented 2 years ago

I guess it will be fine, it's just I already have to parse info string lines for verbose move stats purposes, but I can probably ignore these.

Having said that, I think info lines are generally supposed to be sent during a search. I can't immediately predict what my own GUI will do when they come at other times, let alone other people's GUIs. [OK, I guess mine will ignore it.]

mooskagh commented 2 years ago

Actually, particularly for confirming that the command worked, it may be questionable whether it's good or bad. But for reporting errors, I think has much more pros than cons.

I'm pretty sure that many engines use info string for various purposes outside of search, most frequently to output license/logo/etc at the startup.

Hold on with the changes though, there's another one coming in which may be useful, I hope to add jsoninfo response soon (work in progress here: https://github.com/mooskagh/lc0/tree/json) which should be much easier to parse.

KarlKfoury commented 2 months ago

When you ask for returning something when setparam set, you mean when 'setoption name value' UCI command is provided?

mooskagh commented 2 months ago

Thanks for making the change!

Actually, I think I've changed my mind since 3 years ago... Now I'm inclined not to send anything when the setting is successful (and if we do want to confirm it, it should have been prepended with info string).

However, it still does make sense to output info string error instead of just error on errors.