Make LeelaWatcher show games continued from q+Enter #1581

Nazgand · 2018-06-23T03:20:54Z

modified:   ../autogtp/Game.cpp
modified:   ../autogtp/Game.h
modified:   ../autogtp/main.cpp
modified:   GTP.cpp
modified:   GameState.cpp
modified:   GameState.h

modified: ../autogtp/Game.cpp modified: ../autogtp/Game.h modified: ../autogtp/main.cpp modified: GTP.cpp modified: GameState.cpp modified: GameState.h

Nazgand · 2018-06-23T03:29:46Z

Note: includes the change made in #1580

modified: ../src/GameState.cpp

marcocalignano · 2018-06-25T10:05:04Z

But why do you nee to change Game.cpp if it is a leela watcher? You should change only the code of loadSgf in leela.

Nazgand · 2018-06-25T10:34:30Z

LeelaWatcher is connected to the autogtp process, not the leelaz process. AutoGTP needed to be modified or LeelaWatcher needed to be modified, and I found modifying autogtp process to mimic the output of a game which was not continued from q+Enter when the game was continued from q+Enter to be easier than modifying LeelaWatcher(which I was unable to compile). The modification of Game.cpp changes the output which LeelaWatcher sees, and I am not aware of a way to change the output of AutoGTP as desired by only changing the LeelaZero source.

The code works, and the changes I made do not seem to be bad ones as far as I am aware. If you have any specific feedback on why what I did was bad or not in line with project coding practices, I will likely try to improve to get this pull request accepted.

On Linux, I have tested fsparv/LeelaWatcher#36 with 4 concurrent instances of LeelaWatcher. Closing all 4 and reopening was done without loss of training data. No solution is currently available for native Windows(unless the Windows Subsystem for Linux is considered native), yet someone able to build gradle projects will be able to modify LeelaWatcher to send q\n when the window closes.

EDIT: @marcocalignano Did you mean GameState.cpp rather than Game.cpp? Yes, that code could be moved to GTP.cpp. Would it be better there? Moving the code would make things more difficult because the game_history variable is private to GameState. The functions rewind() with forward_move() would work, as would get_past_board(), yet this seemed less elegant than adding a function to GameState.cpp.

marcocalignano · 2018-06-25T11:17:16Z

I do not like it because it is not the job of autogtp to read the sgf and print out all the saved moves on the output only because LeelaWatcher cannot implement a loadsgf function itself. It also clutters the output in case you are NOT using LeelaWatcher. Autogtp out put the filename to load LeelaWatcher should load that file.

Nazgand · 2018-06-25T11:40:39Z

I would understand if you argued that it is not the job of the leelaz process to print the contents of loaded SGFs over GTP connections, yet I do not understand how it is not the job of the autogtp process or how this change clutters the AutoGTP output more than the AutoGTP output is already cluttered. AutoGTP prints out every move of the game as the game is played, so why should AutoGTP not print out the moves preceding the moves which AutoGTP prints? All the moves printed on games continued from q+Enter are printed without context. Any context is in the SGF which AutoGTP deletes ASAP.

If AutoGTP does not give all the moves of a game, then all programs similar to LeelaWatcher would need to back up all the SGFs in the directory which have a matching .train file, or AutoGTP would need to be modified to not delete the SGFs.

Would copying the SGF parser to the AutoGTP source be more acceptable as this allows AutoGTP to be informative about q+Enter continuation games without requiring leelaz to clutter the GTP connection during SGF loads? Or should a new GTP command reiterate_moves be made? I was originally planning on making a reiterate_moves command, yet thought changing the loadsgf command would be best. With your input, I decided to modify my fork to revert the loadsgf command and add a reiterate_moves command.

…ded the reiterate_moves GTP command. modified: Game.cpp modified: ../src/GTP.cpp

Nazgand · 2018-06-27T16:30:29Z

I do think that AutoGTP did volunteer for the job of listing the moves of the game because AutoGTP does that anyway except in the specific case when continuing a game(does a command line option to silence the moves exist which is not listed in --help?), yet I noticed and advantage to modifying AutoGTP to delay deletion of the .train and .sgf files.

Consider the case when the autogtp process ends via q+Enter or timeout, and AutoGTP is later started and dies without saving a new .train and .sgf (maybe a newb does not know about q+Enter yet knows about timeout, like me not so long ago.). If the .train and .sgf deletion is deferred to when the game is completed or made into new .train and .sgf files, then this situation would not lose as much data as would be lost with the current implementation of AutoGTP.
Also, This would allow programs such as LeelaWatcher access to the SGF without needing to back up every SGF with a matching .train file in the autogtp working directory.

@marcocalignano would the deferred deletion be something you would not complain about?

marcocalignano · 2018-06-28T07:00:33Z

@Nazgand First of all I am not complaining I am just discussing to find the best solution.
Sesond the autogtp output is there to tell the user that the program is still working that why was never taken out.
So we could implement and extra flag for autogtp (--watcher) that tell autogtp to save the sgf in a file that LeelaWatcher can read to recreate the loaded game (could be an sgf or just a dump of the autogtp output. This would avoid the reiterate_moves, would avoid the output of the old moves on the console and would allow LeelaWatcher to show the game that was saved.

Nazgand · 2018-06-28T08:59:54Z

Hmmm. Supposing I add a --watcher flag, should AutoGTP by default output a single character per move such as a period to reduce clutter, only listing moves with the --watcher flag? Perhaps the tag should instead be called --verbose n.
--verbose 0 could run without any output
--verbose 1 could run without any output except periods to show progress
--verbose 2 could run the current default settings
--verbose 3 could output with maximum verbosity. E.g. all the info Lizzie requires from leelaz would be output by autogtp.
Using --verbose 3 could then allow a merger of the LeelaWatcher and Lizzie features into 1 project.

As for the --watcher flag saving the SGF, that does not need to be done because the .train and .sgf files already exist. The problem is AutoGTP deletes those files when continuing a game rather than waiting until the game is either finished or paused again.

marcocalignano · 2018-06-28T09:12:40Z

I think they are too many verbose but it is not a bad Idea.
The better Idea is for sure to leave the two file and delete them when the game finishes or is saved again, so LeelaWatcher can read the sgf when it chatceh the Loading file.sgf output from autogtp. This way no new option are needed. Would that be good enough for LeelaWatcher?

Nazgand · 2018-06-28T09:48:07Z

I will implement the delayed deletion in a different pull request. Delayed deletion should be good enough for LeelaWatcher, yet I have not been able to compile LeelaWatcher thus far because of gradle errors. Delayed deletion will allow those able to compile LeelaWatcher to add an SGF parser. Until that happens or I get around to caring enough about gradle to modify it myself, I will keep this pull request open. I see this pull request as valid until another solution is available.

gcp · 2018-07-06T17:16:24Z

autogtp/main.cpp

@@ -69,6 +69,9 @@ int main(int argc, char *argv[]) {
    QCommandLineOption singleOption(
        { "s", "single" }, "Exit after the first game is completed.",
                          "");
+    QCommandLineOption watcherOption(
+        { "w", "watcher" }, "Reject boring 0 resignation games.",


I really don't like this. 0 resignation games are essential for the learning to detect cases where it's highly convinced of the evaluation but it's wrong, and there's anecdotal evidence from other projects (aside from the published AGZ runs) that having a fair amount of them is required for good performance.

If we didn't think no resignation games were important, we wouldn't be doing them. Allowing opt out is essentially saying "please make the data worse for my own amusement".

I know, yet I expect that the server will give those 0-resignation games to people not using LeelaWatcher.
Does the server make sure each batch has a minimum amount of 0-resignation games, or does the server hope that enough 0-resignation games happen? If the server increases the probability of 0-resignation games when the proportion is too low or has not met the quote when resignation games have met the quota, as I expect it should, then most people opting out is not a problem.
Either way, my experience using LeelaWatcher greatly improved after this change, and I will not go back.

The server hands them out statistically, I think. So a sufficient amount of clients purposefully ignoring games will eventually cause things to go whack.

Either way, my experience using LeelaWatcher greatly improved after this change, and I will not go back.

🙄

The decision is simply made at the time a request is made. It isn't tracked to be sure we're getting a property # returned. So if a lot of people were using LeelaWatcher, we'd have less no-resign games arriving in the database. The missing ones wouldn't be re-requested to other clients.

Then changing the server code to account for this is needed. I will take a look and see if a fix is easy enough for me to care about doing.

I found the part where resignation_percent is assigned, yet the database structure seems to need modification to include network1_0r_wins, network1_0r_losses. SPRT(match.network1_0r_wins, match.network1_0r_losses) may also be a useful indicator.

Quoting 'Mastering the Game of Go without Human Knowledge': 'The resignation threshold v_resign is selected automatically to keep the fraction of false positives (games that could have been won if AlphaGo had not resigned) below 5%. To measure false positives, we disable resignation in 10% of self-play games and play until termination.'

server.js implements 20%(probably) rather than 10%(guaranteed):
if (Math.random() < 0.2) options.resignation_percent = "0";

Things will be fine(probably).

I'd prefer if this was taken out. Would surely make this a lot easier to pull.

gcp · 2018-07-16T12:33:12Z

autogtp/Game.cpp

@@ -330,7 +330,7 @@ bool Game::writeSgf() {
 }

 bool Game::loadTraining(const QString &fileName) {
-    QTextStream(stdout) << "Loading " << fileName + ".train" << endl;
+    //QTextStream(stdout) << "Loading " << fileName + ".train" << endl;


No commented out text, just remove it.

gcp · 2018-07-16T12:34:03Z

autogtp/main.cpp

@@ -69,6 +69,9 @@ int main(int argc, char *argv[]) {
    QCommandLineOption singleOption(
        { "s", "single" }, "Exit after the first game is completed.",
                          "");
+    QCommandLineOption watcherOption(
+        { "w", "watcher" }, "Reject boring 0 resignation games.",


I'd prefer if this was taken out. Would surely make this a lot easier to pull.

gcp · 2018-07-16T12:36:41Z

src/GTP.cpp

@@ -686,6 +687,9 @@ bool GTP::execute(GameState & game, std::string xinput) {
            gtp_fail_printf(id, "cannot load file");
        }
        return true;
+    } else if (command.find("reiterate_moves") == 0) {
+        gtp_printf(id, game.reiterate_moves().c_str());


The discussion in the topic gives some suggestions to avoid this. I think this is a really bad command to add, because obviously printsgf/loadsgf already allow recovering moves, so this is duplicate functionality.

gcp · 2018-07-16T12:38:24Z

The proposed solution here isn't acceptable to me because of a) the flag that voids games that are critical for the project b) adding a superfluous command.

I'll close this now to make it clear it won't be pulled without significant changes.

Make LeelaWatcher show games continued from q+Enter

3b15971

modified: ../autogtp/Game.cpp modified: ../autogtp/Game.h modified: ../autogtp/main.cpp modified: GTP.cpp modified: GameState.cpp modified: GameState.h

appveyor fix attempt 1

565d4cc

modified: ../src/GameState.cpp

Nazgand mentioned this pull request Jun 23, 2018

Add script which saves game progress on exit. fsparv/LeelaWatcher#36

Open

reverted the loadsgf GTP command to reduce unnecessary clutter and ad…

0d666aa

…ded the reiterate_moves GTP command. modified: Game.cpp modified: ../src/GTP.cpp

Nazgand added 2 commits June 28, 2018 09:52

Merge pull request #1 from gcp/next

08ed836

Next

Reject boring games if watcher flag is set.

ec21055

modified: Management.cpp modified: Management.h modified: main.cpp

gcp reviewed Jul 6, 2018

View reviewed changes

Hersmunch mentioned this pull request Jul 15, 2018

Importance of resign data status #421

Open

gcp reviewed Jul 16, 2018

View reviewed changes

gcp closed this Jul 16, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make LeelaWatcher show games continued from q+Enter #1581

Make LeelaWatcher show games continued from q+Enter #1581

Nazgand commented Jun 23, 2018

Nazgand commented Jun 23, 2018

marcocalignano commented Jun 25, 2018

Nazgand commented Jun 25, 2018 •

edited

marcocalignano commented Jun 25, 2018

Nazgand commented Jun 25, 2018 •

edited

Nazgand commented Jun 27, 2018

marcocalignano commented Jun 28, 2018

Nazgand commented Jun 28, 2018

marcocalignano commented Jun 28, 2018

Nazgand commented Jun 28, 2018

gcp Jul 6, 2018 •

edited

Nazgand Jul 6, 2018 •

edited

gcp Jul 6, 2018 •

edited

roy7 Jul 6, 2018

Nazgand Jul 6, 2018

Nazgand Jul 6, 2018

Nazgand Jul 8, 2018

gcp Jul 16, 2018

gcp Jul 16, 2018

gcp Jul 16, 2018

gcp Jul 16, 2018

gcp commented Jul 16, 2018

Make LeelaWatcher show games continued from q+Enter #1581

Make LeelaWatcher show games continued from q+Enter #1581

Conversation

Nazgand commented Jun 23, 2018

Nazgand commented Jun 23, 2018

marcocalignano commented Jun 25, 2018

Nazgand commented Jun 25, 2018 • edited

marcocalignano commented Jun 25, 2018

Nazgand commented Jun 25, 2018 • edited

Nazgand commented Jun 27, 2018

marcocalignano commented Jun 28, 2018

Nazgand commented Jun 28, 2018

marcocalignano commented Jun 28, 2018

Nazgand commented Jun 28, 2018

gcp Jul 6, 2018 • edited

Choose a reason for hiding this comment

Nazgand Jul 6, 2018 • edited

Choose a reason for hiding this comment

gcp Jul 6, 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

gcp commented Jul 16, 2018

Nazgand commented Jun 25, 2018 •

edited

Nazgand commented Jun 25, 2018 •

edited

gcp Jul 6, 2018 •

edited

Nazgand Jul 6, 2018 •

edited

gcp Jul 6, 2018 •

edited