Unify template parameter dispatch for FIL inference and shared memory footprint estimation #4013

levsnv · 2021-06-24T08:32:21Z

No description provided.

…d-memory

…ery inference

…e_type>(predict_params, ...)

…ared-memory

levsnv · 2021-06-25T23:49:32Z

build fails with

ccache /usr/local/cuda/bin/nvcc -forward-unknown-to-host-compiler -ccbin=/usr/local/gcc9/bin/g++ -DCUML_CPP_API -DDISABLE_CUSPARSE_DEPRECATED -DDMLC_CORE_USE_CMAKE -DDMLC_USE_CXX11=1 -DTHRUST_DEVICE_SYSTEM=THRUST_DEVICE_SYSTEM_CUDA -DTHRUST_HOST_SYSTEM=THRUST_HOST_SYSTEM_CPP -D__USE_XOPEN2K8 -Dcuml___EXPORTS -I../include -I/include -I../src -I../src_prims -I_deps/thrust-src -I_deps/thrust-src/dependencies/cub -I_deps/raft-src/cpp/include -I_deps/cuco-src/include -I_deps/libcudacxx-src/include -isystem=$PREFIX/include -isystem=/usr/local/cuda/include -isystem=$PREFIX/include/cumlprims -O3 -DNDEBUG --generate-code=arch=compute_60,code=[sm_60] --generate-code=arch=compute_70,code=[sm_70] --generate-code=arch=compute_75,code=[sm_75] --generate-code=arch=compute_80,code=[compute_80,sm_80] -Xcompiler=-fPIC --expt-extended-lambda --expt-relaxed-constexpr -Xcompiler=-Wno-deprecated-declarations -Xcompiler=-fopenmp -std=c++17 -MD -MT CMakeFiles/cuml++.dir/src/knn/knn_sparse.cu.o -MF CMakeFiles/cuml++.dir/src/knn/knn_sparse.cu.o.d -x cu -c ../src/knn/knn_sparse.cu -o CMakeFiles/cuml++.dir/src/knn/knn_sparse.cu.o
../src/knn/knn_sparse.cu(41): error: no instance of function template "raft::sparse::selection::brute_force_knn" matches the argument list
            argument types are: (const int *, const int *, const float *, size_t, int, int, const int *, const int *, const float *, size_t, int, int, int *, float *, int, cusparseHandle_t, std::shared_ptr<raft::mr::device::allocator>, cudaStream_t, size_t, size_t, raft::distance::DistanceType, float)

1 error detected in the compilation of "../src/knn/knn_sparse.cu".

cpp/src/fil/common.cuh

cpp/src/fil/fil.cu

cpp/src/fil/infer.cu

levsnv

waiting for #4012 to rerun the CI

cpp/src/fil/fil.cu

cpp/src/fil/common.cuh

cpp/src/fil/infer.cu

cpp/src/fil/common.cuh

cpp/src/fil/infer.cu

canonizer

Overall, this pull request is in a good shape, so nice work! The comments are mostly technical.

Specific points I'd like to highlight:

What happens if n_items or leaf_algo are negative?
Would it be possible to pass predict_params by value?

cpp/src/fil/common.cuh

cpp/test/sg/fil_test.cu

cpp/src/fil/common.cuh

canonizer · 2021-10-08T00:41:39Z

cpp/src/fil/common.cuh

+template <class KernelParams, class Func>
+void dispatch_on_n_items(Func func, predict_params& params)
+{
+  if (params.n_items == KernelParams::n_items) {


What happens if params.n_items == -1?

the assert gets triggered

This is fine then.

canonizer · 2021-10-08T00:42:37Z

cpp/src/fil/common.cuh

+template <class KernelParams, class Func>
+void dispatch_on_leaf_algo(Func func, predict_params& params)
+{
+  if (params.leaf_algo == KernelParams::leaf_algo) {


What happens if params.leaf_algo == -1?

the assert gets triggered

This is fine then.

canonizer

Approved, provided that the comments are addressed.

cpp/src/fil/common.cuh

canonizer · 2021-10-09T01:19:09Z

cpp/src/fil/common.cuh

  }
+  return func.run(params);  // appeasing the compiler


Is there a better way of appeasing the compiler?

I could remove elses, since the branches return, and flip the if constexpr condition. But I thought you prefer else after return?
Alternatively, return dispatch_on_n_items<KernelParams>(func, params) might work.
Last, perhaps template <class KernelParams, class Func, class T = decltype(declval<Func>().run(params))> might resolve both this and the signature decltype question?

Do we need T to be in the template parameters? I guess yes if we want to use it in the return type, and no if we only need it in the function body.

In any case, even with T in the template parameters, it looks better if return T() or return T{} works. Perhaps you could add a documentation comment as to why you're doing this.

cpp/src/fil/fil.cu

cpp/test/sg/fil_test.cu

codecov-commenter · 2021-10-09T03:43:10Z

Codecov Report

❗ No coverage uploaded for pull request base (branch-21.12@51c41c4). Click here to learn what that means.
The diff coverage is n/a.

@@               Coverage Diff               @@
##             branch-21.12    #4013   +/-   ##
===============================================
  Coverage                ?   86.06%           
===============================================
  Files                   ?      231           
  Lines                   ?    18691           
  Branches                ?        0           
===============================================
  Hits                    ?    16087           
  Misses                  ?     2604           
  Partials                ?        0

Flag	Coverage Δ
dask	`47.01% <0.00%> (?)`
non-dask	`78.75% <0.00%> (?)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 51c41c4...9528148. Read the comment docs.

dantegd · 2021-10-27T13:33:40Z

@gpucibot merge

… footprint estimation (rapidsai#4013) Authors: - Levs Dolgovs (https://github.com/levsnv) Approvers: - Andy Adinets (https://github.com/canonizer) - Dante Gama Dessavre (https://github.com/dantegd) URL: rapidsai#4013

levsnv added 14 commits April 16, 2021 23:29

try 1

53e0683

draft of set-and-launch

31b885f

Merge remote-tracking branch 'rapidsai/branch-21.06' into extra-share…

589f9ad

…d-memory

set carveout and occupancy-affecting preferred cache config before ev…

26480b0

…ery inference

other review comments

a037f16

DRY: rewrote in terms of dispatch_on_FIL_template_params<func, storag…

2a1d622

…e_type>(predict_params, ...)

Merge branch 'branch-21.08' of github.com:rapidsai/cuml into extra-sh…

bd3c505

…ared-memory

style, clean up diff

5cf38d3

Merge branch 'branch-21.08' of github.com:rapidsai/cuml into extra-sh…

7959f4d

…ared-memory

fixed bugs and linker issues

258e674

removed unnecessary specialization in dispatch

e0f53ea

simplified code to template-based dispatch

ace36c0

Merge branch 'branch-21.08' of github.com:rapidsai/cuml into dispatch

98509b0

reverted max_shm changes

bcd05bc

github-actions bot added the CUDA/C++ label Jun 24, 2021

levsnv added improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Jun 24, 2021

simplified templates, fixed bug

36b27f1

levsnv requested a review from canonizer June 24, 2021 08:54

levsnv marked this pull request as ready for review June 24, 2021 08:54

levsnv requested a review from a team as a code owner June 24, 2021 08:54

remove unnecessary forward declarations

06650f8

levsnv added the 0 - Blocked Cannot progress due to external reasons label Jun 25, 2021

levsnv added 3 - Ready for Review Ready for review by team and removed 0 - Blocked Cannot progress due to external reasons labels Jun 25, 2021

canonizer suggested changes Jun 28, 2021

View reviewed changes

levsnv commented Jun 29, 2021

View reviewed changes

cpp/src/fil/fil.cu Outdated Show resolved Hide resolved

cpp/src/fil/common.cuh Outdated Show resolved Hide resolved

cpp/src/fil/infer.cu Outdated Show resolved Hide resolved

levsnv mentioned this pull request Jun 29, 2021

Use opt-in shared memory carveout for FIL #3759

Merged

halfway change

4dd4f8a

levsnv commented Oct 8, 2021

View reviewed changes

cpp/src/fil/common.cuh Outdated Show resolved Hide resolved

levsnv commented Oct 8, 2021

View reviewed changes

cpp/src/fil/infer.cu Outdated Show resolved Hide resolved

canonizer suggested changes Oct 8, 2021

View reviewed changes

levsnv added 6 commits October 8, 2021 17:04

change-by-reference into accept-and-return-by-value

798171f

variable renames

7e78203

unnecessary changes

6ae0914

finish case adjustments

0978fc0

NextLeafAlgo

46eb819

MAX_N_ITEMS

4614ca2

levsnv requested a review from canonizer October 9, 2021 00:44

stray changes

9528148

canonizer approved these changes Oct 9, 2021

View reviewed changes

levsnv added 4 - Waiting on Author Waiting for author to respond to review and removed 3 - Ready for Review Ready for review by team 4 - Waiting on Reviewer Waiting for reviewer to review or respond labels Oct 19, 2021

levsnv requested a review from dantegd October 19, 2021 21:26

levsnv added 3 commits October 19, 2021 22:28

removed decltype

0b6cea8

next_leaf_algo, LEAF_ALGO_INVALID->MAX_LEAF_ALGO, ...

a5a6ca6

misc

2b4555f

levsnv added 4 - Waiting on Reviewer Waiting for reviewer to review or respond and removed 4 - Waiting on Author Waiting for author to respond to review labels Oct 20, 2021

levsnv removed the request for review from a team October 20, 2021 06:32

dantegd approved these changes Oct 27, 2021

View reviewed changes

rapids-bot bot merged commit 92a40e0 into rapidsai:branch-21.12 Oct 27, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unify template parameter dispatch for FIL inference and shared memory footprint estimation #4013

Unify template parameter dispatch for FIL inference and shared memory footprint estimation #4013

levsnv commented Jun 24, 2021

levsnv commented Jun 25, 2021

levsnv left a comment

canonizer left a comment

canonizer Oct 8, 2021

levsnv Oct 9, 2021

canonizer Oct 19, 2021

canonizer Oct 8, 2021

levsnv Oct 9, 2021

canonizer Oct 19, 2021

canonizer left a comment

canonizer Oct 9, 2021

levsnv Oct 15, 2021 •

edited

Loading

canonizer Oct 19, 2021 •

edited

Loading

codecov-commenter commented Oct 9, 2021

dantegd commented Oct 27, 2021

Unify template parameter dispatch for FIL inference and shared memory footprint estimation #4013

Unify template parameter dispatch for FIL inference and shared memory footprint estimation #4013

Conversation

levsnv commented Jun 24, 2021

levsnv commented Jun 25, 2021

levsnv left a comment

Choose a reason for hiding this comment

canonizer left a comment

Choose a reason for hiding this comment

canonizer Oct 8, 2021

Choose a reason for hiding this comment

levsnv Oct 9, 2021

Choose a reason for hiding this comment

canonizer Oct 19, 2021

Choose a reason for hiding this comment

canonizer Oct 8, 2021

Choose a reason for hiding this comment

levsnv Oct 9, 2021

Choose a reason for hiding this comment

canonizer Oct 19, 2021

Choose a reason for hiding this comment

canonizer left a comment

Choose a reason for hiding this comment

canonizer Oct 9, 2021

Choose a reason for hiding this comment

levsnv Oct 15, 2021 • edited Loading

Choose a reason for hiding this comment

canonizer Oct 19, 2021 • edited Loading

Choose a reason for hiding this comment

codecov-commenter commented Oct 9, 2021

Codecov Report

dantegd commented Oct 27, 2021

levsnv Oct 15, 2021 •

edited

Loading

canonizer Oct 19, 2021 •

edited

Loading