go nodes 0 does infinite search

[mooskagh]

It should stop immediately instead (or do one iteration when the tree is empty).

[sovietpanzzer]

Here seems to be the problem :

In the file src/mcts/stoppers/stoppers.h

class PlayoutsStopper : public SearchStopper {
 public:
  PlayoutsStopper(int64_t limit, bool populate_remaining_playouts)
      : nodes_limit_(limit ? limit : 4000000000ll), \\Here seems to be the problem
        populate_remaining_playouts_(populate_remaining_playouts) {}
  int64_t GetVisitsLimit() const { return nodes_limit_; }
  bool ShouldStop(const IterationStats&, StoppersHints*) override;

 private:
  const int64_t nodes_limit_;
  const bool populate_remaining_playouts_;
};

For some reason when limit = 0, nodes_limit is changed to 4000000000ll which makes go nodes 0 similar to infinite search.
A simple fix is to remove this condition. Does it affect other parts of the code ?

[mooskagh]

This 4'000'000'000 was introduced to prevent number of nodes overflowing uint32 (e.g. in go infinite case), and we'd like to keep that check. But I believe it should be not in the stoppers constructors (so I agree with removing it from there), but in the place where the stoppers are created:

// common.cc

  // "go nodes" stopper.
  int64_t node_limit = 0;
  if (params.nodes) {
    if (options.Get<bool>(kNodesAsPlayoutsId)) {
      stopper->AddStopper(std::make_unique<PlayoutsStopper>(
          *params.nodes, options.Get<float>(kSmartPruningFactorId) > 0.0f));
    } else {
      node_limit = *params.nodes;
    }
  }
  // always limit nodes to avoid exceeding the limit 4000000000. That number is
  // default when node_limit = 0.
  stopper->AddStopper(std::make_unique<VisitsStopper>(
      node_limit, options.Get<float>(kSmartPruningFactorId) > 0.0f));

Also I can see that you posted a snippet from PlayoutsStopper, but I cannot find this in the code; it's VisitsStopper that has this check. Are you sure that you are editing the latest version?

[sovietpanzzer]

Indeed i confused VisitsStopper and PlayoutsStopper, the code I was refering to is :

// src/mcts/stoppers/stoppers.h

// Watches visits (total tree nodes) and predicts remaining visits.
class VisitsStopper : public SearchStopper {
 public:
  VisitsStopper(int64_t limit, bool populate_remaining_playouts)
    : nodes_limit_(limit ? limit : 4000000000ll),
        populate_remaining_playouts_(populate_remaining_playouts) {}
  int64_t GetVisitsLimit() const { return nodes_limit_; }
  bool ShouldStop(const IterationStats&, StoppersHints*) override;

 private:
  const int64_t nodes_limit_;
  const bool populate_remaining_playouts_;
};

But how exactly is this preventing uint32 overflow ? Since a check on 0 doesn't mean it has overflowed for more than 0 ? Or am I missing something

[mooskagh]

If the limit is set to 0, the nodes_limit_ should indeed be zero.
But if the limit is not set at all, it should still be 4'000'000'000. As I said above, it's better to do in the calling code in common.cc though, and not in the stopper itself.

[KarlKfoury]

since GoParams.nodes is an std::optional, it's initialization status can be checked through params.nodes.has_value() instead of params.nodes, which returns false if params.nodes is set to 0 (source of the problem).

here

I find it odd that addStopper initiated with node_limit is run outside of the 'else' block. Meaning that if params.nodes is initiated 2 stoppers, one having 0 as a node_limit are initiated.

here

Is there a particular reason for this?

[KarlKfoury]

i believe PR #2056 fixes the issue