[REVIEW] OPG degree #840
[REVIEW] OPG degree #840
Conversation
|
Expected output |
Codecov Report
@@ Coverage Diff @@
## branch-0.14 #840 +/- ##
============================================
Coverage 47.92% 47.92%
============================================
Files 44 44
Lines 1327 1327
============================================
Hits 636 636
Misses 691 691 Continue to review full report at Codecov.
|
| @@ -330,6 +330,7 @@ link_directories( | |||
| "${CMAKE_CUDA_IMPLICIT_LINK_DIRECTORIES}") | |||
|
|
|||
| add_library(cugraph SHARED | |||
| src/comms/mpi/comms_mpi.cpp | |||
There was a problem hiding this comment.
Should this also be in a "BUILD_MPI" block? there is no need to include the file if mpi is not going to be built.
There was a problem hiding this comment.
When BUILD_MPI is OFF, the communicator member functions become no-op. This allows running through the existing path without contaminating the whole code base with #ifdef ENABLE_OPGoutside of the communicator class.
cpp/src/comms/mpi/comms_mpi.cpp
Outdated
| // CUDA | ||
|
|
||
| CUDA_TRY(cudaGetDeviceCount(&_device_count)); | ||
| _device_id = _rank % _device_count; // FixMe : assumes each node has the same number of GPUs |
There was a problem hiding this comment.
This seems like it could be an issues. We should discuss configuration and how to handle
you also only set one device and not one per rank
There was a problem hiding this comment.
This seems like it could be an issues. We should discuss configuration and how to handle
I think it is fine. We expect python to set the device and this code to be removed. This is just for C++ smoke tests for now so that I can validate the progress on the C++ part.
you also only set one device and not one per rank
No, it does set one device per rank. In an OPG environment, this section of the code is traversed by all ranks, each one will retrieve and set its own GPU based on its rank.
cpp/src/structure/graph.cu
Outdated
| degree_from_vertex_ids(GraphBase<VT,ET,WT>::number_of_edges, src_indices, degree, stream); | ||
| if (GraphBase<VT,ET,WT>::comm.get_p()) // FixMe retrieve global source indexing for the allreduce work | ||
| CUGRAPH_FAIL("OPG degree not implemented for OUT degree"); | ||
| degree_from_vertex_ids(GraphBase<VT,ET,WT>::comm, GraphBase<VT,ET,WT>::number_of_vertices, GraphBase<VT,ET,WT>::number_of_edges, src_indices, degree, stream); |
There was a problem hiding this comment.
Is it GraphBase or new GraphBaseView? PR #799 might need to be merged first
There was a problem hiding this comment.
We would need to reconcile either way.
|
It would be good to be able to run these changes on CI with mpi build ON |
close #810