Refactor Network Usage #5100

Disservin · 2024-03-10T08:56:55Z

Continuing from PR #4968, this update improves how Stockfish handles network usage, making it easier to manage and modify networks in the future.

With the introduction of a dedicated Network class, creating networks has become straightforward. See uci.cpp:

NN::NetworkBig({EvalFileDefaultNameBig, "None", ""}, NN::embeddedNNUEBig)

The new Network encapsulates all network-related logic, significantly reducing the complexity previously required to support multiple network types, such as the distinction between small and big networks #4915.

Non-Regression STC:
https://tests.stockfishchess.org/tests/view/65edd26c0ec64f0526c43584
LLR: 2.94 (-2.94,2.94) <-1.75,0.25>
Total: 33760 W: 8887 L: 8661 D: 16212
Ptnml(0-2): 143, 3795, 8808, 3961, 173

Non-Regression SMP STC:
https://tests.stockfishchess.org/tests/view/65ed71970ec64f0526c42fdd
LLR: 2.96 (-2.94,2.94) <-1.75,0.25>
Total: 59088 W: 15121 L: 14931 D: 29036
Ptnml(0-2): 110, 6640, 15829, 6880, 85

Compiled with make -j profile-build

bash ./bench_parallel.sh ./stockfish ./stockfish-nnue 13 50

sf_base =  1568540 +/-   7637 (95%)
sf_test =  1573129 +/-   7301 (95%)
diff    =     4589 +/-   8720 (95%)
speedup = 0.29260% +/- 0.556% (95%)

Compiled with make -j build

bash ./bench_parallel.sh ./stockfish ./stockfish-nnue 13 50

sf_base =  1472653 +/-   7293 (95%)
sf_test =  1491928 +/-   7661 (95%)
diff    =    19275 +/-   7154 (95%)
speedup = 1.30886% +/- 0.486% (95%)

No functional change

Disservin · 2024-03-10T19:16:58Z

By the way @mstembera I had something like this in this in my mind when you wrote the dual net patch, but of course doing all this in one PR (with dual net) would have been too much.

Sopel97 · 2024-03-10T20:53:34Z

src/nnue/network.h

+extern const EmbeddedNNUE embeddedNNUESmall;
+
+template<typename Arch, typename Transformer>
+class Network {


I almost feel like the Network and NetworkArchitecture classes could now naturally combine into one. Perhaps worth a try to see if it would work like that.

Other than that I like how this PR looks, I had a few minor issues but they have been since resolved. I like how everything got untangled from a big mess into a very simple form. Good work.

XInTheDark · 2024-03-11T02:13:23Z

src/search.cpp

-            return (ss->ply >= MAX_PLY && !ss->inCheck) ? evaluate(pos, thisThread->optimism[us])
-                                                        : value_draw(thisThread->nodes);
+            return (ss->ply >= MAX_PLY && !ss->inCheck)
+                   ? evaluate(networks, pos, thisThread->optimism[us])


I feel like passing networks to the evaluate function in search every time is quite counterproductive. Perhaps the function call in search could be kept the same?

how else would evaluate() access the networks?

@Disservin @Sopel97
Perhaps it would be more natural for the Eval namespace to store a reference to the networks rather than the Worker class storing it? It seems like search really should not know anything about any networks.

@mstembera ideally what we would like to achieve is to make it possible to create multiple stockfish's so that we have proper isolation for #4626, when you move it into the eval namespace I think this is going a step back again? Though I agree that it is perhaps a bit weird for search to know about the networks.. ideally search should get an "evaluator" which handles evaluation I guess?

search knowing networks is nothing wrong, since logically it uses them as a parameter of evaluation. Just normal dependency injection

@Disservin I think making SF a library is a nice goal and I don't want to get in the way of that. In either case though there is only one actual instance of the networks and the rest are just references to it whether in Eval or Worker so it should not be a step backwards? Maybe I'm missing something.

@Sopel97 I didn't mean to imply that it is wrong but I think less dependencies if they are unnecessary is better.

Maybe if I can make it work I will open a RFC PR so we can discuss more concretely.

Maybe if I can make it work I will open a RFC PR so we can discuss more concretely.

Sure!

Maybe I'm missing something.

What if the user instantiates two stockfish engines, with different network weights? I don't see how that would exactly work with your approach.

Ah ok I didn't know we also wanted to support different network weights for different instances. In that case I think your idea of turning the Eval namespace into and Evaluator class would solve it.

Yeah I think overall it'd be good to have.. on the weekend i quickly spun something to have uci and the engine a bit more split up master...Disservin:Stockfish:uci-split-engine, I imagine something in that direction, though that version isnt particularly useful right now for a library since everything returns void. Though I think it already looks quite good in terms of how "straight forward" the uci implementation is.

If you want to propose something for the evaluator class feel free to open a draft or the like.

Continuing from PR official-stockfish#4968, this update improves how Stockfish handles network usage, making it easier to manage and modify networks in the future. With the introduction of a dedicated Network class, creating networks has become straightforward. See uci.cpp: ```cpp NN::NetworkBig({EvalFileDefaultNameBig, "None", ""}, NN::embeddedNNUEBig) ``` The new `Network` encapsulates all network-related logic, significantly reducing the complexity previously required to support multiple network types, such as the distinction between small and big networks official-stockfish#4915. _This PR builds on top of official-stockfish#5098 to avoid merge conflicts._ Non-Regression STC: https://tests.stockfishchess.org/tests/view/65edd26c0ec64f0526c43584 LLR: 2.94 (-2.94,2.94) <-1.75,0.25> Total: 33760 W: 8887 L: 8661 D: 16212 Ptnml(0-2): 143, 3795, 8808, 3961, 173 Non-Regression SMP STC: https://tests.stockfishchess.org/tests/view/65ed71970ec64f0526c42fdd LLR: 2.96 (-2.94,2.94) <-1.75,0.25> Total: 59088 W: 15121 L: 14931 D: 29036 Ptnml(0-2): 110, 6640, 15829, 6880, 85 Compiled with `make -j profile-build` ``` bash ./bench_parallel.sh ./stockfish ./stockfish-nnue 13 50 sf_base = 1568540 +/- 7637 (95%) sf_test = 1573129 +/- 7301 (95%) diff = 4589 +/- 8720 (95%) speedup = 0.29260% +/- 0.556% (95%) ``` Compiled with `make -j build` ``` bash ./bench_parallel.sh ./stockfish ./stockfish-nnue 13 50 sf_base = 1472653 +/- 7293 (95%) sf_test = 1491928 +/- 7661 (95%) diff = 19275 +/- 7154 (95%) speedup = 1.30886% +/- 0.486% (95%) ``` closes official-stockfish#5100 No functional change

vondele · 2024-03-11T15:12:24Z

LGTM

No functional change

Continuing from PR official-stockfish#4968, this update improves how Stockfish handles network usage, making it easier to manage and modify networks in the future. With the introduction of a dedicated Network class, creating networks has become straightforward. See uci.cpp: ```cpp NN::NetworkBig({EvalFileDefaultNameBig, "None", ""}, NN::embeddedNNUEBig) ``` The new `Network` encapsulates all network-related logic, significantly reducing the complexity previously required to support multiple network types, such as the distinction between small and big networks official-stockfish#4915. Non-Regression STC: https://tests.stockfishchess.org/tests/view/65edd26c0ec64f0526c43584 LLR: 2.94 (-2.94,2.94) <-1.75,0.25> Total: 33760 W: 8887 L: 8661 D: 16212 Ptnml(0-2): 143, 3795, 8808, 3961, 173 Non-Regression SMP STC: https://tests.stockfishchess.org/tests/view/65ed71970ec64f0526c42fdd LLR: 2.96 (-2.94,2.94) <-1.75,0.25> Total: 59088 W: 15121 L: 14931 D: 29036 Ptnml(0-2): 110, 6640, 15829, 6880, 85 Compiled with `make -j profile-build` ``` bash ./bench_parallel.sh ./stockfish ./stockfish-nnue 13 50 sf_base = 1568540 +/- 7637 (95%) sf_test = 1573129 +/- 7301 (95%) diff = 4589 +/- 8720 (95%) speedup = 0.29260% +/- 0.556% (95%) ``` Compiled with `make -j build` ``` bash ./bench_parallel.sh ./stockfish ./stockfish-nnue 13 50 sf_base = 1472653 +/- 7293 (95%) sf_test = 1491928 +/- 7661 (95%) diff = 19275 +/- 7154 (95%) speedup = 1.30886% +/- 0.486% (95%) ``` closes official-stockfish#5100 No functional change

Disservin added the no-functional-change label Mar 10, 2024

Disservin changed the title ~~Refactor Network Creation~~ Refactor Network Usage Mar 10, 2024

Disservin marked this pull request as ready for review March 10, 2024 20:27

Sopel97 reviewed Mar 10, 2024

View reviewed changes

XInTheDark reviewed Mar 11, 2024

View reviewed changes

Disservin force-pushed the refactor-nnue branch from 15ce838 to da43e2c Compare March 11, 2024 09:23

Disservin force-pushed the refactor-nnue branch from da43e2c to aa8ff60 Compare March 11, 2024 09:42

Small cleanup

131dc64

No functional change

Disservin added the to be merged Will be merged shortly label Mar 12, 2024

Disservin closed this in 1a26d69 Mar 12, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor Network Usage #5100

Refactor Network Usage #5100

Disservin commented Mar 10, 2024 •

edited

Loading

Disservin commented Mar 10, 2024 •

edited

Loading

Sopel97 Mar 10, 2024

XInTheDark Mar 11, 2024

Sopel97 Mar 11, 2024

mstembera Mar 14, 2024

Disservin Mar 15, 2024

Sopel97 Mar 15, 2024 •

edited

Loading

mstembera Mar 17, 2024

Disservin Mar 17, 2024 •

edited

Loading

mstembera Mar 17, 2024

Disservin Mar 17, 2024

vondele commented Mar 11, 2024

Refactor Network Usage #5100

Refactor Network Usage #5100

Conversation

Disservin commented Mar 10, 2024 • edited Loading

Disservin commented Mar 10, 2024 • edited Loading

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

Sopel97 Mar 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Disservin Mar 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vondele commented Mar 11, 2024

Disservin commented Mar 10, 2024 •

edited

Loading

Disservin commented Mar 10, 2024 •

edited

Loading

Sopel97 Mar 15, 2024 •

edited

Loading

Disservin Mar 17, 2024 •

edited

Loading