[REVIEW] migrating cumlHandle_impl (and related dependencies) into raft #3

teju85 · 2020-04-28T06:40:43Z

@dantegd @JohnZed @cjnolet @afender for review.

Notable changes from cuML:

a base class called raft::Allocator to reduce code duplication between deviceAllocator and hostAllocator
handle is called raft::handle_t, but I'm open to suggestions for a better name
disabling of logging features, as we need to see how to expose the current spdlog wrapper in cuml, without actually needing users of raft (cugraph and cuml currently) worry about underlying spdlog library.
raft::handle_t doesn't contain the comms interface. It is better tackled in a separate PR.
Enable clang-tidy checks to enforce consistent naming convention.

This PR is still a WIP, but filing it early to solicit feedback about the interface.

seunghwak · 2020-04-28T14:25:12Z

I see multiple styles in naming classes,

e.g.
class Allocator (started with upppercase)
class deviceAllocator (start with_lowercase)
class buffer_base (use _ in the middle)
class handle_t (use _t at the end)

And when do we use which style?

cjnolet · 2020-04-28T14:44:18Z

raft::handle_t doesn't contain the comms interface. It is better tackled in a separate PR.

This is perfect. I've been planning to move the comms over right after the handle is moved over.

cpp/include/raft/allocator.hpp

cpp/include/raft/buffer_base.hpp

cpp/include/raft/cudart_utils.h

cpp/include/raft/handle.hpp

cpp/include/raft/host_buffer.hpp

afender

Excellent! That looks in pretty good shape to me. I added a few notes.
I'd agree that a consistent naming convention from the beginning will help in the long run.

cpp/include/raft/cudart_utils.h

cpp/include/raft/allocator.hpp

cpp/include/raft/cudart_utils.h

cpp/include/raft/buffer_base.hpp

… run successfully

dillon-cullinan · 2020-05-01T17:10:55Z

rerun tests

GPUtester · 2020-05-01T17:11:15Z

Please update the changelog in order to start CI tests.

View the gpuCI docs here.

…ea-ext-migrate-cumlHandle_impl-into-raft

…mespaces for clarity

…meter

teju85 · 2020-05-11T10:30:34Z

Hey @dantegd @cjnolet @JohnZed @afender @seunghwak ,
I think the PR is now ready to be looked at again with all the comments from above addressed. Can you folks check if this now looks good to be merged or not?

seunghwak

I have a concern about using preceding _ (over trailing _) in class private member variable names (see my last comment), but besides this, this PR looks great to me :-)

cpp/include/raft/buffer_base.hpp

dillon-cullinan · 2020-05-11T19:40:24Z

rerun tests

dantegd

@teju85 the one thing missing now that CI is totally up and running is enable the googletests in the gpuci build script:

raft/ci/gpu/build.sh

Line 112 in 0d0c91f

# GTEST_OUTPUT="xml:${WORKSPACE}/test-results/raft_cpp/" ./test_raft

…ea-ext-migrate-cumlHandle_impl-into-raft

teju85 · 2020-05-12T03:17:04Z

All I need is another approval from someone from cuML team. @JohnZed mentioned that he's going to take a look at this one soon.

In the meanwhile, if @dantegd can approve this, it'll be great. Asking for more approvals and reviews as I think we need to thoroughly review such a fundamental change to raft!

JohnZed

Looks great! I can't claim I fully get all the rules in this clang tidy format, but it seems to match the google style guide and the code style clearly complies with our standard, so it looks great to me.

JohnZed · 2020-04-30T22:39:08Z

cpp/include/raft/allocator.hpp

+#include <cuda_runtime.h>
+#include "cudart_utils.h"
+
+namespace raft {


I'd like RAFT or Raft as we use capitalized MLCommon in cuML

cuml's current naming convention are a rather exception than rule. cuML started out much earlier than other projects in RAPIDS. At that time, we did a mistake of not standardizing any of these rules (mostly due to lack of time for thinking about these things)!

If you look at rmm, cudf or cugraph, they too are following the lowercase namespace. Thus, in order to be aligned with the rest of RAPIDS project, even here these are being named in lowercase.

cpp/include/raft/handle.hpp

cpp/include/raft/mr/allocator.hpp

Update fork from latest release

Refactor mst

Fix bugs in NN-Descent

Demangle the error stack trace provided by GCC. Example output: ```bash RAFT failure at file=/workspace/raft/cpp/bench/ann/src/raft/raft_ann_bench_utils.h line=127: Ooops! Obtained 16 stack frames #1 in /workspace/raft/cpp/build/libraft_ivf_pq_ann_bench.so: raft::logic_error::logic_error(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) +0x5e [0x7fb20acce45e] #2 in /workspace/raft/cpp/build/libraft_ivf_pq_ann_bench.so: raft::bench::ann::configured_raft_resources::stream_wait(CUstream_st*) const +0x2e3 [0x7fb20acd0ac3] #3 in /workspace/raft/cpp/build/libraft_ivf_pq_ann_bench.so: raft::bench::ann::RaftIvfPQ<float, long>::search(float const*, int, int, unsigned long*, float*, CUstream_st*) const +0x63e [0x7fb20acd44fe] #4 in ./cpp/build/ANN_BENCH: void raft::bench::ann::bench_search<float>(benchmark::State&, raft::bench::ann::Configuration::Index, unsigned long, std::shared_ptr<raft::bench::ann::Dataset<float> const>, raft::bench::ann::Objective) +0xf76 [0x55853859f586] #5 in ./cpp/build/ANN_BENCH: benchmark::internal::LambdaBenchmark<benchmark::RegisterBenchmark<void (&)(benchmark::State&, raft::bench::ann::Configuration::Index, unsigned long, std::shared_ptr<raft::bench::ann::Dataset<float> const>, raft::bench::ann::Objective), raft::bench::ann::Configuration::Index&, unsigned long&, std::shared_ptr<raft::bench::ann::Dataset<float> const>&, raft::bench::ann::Objective&>(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, void (&)(benchmark::State&, raft::bench::ann::Configuration::Index, unsigned long, std::shared_ptr<raft::bench::ann::Dataset<float> const>, raft::bench::ann::Objective), raft::bench::ann::Configuration::Index&, unsigned long&, std::shared_ptr<raft::bench::ann::Dataset<float> const>&, raft::bench::ann::Objective&)::{lambda(benchmark::State&)#1}>::Run(benchmark::State&) +0x84 [0x558538548f14] #6 in ./cpp/build/ANN_BENCH: benchmark::internal::BenchmarkInstance::Run(long, int, benchmark::internal::ThreadTimer*, benchmark::internal::ThreadManager*, benchmark::internal::PerfCountersMeasurement*) const +0x168 [0x5585385d6498] #7 in ./cpp/build/ANN_BENCH(+0x149108) [0x5585385b7108] #8 in ./cpp/build/ANN_BENCH: benchmark::internal::BenchmarkRunner::DoNIterations() +0x34f [0x5585385b8c7f] #9 in ./cpp/build/ANN_BENCH: benchmark::internal::BenchmarkRunner::DoOneRepetition() +0x119 [0x5585385b99b9] #10 in ./cpp/build/ANN_BENCH(+0x13afdd) [0x5585385a8fdd] #11 in ./cpp/build/ANN_BENCH: benchmark::RunSpecifiedBenchmarks(benchmark::BenchmarkReporter*, benchmark::BenchmarkReporter*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >) +0x58e [0x5585385aa8fe] #12 in ./cpp/build/ANN_BENCH: benchmark::RunSpecifiedBenchmarks() +0x6a [0x5585385aaada] #13 in ./cpp/build/ANN_BENCH: raft::bench::ann::run_main(int, char**) +0x11ed [0x5585385980cd] #14 in /lib/x86_64-linux-gnu/libc.so.6(+0x28150) [0x7fb213e28150] #15 in /lib/x86_64-linux-gnu/libc.so.6: __libc_start_main +0x89 [0x7fb213e28209] #16 in ./cpp/build/ANN_BENCH(+0xbfcef) [0x55853852dcef] ``` Authors: - Artem M. Chirkin (https://github.com/achirkin) Approvers: - Tamas Bela Feher (https://github.com/tfeher) URL: #2188

[skip ci]

teju85 added 8 commits April 27, 2020 20:14

FEA initial migration of device and host buffers into raft

ac5ed0f

FEA added cudaruntime utils which will be used in raft repo

2f81d83

FEA added allocator interface

92aed07

DOC doxygen updates

dea9676

FEA added raft::handle_t

1921918

FEA added cusolver/cublas/cusparse wrappers and updated raft/handle.hpp

5770850

FIX updated project name in cmake file

92daf4d

ENH added googletest build for unit-testing

c93afda

seunghwak reviewed Apr 28, 2020

View reviewed changes

afender reviewed Apr 28, 2020

View reviewed changes

cpp/include/raft/cudart_utils.h Outdated Show resolved Hide resolved

cpp/include/raft/allocator.hpp Outdated Show resolved Hide resolved

cpp/include/raft/cudart_utils.h Outdated Show resolved Hide resolved

seunghwak reviewed Apr 28, 2020

View reviewed changes

cpp/include/raft/buffer_base.hpp Outdated Show resolved Hide resolved

teju85 added 2 commits April 28, 2020 11:05

ENH added proper include-dirs and link libraries for the unit-test to…

0d369d2

… run successfully

ENH initial commit for cudart_utils unit-tests

81d5a70

teju85 added 13 commits May 4, 2020 10:45

Merge branch 'branch-0.14' of https://github.com/rapidsai/raft into f…

731b238

…ea-ext-migrate-cumlHandle_impl-into-raft

FIX enabled ABI to make gtests running again

7039a55

FIX updated allocator header include in device buffer

b398243

TEST added unit-tests for device_buffer

64377cb

FIX updated allocator header include in host buffer

2946799

TEST added unit-tests for host_buffer

b058aca

FIX handle/cublas/cusolver header compilation issues

48390f6

TEST added unit-tests for handle_t

ed543ce

CI updated style.sh with the latest checks

db38255

FIX ignore commented lines in include_checker

a07b3af

ENH added .clang-format file

6aecf50

FIX clang-format fixes

46949e8

DOC update changelog

f8b9b4c

teju85 added 2 commits May 11, 2020 00:00

FIX reduced code duplication in create_resources

73b75f5

FIX apply critical section on various "get handle" methods

93c1a98

teju85 mentioned this pull request May 11, 2020

[FEA] also reconcile error_utils from cugraph #8

Closed

teju85 added 6 commits May 11, 2020 00:32

ENH updated build to find rmm installation from conda-env

4a55d57

ENH made default allocator to use RMM

0ac33c7

ENH moved all allocator and buffer logic under appropriate folders/na…

90c139a

…mespaces for clarity

ENH updated tests and other interfaces to the new mr design

e09393f

allow users to call buffer operations without an explicit stream para…

7a5b15f

…meter

ENH added support for constructing host buffer from device buffer

58c490c

teju85 changed the title ~~[WIP] migrating cumlHandle_impl (and related dependencies) into raft~~ [REVIEW] migrating cumlHandle_impl (and related dependencies) into raft May 11, 2020

FIX clang-format fixes

2fb8f74

seunghwak approved these changes May 11, 2020

View reviewed changes

cpp/include/raft/buffer_base.hpp Outdated Show resolved Hide resolved

dantegd requested changes May 11, 2020

View reviewed changes

teju85 added 2 commits May 11, 2020 19:58

Merge branch 'branch-0.14' of https://github.com/rapidsai/raft into f…

e47a7a0

…ea-ext-migrate-cumlHandle_impl-into-raft

CI enabled googletest

eb4db02

JohnZed approved these changes May 12, 2020

View reviewed changes

FIX use RAII-style lock_guard

4b0619e

dantegd approved these changes May 12, 2020

View reviewed changes

dantegd merged commit e003de2 into rapidsai:branch-0.14 May 12, 2020

teju85 deleted the fea-ext-migrate-cumlHandle_impl-into-raft branch May 13, 2020 01:53

afender mentioned this pull request May 28, 2020

[FEA] Accept RAFT handle everywhere rapidsai/cugraph#815

Closed

2 tasks

afender pushed a commit that referenced this pull request Jul 29, 2020

Merge pull request #3 from rapidsai/branch-0.15

e3699a4

Update fork from latest release

divyegala referenced this pull request in divyegala/raft Oct 28, 2020

Merge pull request #3 from divyegala/refactor

26bd885

Refactor mst

rapids-bot bot pushed a commit that referenced this pull request Aug 22, 2023

Merge pull request #3 from RayWang96/port-nn-descent

1f1f32d

Fix bugs in NN-Descent

dantegd pushed a commit to dantegd/raft that referenced this pull request Jul 23, 2024

Remove usages of rapids-env-update (rapidsai#3)

d8fd59e

[skip ci]

kun429973 mentioned this pull request Oct 24, 2024

Raft Cpp Test Error #2476

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[REVIEW] migrating cumlHandle_impl (and related dependencies) into raft #3

[REVIEW] migrating cumlHandle_impl (and related dependencies) into raft #3

teju85 commented Apr 28, 2020 •

edited

Loading

seunghwak commented Apr 28, 2020

cjnolet commented Apr 28, 2020

afender left a comment

dillon-cullinan commented May 1, 2020

GPUtester commented May 1, 2020

teju85 commented May 11, 2020

seunghwak left a comment

dillon-cullinan commented May 11, 2020

dantegd left a comment

teju85 commented May 12, 2020

JohnZed left a comment

JohnZed Apr 30, 2020

teju85 May 12, 2020

[REVIEW] migrating cumlHandle_impl (and related dependencies) into raft #3

[REVIEW] migrating cumlHandle_impl (and related dependencies) into raft #3

Conversation

teju85 commented Apr 28, 2020 • edited Loading

seunghwak commented Apr 28, 2020

cjnolet commented Apr 28, 2020

afender left a comment

Choose a reason for hiding this comment

dillon-cullinan commented May 1, 2020

GPUtester commented May 1, 2020

teju85 commented May 11, 2020

seunghwak left a comment

Choose a reason for hiding this comment

dillon-cullinan commented May 11, 2020

dantegd left a comment

Choose a reason for hiding this comment

teju85 commented May 12, 2020

JohnZed left a comment

Choose a reason for hiding this comment

JohnZed Apr 30, 2020

Choose a reason for hiding this comment

teju85 May 12, 2020

Choose a reason for hiding this comment

teju85 commented Apr 28, 2020 •

edited

Loading