Add Validation support for specifying GTP commands for each binary #1459

Hersmunch · 2018-05-20T14:41:00Z

Fixes #1455.
Also gitignore some local files and outputs.

gcp · 2018-05-20T17:54:01Z

gcp · 2018-05-28T09:03:47Z

Marco didn't reply?

I don't see anything wrong with this, but I have one concern: this seems to be a special case of the more generic problem of wanting to specify some GTP commands to be run at startup.

Would you be willing to rework this to just take some arbitrary startup command(s)? "time_settings blah" would then be one of them?

Hersmunch · 2018-05-28T09:52:44Z

I was actually thinking about trying that anyway so that handicap games could be tested too.
Would you prefer specifying them as a string or to give a file name?
I’ll have a go at it during the week.

marcocalignano · 2018-05-28T09:59:38Z

It is not a easy review because Game.cpp is used also by autogtp. And I was busy last week I see what I can do this week.

gcp · 2018-05-28T12:35:42Z

Would you prefer specifying them as a string or to give a file name?

Multiple strings I think. Asking the user to include "\n" in the command is messy. Better to allow the same command line option multiple times.

marcocalignano

autogtp seems to work with this patch

Hersmunch · 2018-05-28T22:11:52Z

Thanks guys.
One more question, do we want to be able to give each side a different set of commands or is one set for both always going to be sufficient?

marcocalignano · 2018-05-28T22:40:49Z

What do you mean by:

different set of commands

Hersmunch · 2018-05-28T22:58:40Z

As in two different sets of initial gtp commands, one for each binary, as per above:

Would you be willing to rework this to just take some arbitrary startup command(s)? "time_settings blah" would then be one of them?

marcocalignano · 2018-05-29T05:52:43Z

The problem that I see is that the Game object is shared with autogtp so you have to be careful to port the changes even in autogtp. I would a different member function that accept a string list of GTP commands. Also I would concentrate on that (Game object changes) first before you add the list in main. And yes then you should do one list for each player.
If you need help just tell me I pulled you branch so I can have a look at your commits. And if you think it is difficult I will take over no problem.

Hersmunch · 2018-05-29T06:58:25Z

Got it, I appreciate the guidance.
Any thoughts on how to identify which gtp commands from the argument list are for which binary? If the gtp commands can be different then I assume there might a different amount with no obvious first command for the second binary.

marcocalignano · 2018-05-29T07:45:14Z

Since you use parser.values the string list will be built in the order the option are found so you can have someting like:

validation -b leeaz --gtp-command "load_sgf  mysgf; set_free_handicap gc" -b leelaz_deepsearch --gtp-command "time setting 010"

Hersmunch · 2018-05-29T16:58:11Z

That sounds simple but @gcp said it is messy above. Another way that does follow his preference would be to also specify the side the commands are for e.g.
validation -b leeaz --gtp-command 0 "load_sgf mysgf" --gtp_command 0 "set_free_handicap gc" -b leelaz_deepsearch --gtp-command 1 "time setting 010"
The order would be the same as they are provided.
@gcp which do you want?

gcp · 2018-05-30T06:45:08Z

It would become

validation -b leeaz --gtp-command "load_sgf mysgf" --gtp-command "set_free_handicap gc" -b leelaz_deepsearch --gtp-command "time setting 0 1 0"

That is, the commands refer to the last specified engine. No need to specify which one then.

To get some idea of how this can work, take a look at cutechess-cli.

marcocalignano · 2018-05-30T06:53:04Z

@gcp Don't you like the suggestion with the ';' separator between commands? It would be easier to implement with the QT CommandLineParser.

gcp · 2018-05-30T06:56:55Z

The problem I see is that if a GTP command would contain a ";" this solution would break.

marcocalignano · 2018-05-30T07:12:55Z

@gcp we can start with the separator character (a bit like sed style) so it would be:
--gtp-commnad "; loasgf my.sgf ; time settings 0 1 0" or --gtp-command "| loadsgf my.sgf | put-char ; "`
but maybe is to complicated for the user.

killerducky · 2018-05-30T13:37:03Z

How about --gtp-command-file commands.txt? And put all your gtp commands in a text file.

gcp · 2018-05-31T14:59:25Z

we can start with the separator character

No

but maybe is to complicated for the user.

Yes

gcp · 2018-05-31T15:00:51Z

And put all your gtp commands in a text file.

What's wrong with the command line?

The situation I described literally already exists in cutechess-cli, which is also written in Qt IIRC. I'm not asking for rocket science here, I'm asking to replicate an existing solution that works really well :(

Hersmunch · 2018-05-31T17:23:11Z

Thanks for the clarity. I'll make a start tonight.

Hersmunch · 2018-05-31T19:37:37Z

I've had a look at cutechess-cli's code and instructions on running and following that will result in e.g.:
validation -b name=leeaz gtp-command="load_sgf mysgf" gtp-command="set_free_handicap gc" -b name=leelaz_deepsearch gtp-command="time setting 0 1 0" -each n="networkfile"
I'll start with this approach. Shout if you want something else though.

Hersmunch · 2018-05-31T22:19:17Z

For this evening I have managed to change Game and Validation to be able to cope with multiple GTP commands but have left the Validations options as they were. Might get a bit more time tomorrow or sat morning.

marcocalignano · 2018-06-01T14:52:55Z

@Hersmunch What I suggest you should define the name of the binary (leelaz) aspositional arguments and set the ParseAsPositionalArguments flag.
In this way all option after the binary name will also be consider as positional arguments so you can keep the order in the list.
Of course all the general options should be before the binary name to be parse as options in from otherwise you need to parse them twice. (which I guess we need to do anyway).

alreadydone · 2018-06-02T08:29:19Z

I am trying to use validation for handicap games with a fixed binary playing black; after the existing commits I can append fixed_handicap 2 to the GTP command list by modifying the source, but black still plays the first move; I think the following changes switches the first player to white, but I am not sure if I am missing something.
https://github.com/Hersmunch/leela-zero/blob/6c4e1cc9802ede637cee392cd1dddafa42e0b5b6/validation/Validation.cpp#L48
switch play black and play white
https://github.com/Hersmunch/leela-zero/blob/6c4e1cc9802ede637cee392cd1dddafa42e0b5b6/autogtp/Game.cpp#L200
replace 0 by 1

Also, for not switching binaries between games, does commenting out the following two places suffice?
https://github.com/Hersmunch/leela-zero/blob/6c4e1cc9802ede637cee392cd1dddafa42e0b5b6/validation/Validation.cpp#L97
https://github.com/Hersmunch/leela-zero/blob/6c4e1cc9802ede637cee392cd1dddafa42e0b5b6/validation/Validation.cpp#L158

Thank you in advance for reviewing.

alreadydone · 2018-06-02T18:44:30Z

Follow-up: I modified https://github.com/Hersmunch/leela-zero/blob/6c4e1cc9802ede637cee392cd1dddafa42e0b5b6/autogtp/Game.cpp#L32 also and everything seems to work.

Hersmunch · 2018-06-03T23:12:34Z

@alreadydone I'll leave the support for handicap games that you mention for a separate PR.

marcocalignano · 2018-06-04T06:49:11Z

@Hersmunch no that is not what I meant!
TheQCommandLineOption should stay like they were before.
You only delete the binaryOption and add a positional argument for the binary files.
And you add a new option --gtp-command
After that you set the ParseAsPositionalArguments flag in the parser so everything after the first binary file will be consider a positional argument and you can get an order QStringList of that.
And please do not parse the command line with if else if.

Hersmunch · 2018-06-04T17:20:27Z

Haha, whoops! Sorry about that. OK, I'll have one more go at it :)

l1t1 · 2018-06-07T11:21:08Z

can we learn the gogui twogtp command format,

Hersmunch · 2018-06-07T19:13:00Z

@marcocalignano, I gave up trying to find a way for general options to be specified after a binary is specified. Hopefully this is more what you were thinking?

Usage: validation.exe [options] binary

Options:
  -?, -h, --help                    Displays this help.
  -v, --version                     Displays version information.
  -g, --gamesNum <num>              Play 'gamesNum' games on one GPU at the
                                    same time.
  -u, --gpus <num>                  Index of the GPU to use for multiple GPUs
                                    support.
  -s, --sprt <lower:upper>          Set the SPRT hypothesis (default
                                    '0.0:35.0').
  -k, --keepSgf <output directory>  Save SGF files after each self-play game.
  -n, --network <filename>          Networks to use as players in competition
                                    mode (two are needed).
  -o, --options <opt_string>        Options for the binary given by -b (default
                                    "-g -v 3200 --noponder -t 1 -q -d -r 0 -w").

  -c, --gtp-command <command>       GTP command to send to the binary on
                                    startup (default "time_settings 0 1 0").
                                    Multiple commands are sent in the order they

                                    are specified.
                                    Commands apply to the preceeding binary or
                                    both if specified before all binaries.

Arguments:
  binary                            Binary to execute for the game (default
                                    ./leelaz).
                                    Only --gtp-command options are parsed after
                                    a binary is specified

Example:
validation -n "networkfile" "leeaz" --gtp-command "load_sgf mysgf" --gtp-command "set_free_handicap gc" "leelaz_deepsearch" --gtp-command="time setting 0 1 0"

Hersmunch · 2018-06-07T19:16:34Z

can we learn the gogui twogtp command format,

@l1t1 I had a look at https://www.mankier.com/1/gogui-twogtp but it doesn't appear to support specifying GTP commands to each side or allowing for handicap to be specified (except for maybe in an .sgf?).

l1t1 · 2018-06-08T02:43:59Z

@Hersmunch I found following from your link, but i haven't used them

GTP Extensions

gogui-interrupt

    Indicate interrupt ability for GoGui. TwoGtp will forward the interrupt to both programs, with the appropriate method, if they implement either gogui-interrupt or gogui-sigint. See the chapter "Interrupting Commands" in the GoGui documentation.
gogui-title

    Return a title for the current game, consisting of the game number (if option -sgffile was used) and the player names.
gogui-twogtp-black command

    Send command to the black player.
gogui-twogtp-observer command

    Send command to the observer program.
gogui-twogtp-referee command

    Send command to the referee program.
gogui-twogtp-white command

    Send command to the white player.

marcocalignano · 2018-06-08T06:05:19Z

@Hersmunch Yes that what I was thinking, I would also specify the command line a bit better:
Usage: validation.exe [option...] [gtp-command...] [binary1] [gtp-command...] [binary2] [gtp-comand...]
Did you already wrote some of the code to parse the command line?

Hersmunch · 2018-06-08T17:06:24Z

@marcocalignano I had a search around but haven't found out how to change the usage information. I agree it would be better as you describe.

Did you already wrote some of the code to parse the command line?

I'm not sure what you are asking. You can see the content of my commits, right?

marcocalignano · 2018-06-15T08:12:48Z

autogtp/Game.cpp

@@ -23,11 +23,12 @@
 #include <QFileInfo>
 #include "Game.h"

-Game::Game(const QString& weights, const QString& opt, const QString& binary) :
+Game::Game(const QString& weights, const QString& opt, const QString& binary,


Game is used from autogtp too. So I guess this change breaks autogtp.

I'll double check but the new parameter is defaulted in the constructor in the Game.h

Yep, still works for me.

marcocalignano · 2018-06-15T08:13:38Z

autogtp/Game.cpp

+    for (auto command : m_commands) {
+        sendGtpCommand(command);
+    }
+    QTextStream(stdout) << "Thinking time set." << endl;


You have to wait for and parse the answer of each command before you send the next.

Actually, doesn't sendGtpCommand() already do this?
I'll add catching of a it returning false (command failing) and exit though.

marcocalignano · 2018-06-15T08:18:13Z

validation/Validation.h

+    QString network;
+    QStringList commands;
+} engine_t;
+


I would make a proper Object with copy constructor, maybe even a move constructor and call it Engine. And maybe adding a member to handle the list of command.

Do you have a preference between using a struct or a class?
What do you mean by "handle the list of command"? I'm not understanding what I could add that would be helpful.

marcocalignano · 2018-06-15T08:22:16Z

validation/main.cpp

@@ -116,16 +118,35 @@ int main(int argc, char *argv[]) {
            return EXIT_FAILURE;
        }
    }
+
+    QStringList commandList = {"time_settings 0 1 0"};


You do not want to add this for default, what if someone wants to change the time settings?

gtp-commands given by the user are added to the end of the list so any other time_settings commands that follow will be done after this one, overwriting what this first one does. Seems to work from when I've used it.
I thought this would be easier than parsing commandList after all the user's commands had been added and looking to see if a time_settings one had been provided. It would seem cleaner to not always send this initial command though so I'd be happy to change it if anyone has ideas on a better approach.

marcocalignano · 2018-06-15T08:29:17Z

validation/main.cpp

+        pos_args = parser.positionalArguments();
+        engine_idx++;
+    }
+


Does this while loop works like we want?

From my limited usage of the changes it appears to work how I expect. Have you got any particular concerns? I might have misunderstood something :)
I was surprised at first that I didn't have to remove/pop the binary from pos_args but QCommandLineParser::process() expects the string list of arguments to be from the command line and therefore ignores the first item as "Usually arguments().at(0) is the program name..." http://doc.qt.io/qt-5/qcoreapplication.html#arguments. I have only got Windows to test on though.
I could add some comment(s) if that helps?

gcp · 2018-06-26T18:43:36Z

@Hersmunch I presume you're still working on this when you find some spare time?

Also gitignore some local files and outputs

… up but left the Validations options untouched.

…. Replaced time settings option with ability to specify any GTP commands.

…xisting option parser. Also changed default binary options from -p 1600 to -v 3200.

Hersmunch · 2018-06-27T07:32:31Z

Yes, I intend to but just haven't had a lot of time lately.
I've got a bit more done locally that I'll tidy up and push later today or tomorrow. It'll probably need a few more commits after that though.
Edit: Actually, I'm done unless @marcocalignano has anything else for me to tidy up?

gcp · 2018-07-03T10:28:38Z

@marcocalignano OK to merge?

marcocalignano · 2018-07-03T11:06:18Z

@gcp I didn't have time to test it but it looks fine and he tested it so go for it!

gcp · 2018-07-16T12:51:13Z

I had to back this out :-(

../validation/main.cpp: In function ‘int main(int, char**)’:
../validation/main.cpp:87:12: error: ‘class QCommandLineParser’ has no member named ‘setOptionsAfterPositionalArgumentsMode’; did you mean ‘clearPositionalArguments’?
     parser.setOptionsAfterPositionalArgumentsMode(QCommandLineParser::ParseAsPositionalArguments);
            ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
            clearPositionalArguments
../validation/main.cpp:87:71: error: ‘ParseAsPositionalArguments’ is not a member of ‘QCommandLineParser’
     parser.setOptionsAfterPositionalArgumentsMode(QCommandLineParser::ParseAsPositionalArguments);
                                                                       ^~~~~~~~~~~~~~~~~~~~~~~~~~

It looks like this was changed in Qt 5.6. We require Qt 5.3 only. The default Qt in Ubuntu 16.04 is 5.5.1.

At the very least, we'll need to bump our Qt dependency to 5.6, or avoid this. Ubuntu 18.04.1 is due in about 10 days at which point 16.04 upgrades will be possible. That sounds like a reasonable time to require a newer baseline, but we'd still screw some people over with this, though.

Hersmunch · 2018-07-16T17:31:29Z

:( sorry about that. Having a look at the Qt 5.2 documentation it looks like it might be possible to work around this by requiring a -- preceding each of the engines' options... is that worth trying?

marcocalignano approved these changes May 28, 2018

View reviewed changes

Hersmunch mentioned this pull request May 30, 2018

Continue searching deeper on cache hit #1492

Closed

Hersmunch force-pushed the val_timesettings branch from 6032cb2 to 6c4e1cc Compare May 31, 2018 22:16

Hersmunch changed the title ~~Added Validation support for specifying time_settings~~ WIP: Add Validation support for specifying GTP commands for each binary May 31, 2018

alreadydone mentioned this pull request Jun 1, 2018

Separate black and white networks for handicap games #1491

Closed

Hersmunch changed the title ~~WIP: Add Validation support for specifying GTP commands for each binary~~ Add Validation support for specifying GTP commands for each binary Jun 3, 2018

gcp force-pushed the next branch from 973dfc6 to d362ee8 Compare June 6, 2018 07:44

Hersmunch force-pushed the val_timesettings branch from 0152274 to 7d3f8b2 Compare June 7, 2018 19:09

marcocalignano reviewed Jun 15, 2018

View reviewed changes

Andy Hersee added 5 commits June 26, 2018 20:31

Added Validation support for specifying time_settings

d7789c0

Also gitignore some local files and outputs

Tidy up changes to gitignore

915f6a3

Changed Validation and Game to support multiple GTP commands at start…

d3436e0

… up but left the Validations options untouched.

Separated engine options (as positional arguments) from match options…

729768d

…. Replaced time settings option with ability to specify any GTP commands.

Reverted previous changes and added --gtp-command options using the e…

7a028d4

…xisting option parser. Also changed default binary options from -p 1600 to -v 3200.

Hersmunch force-pushed the val_timesettings branch from 7d3f8b2 to 15580cb Compare June 27, 2018 21:37

Andy Hersee added 2 commits June 27, 2018 22:42

Initial changes to use Engine Objects

7fa0688

Exits on failed GTP command

60a61e0

Hersmunch force-pushed the val_timesettings branch from 15580cb to 60a61e0 Compare June 27, 2018 21:43

gcp merged commit 81c484e into leela-zero:next Jul 16, 2018

Hersmunch mentioned this pull request Jul 24, 2018

Add Validation support for specifying GTP commands for each binary (2) #1652

Merged

Add Validation support for specifying GTP commands for each binary #1459

Add Validation support for specifying GTP commands for each binary #1459

Conversation

Hersmunch commented May 20, 2018

gcp commented May 20, 2018

gcp commented May 28, 2018

Hersmunch commented May 28, 2018

marcocalignano commented May 28, 2018

gcp commented May 28, 2018

marcocalignano left a comment

Choose a reason for hiding this comment

Hersmunch commented May 28, 2018

marcocalignano commented May 28, 2018

Hersmunch commented May 28, 2018

marcocalignano commented May 29, 2018

Hersmunch commented May 29, 2018

marcocalignano commented May 29, 2018

Hersmunch commented May 29, 2018

gcp commented May 30, 2018 • edited

marcocalignano commented May 30, 2018

gcp commented May 30, 2018

marcocalignano commented May 30, 2018

killerducky commented May 30, 2018

gcp commented May 31, 2018

gcp commented May 31, 2018 • edited

Hersmunch commented May 31, 2018

Hersmunch commented May 31, 2018 • edited

Hersmunch commented May 31, 2018

marcocalignano commented Jun 1, 2018

alreadydone commented Jun 2, 2018

alreadydone commented Jun 2, 2018

Hersmunch commented Jun 3, 2018

marcocalignano commented Jun 4, 2018

Hersmunch commented Jun 4, 2018

l1t1 commented Jun 7, 2018

Hersmunch commented Jun 7, 2018 • edited

Hersmunch commented Jun 7, 2018 • edited

l1t1 commented Jun 8, 2018 • edited

marcocalignano commented Jun 8, 2018

Hersmunch commented Jun 8, 2018

Choose a reason for hiding this comment

Hersmunch Jun 15, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gcp commented Jun 26, 2018

Hersmunch commented Jun 27, 2018 • edited

gcp commented Jul 3, 2018

marcocalignano commented Jul 3, 2018

gcp commented Jul 16, 2018

Hersmunch commented Jul 16, 2018

gcp commented May 30, 2018 •

edited

gcp commented May 31, 2018 •

edited

Hersmunch commented May 31, 2018 •

edited

Hersmunch commented Jun 7, 2018 •

edited

Hersmunch commented Jun 7, 2018 •

edited

l1t1 commented Jun 8, 2018 •

edited

Hersmunch Jun 15, 2018 •

edited

Hersmunch commented Jun 27, 2018 •

edited