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

python3Packages.catboost: 1.0.5 -> 1.2.2; build with cmake #235226

Merged
merged 3 commits into from
Oct 16, 2023

Conversation

natsukium
Copy link
Member

@natsukium natsukium commented May 31, 2023

Description of changes

Diff: catboost/catboost@v1.0.5...v1.2.2
Changelog: https://github.com/catboost/catboost/releases/tag/v1.2.2

CatBoost's build system has been switched from Ya Make (Yandex's build system) to CMake. This means more transparency in the build process and more familiar tools for Open Source developers.
For now it is possible to build CatBoost for:
Linux on x86-64 with or without CUDA
Linux on aarch64 with or without CUDA
macOS on x86-64 and arm64, including creating universal binaries
Windows on x86-64 with or without CUDA
Android (only model applier) on All supported ABIs.

Following @nviets suggestion in #208317, I split the cli application and shared libraries into separate packages.
I have successfully built cli and python-module.

The rest of the tasks are

  • build on other than x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • build with cuda

Skip in this PR

  • build the R libraries

I'm not familiar with JVM.

  • build the JVM libraries
  • build the spark libraries

cc @veprbl @PlushBeaver

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 23.05 Release Notes (or backporting 22.11 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

@nviets
Copy link
Contributor

nviets commented Jun 1, 2023

Exciting work, @natsukium! That conan part really got me when I gave this a try - cool to see you could just drop it.

The R package should be handled similarly to xgboost, since catboost isn't in CRAN and we'd like the CUDA support. The R support can be handled in a separate commit, though, so feel free to merge when you're ready.

@nviets
Copy link
Contributor

nviets commented Jun 16, 2023

Result of nixpkgs-review pr 235226 run on x86_64-linux 1

2 packages marked as broken and skipped:
  • python310Packages.shap
  • python310Packages.shap.dist
2 packages failed to build:
  • python311Packages.catboost
  • python311Packages.catboost.dist
4 packages built:
  • catboost
  • catboost.dev
  • python310Packages.catboost
  • python310Packages.catboost.dist

@nviets
Copy link
Contributor

nviets commented Jun 16, 2023

Failed derivations

@natsukium
Copy link
Member Author

Thanks for trying.
Looking at the logs, it looks like we need to match the python version when compiling libcatboost.
I will work on it this weekend.

@nviets
Copy link
Contributor

nviets commented Jun 19, 2023

Result of nixpkgs-review pr 235226 --extra-nixpkgs-config '{ cudaCapabilities = [ "8.6" ]; }' run on x86_64-linux 1

2 packages marked as broken and skipped:
  • python310Packages.shap
  • python310Packages.shap.dist
6 packages built:
  • catboost
  • catboost.dev
  • python310Packages.catboost
  • python310Packages.catboost.dist
  • python311Packages.catboost
  • python311Packages.catboost.dist

@nviets
Copy link
Contributor

nviets commented Jun 20, 2023

@Zhuk66 - FYI

@nviets
Copy link
Contributor

nviets commented Jun 22, 2023

I think it's enough for the first PR to release the CLI library and Python package. The R library and other features can be handled in separate PRs. There's plenty of value in getting the initial update to 1.2 released. Thanks again for your work on this one!

@natsukium natsukium force-pushed the catboost/update branch 2 times, most recently from 48c6009 to 8e6db03 Compare June 24, 2023 02:42
Comment on lines 64 to 69
] ++ lib.optionals cudaSupport [
(cudatoolkit.override { backendStdenv = stdenv; })
];

env = {
CUDAHOSTCXX = "${stdenv.cc}/bin/clang";
Copy link
Member Author

Choose a reason for hiding this comment

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

@NixOS/cuda-maintainers
In case backendStdenv = clangStdenv, it needs to set CUDAHOSTCXX = ${stdenv.cc}/bin/clang explicitly.
This is because -DCUDA_HOST_COMPILER=${backendStdenv.cc}/bin is set here.

cmakeFlags+=' -DCUDA_HOST_COMPILER=${backendStdenv.cc}/bin'
cmakeFlags+=' -DCMAKE_CUDA_HOST_COMPILER=${backendStdenv.cc}/bin'
if [ -z "\''${CUDAHOSTCXX-}" ]; then
export CUDAHOSTCXX=${backendStdenv.cc}/bin;
fi

Since nvcc looks for gcc by default, it tries to find clang/bin/gcc without the CUDAHOSTCXX.
Is it worth setting -DCUDA_HOST_COMPILER=${lib.getExe backendStdenv.cc.cc} in cudatoolkit?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi! Could you give some more intro on why this needs to be built with clang?

The reason the backendStdenv attribute is there is because different cudatoolkit versions support different backend compiler releases (e.g. nvcc from cudatoolkit 11.7 will simply error out when used with gcc12). At the moment we only maintain a table with nvcc-gcc versions compatibility, but in principle one could add llvm as well. The (cudatoolkit.override { backendStdenv = clangStdenv; }) might work but only accidentally and until nixpkgs bumps the default llvm major version.

-DCUDA_HOST_COMPILER=${lib.getExe backendStdenv.cc.cc}

There are two issues with this line:

  1. Both the legacy CUDA_HOST_COMPILER and the CMAKE_CUDA_HOST_COMPILER map into nvcc's -ccbin, which expects a directory. Iirc it'd use ${ccbin}/cc or ${ccbin}/cxx as the actual compiler. I don't remember exactly, but I think CUDAHOSTCXX works the same
  2. The un-wrapped backendStdenv.cc.cc (opposed to backendStdenv.cc) is something you probably don't want to use. E.g. cudaPackages_11.backendStdenv.cc.cc is gcc11 and it links its artifacts to gcc11's libstdc++, which won't work with the rest of nixpkgs. Similarly, if you were using clang, you'd need to wrap it so that it would link gcc's libstdc++ instead of llvm's libcxx. I'm sorry about this mess, there's an open issue tracking this here: cudaPackages: extend cc-wrapper and rewrite cudaStdenv to properly solve libstdc++ issues #226165

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, you probably want to use cudaPackages.cuda_{nvcc,cudart,..} instead of cudatoolkit

Copy link
Member Author

Choose a reason for hiding this comment

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

@SomeoneSerge san, Thanks for the clarification.
I need clang12+ to build this catboost and can't build it with GCC.
ref: https://catboost.ai/en/docs/installation/build-environment-setup-for-cmake#compilers,-linkers-and-related-tools
I understand the role of backendStdenv, and even if accidentally, I should fix the clang version here.

Both the legacy CUDA_HOST_COMPILER and the CMAKE_CUDA_HOST_COMPILER map into nvcc's -ccbin, which expects a directory. Iirc it'd use ${ccbin}/cc or ${ccbin}/cxx as the actual compiler. I don't remember exactly, but I think CUDAHOSTCXX works the same

Yes, -ccbin expects a directory, but the documentation says it is possible to specify an executable.
The backendStdenv=clangStdenv case is a problem because it tries to run ${ccbin}/gcc instead of ${ccbin}/cc as mentioned above.
In fact, building this PR change without CUDAHOSTCXX fails.

The un-wrapped backendStdenv.cc.cc (opposed to backendStdenv.cc) is something you probably don't want to use. E.g. cudaPackages_11.backendStdenv.cc.cc is gcc11 and it links its artifacts to gcc11's libstdc++, which won't work with the rest of nixpkgs. Similarly, if you were using clang, you'd need to wrap it so that it would link gcc's libstdc++ instead of llvm's libcxx. I'm sorry about this mess, there's an open issue tracking this here: cudaPackages: extend cc-wrapper and rewrite cudaStdenv to properly solve libstdc++ issues

Thanks for letting me know about the cc-wrapper issue.
I will change it to link libstdc++.

Copy link
Contributor

Choose a reason for hiding this comment

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

Btw, if you'd like to prevent cudatoolkit/cuda_nvcc from exporting CUDAHOSTCXX&c, you should be able to use dontSetupCUDAToolkitCompilers = true once https://github.com/NixOS/nixpkgs/pull/233581/files#diff-412326f1eb2e17a306d14d70da7fe2423b4f244aa3c9c81d13f1bb1cf3be084bR20 is merged

Copy link
Contributor

Choose a reason for hiding this comment

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

Hello again! You mentioned earlier that

[Since] nvcc looks for gcc by default [...].

I may be now facing a similar issue with cross-compilation, so I was wondering if you really meant nvcc (rather than cmake, or even findcuda/findcudatoolkit specifically), and if you've got a reference documenting this behaviour?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry, I can't answer this question for sure, but I guess it is the behavior of nvcc.
Looking at CMake's FindCUDAToolkit and other CUDA-related sources, it does not seem to refer specifically to gcc.
I can't find any documentation on this behavior, but JetBrain's CLion page says the following for reference.
https://www.jetbrains.com/help/clion/cuda-projects.html#custom-host-compiler

For compiling host code, NVCC calls the system's default C++ compiler (gcc/g++ on Linux and cl.exe on Windows).

@natsukium natsukium force-pushed the catboost/update branch 2 times, most recently from 72a67a9 to e2a1eb9 Compare June 24, 2023 05:01
@natsukium natsukium marked this pull request as ready for review June 24, 2023 05:02
@natsukium
Copy link
Member Author

Now that the cleanup is complete, it is ready for review.

@natsukium
Copy link
Member Author

natsukium commented Jul 11, 2023

I've changed cudatoolkit to cuda_{nvcc,cudart,cccl}.
I've also fixed clangStdenv to clang12Stdenv to prevent accidental breakage with the clang update.
It looks like I didn't need to change backendStdenv.

pkgs/development/libraries/catboost/default.nix Outdated Show resolved Hide resolved
pkgs/development/libraries/catboost/default.nix Outdated Show resolved Hide resolved
pkgs/development/python-modules/catboost/default.nix Outdated Show resolved Hide resolved
pkgs/development/python-modules/catboost/default.nix Outdated Show resolved Hide resolved
pkgs/top-level/python-packages.nix Outdated Show resolved Hide resolved
@nviets
Copy link
Contributor

nviets commented Sep 6, 2023

1.2.1 was released. Maybe bump the version before merging?

@natsukium natsukium force-pushed the catboost/update branch 2 times, most recently from f49bf8e to 4792a30 Compare September 9, 2023 00:43
@natsukium natsukium force-pushed the catboost/update branch 2 times, most recently from bc699ed to 97135d6 Compare September 9, 2023 03:25
@natsukium
Copy link
Member Author

It's ready for review again.

The main changes are.

  • Update from 1.2 to 1.2.1
  • Adopt Sandro's comments (with some exceptions)
  • Use cuda-redist

@SuperSandro2000, @NixOS/cuda-maintainers Could you review this PR again?

Comment on lines +65 to +44
sed -i 's/-gencode=arch=compute_89,code=sm_89//g' $cmakelists
sed -i 's/-gencode=arch=compute_90,code=sm_90//g' $cmakelists
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this dropping the support for 89, 90? Is there a way to specify the list of gencodes for which to build the kernels?

Copy link
Member Author

Choose a reason for hiding this comment

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

Unfortunately, probably not.
I could not find any flags that would set compute capability.

Copy link
Contributor

Choose a reason for hiding this comment

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

Seeing how moving cuda_* to *[bB]uildInputs has worked, and how the build logs seem to mention FindCUDAToolkit.cmake: https://gist.github.com/SomeoneSerge/395dfc734cc220aae928cd9d501acabd#file-gistfile0-txt-L52

...I think it should be possible to control the target cuda architectures by setting https://cmake.org/cmake/help/latest/variable/CMAKE_CUDA_ARCHITECTURES.html, unless upstream overrides the variable. The configurable way to choose capabilities at the moment would be to use cudaPackages.cudaFlags, which respect config.cudaCapabilities (and whatever they might get replaced with later). Examples:

"-DCMAKE_CUDA_ARCHITECTURES=${builtins.concatStringsSep ";" (map dropDot cudaCapabilities)}"

# E.g. [ "80" "86" "90" ]
cudaArchitectures = (builtins.map cudaFlags.dropDot cudaCapabilities);
cudaArchitecturesString = strings.concatStringsSep ";" cudaArchitectures;

Copy link
Contributor

Choose a reason for hiding this comment

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

I just tested this in SomeoneSerge@9780f86 and it doesn't work because upstream passes some ad hoc flags instead of using CMAKE_CUDA_ARCHITECTURES:

https://github.com/catboost/catboost/blob/ad3934317eb5357781e2389f1c24582c01f738ca/catboost/cuda/ctrs/CMakeLists.linux-x86_64-cuda.txt#L34-L56 https://github.com/catboost/catboost/blob/ad3934317eb5357781e2389f1c24582c01f738ca/cmake/cuda.cmake#L163-L165

I think we should contact upstream about changing that

@natsukium
Copy link
Member Author

things done

https://github.com/NixOS/nixpkgs/compare/97135d6fa36159f04c37529ab90820a4462d37f1..7425a11b43a3f3ce8e7880f61d1ec42063230267

  • use config.cudaSupport without or false
  • remove symlinkJoin
  • add a comment for using llvmPackages_12
  • drop autoPatchelfHook

https://github.com/NixOS/nixpkgs/compare/7425a11b43a3f3ce8e7880f61d1ec42063230267..0b93e56237687efe526ab2a595356debee9da50c

  • update 1.2.1 -> 1.2.2

@SomeoneSerge
Copy link
Contributor

Result of nixpkgs-review pr 235226 --extra-nixpkgs-config '{ cudaCapabilities = [ "8.6" ]; cudaSupport = true; cudaForwardCompat = false; }' run on x86_64-linux 1

2 packages failed to build:
  • python310Packages.shap
  • python310Packages.shap.dist
6 packages built:
  • catboost
  • catboost.dev
  • python310Packages.catboost
  • python310Packages.catboost.dist
  • python311Packages.catboost
  • python311Packages.catboost.dist

]);

env = {
CUDAHOSTCXX = lib.optionalString cudaSupport "${stdenv.cc}/bin/cc";
Copy link
Contributor

Choose a reason for hiding this comment

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

We set this variable in setup-cuda-hook.sh (propagated by cuda_nvcc in natvieBuildInputs), so this should be a no-op. Also, if upstream uses FindCUDAToolkit and enable_language(CUDA), then CUDAHOSTCXX is superseded by CMAKE_CUDA_HOST_COMPILER (which the hook should set as well)

Copy link
Member Author

@natsukium natsukium Oct 9, 2023

Choose a reason for hiding this comment

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

As reported in the first comment, nvcc uses gcc instead of clang as the backendStdenv without it.
We will get the following error message if we do not set this environment variable.

Error log
catboost> [2892/4763] Building CUDA object library/cpp/cuda/wrappers/CMakeFiles/cpp-cuda-wrappers.dir/kernel.cu.o
catboost> FAILED: library/cpp/cuda/wrappers/CMakeFiles/cpp-cuda-wrappers.dir/kernel.cu.o 
catboost> /nix/store/mr3zd7dhyxdwr9y9gf6l74fah1rkjkji-cuda_nvcc-11.8.89/bin/nvcc -forward-unknown-to-host-compiler -ccbin=/nix/store/848ld56pzbywlxy1pv9xi5d8gsvzb6hl-gcc-wrapper-11.4.0/bin/c++ -DCATBOOST_OPENSOURCE=yes -I/build/source -I/build/source/build -I/build/source/contrib/libs/nvidia/thrust -I/build/source/contrib/libs/nvidia/cub -I/build/source/contrib/libs/linux-headers -I/build/source/contrib/libs/linux-headers/_nf -I/build/source/contrib/libs/cxxsupp/libcxx/include -I/build/source/contrib/libs/cxxsupp/libcxxrt/include -I/build/source/contrib/libs/clang14-rt/include -I/build/source/contrib/libs/zlib/include -I/build/source/contrib/libs/double-conversion -I/build/source/contrib/libs/libc_compat/include/readpassphrase -I/build/source/contrib/libs/libc_compat/reallocarray -I/build/source/contrib/libs/libc_compat/random -isystem /nix/store/mr3zd7dhyxdwr9y9gf6l74fah1rkjkji-cuda_nvcc-11.8.89/include -D_THREAD_SAFE -D_PTHREADS -D_REENTRANT -D_LARGEFILE_SOURCE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__LONG_LONG_SUPPORTED -D_GNU_SOURCE -DLIBCXX_BUILDING_LIBCXXRT -D_FILE_OFFSET_BITS=64 -D_YNDX_LIBUNWIND_ENABLE_EXCEPTION_BACKTRACE -DSSE_ENABLED=1 -DSSE3_ENABLED=1 -DSSSE3_ENABLED=1 -DSSE41_ENABLED=1 -DSSE42_ENABLED=1 -DPOPCNT_ENABLED=1 -DCX16_ENABLED=1 --compiler-options -fexceptions,-fno-common,-fcolor-diagnostics,-fdebug-default-version=4,-ffunction-sections,-fdata-sections,-Wall,-Wextra,-Wno-parentheses,-Wno-implicit-const-int-float-conversion,-Wno-unknown-warning-option,-pipe,-fuse-init-array,-m64,-mpopcnt,-mcx16,-Woverloaded-virtual,-Wimport-preprocessor-directive-pedantic,-Wno-undefined-var-template,-Wno-return-std-move,-Wno-defaulted-function-deleted,-Wno-pessimizing-move,-Wno-deprecated-anon-enum-enum-conversion,-Wno-deprecated-enum-enum-conversion,-Wno-deprecated-enum-float-conversion,-Wno-ambiguous-reversed-operator,-Wno-deprecated-volatile --expt-extended-lambda --expt-relaxed-constexpr --compiler-options -std=c++14 -DTHRUST_IGNORE_CUB_VERSION_CHECK --threads 0 -O3 -DNDEBUG -std=c++14 -Xcompiler=-fPIC -nostdinc++ -DLIBCXX_BUILDING_LIBCXXRT -D_libunwind_ -gencode arch=compute_35,code=sm_35 -gencode arch=compute_50,code=compute_50 -gencode arch=compute_52,code=sm_52 -gencode arch=compute_60,code=compute_60 -gencode arch=compute_61,code=compute_61 -gencode arch=compute_61,code=sm_61 -gencode arch=compute_70,code=sm_70 -gencode arch=compute_70,code=compute_70 --ptxas-options=-v -lineinfo --use_fast_math -gencode=arch=compute_80,code=sm_80 -gencode=arch=compute_86,code=sm_86 -gencode=arch=compute_89,code=sm_89 -gencode=arch=compute_90,code=sm_90 -MD -MT library/cpp/cuda/wrappers/CMakeFiles/cpp-cuda-wrappers.dir/kernel.cu.o -MF library/cpp/cuda/wrappers/CMakeFiles/cpp-cuda-wrappers.dir/kernel.cu.o.d -x cu -c /build/source/library/cpp/cuda/wrappers/kernel.cu -o library/cpp/cuda/wrappers/CMakeFiles/cpp-cuda-wrappers.dir/kernel.cu.o
catboost> nvcc warning : incompatible redefinition for option 'compiler-bindir', the last value of this option was used
catboost> nvcc warning : The 'compute_35', 'compute_37', 'sm_35', and 'sm_37' architectures are deprecated, and may be removed in a future release (Use -Wno-deprecated-gpu-targets to suppress warning).
catboost> g++: error: unrecognized command-line option '-fcolor-diagnostics'
catboost> g++: error: unrecognized command-line option '-fdebug-default-version=4'
catboost> g++: error: unrecognized command-line option '-fuse-init-array'
catboost> g++: error: unrecognized command-line option '-Wimport-preprocessor-directive-pedantic'
catboost> g++: error: unrecognized command-line option '-fcolor-diagnostics'
catboost> g++: error: unrecognized command-line option '-fdebug-default-version=4'
catboost> g++: error: unrecognized command-line option '-fuse-init-array'
catboost> g++: error: unrecognized command-line option '-Wimport-preprocessor-directive-pedantic'
catboost> g++: error: unrecognized command-line option '-fcolor-diagnostics'
catboost> g++: error: unrecognized command-line option '-fdebug-default-version=4'
catboost> g++: error: unrecognized command-line option '-fuse-init-array'
catboost> g++: error: unrecognized command-line option '-Wimport-preprocessor-directive-pedantic'
catboost> g++: error: unrecognized command-line option '-fcolor-diagnostics'
catboost> g++: error: unrecognized command-line option '-fdebug-default-version=4'
catboost> g++: error: unrecognized command-line option '-fuse-init-array'
catboost> g++: error: unrecognized command-line option '-Wimport-preprocessor-directive-pedantic'
catboost> g++: error: unrecognized command-line option '-fcolor-diagnostics'
catboost> g++: error: unrecognized command-line option '-fdebug-default-version=4'
catboost> g++: error: unrecognized command-line option '-fuse-init-array'
catboost> g++: error: unrecognized command-line option '-Wimport-preprocessor-directive-pedantic'
catboost> g++: error: unrecognized command-line option '-fcolor-diagnostics'
catboost> g++: error: unrecognized command-line option '-fdebug-default-version=4'
catboost> g++: error: unrecognized command-line option '-fuse-init-array'
catboost> g++: error: unrecognized command-line option '-Wimport-preprocessor-directive-pedantic'
catboost> g++: error: unrecognized command-line option '-fcolor-diagnostics'
catboost> g++: error: unrecognized command-line option '-fdebug-default-version=4'
catboost> g++: error: unrecognized command-line option '-fuse-init-array'
catboost> g++: error: unrecognized command-line option '-Wimport-preprocessor-directive-pedantic'
catboost> g++: error: unrecognized command-line option '-fcolor-diagnostics'
catboost> g++: error: unrecognized command-line option '-fdebug-default-version=4'
catboost> g++: error: unrecognized command-line option '-fuse-init-array'
catboost> g++: error: unrecognized command-line option '-Wimport-preprocessor-directive-pedantic'
catboost> g++: error: unrecognized command-line option '-fcolor-diagnostics'
catboost> g++: error: unrecognized command-line option '-fdebug-default-version=4'
catboost> g++: error: unrecognized command-line option '-fuse-init-array'
catboost> g++: error: unrecognized command-line option '-Wimport-preprocessor-directive-pedantic'
catboost> g++: error: unrecognized command-line option '-fcolor-diagnostics'
catboost> g++: error: unrecognized command-line option '-fdebug-default-version=4'
catboost> g++: error: unrecognized command-line option '-fuse-init-array'
catboost> g++: error: unrecognized command-line option '-Wimport-preprocessor-directive-pedantic'
catboost> g++: error: unrecognized command-line option '-fcolor-diagnostics'
catboost> g++: error: unrecognized command-line option '-fdebug-default-version=4'
catboost> g++: error: unrecognized command-line option '-fuse-init-array'
catboost> g++: error: unrecognized command-line option '-Wimport-preprocessor-directive-pedantic'
catboost> fatal   : Could not open input file /build/tmpxft_0000501d_00000000-16_kernel.compute_35.cpp1.ii

Comment on lines +36 to +37
--replace "\''${RAGEL_BIN}" "${ragel}/bin/ragel" \
--replace "\''${YASM_BIN}" "${yasm}/bin/yasm"
Copy link
Contributor

Choose a reason for hiding this comment

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

Cosmetic: lib.getExe ragel would probably work too

Copy link
Member Author

Choose a reason for hiding this comment

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

Neither package has mainProgram set yet, and using getExe will cause a warning.😞

Copy link
Contributor

@SomeoneSerge SomeoneSerge left a comment

Choose a reason for hiding this comment

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

I've only skimmed the changes this time, but I think this is ready. The failing packages in nixpkgs-review are broken on master as well (cf hydra). Thank you a lot @natsukium!

@nviets
Copy link
Contributor

nviets commented Oct 2, 2023

@natsukium, @SomeoneSerge - are you able to run the GPU version successfully with CLI and/or Python? I started a branch on your work to add the R package, and everything is working except for the GPU. My branch is here, and I am waiting for this one to merge before opening the draft PR. Mine fails on:

➜ ./result/bin/Rscript test.R
Loading required package: ggplot2
Loading required package: lattice
Error in catboost.train(pool_train, pool_test, params) :
  /build/source/catboost/cuda/cuda_lib/cuda_base.h:281: CUDA error 35: CUDA driver version is insufficient for CUDA runtime version
Execution halted

I guess my two questions are 1) am I handling the R library build correctly because I'm not sure if its supposed to reference the /build/* directory like that and 2) why is it failing on this CUDA message? I tried several versions of CUDA, which makes me think I'm missing a header file or something.

Your work looks good to go, so I don't want this question to hold up your merge. We can check it out on my follow up PR later. Thanks!

@SomeoneSerge
Copy link
Contributor

@nviets thanks, good catch, I tested with SomeoneSerge@56e2da2 and got the same error: it's just _catboost.so trying to dlopen libcuda.so. We need to add the respective runpath: SomeoneSerge@a290d7c

@nviets
Copy link
Contributor

nviets commented Oct 4, 2023

Thanks @SomeoneSerge - that did the trick for the R package too.

@veprbl
Copy link
Member

veprbl commented Oct 8, 2023

Result of nixpkgs-review pr 235226 run on x86_64-darwin 1

2 packages marked as broken and skipped:
  • python310Packages.shap
  • python310Packages.shap.dist
6 packages built:
  • catboost
  • catboost.dev
  • python310Packages.catboost
  • python310Packages.catboost.dist
  • python311Packages.catboost
  • python311Packages.catboost.dist

Copy link
Member

@veprbl veprbl left a comment

Choose a reason for hiding this comment

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

LGTM

@natsukium
Copy link
Member Author

@nviets Thanks for the report. I didn't notice that since I was working with a WSL that handled LD_LIBRARY_PATH differently.

am I handling the R library build correctly because I'm not sure if its supposed to reference the /build/* directory like that

All the built libraries and binaries are contained under $dev, and the directory tree follows the output location in the official documentation.

@SomeoneSerge Thanks for investigating the runpath issue.
Is this an issue that should be fixed in magma rather than catboost?

@natsukium natsukium changed the title python3Packages.catboost: 1.0.5 -> 1.2; build with cmake python3Packages.catboost: 1.0.5 -> 1.2.2; build with cmake Oct 16, 2023
@natsukium
Copy link
Member Author

I'll merge this to move forward.

@natsukium natsukium merged commit 5b87022 into NixOS:master Oct 16, 2023
24 checks passed
@natsukium natsukium deleted the catboost/update branch October 16, 2023 05:09
@natsukium natsukium mentioned this pull request Oct 30, 2023
12 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants