[REVIEW] Migrating cuml comms to raft::comms #7

cjnolet · 2020-05-07T00:37:11Z

This is based on top of initial work by @teju85 in #3. This is simply a copy (and reduction) of the current cuml comms to header-only form.

GPUtester · 2020-05-07T00:37:35Z

Please update the changelog in order to start CI tests.

View the gpuCI docs here.

…-comms Conflicts: cpp/include/raft/handle.hpp

…tle bit

cpp/include/raft/comms/CMakeLists.txt

cpp/include/raft/comms/std_comms.hpp

cpp/include/raft/comms/comms.hpp

cpp/include/raft/comms/std_comms.hpp

teju85 · 2020-05-20T06:56:51Z

cpp/include/raft/comms/comms.hpp

+ public:
+  virtual ~comms_iface();
+
+  virtual int getSize() const = 0;


I think it is better to enable clang-tidy in CI, now that we have it ready in cuML too?

teju85

some very minor nitpicks. Otherwise overall the changes LGTM!

cpp/include/raft/comms/comms_helper.hpp

cpp/include/raft/comms/nccl_helper.hpp

cpp/include/raft/comms/std_comms.hpp

Iroy30

I have been integrating this PR with cugraph. Just had a few import issues

python/raft/common/__init__.py

python/raft/dask/__init__.py

python/raft/dask/common/__init__.py

python/raft/dask/common/comms.py

cjnolet · 2020-05-26T20:16:10Z

@teju85 @Iroy30 I have implemented your requested changes. Mind giving this PR a re-review?

cjnolet · 2020-05-27T17:33:12Z

@seunghwak @afender, making the nccl type and data type size functions static fixes the GCC issue for now. I can create a new issue for us to knock out in the future so that we can get this merged soon. What do you think?

seunghwak · 2020-05-27T17:48:54Z

@seunghwak @afender, making the nccl type and data type size functions static fixes the GCC issue for now. I can create a new issue for us to knock out in the future so that we can get this merged soon. What do you think?

Sounds good for me, thanks for the effort!

teju85

Sorry for the delay in my response. Just a couple of minor things.

teju85 · 2020-06-03T05:22:49Z

cpp/CMakeLists.txt

@@ -67,6 +67,8 @@ find_package(CUDA 10.0 REQUIRED)

 set(CMAKE_MODULE_PATH ${CMAKE_CURRENT_SOURCE_DIR}/cmake)

+message("HELLO!")


this is probably a remnant of a debug session?

teju85 · 2020-06-03T05:23:52Z

cpp/cmake/comms.cmake

+cmake_minimum_required(VERSION 3.14 FATAL_ERROR)
+project(comms LANGUAGES CXX CUDA)
+
+set(CMAKE_MODULE_PATH ${CMAKE_MODULE_PATH} "${CMAKE_CURRENT_SOURCE_DIR}/cmake")


why do we need this module-path set command here, in a module?

cjnolet · 2020-06-03T15:06:27Z

@teju85 Thank you for the re-review. I've addressed the latest comments. Is this ready to be merged?

teju85

Thanks @cjnolet . Changes LGTM.

cjnolet · 2020-06-03T16:45:38Z

@dantegd, I understood you and Ishika both needed the relative Python imports. Do you have any further review feedback?

afender · 2020-06-03T17:31:50Z

Synced up with Dante on Slack 👍 Ishika is going to add a PR on top of this in the next few hours/days so we can keep adding enhancements there.

Merging 🚀

add cross products for yaml->json configs

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

cjnolet added 2 commits May 6, 2020 20:30

Initial commit of raft comms

fd38dbb

Removing cuml verbiage

8a6e054

cjnolet added 11 commits May 7, 2020 20:25

Renaming to comms_t, removing logs for now, adding to handle

2bfc0dc

Looking like it's building!

f129710

Adding NCCL and UCX build for tests

a0c13af

Merge branch 'fea-ext-migrate-cumlHandle_impl-into-raft' into fea-ext…

7ad2053

…-comms Conflicts: cpp/include/raft/handle.hpp

Consolidating injection functions

cb19482

Merge branch 'fea-ext-migrate-cumlHandle_impl-into-raft' into fea-ext…

1d66637

…-comms Conflicts: cpp/include/raft/handle.hpp

Making progress on Python. Need to refactor the comms interface a lit…

1548177

…tle bit

Cython is building!

5d37e9e

The comms tests pass!!!

bd1b876

Fixing flake8 style

98496d3

Running clang format and removing getDataType

fb52f79

cjnolet changed the title ~~[WIP] Migrating cuml comms to raft::comms~~ [REVIEW] Migrating cuml comms to raft::comms May 13, 2020

cjnolet added 15 commits May 13, 2020 14:40

Merge remote-tracking branch 'github/branch-0.14' into fea-ext-comms

d666314

Cleaning up

46a27eb

Fixing python style

fc6ba2e

Fixing cpp style

842f533

Adding copyright headers

43f0d69

Adding init py for tests

ac6e699

Adding license headers and consistent namespacing

c442d87

More cleanup

32e63c1

Cleaning up raft.dask.common.Comms

d377347

Ignoring raft egg artifacts

e1b4ea7

Cleaning up raft dask utils

736696b

More cleanup, copyright headers, and docs

55d9dfd

Removing the last of references to cuml

6eb0325

Fixing python style

b65e20d

Fixing c++ style

7a845cb

teju85 reviewed May 20, 2020

View reviewed changes

cpp/include/raft/comms/CMakeLists.txt Outdated Show resolved Hide resolved

cpp/include/raft/comms/std_comms.hpp Outdated Show resolved Hide resolved

teju85 reviewed May 20, 2020

View reviewed changes

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

cpp/include/raft/comms/std_comms.hpp Outdated Show resolved Hide resolved

teju85 reviewed May 20, 2020

View reviewed changes

teju85 requested changes May 20, 2020

View reviewed changes

Iroy30 mentioned this pull request May 21, 2020

[REVIEW] dask infra updates rapidsai/cugraph#896

Merged

Iroy30 reviewed May 22, 2020

View reviewed changes

python/raft/common/__init__.py Outdated Show resolved Hide resolved

python/raft/dask/__init__.py Outdated Show resolved Hide resolved

python/raft/dask/common/__init__.py Outdated Show resolved Hide resolved

python/raft/dask/common/comms.py Outdated Show resolved Hide resolved

cjnolet added 2 commits May 26, 2020 15:42

More review feedback

6e9025d

Running cpp style check

28f8101

cjnolet added 4 commits May 27, 2020 10:54

Nccl red op

1a18553

Raising an exception to get around gcc issue

a22dd62

Using static for functions for now

fbd12aa

Fixing style

cc71ccb

Fixing more relative imports

dc123b2

afender mentioned this pull request May 28, 2020

[FEA] communicator abstraction rapidsai/cugraph#814

Closed

cjnolet mentioned this pull request Jun 1, 2020

[QST]Running cuML with NCCL and MPI rapidsai/cuml#2351

Closed

Iroy30 approved these changes Jun 1, 2020

View reviewed changes

teju85 requested changes Jun 3, 2020

View reviewed changes

final updates based on feedback

60597f3

teju85 approved these changes Jun 3, 2020

View reviewed changes

afender changed the base branch from branch-0.14 to branch-0.15 June 3, 2020 16:43

Merge branch 'branch-0.15' into fea-ext-comms

5bbb8be

afender merged commit 314eb6b into rapidsai:branch-0.15 Jun 3, 2020

copy-pr-bot bot pushed a commit that referenced this pull request Oct 24, 2023

Merge pull request #7 from divyegala/fea-2312-bench-ann-conf

2532571

add cross products for yaml->json configs

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[REVIEW] Migrating cuml comms to raft::comms #7

[REVIEW] Migrating cuml comms to raft::comms #7

cjnolet commented May 7, 2020 •

edited

Loading

GPUtester commented May 7, 2020

teju85 May 20, 2020

teju85 left a comment

Iroy30 left a comment

cjnolet commented May 26, 2020

cjnolet commented May 27, 2020

seunghwak commented May 27, 2020

teju85 left a comment

teju85 Jun 3, 2020

teju85 Jun 3, 2020

cjnolet commented Jun 3, 2020

teju85 left a comment

cjnolet commented Jun 3, 2020

afender commented Jun 3, 2020 •

edited

Loading

		@@ -67,6 +67,8 @@ find_package(CUDA 10.0 REQUIRED)

		set(CMAKE_MODULE_PATH ${CMAKE_CURRENT_SOURCE_DIR}/cmake)

		message("HELLO!")

[REVIEW] Migrating cuml comms to raft::comms #7

[REVIEW] Migrating cuml comms to raft::comms #7

Conversation

cjnolet commented May 7, 2020 • edited Loading

GPUtester commented May 7, 2020

teju85 May 20, 2020

Choose a reason for hiding this comment

teju85 left a comment

Choose a reason for hiding this comment

Iroy30 left a comment

Choose a reason for hiding this comment

cjnolet commented May 26, 2020

cjnolet commented May 27, 2020

seunghwak commented May 27, 2020

teju85 left a comment

Choose a reason for hiding this comment

teju85 Jun 3, 2020

Choose a reason for hiding this comment

teju85 Jun 3, 2020

Choose a reason for hiding this comment

cjnolet commented Jun 3, 2020

teju85 left a comment

Choose a reason for hiding this comment

cjnolet commented Jun 3, 2020

afender commented Jun 3, 2020 • edited Loading

cjnolet commented May 7, 2020 •

edited

Loading

afender commented Jun 3, 2020 •

edited

Loading