Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

NNUE integration work. #732

Closed
10 tasks done
vondele opened this issue Jul 25, 2020 · 20 comments
Closed
10 tasks done

NNUE integration work. #732

vondele opened this issue Jul 25, 2020 · 20 comments
Labels

Comments

@vondele
Copy link
Member

vondele commented Jul 25, 2020

some work will be needed to allow for NNUE testing, as mentioned official-stockfish/Stockfish#2823

I'm of course open to suggestions and welcome all help here.

Enabling or disabling NNUE can be done with a 'Use NNUE' option. If this is true we must download (either from the official-stockfish/networks or from the testing branch if this doesn't exist) the net and pass this with a cutechess option.

There are broadly three tasks I see right now.

  • communicate to users
  • building the binary
    • requires C++17 (done in the makefile)
    • picks the right target for building (Worker changes for NNUE testing #738)
    • only participate in NNUE enabled test if hardware has avx2 or better?
  • getting the network
    • get from the binary the name (UCI output provides EvalFile, the default value nn-sha256[12 digits].bin)
    • download if needed, using new infrastructure (merge: NN upload/download support #736)
  • instructing cutechess to do the right thing (merge: Worker changes for NNUE testing #738)
  • change Contempt=24 in Use NNUE=true for Test options (to be done after merge of NNUE in master)
@vondele
Copy link
Member Author

vondele commented Jul 25, 2020

One item that came to my mind, while we could compile for x86-64-avx2 on machines that support it, for regression tests vs. SF11 we would need to compile that branch still with x86-64-modern, as it is the only thing it supports. This transition needs some solution.

@nodchip
Copy link

nodchip commented Jul 25, 2020

I have several concerns.

AVX2 on some AMD CPUs
AMD CPUs from Excavator micro architecture to Zen+ micro architecture executes AVX2 instructions using 128-bit registers internally, and it causes speed down. In those CPUs, the results of comparisons between Stockfish NNUE and the original Stockfish will be unfair.
An idea to solve this problem is to create a black list of CPUIDs. I'm not sure if this can be implemented on fishtest.

BMI2 on AMD CPUs
BMI2 instructions are slow on AMD CPUs. If we disable BMI2 on AMD CPUs, nps will be increased. On the other hand, BMI2 instructions are fast on Intel CPUs. We could use x86-64-avx2 for AMD CPUs, and x86-64-bmi2 for Intel CPUs.

Three-way struggle
In computer shogi, it sometimes happens that a network A is stronger than a network B, the network B is stronger than a network C, and the network C is stronger than the network A. If we compare a new network and the strongest networks, it might pass SPRT but the elo might not be increased globally.
An idea to solve this problem is to compare a new network with the N strongest networks. Networks created by an author sometimes have similar characteristics in computer shogi. It may be better to compare a new network with the N strongest networks created by different creators.

These may be not critical problems. It may be better to postpone struggling against these problems after the first stage of the integration is done.

@vondele
Copy link
Member Author

vondele commented Jul 25, 2020

BMI2 on AMD: already now, the user can specify an arch file with custom_make.txt which allows for overriding the default and selecting the best architecture, some users specify bmi2 if it is beneficial. Initially that could be the best way forward. Nevertheless, we will indeed need to implement better processor selection within fishtest, probably best done automatically. Maybe we need to revisit the Makefile to have a 'native' target that just does the right thing setting the flags? The comparison classical vs NNUE will be dominated by the noob workers which will have avx2 or bmi2.

The Three-way struggle is indeed something that can happen with the handcrafted evaluation as well. So far, this has not been a critical issue. That might be different with the trained nets. Initially I would ignore this until the problem arises.

@noobpwnftw
Copy link
Contributor

It would be beneficial to include processor information(or at least the capabilities of bmi2, avx2, etc plus vendor) as a part of fishtest machine registration. Then the server can selectively assign tests to workers that qualify certain criteria if necessary.

@vondele
Copy link
Member Author

vondele commented Jul 25, 2020

Is it an option to include a python package in the worker?

Something like:
https://github.com/workhorsy/py-cpuinfo (see also https://pypi.org/project/py-cpuinfo/ ) would allow for getting the needed info quite conveniently.

@gonzalezjo
Copy link

gonzalezjo commented Jul 25, 2020

Is it an option to include a python package in the worker?

Something like:
https://github.com/workhorsy/py-cpuinfo (see also https://pypi.org/project/py-cpuinfo/ ) would allow for getting the needed info quite conveniently.

Fishtest already already includes the requests library as a subfolder in the worker directory. I can't see anyone objecting to adding one more.

@crossbr
Copy link

crossbr commented Jul 31, 2020

Regarding @nodchip comment's above about Zen architecture, for classical vs. nnue comparisons, why not run two benches, one with classical and one with nnue, and allot corresponding time controls according to the relative speeds?

(Also, on my Ryzen, nnue avx2 is bout 20% faster than nnue modern (but also about 12 degrees C hotter). Using avx2 for classical SF on fishtest could increase fishtest speed for workers like mine.)

@vondele
Copy link
Member Author

vondele commented Jul 31, 2020

@noobpwnftw I was wondering how you use custom_make.txt. I guess that it is more or less a standard compile basically make -j ARCH=x86-64-modern profile-build or make -j ARCH=x86-64-bmi2 profile-build depending on the architecture, right?

I think we should get rid of of the custom_make.txt, and instead try to infer the target in the worker. We already do that in part (windows vs. non-windows, and 32 vs 64 bit). Retaining custom_make.txt hinders the transition, e.g. we need a scheme that is smart enough for regression tests, e.g. build with x86-64-modern or x86-64-avx2 on amd depending on the targets exposed by the makefile. More advanced functionality that custom_make.txt offers should be enabled via options passed to the worker (e.g. skip profile-build if known broken).

This might be one of the next things I'll look into, but would need some advice from @ppigazzini or @tomtor, how do you feel about including py-cpuinfo like we do the requests package? It is python only, and if I'm right would be just one file. However, I'm not sure how to properly 'integrate' a package in the python world, if not just copying a file. If I know how to integrate the package, I can give a try at writing the code that does the compilation step.

@tomtor
Copy link
Contributor

tomtor commented Jul 31, 2020

I am not to happy about the requests python package being a static part of the fishtest tree, but until we replace that with eg a python -m pip install -user requests in a first stage worker startup script (would probably force reinstalling workers to be at least Python3) adding py-cpuinfo to the fishtest source tree is fine with me.

@vondele For pure python libraries (like py-cpuinfo) you can just add the files to the repo. No magic required.

@ppigazzini
Copy link
Collaborator

ppigazzini commented Jul 31, 2020

@tomtor @vondele I like more a virtual env in the worker folder to avoid packages conflict, .gitignore is already configured. In the wiki the linux worker script (used also for the windows subsystem for linux) creates a virtual env and installs the requests package, we can add the py-cpuinfo package. I will write the new user friendly script for windows msys2 to do the same thing and to retire the portable installer.

As plan B we could run make to analyze the CPU features, here a snippet of a wiki script:

arch_cpu=x86-64

if [ "$(g++ -Q -march=native --help=target | grep mbmi2 | grep enabled)" ] ; then
  if [ "$(g++ -Q -march=native --help=target | grep march | grep 'znver[12]')" ] ; then
    arch_cpu=x86-64-modern
  else
    arch_cpu=x86-64-bmi2
  fi
elif [ "$(g++ -Q -march=native --help=target | grep mpopcnt | grep enabled)" ] ; then
  arch_cpu=x86-64-modern
fi

@dorzechowski
Copy link

dorzechowski commented Aug 3, 2020

Would it be possible to distinguish somehow (for example with colour) the default net on the NNUE stats page? It can be found perhaps the same way as in Makefile, i.e. by grep'ing ucioption.cpp.

@vondele
Copy link
Member Author

vondele commented Aug 4, 2020

@dorzechowski the server doesn't really have the sources around, so it would require some additional code to do so.

@vdbergh
Copy link
Contributor

vdbergh commented Aug 4, 2020

I have been thinking that the NN page perhaps should include some metadata for the NNs... Like a comment field, a URL, a full author name, etc....

Also perhaps a user should be able to delete their own NNs (just in the html page, the actual api link would continue to exist).

@vondele
Copy link
Member Author

vondele commented Aug 4, 2020

More metadata is possibly somewhat problematic, i.e. need to sanitize/police comment fields on a public upload site, or not everybody wants his full name on github. Approvers can see the fishtest registration (i.e. email) of the author, so there is a little more than what is displayed. Right now, it really is a database for storing the nets, and the page reflects database content. I also would like things to be persistent, i.e. once added never removed (and always visible).

However, in principle, for each net in the database, we would hope/expect that the author performs a corresponding test on fishtest. The commit message that corresponds to that test can contain any info that the author wants to show. If the test is successful some of this info will be part of the merge commit into official-stockfish. So any net that has the 'default net' status (at any point in the git history), can have quite some (sanitized) metadata associated. I would be in favour of having some input on the generation of the nets for example in such a commit message.

As an additional idea, the network data file itself could have metadata fields .... in that way the information is much more closely attached to the net. That's maybe something for @nodchip to consider.

@nodchip
Copy link

nodchip commented Aug 4, 2020

Technically, we can add a comment to a net file, and add a new command to edit a net file comment to the software.

@vdbergh Could you please create an issue on my repository? I will take a look at it.

@vondele
Copy link
Member Author

vondele commented Aug 4, 2020

comment, and/or things like author name, creation date, ...

@vdbergh
Copy link
Contributor

vdbergh commented Aug 4, 2020

Perhaps it is better if @vondele creates the issue? @nodchip

@gekkehenker
Copy link

On the upload page it might be a good idea to make it more explicit that the (original) author should upload the net.

@vondele
Copy link
Member Author

vondele commented Aug 7, 2020

See #744

@vondele
Copy link
Member Author

vondele commented Aug 7, 2020

I'll close this issue now, as most of the work has been done. Refinements will follow of course.

@vondele vondele closed this as completed Aug 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

10 participants