merge updates #3

dongxuy04 · 2022-08-15T10:51:43Z

Merged updates from WholeMemory and update docs.

include/whole_memory_graph.h

Dockerfile

test/CMakeLists.txt

teju85 · 2022-08-16T06:58:29Z

@robertmaynard can we get some review on the cmake logic here, please?

docs/CppAPI.md

docs/GNNExample.md

docs/PyTorchAPI.md

docs/WholeMemoryIntroduction.md

Co-authored-by: Thejaswi. N. S <rao.thejaswi@gmail.com>

wholegraph/parallel_utils.cc

wholegraph/bootstrap_communicator.cc

fetch_rapids.cmake

CMakeLists.txt

wholegraph/torch/CMakeLists.txt

CMakeLists.txt

robertmaynard · 2022-08-17T13:26:33Z

CMakeLists.txt

@@ -7,41 +20,53 @@ include(rapids-cuda)
 include(rapids-export)
 include(rapids-find)

-rapids_cuda_init_architectures(WHOLEGRAPH)
-
+set(CMAKE_CUDA_ARCHITECTURES 70-real 75-real 80-real 86)


We should go with rapids_cuda_init_architectures(WHOLEGRAPH) instead of the explicit value set. This will allow users to compile for a subset, and make it easier to support new architectures

As we use some new CUDA features that only supported after 70 and newer architectures now, like memory consistency model and nanosleep. So we would like to set CUDA architectures newer than or equal to 70. It seems to me that rapids cmake supports ALL and NATIVE, is it able to set values >= 70?

As far as I am aware all of RAPIDS needs to support sm_60 as that is still a major deployment target.
rapids-cmake ALL keyword maps to 60-real, 70-real, 75-real, ....

Either way what you should do is call rapids_cuda_init_architectures(WHOLEGRAPH).

This will allow the user to specify a value for CMAKE_CUDA_ARCHITECTURES. After the project call you can always iterate that value and produce an error if it contains an sm value that is too low.

Do you mean I can use set(CMAKE_CUDA_ARCHITECTURES 70-real 75-real 80-real 86) after rapids_cuda_init_architectures(WHOLEGRAPH) and project call?
Maybe like this?
rapids_cuda_init_architectures(WHOLEGRAPH)
project(wholegraph CXX CUDA)
set(CMAKE_CUDA_ARCHITECTURES 70-real 75-real 80-real 86)

What I am saying is that you shouldn't overwrite what the user has specified. If a user wants to just build the GPU on the local machine they should be able to without changing any CMake code. They should be able to specify -DCMAKE_CUDA_ARCHITECTURE=86-real or -DCMAKE_CUDA_ARCHITECTURE=NATIVE.

Therefore what you should do is:

rapids_cuda_init_architectures(WHOLEGRAPH) project(wholegraph CXX CUDA)

and have a C++ side check like:

#if __CUDA_ARCH__ < 700 #error "wholegraph doesn't support architectures ..... #endif

Updated the cmake logic, set default CMAKE_CUDA_ARCHITECTURES if user doesn't specify it. And call rapids_cuda_init_architectures to support ALL and NATIVE

if (NOT DEFINED CMAKE_CUDA_ARCHITECTURES) set(CMAKE_CUDA_ARCHITECTURES 70-real 75-real 80-real 86) endif () rapids_cuda_init_architectures(WHOLEGRAPH) project(wholegraph CXX CUDA)

Also updated CUDA C++ code as suggested.

CMakeLists.txt

robertmaynard · 2022-08-17T14:34:08Z

CMakeLists.txt

-# Configure path to modules (for find_package)
-set(CMAKE_MODULE_PATH ${CMAKE_MODULE_PATH} "${PROJECT_SOURCE_DIR}/cmake/modules/")
+# enable assert in RelWithDebInfo build type
+set(CMAKE_CXX_FLAGS_RELWITHDEBINFO "-O3 -g")


Why do you need to overwrite the default flag values for RELWITHDEBINFO?

I would like to remove -DNDEBUG in RELWITHDEBINFO to enable assert. Is there a better way to do this?

CMakeLists.txt

robertmaynard · 2022-08-17T14:37:46Z

test/CMakeLists.txt

-add_executable(whole_graph_sp_test whole_graph_sp_test.cu)
-target_link_libraries(whole_graph_sp_test whole_graph)
+add_executable(whole_memory_mp_test whole_memory_mp_test.cu)
+target_link_libraries(whole_memory_mp_test whole_graph)


All the target_link_libraries should be updated to be target_link_libraries( <target> PRIVATE whole_graph)

wholegraph/torch/CMakeLists.txt

robertmaynard · 2022-08-17T14:39:12Z

CMakeLists.txt

+target_compile_definitions(whole_graph PUBLIC -D_FILE_OFFSET_BITS=64)
+if (${USE_CXX11_ABI})
+    message(STATUS "Using CXX ABI = 1")
+    target_compile_definitions(whole_graph PUBLIC -D_GLIBCXX_USE_CXX11_ABI=1)


Do these need to be public? does whole_graph have a public api that includes C++ types?

Yes, whole_graph provides api with C++ types.

Co-authored-by: Robert Maynard <robertjmaynard@gmail.com>

…CTURES

teju85

couple of very minor nitpicks.

teju85 · 2022-08-22T05:02:06Z

wholegraph/whole_graph_negative_sampler.cu

+  thread_local std::mt19937 gen(rd());
+  thread_local std::uniform_int_distribution<unsigned long long> distrib;
+  unsigned long long random_seed = distrib(gen);
+  WM_CUDA_CHECK(cudaStreamSynchronize(stream));


do we need this stream-sync?

It is not needed, removed, thanks!

teju85 · 2022-08-22T05:16:42Z

wholegraph/whole_memory_embedding.cu

+  char *ptr_to = (char *) to;
+  const char *ptr_from = (const char *) from;
+  for (int i = 0; i < DataSize; i++) {
+    ptr_to[i] = ptr_from[i];
+  }


It's simpler to use memcpy function instead.

Thanks for your suggestion! Yes it will be simpler. Here DataSize is template parameter, and should be not large in normal cases, maybe we would prefer compiler to optimize for it instead of device function call.

teju85

Pre-approving. Overall LGTM.

teju85 · 2022-08-22T07:25:43Z

Thanks @dongxuy04 . Appreciate your patience during the PR review process.

@BradReesWork we are now be ready to merge this one!

dongxuy04 · 2022-08-23T07:17:19Z

Thanks @teju85 @robertmaynard @BradReesWork for many good suggestions and great help during the PR review process! @BradReesWork shall we get this PR merged?

dongxuy04 and others added 2 commits August 15, 2022 16:49

merge and rename

87ba9c5

Update README.md

8aa1cb2

This was referenced Aug 16, 2022

Use RAFT communicator instead of a separate communicator in WholeMemory #4

Open

Use top-k from RAFT #5

Closed

teju85 requested changes Aug 16, 2022

View reviewed changes

include/whole_memory_graph.h Outdated Show resolved Hide resolved

Dockerfile Show resolved Hide resolved

test/CMakeLists.txt Show resolved Hide resolved

remove unused API and add license text

8478341

dongxuy04 requested a review from teju85 August 16, 2022 06:30

teju85 requested a review from robertmaynard August 16, 2022 06:58

teju85 requested changes Aug 16, 2022

View reviewed changes

docs/CppAPI.md Outdated Show resolved Hide resolved

docs/GNNExample.md Outdated Show resolved Hide resolved

docs/GNNExample.md Outdated Show resolved Hide resolved

docs/PyTorchAPI.md Outdated Show resolved Hide resolved

docs/WholeMemoryIntroduction.md Outdated Show resolved Hide resolved

dongxuy04 and others added 5 commits August 16, 2022 15:44

Fix typo in docs/CppAPI.md

c164c86

Co-authored-by: Thejaswi. N. S <rao.thejaswi@gmail.com>

Fix typo in docs/GNNExample.md

7d1057c

Co-authored-by: Thejaswi. N. S <rao.thejaswi@gmail.com>

Fix typo in docs/GNNExample.md

2a22c2a

Co-authored-by: Thejaswi. N. S <rao.thejaswi@gmail.com>

Fix typo in docs/PyTorchAPI.md

9ccd365

Co-authored-by: Thejaswi. N. S <rao.thejaswi@gmail.com>

Fix typo in docs/WholeMemoryIntroduction.md

638c430

Co-authored-by: Thejaswi. N. S <rao.thejaswi@gmail.com>

teju85 reviewed Aug 16, 2022

View reviewed changes

wholegraph/parallel_utils.cc Show resolved Hide resolved

teju85 requested changes Aug 16, 2022

View reviewed changes

wholegraph/bootstrap_communicator.cc Outdated Show resolved Hide resolved

wholegraph/bootstrap_communicator.cc Outdated Show resolved Hide resolved

remove not needed and unused code in bootstrap_communicator.cc

cb22bae

robertmaynard suggested changes Aug 16, 2022

View reviewed changes

update CMake

a060f43

dongxuy04 requested review from robertmaynard and teju85 August 17, 2022 03:24

robertmaynard suggested changes Aug 17, 2022

View reviewed changes

dongxuy04 and others added 2 commits August 17, 2022 22:46

Update CMakeLists.txt

2f85423

Co-authored-by: Robert Maynard <robertjmaynard@gmail.com>

update CMake

c8110e6

dongxuy04 requested a review from robertmaynard August 17, 2022 15:41

BradReesWork approved these changes Aug 17, 2022

View reviewed changes

dongxuy04 added 2 commits August 18, 2022 14:23

use rapids_cuda_init_architectures and add default CMAKE_CUDA_ARCHITE…

621b331

…CTURES

clean up unused code

ae2b58d

robertmaynard approved these changes Aug 18, 2022

View reviewed changes

teju85 requested changes Aug 22, 2022

View reviewed changes

teju85 approved these changes Aug 22, 2022

View reviewed changes

remove unused sync

4af140b

BradReesWork merged commit bef14e0 into rapidsai:main Aug 29, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

merge updates #3

merge updates #3

dongxuy04 commented Aug 15, 2022

teju85 commented Aug 16, 2022

robertmaynard Aug 17, 2022

dongxuy04 Aug 17, 2022

robertmaynard Aug 17, 2022

robertmaynard Aug 17, 2022

dongxuy04 Aug 17, 2022

robertmaynard Aug 17, 2022

dongxuy04 Aug 18, 2022

robertmaynard Aug 17, 2022

dongxuy04 Aug 17, 2022

robertmaynard Aug 17, 2022

dongxuy04 Aug 18, 2022

robertmaynard Aug 17, 2022

dongxuy04 Aug 17, 2022

teju85 left a comment

teju85 Aug 22, 2022

dongxuy04 Aug 22, 2022

teju85 Aug 22, 2022

dongxuy04 Aug 22, 2022

teju85 left a comment

teju85 commented Aug 22, 2022

dongxuy04 commented Aug 23, 2022

merge updates #3

merge updates #3

Conversation

dongxuy04 commented Aug 15, 2022

teju85 commented Aug 16, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

teju85 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

teju85 left a comment

Choose a reason for hiding this comment

teju85 commented Aug 22, 2022

dongxuy04 commented Aug 23, 2022