Handling UCI protocol and Stockfish / Glaurung behavior

[mcostalba]

True, but as Pascal and Gian-Carlo points out, this is a bug. The engine shouldn't send a "bestmove" before receiving a "stop" command when doing an infinite search.

Yes. This is a bug.

I have no time to check right now, but I think this can be fixed by changing a single line of code. In the id_loop() function, change this code fragment:

Code: Select all

    // If we are pondering, we shouldn't print the best move before we
    // are told to do so
    if (PonderSearch)
      wait_for_stop_or_ponderhit();

To this:

Code: Select all

    // If we are pondering, we shouldn't print the best move before we
    // are told to do so
    if (PonderSearch || InfiniteSearch)
      wait_for_stop_or_ponderhit();

This is a quick fix but breaks stockfish bench command because from there think() is called with infinite = true.

I am working right now in a more invasive patch that fixes that bug and reorganize a bit the time management flags.

I will post shortly......

[pgeorges]

True, but as Pascal and Gian-Carlo points out, this is a bug. The engine shouldn't send a "bestmove" before receiving a "stop" command when doing an infinite search.

Tord, I am not completely sure that the first "bestmove" sent by Glaurung or StockFish is a bug. It may even be mandatory depending how you interpret what I put in bold below :

* bestmove <move1> [ ponder <move2> ]
the engine has stopped searching and found the move <move> best in this position.
the engine can send the move it likes to ponder on. The engine must not start pondering automatically.
this command must always be sent if the engine stops searching, also in pondering mode if there is a
"stop" command, so for every "go" command a "bestmove" command is needed!
Directly before that the engine should send a final "info" command with the final search information,
the the GUI has the complete statistics about the last search.

But for sure there is a buggy behavior when not replying correctly to the "stop" command.

[Gian-Carlo Pascutto]

I think the first one is also a bug. It's allowed to send bestmove when you stop searching, but you're not allowed to stop searching in analysis (=infinite) mode.

During a game, the engine can send it and stop by itself, of course.

[mcostalba]

I think some UCI engine does not handle correctly UCI protocol in some special circumstances, among them Stockfish (and of course some Glaurung versions, I did not check all).

Hi Pascal,

you are correct. It is a bug.


According to UCI specification http://wbec-ridderkerk.nl/html/UCIProtocol.html after a 'go infinite' command engine should "search until the "stop" command. Do not exit the search without being told so in this mode!"

Here is the patch against 1.6.2 that fixes the bug. You can use tool 'patch' http://gnuwin32.sourceforge.net/packages/patch.htm to apply even under Windows.

Code: Select all

Date: Sun, 10 Jan 2010 12:38:59 +0100
Subject: [PATCH] Fix sending of best move during an infinite search

According to UCI standard once engine receives 'go infinite'
command it should search until the "stop" command and do not exit
the search without being told so, even if PLY_MAX has been reached.

Patch is quite invasive because it cleanups some hacks used
by fixed depth and fixed nodes modes, mainly during benchmarks.

Bug found by Pascal Georges.

---
 src/benchmark.cpp |    2 +-
 src/search.cpp    |  105 ++++++++++++++++++++++++++---------------------------
 2 files changed, 53 insertions(+), 54 deletions(-)

diff --git a/src/benchmark.cpp b/src/benchmark.cpp
index d797264..3abcda6 100644
--- a/src/benchmark.cpp
+++ b/src/benchmark.cpp
@@ -155,7 +155,7 @@ void benchmark(const string& commandLine) {
       cerr << "\nBench position&#58; " << cnt << '/' << positions.size&#40;) << endl << endl;
       if &#40;limitType == "perft")
           totalNodes += perft&#40;pos, maxDepth * OnePly&#41;;
-      else if (!think&#40;pos, true, false, 0, dummy, dummy, 0, maxDepth, maxNodes, secsPerPos, moves&#41;)
+      else if (!think&#40;pos, false, false, 0, dummy, dummy, 0, maxDepth, maxNodes, secsPerPos, moves&#41;)
           break;
       totalNodes += nodes_searched&#40;);
   &#125;
diff --git a/src/search.cpp b/src/search.cpp
index b56cf9a..faac667 100644
--- a/src/search.cpp
+++ b/src/search.cpp
@@ -227,6 +227,7 @@ namespace &#123;
   int MaxNodes, MaxDepth;
   int MaxSearchTime, AbsoluteMaxSearchTime, ExtraSearchTime, ExactMaxTime;
   int RootMoveNumber;
+  bool UseTimeManagement;
   bool InfiniteSearch;
   bool PonderSearch;
   bool StopOnPonderhit;
@@ -368,21 +369,6 @@ bool think&#40;const Position& pos, bool infinite, bool ponder, int side_to_move,
            int time&#91;&#93;, int increment&#91;&#93;, int movesToGo, int maxDepth,
            int maxNodes, int maxTime, Move searchMoves&#91;&#93;) &#123;
 
-  // Look for a book move
-  if (!infinite && !ponder && get_option_value_bool&#40;"OwnBook"))
-  &#123;
-      Move bookMove;
-      if &#40;get_option_value_string&#40;"Book File") != OpeningBook.file_name&#40;))
-          OpeningBook.open&#40;get_option_value_string&#40;"Book File"));
-
-      bookMove = OpeningBook.get_move&#40;pos&#41;;
-      if &#40;bookMove != MOVE_NONE&#41;
-      &#123;
-          std&#58;&#58;cout << "bestmove " << bookMove << std&#58;&#58;endl;
-          return true;
-      &#125;
-  &#125;
-
   // Initialize global search variables
   Idle = false;
   SearchStartTime = get_system_time&#40;);
@@ -401,6 +387,24 @@ bool think&#40;const Position& pos, bool infinite, bool ponder, int side_to_move,
   FailLow = false;
   Problem = false;
   ExactMaxTime = maxTime;
+  MaxDepth = maxDepth;
+  MaxNodes = maxNodes;
+  UseTimeManagement = !ExactMaxTime && !MaxDepth && !MaxNodes && !InfiniteSearch;
+
+  // Look for a book move, only during games, not tests
+  if &#40;UseTimeManagement && !ponder && get_option_value_bool&#40;"OwnBook"))
+  &#123;
+      Move bookMove;
+      if &#40;get_option_value_string&#40;"Book File") != OpeningBook.file_name&#40;))
+          OpeningBook.open&#40;get_option_value_string&#40;"Book File"));
+
+      bookMove = OpeningBook.get_move&#40;pos&#41;;
+      if &#40;bookMove != MOVE_NONE&#41;
+      &#123;
+          std&#58;&#58;cout << "bestmove " << bookMove << std&#58;&#58;endl;
+          return true;
+      &#125;
+  &#125;
 
   if &#40;button_was_pressed&#40;"New Game"))
       loseOnTime = false; // reset at the beginning of a new game
@@ -463,48 +467,42 @@ bool think&#40;const Position& pos, bool infinite, bool ponder, int side_to_move,
   // Set thinking time
   int myTime = time&#91;side_to_move&#93;;
   int myIncrement = increment&#91;side_to_move&#93;;
-
-  if (!movesToGo&#41; // Sudden death time control
+  if &#40;UseTimeManagement&#41;
   &#123;
-      if &#40;myIncrement&#41;
+    if (!movesToGo&#41; // Sudden death time control
       &#123;
-          MaxSearchTime = myTime / 30 + myIncrement;
-          AbsoluteMaxSearchTime = Max&#40;myTime / 4, myIncrement - 100&#41;;
-      &#125; else &#123; // Blitz game without increment
-          MaxSearchTime = myTime / 30;
-          AbsoluteMaxSearchTime = myTime / 8;
+          if &#40;myIncrement&#41;
+          &#123;
+              MaxSearchTime = myTime / 30 + myIncrement;
+              AbsoluteMaxSearchTime = Max&#40;myTime / 4, myIncrement - 100&#41;;
+          &#125; else &#123; // Blitz game without increment
+              MaxSearchTime = myTime / 30;
+              AbsoluteMaxSearchTime = myTime / 8;
+          &#125;
       &#125;
-  &#125;
-  else // &#40;x moves&#41; / &#40;y minutes&#41;
-  &#123;
-      if &#40;movesToGo == 1&#41;
+      else // &#40;x moves&#41; / &#40;y minutes&#41;
       &#123;
-          MaxSearchTime = myTime / 2;
-          AbsoluteMaxSearchTime =
-             &#40;myTime > 3000&#41;? &#40;myTime - 500&#41; &#58; (&#40;myTime * 3&#41; / 4&#41;;
-      &#125; else &#123;
-          MaxSearchTime = myTime / Min&#40;movesToGo, 20&#41;;
-          AbsoluteMaxSearchTime = Min&#40;&#40;4 * myTime&#41; / movesToGo, myTime / 3&#41;;
+          if &#40;movesToGo == 1&#41;
+          &#123;
+              MaxSearchTime = myTime / 2;
+              AbsoluteMaxSearchTime =
+                 &#40;myTime > 3000&#41;? &#40;myTime - 500&#41; &#58; (&#40;myTime * 3&#41; / 4&#41;;
+          &#125; else &#123;
+              MaxSearchTime = myTime / Min&#40;movesToGo, 20&#41;;
+              AbsoluteMaxSearchTime = Min&#40;&#40;4 * myTime&#41; / movesToGo, myTime / 3&#41;;
+          &#125;
       &#125;
-  &#125;
 
-  if &#40;PonderingEnabled&#41;
-  &#123;
-      MaxSearchTime += MaxSearchTime / 4;
-      MaxSearchTime = Min&#40;MaxSearchTime, AbsoluteMaxSearchTime&#41;;
+      if &#40;PonderingEnabled&#41;
+      &#123;
+          MaxSearchTime += MaxSearchTime / 4;
+          MaxSearchTime = Min&#40;MaxSearchTime, AbsoluteMaxSearchTime&#41;;
+      &#125;
   &#125;
 
-  // Fixed depth or fixed number of nodes?
-  MaxDepth = maxDepth;
-  if &#40;MaxDepth&#41;
-      InfiniteSearch = true; // HACK
-
-  MaxNodes = maxNodes;
+  // Set best NodesBetweenPolls interval
   if &#40;MaxNodes&#41;
-  &#123;
       NodesBetweenPolls = Min&#40;MaxNodes, 30000&#41;;
-      InfiniteSearch = true; // HACK
-  &#125;
   else if &#40;myTime && myTime < 1000&#41;
       NodesBetweenPolls = 1000;
   else if &#40;myTime && myTime < 5000&#41;
@@ -782,7 +780,7 @@ namespace &#123;
 
         Problem = false;
 
-        if (!InfiniteSearch&#41;
+        if &#40;UseTimeManagement&#41;
         &#123;
             // Time to stop?
             bool stopSearch = false;
@@ -835,9 +833,9 @@ namespace &#123;
 
     rml.sort&#40;);
 
-    // If we are pondering, we shouldn't print the best move before we
-    // are told to do so
-    if &#40;PonderSearch&#41;
+    // If we are pondering or in infinite search, we shouldn't print the
+    // best move before we are told to do so.
+    if &#40;PonderSearch || InfiniteSearch&#41;
         wait_for_stop_or_ponderhit&#40;);
     else
         // Print final search statistics
@@ -2651,7 +2649,7 @@ namespace &#123;
                      || (  !FailHigh && !FailLow && !fail_high_ply_1&#40;) && !Problem
                          && t > 6*&#40;MaxSearchTime + ExtraSearchTime&#41;);
 
-    if (   &#40;Iteration >= 3 && (!InfiniteSearch && overTime&#41;)
+    if (   &#40;Iteration >= 3 && &#40;UseTimeManagement && overTime&#41;)
         || &#40;ExactMaxTime && t >= ExactMaxTime&#41;
         || &#40;Iteration >= 3 && MaxNodes && nodes_searched&#40;) >= MaxNodes&#41;)
         AbortSearch = true;
@@ -2666,8 +2664,9 @@ namespace &#123;
 
     int t = current_search_time&#40;);
     PonderSearch = false;
+
     if &#40;Iteration >= 3 &&
-       (!InfiniteSearch && &#40;StopOnPonderhit ||
+       &#40;UseTimeManagement && &#40;StopOnPonderhit ||
                             t > AbsoluteMaxSearchTime ||
                             &#40;RootMoveNumber == 1 &&
                              t > MaxSearchTime + ExtraSearchTime && !FailLow&#41; ||
-- 
1.6.4.msysgit.0

I am quite satisfied we are fixing many very old bugs after the releasing of 1.6.2, it means many people is testing / using it.


Thanks again
Marco

[Eelco de Groot]

Hello Marco,

Thanks for the patches and bugfixes! I was just wondering if there is a way that we can look how exactly the new search.cpp and bench.cpp etc. should look like. I understand you guys don't want to bring out 1.6.3, ..4 etc. right away after every bugfix. I just think if I am going to try to insert the new code and patches by hand I'm going to make mistakes. I am not familiar with the patch utility, is that the standard tool for this? If there was some repository somewhere with some new files just those with the proposed bugfixes, for instance also for the EPD handling proposed by Justin Blanchard, that would be very helpful. But if you think we should just try the utility because that is standard programmer procedure, just say so. Any advice appreciated!

Regards,
Eelco

[Sven]

I think some UCI engine does not handle correctly UCI protocol in some special circumstances, among them Stockfish (and of course some Glaurung versions, I did not check all).

Hi Pascal,

you are correct. It is a bug.


According to UCI specification http://wbec-ridderkerk.nl/html/UCIProtocol.html after a 'go infinite' command engine should "search until the "stop" command. Do not exit the search without being told so in this mode!"

Here is the patch against 1.6.2 that fixes the bug. You can use tool 'patch' http://gnuwin32.sourceforge.net/packages/patch.htm to apply even under Windows.

So what does the new version do now if either of these conditions applies:
a) The search has found a mate in N and it is 100% sure that a shorter mate is impossible.
b) An internal implementation limit of data structures has been reached (e.g. PLY_MAX) so that further searching would run into technical trouble.

Does the search stop but just postpone sending "bestmove" until "stop" is arriving, or does it continue infinite "searching" without really going deeper, i.e. it always repeats searching the same tree and printing the same PV and evaluation, which may produce a huge useless output within short time?

I would expect the first, although the standard seems to require that the search is really not stopped at all. It would be clearly wrong to exceed the internal PLY_MAX limit, and it would also be useless to go through 21589 iterations while repeatedly finding the mate in 2. I would expect that the engine behaves like "I'm done, I know the perfect result of analysis that I can possibly find with my resources but I'll tell you only when you ask for it". Some may argue that the engine would look like somehow "hanging" because no progress seems to be made but I don't think other behaviour would be more appropriate.

Btw the same case can be extended to WinBoard engines, too, since also there we have an "infinite analysis" mode which is only left on demand ("exit" in case of WB). The main difference for me is that in WB there is nothing like the UCI "bestmove" answer which is sent once by the engine, but instead the engine has to respond with a status update each time it is asked to do so by the "." command. But the question what would have to be done when the engine knows "the perfect answer" remains the same as with UCI.

Sven

[mcostalba]

Hello Marco,

Thanks for the patches and bugfixes! I was just wondering if there is a way that we can look how exactly the new search.cpp and bench.cpp etc. should look like. I understand you guys don't want to bring out 1.6.3, ..4 etc. right away after every bugfix. I just think if I am going to try to insert the new code and patches by hand I'm going to make mistakes. I am not familiar with the patch utility, is that the standard tool for this? If there was some repository somewhere with some new files just those with the proposed bugfixes, for instance also for the EPD handling proposed by Justin Blanchard, that would be very helpful. But if you think we should just try the utility because that is standard programmer procedure, just say so. Any advice appreciated!

Regards,
Eelco

Hello Eelco,

unfortunatly I cannot post the current development version of search.cpp because is already changed with comparison with 1.6.2 so it won't compile. For instance, to prapare this patch I needed, first to write and test against current development version and then backport to 1.6.2 and it took a while.

I suggest to use 'patch' tool, but if you are more confortable with other source control systems like git, tortoise, svn, cvs, etc.. it is the same because from any SCM tool you can easily import the patch and it will apply correctly.

In case you have problems finding / using the tool please don't hesitate to ask me, I will try to help you as I can.

Releasing another version IMHO is a bit too cumbersome first becasue there has been already a bit of mess in the last weeks about this point, second becasue the patch is no critical, third because does exists already an (out of law ) 1.6.3 released by _someone_ .....

Ok, apart from the jokes, I take the opportunity to kindly ask you to avoid, if possible, using the same release numbering used by SF for a derivative work of yours. It is already a bit uneasy to follow, as we have seen, if the derivative keeps the same name, but this is not a problem for very small changes, but please at least use a different release numbering scheme .


Thanks
Marco

[mcostalba]

I would expect the first, although the standard seems to require that the search is really not stopped at all.

Yes, it is like this, engine waits for GUI sending stop command. To be precise engine blocks on a std::getline() in wait_for_stop_or_ponderhit() function.

[Uri Blass]

Then just after this, if you enter

Code: Select all

stop

SF replies

Code: Select all

Unknown command: stop

Because has already stopped.

"stop" and "ponderhit" are handled while searching, in poll() function in search.cpp, while other commands that are handled during waiting for user input are handled in uci.cpp in function handle_command()

So answer is "unknown command" because you send this command once engine has already terminated the search and is already stopped (and as already sent a best move BTW).

True, but as Pascal and Gian-Carlo points out, this is a bug. The engine shouldn't send a "bestmove" before receiving a "stop" command when doing an infinite search.

I have no time to check right now, but I think this can be fixed by changing a single line of code. In the id_loop() function, change this code fragment:

Code: Select all

    // If we are pondering, we shouldn't print the best move before we
    // are told to do so
    if (PonderSearch)
      wait_for_stop_or_ponderhit();

To this:

Code: Select all

    // If we are pondering, we shouldn't print the best move before we
    // are told to do so
    if (PonderSearch || InfiniteSearch)
      wait_for_stop_or_ponderhit();

As for outputting 200 lines despite finding a mate quickly, I don't consider this a serious problem, and I don't want to do anything about it. When playing games, Stockfish makes a move quickly when it has found a forced mate. In analysis mode, however, it keeps searching, because it is possible that a faster mate will be found at some later iteration. Of course this won't ever happen for a mate in 2, but writing a special case for a mate in 2 is a little too ugly for my taste.

Thanks for the bug report, Pascal!

I think that the best solution is simply not to print new pv unless stockfish changes the content of the pv.
You do not need a special case for mate in 2 for it.

Uri

[Eelco de Groot]

Thanks Marco!

Oh well, silly Stockfish 1.6.3s is just a private version at the moment. And the changes to 1.6.2s were so little that giving it another name seemed silly as well. The number is not really that important but it was just to give it a minimal difference, and playing a little on all the confusion of course. I mean if Rybka can have rip offs But maybe there will be more changes to the 1.6.3s version, if I can just figure out how I can sense that it is becoming more difficult to find areas were improvement is obviously possible/needed, because Stockfish getting better now. The strange PVs when Stockfish's aspiration windows became too narrow for the PV, those seem to be almost gone for instance and I have not yet figured out how you guys changed that. There are so many changes in the code. But that is only good.

Had not much luck with the patch utility, I probably best stick to trying to insert code manually. That forces me to look at the new code better and that would not be wasted time anyway. I tried to install Gnu's patch but strangely in the Start menu I can find the documentation but not the executable. I can just place the utility in the file with the sources but I figure then I still have to run it from the commandline in a DOS box, and the command line can't seem to find my E: partition where I would like to run the patch utility. I can do a DIR E: that works but not CD E: Maybe it is some sort of security issue in Windows XP or I forgot the proper commands. At that point I stopped, it can wait till later.

Thanks!
Eelco