allow backends to suggest minibatch size #1877
Conversation
| @@ -222,6 +222,13 @@ class SearchWorker { | |||
| this->RunTasks(i); | |||
| }); | |||
| } | |||
| target_minibatch_size_ = params_.GetMiniBatchSize(); | |||
There was a problem hiding this comment.
Not sure whether it's better, but maybe make params_.GetMiniBatchSize() an std::option<int>?
Then it would be possible to do target_minibatch_size_ = params_.GetMiniBatchSize().value_or(search_->network_->GetMiniBatchSize()).
Also, why not put target_minibatch_size_ and max_out_of_order_ into search_ rather than SearchWorker?
There was a problem hiding this comment.
The change to std::optional<int> seems a bit overcomplicated, even more since this way allows us to keep the 32 minibatch-size default for selfplay with no changes.
It seemed cleaner to put target_minibatch_size_ and max_out_of_order_ in SearchWorker since they are only used there.
| @@ -107,6 +107,7 @@ class Network { | |||
| virtual const NetworkCapabilities& GetCapabilities() const = 0; | |||
| virtual std::unique_ptr<NetworkComputation> NewComputation() = 0; | |||
| virtual void InitThread(int /*id*/) {} | |||
| virtual int GetMiniBatchSize() const { return 256; } | |||
There was a problem hiding this comment.
I wonder whether it would be cleaner to add the recommended minibatch size into the NetworkCapabilities rather than adding a new function (not sure which variant I prefer though).
There was a problem hiding this comment.
The 'NetworkCapabilities' so far only stores properties of the weights file, and I have plans to expand the interface to pass the search minibatch-size value so that the backend can adjust the memory buffer allocations as needed.
There was a problem hiding this comment.
Maybe NetworkCapabilities is a poor name, but it still looks cleaner to have all information that backend tells to the calling code in a structure, to make interface less leaky.
It's fine to do refactor this later though.
There was a problem hiding this comment.
I'm warming up to the idea, but I want to finish with the rest of my plan before doing it. There are some cornet cases I haven't though through yet.
| @@ -107,6 +107,7 @@ class Network { | |||
| virtual const NetworkCapabilities& GetCapabilities() const = 0; | |||
| virtual std::unique_ptr<NetworkComputation> NewComputation() = 0; | |||
| virtual void InitThread(int /*id*/) {} | |||
| virtual int GetMiniBatchSize() const { return 256; } | |||
There was a problem hiding this comment.
Maybe NetworkCapabilities is a poor name, but it still looks cleaner to have all information that backend tells to the calling code in a structure, to make interface less leaky.
It's fine to do refactor this later though.
There was a problem hiding this comment.
It would also make sense to look into proxy backends (i.e. multiplexing etc), whether they have to forward the information from their backends.
UPD: and also it makes me wonder how this new functionality works with the selfplay, as we use multiplexing there and potentially number of simultaneous training games depends on the batch size.
|
The proxy backends will be done in a next PR - it needs a similar treatment for threads first. |
* allow backends to suggest minibatch size * simple cuda heuristic (cherry picked from commit 8539794)
I only added actual suggestions where it was easy (blas, onednn and onnx).