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

[WIP] New package: KataGo-1.11.0 #38190

Closed
wants to merge 0 commits into from
Closed

[WIP] New package: KataGo-1.11.0 #38190

wants to merge 0 commits into from

Conversation

jason1987d
Copy link
Contributor

@jason1987d jason1987d commented Jul 22, 2022

Testing the changes

  • I tested the changes in this PR: NO
    This is a work in progress.

New package

Main question so far, CMakeLists.txt is found in ${wrksrc}/cpp, what configure_flag or other variable do I set to make it work and find that? I've set cmake_builddir to that which didn't work.

I discussed briefly with @Chocimier about this package when I submitted the new package q5go #35831 and will have other architecture questions.

This package has multiple backends: OpenCL, Eigen, TensorRT. I use an AMD Ryzen system and notice I already have mesa-opencl installed, I see there is another opencl implementation for Nvidia cards, and I suppose others (i.e. Intel) could even be ported to void. Eigen is available in the repos. Would this be multiple templates i.e. KataGo-OpenCL KataGo-Eigen etc or would I need to set up KataGo as some sort of metapackage?

Copy link
Member

@Chocimier Chocimier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For different backends it will be easier to add sepearate templates that use alternatives=.

based on homebrew formula phases not defined by upstream would be

do_check() {
	build/katago runtests
}

do_install() {
	vbin build/katago
}

built, untested, noatomics platform need standard handling

#create_wrksrc=yes
build_style=cmake
#configure_args=""
make_build_args="-DUSE_BACKEND=OPENCL"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

configure_args="-DUSE_BACKEND=OPENCL -DNO_GIT_REVISION=1"

#conf_files=""
#make_dirs="/var/log/dir 0755 root root"
hostmakedepends=""
makedepends="mesa-opencl"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

makedepends="ocl-icd-devel zlib-devel libzip-devel", so ocl-icd is used to find right opencl implementation

depends=""
short_desc="KataGo Go/Weiqi/Baduk analysis engine (OpenCL backend)"
maintainer="Jason Manley <jason@jasondavid.tv>"
license="GPL-3.0-or-later"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's mostly MIT, will need to include all license files

srcpkgs/KataGo-OpenCL/template Outdated Show resolved Hide resolved
@classabbyamp classabbyamp added the new-package This PR adds a new package label Jul 22, 2022
@jason1987d
Copy link
Contributor Author

Fixed most of the issues listed here. I will read up and do what I can to figure out the alternatives system, and will also have a template where this builds using eigen as a backend.

@jason1987d
Copy link
Contributor Author

So it's failing for architectures that I would guess don't have SSE extensions? i686 and armv6l-musl. Should I set archs= and exclude those?

@Chocimier
Copy link
Member

armv6l is fixed by standard

if [ "$XBPS_TARGET_NO_ATOMIC8" ]; then
       configure_args+=" -DCMAKE_CXX_STANDARD_LIBRARIES=-latomic"
       makedepends+=" libatomic-devel"
fi

For i686 upstream would need to fix errors like below, could you report?

/builddir/KataGo-1.11.0/cpp/search/search.cpp: In member function 'void Search::runWholeSearch(std::atomic<bool>&, std::function<void()>*, bool, const TimeControls&, double)':
/builddir/KataGo-1.11.0/cpp/search/search.cpp:493:114: error: no matching function for call to 'min(double&, double_t&)'
  493 |     double upperBoundVisits = computeUpperBoundVisitsLeftDueToTime(rootVisits, timeUsed, std::min(tcLimit,maxTime));

@jason1987d
Copy link
Contributor Author

jason1987d commented Jul 22, 2022

For the first, made a fix. For the second, digging through issues https://github.com/lightvector/KataGo/issues .

For the second, not really a c++ ninja, but checking the types here "no matching function"

error: no matching function for call .... the types are off double vs double_t

double_t maxTime = pondering ? searchParams.maxTimePondering : searchParams.maxTime; double tcLimit = 1e30;
I would assume patch to define std::min for types double:double_t or just cast min(....,....) etc?

lightvector/KataGo#662

@jason1987d
Copy link
Contributor Author

Got the upstream to fix that error in several places, and I set up patches in my pull request.

@jason1987d
Copy link
Contributor Author

FATAL ERROR:
Failed test assert: sizeof(size_t) == 8

Copy link
Member

@Chocimier Chocimier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sizeof(size_t) == 8 is something that upstream should look at. As they did not reject your report, they likely want katago to run on 32bit archirectures.


if [ "$XBPS_TARGET_NO_ATOMIC8" ]; then
configure_args+=" -DCMAKE_CXX_STANDARD_LIBRARIES=-latomic"
makedepends+=" libatomic-devel"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if blocks usually go after variables block. In this case it is not only matter of style: makedepends is overwritten later on. Please move below checksum.

vbin build/katago
}

post_install() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do_check should go before do_install, same as execution order. No need to split post_install when do_install is defined in template.

@jason1987d
Copy link
Contributor Author

Is there any way to cancel the CI runs to save void resources if I pushed by mistake?

@Chocimier
Copy link
Member

If you when job is still queued, old one will be dropped. Cancelling can only be done manually by maintainer. Do not worry about that too much though.

@jason1987d
Copy link
Contributor Author

jason1987d commented Jul 29, 2022

It actually built for i686 on my machine, why would it then fail here on something like this?

APPROX_EQ(betapdf(1-1e-4,.5e5,.5e1), 8773.80701229644182604, 1e-14);

@jason1987d
Copy link
Contributor Author

Although yes I was able to get this built and installed on my system, I'll need the AMD opencl setup. Specifically, was complaining about the lack of this file:
/usr/share/clc/gfx909-amdgcn-mesa-mesa3d.bc'
which is apparently part of that. Similar named files i.e. starting with gfx900,902,904,906 were all there.

I'm not really sure how to actually get that to even package. void has similar available for nvidia at least so it should be possible, some advice would be useful.

I will try a template in here using Eigen.

@jason1987d
Copy link
Contributor Author

jason1987d commented Jul 30, 2022

For Eigen (CPU based) specifically, I found this error.

CMake Error at CMakeLists.txt:296 (find_package):
  By not providing "FindEigen3.cmake" in CMAKE_MODULE_PATH this project has
  asked CMake to find a package configuration file provided by "Eigen3", but
  CMake did not find one.

  Could not find a package configuration file provided by "Eigen3" with any
  of the following names:

    Eigen3Config.cmake
    eigen3-config.cmake

  Add the installation prefix of "Eigen3" to CMAKE_PREFIX_PATH or set
  "Eigen3_DIR" to a directory containing one of the above files.  If "Eigen3"
  provides a separate development package or SDK, be sure it has been
  installed.

I see 'eigen' itself listed in void repos, but not eigen-devel like how the KataGo docs mention libeigen3-dev for apt based distros. Maybe I need to edit the eigen template to add that -devel subpackage?

Also seeing the eigen package itself is a minor version out of date and listed as Orphaned, so I'll have a template update for that, and may be able to add a -devel subpackage there. Assuming I do this, does the directory in srcpkgs name need to change from eigen to eigen-devel ?

@Chocimier
Copy link
Member

According to llvm/llvm-project#44186, symlinking gfx909 to same file gfx900 will work. However, Void will not accept it until upstream accept the fix.

Eigen is header only library. In this case, instead of creating empty base package and putting everything into -devel, only base package exists. Simply put eigen into makedepends.

@jason1987d
Copy link
Contributor Author

According to llvm/llvm-project#44186, symlinking gfx909 to same file gfx900 will work. However, Void will not accept it until upstream accept the fix.

Eigen is header only library. In this case, instead of creating empty base package and putting everything into -devel, only base package exists. Simply put eigen into makedepends.

Putting eigen into makedepends had it build for x86_64 at least on my own machine. I'll post if any errors. I figured that would probably work but forgot to try that.

Upstream (lightvector) responded about my failed test on here with i686. As I suspected, different machines could get slight rounding errors in floating point values that would affect tests like that. They're suggesting other test floating point values that I would need to work through. Again this built for i686 on my own machine.

Have not yet tried the symlink for OpenCL clc yet, though that sounds awesome if true.

I am going to include a template update for 3.4.0 in this too if that's okay.

@jason1987d
Copy link
Contributor Author

From the most recent push, I have no idea what caused these errors. I built it at least for i686 and x86_64 on my own machine (both opencl and eigen based), and have it running (with eigen anyway).

Copy link
Member

@Chocimier Chocimier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CI runs check phase (xbps-src -Q) and fails on it. Eigen needs make_check_target="check" to pass and make_check=extended because it takes way too much time to finish.

Please rename installed binaries so variants will not overwrite one another and add alternatives.

build_style=cmake
configure_args="-DUSE_BACKEND=OPENCL -DNO_GIT_REVISION=1"
makedepends="ocl-icd-devel zlib-devel libzip-devel"
depends="mesa-opencl"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

People are expected to install right implementation themself.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

People are expected to install right implementation themself.

of OpenCL? How would that change this section? Do I just remove that depends= line?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes

build_style=cmake
configure_args="-DUSE_BACKEND=EIGEN -DNO_GIT_REVISION=1"
makedepends="zlib-devel libzip-devel eigen"
depends="eigen"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a header only library, eigen is compiled in not required on disk runtime.

@jason1987d jason1987d closed this Sep 11, 2022
@jason1987d
Copy link
Contributor Author

Reopening. Accidentally closed this. How would I get it reopened? In my testing with xbps-src pkg -Q KataGo- -a it has been building, yet the CI has errored out. I haven't figured it out yet and I can't see that error anymore.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new-package This PR adds a new package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants