Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
python3Packages.catboost: 1.0.5 -> 1.2.2; build with cmake #235226
Changes from all commits
08b7a46
fc84d02
0b93e56
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
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 tooThere was a problem hiding this comment.
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.😞
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 respectconfig.cudaCapabilities
(and whatever they might get replaced with later). Examples:nixpkgs/pkgs/development/libraries/science/math/faiss/default.nix
Line 103 in b5213d5
nixpkgs/pkgs/development/libraries/science/math/magma/generic.nix
Lines 78 to 81 in b5213d5
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 usesFindCUDAToolkit
andenable_language(CUDA)
, thenCUDAHOSTCXX
is superseded byCMAKE_CUDA_HOST_COMPILER
(which the hook should set as well)There was a problem hiding this comment.
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