Merge pull request #105 from seunghwak/bug_ext_python_test

[REVIEW] Bug ext python test

CHANGELOG.md

@@ -22,11 +22,13 @@
 - PR #96 Relocated mmio.c and mmio.h (external files) to thirdparty/mmio
 - PR #97 Updated python tests to speed them up
 - PR #100 Added testing for returned vertex and edge identifiers  
+- PR #105 Updated python code to follow PEP8 (fixed flake8 complaints)
 
 ## Bug Fixes
 
 - PR #48 ABI Fixes
 - PR #72 Bug fix for segfault issue getting transpose from adjacency list
+- PR #105 Bug fix for memory leaks and python test failures
 - PR #110 Bug fix for segfault calling Louvain with only edge list
 - PR #116 Added netscience.mtx dataset to datasets.tar.gz
 

cpp/include/rmm_utils.h

@@ -45,7 +45,7 @@ class rmm_allocator : public thrust::device_malloc_allocator<T>
     ~rmm_allocator() {}
 
 private:
-  	cudaStream_t stream;
+    cudaStream_t stream;
 };
 
 using rmm_temp_allocator = rmm_allocator<char>; // Use this alias for thrust::cuda::par(allocator).on(stream)

cpp/include/types.h

@@ -94,7 +94,7 @@ struct gdf_graph{
   gdf_dynamic *dynAdjList; //dynamic 
   gdf_graph_properties *prop;
   gdf_graph() : edgeList(nullptr), adjList(nullptr), transposedAdjList(nullptr), dynAdjList(nullptr), prop(nullptr) {}
-   ~gdf_graph() {
+  ~gdf_graph() {
     if (edgeList) 
         delete edgeList;
     if (adjList) 

cpp/src/cugraph.cu

@@ -29,14 +29,25 @@ void gdf_col_delete(gdf_column* col) {
         {
         ALLOC_FREE_TRY(col->data, nullptr);
         }
+#if 1
+// If delete col is executed, the memory pointed by col is no longer valid and
+// can be used in another memory allocation, so executing col->data = nullptr
+// after delete col is dangerous, also, col = nullptr has no effect here (the
+// address is passed by value, for col = nullptr should work, the input
+// parameter should be gdf_column*& col (or alternatively, gdf_column** col and
+// *col = nullptr also work)
+    col->data = nullptr;
+    delete col;
+#else
     delete col;
     col->data = nullptr;
-    col = nullptr;  
-  }                                                       
+    col = nullptr;
+#endif
+  }
 }
 
 void gdf_col_release(gdf_column* col) {
-  delete col;                                                          
+  delete col;
 }
 
 void cpy_column_view(const gdf_column *in, gdf_column *out) {
@@ -284,8 +295,8 @@ gdf_error gdf_pagerank_impl (gdf_graph *graph,
 
 gdf_error gdf_add_adj_list(gdf_graph *graph)
 { 
-	if (graph->adjList != nullptr)
-		return GDF_SUCCESS;
+  if (graph->adjList != nullptr)
+    return GDF_SUCCESS;
 
   GDF_REQUIRE( graph->edgeList != nullptr , GDF_INVALID_API_CALL);
   GDF_REQUIRE( graph->adjList == nullptr , GDF_INVALID_API_CALL);
@@ -304,8 +315,8 @@ gdf_error gdf_add_adj_list(gdf_graph *graph)
 
 gdf_error gdf_add_transpose(gdf_graph *graph)
 {
-	if (graph->edgeList == nullptr)
-		gdf_add_edge_list(graph);
+  if (graph->edgeList == nullptr)
+    gdf_add_edge_list(graph);
   if (graph->edgeList->edge_data != nullptr) {
     switch (graph->edgeList->edge_data->dtype) {
       case GDF_FLOAT32:   return gdf_add_transpose_impl<float>(graph);
@@ -349,26 +360,26 @@ gdf_error gdf_pagerank(gdf_graph *graph, gdf_column *pagerank, float alpha, floa
 }
 
 gdf_error gdf_bfs(gdf_graph *graph, gdf_column *distances, gdf_column *predecessors, int start_node, bool directed) {
-	GDF_REQUIRE(graph->adjList != nullptr || graph->edgeList != nullptr, GDF_INVALID_API_CALL);
-	gdf_error err = gdf_add_adj_list(graph);
-	if (err != GDF_SUCCESS)
-		return err;
-	GDF_REQUIRE(graph->adjList->offsets->dtype == GDF_INT32, GDF_UNSUPPORTED_DTYPE);
-	GDF_REQUIRE(graph->adjList->indices->dtype == GDF_INT32, GDF_UNSUPPORTED_DTYPE);
-	GDF_REQUIRE(distances->dtype == GDF_INT32, GDF_UNSUPPORTED_DTYPE);
-	GDF_REQUIRE(predecessors->dtype == GDF_INT32, GDF_UNSUPPORTED_DTYPE);
-
-	int n = graph->adjList->offsets->size - 1;
-	int e = graph->adjList->indices->size;
-	int* offsets_ptr = (int*)graph->adjList->offsets->data;
-	int* indices_ptr = (int*)graph->adjList->indices->data;
-	int* distances_ptr = (int*)distances->data;
-	int* predecessors_ptr = (int*)predecessors->data;
-	int alpha = 15;
-	int beta = 18;
-
-	cugraph::Bfs<int> bfs(n, e, offsets_ptr, indices_ptr, directed, alpha, beta);
-	bfs.configure(distances_ptr, predecessors_ptr, nullptr);
-	bfs.traverse(start_node);
-	return GDF_SUCCESS;
+  GDF_REQUIRE(graph->adjList != nullptr || graph->edgeList != nullptr, GDF_INVALID_API_CALL);
+  gdf_error err = gdf_add_adj_list(graph);
+  if (err != GDF_SUCCESS)
+    return err;
+  GDF_REQUIRE(graph->adjList->offsets->dtype == GDF_INT32, GDF_UNSUPPORTED_DTYPE);
+  GDF_REQUIRE(graph->adjList->indices->dtype == GDF_INT32, GDF_UNSUPPORTED_DTYPE);
+  GDF_REQUIRE(distances->dtype == GDF_INT32, GDF_UNSUPPORTED_DTYPE);
+  GDF_REQUIRE(predecessors->dtype == GDF_INT32, GDF_UNSUPPORTED_DTYPE);
+
+  int n = graph->adjList->offsets->size - 1;
+  int e = graph->adjList->indices->size;
+  int* offsets_ptr = (int*)graph->adjList->offsets->data;
+  int* indices_ptr = (int*)graph->adjList->indices->data;
+  int* distances_ptr = (int*)distances->data;
+  int* predecessors_ptr = (int*)predecessors->data;
+  int alpha = 15;
+  int beta = 18;
+
+  cugraph::Bfs<int> bfs(n, e, offsets_ptr, indices_ptr, directed, alpha, beta);
+  bfs.configure(distances_ptr, predecessors_ptr, nullptr);
+  bfs.traverse(start_node);
+  return GDF_SUCCESS;
 }

cpp/src/tests/test_utils.h

@@ -663,8 +663,19 @@ void gdf_col_delete(gdf_column* col) {
     cudaStream_t stream{nullptr}; 
     if(col->data)
       ALLOC_FREE_TRY(col->data, stream);
+#if 1
+// If delete col is executed, the memory pointed by col is no longer valid and
+// can be used in another memory allocation, so executing col->data = nullptr
+// after delete col is dangerous, also, col = nullptr has no effect here (the
+// address is passed by value, for col = nullptr should work, the input
+// parameter should be gdf_column*& col (or alternatively, gdf_column** col and
+// *col = nullptr also work)
+    col->data = nullptr;
+    delete col;
+#else
     delete col;
     col->data = nullptr;
-    col = nullptr;  
+    col = nullptr;
+#endif
   }                                                       
 }

python/cugraph/bfs/test_bfs.py

@@ -11,75 +11,85 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
-import cugraph
-import cudf
+import queue
 import time
-from scipy.io import mmread
-import pytest
+
 import numpy as np
+import pytest
+from scipy.io import mmread
 
-def ReadMtxFile(mmFile):
-    print('Reading ' + str(mmFile) + '...')
-    return mmread(mmFile).asfptype()
+import cudf
+import cugraph
 
 
-def cugraph_Call(M, start_vertex):
+def read_mtx_file(mm_file):
+    print('Reading ' + str(mm_file) + '...')
+    return mmread(mm_file).asfptype()
 
+
+def cugraph_call(M, start_vertex):
     # Device data
     M = M.tocsr()
     sources = cudf.Series(M.indptr)
     destinations = cudf.Series(M.indices)
     values = cudf.Series(M.data)
-    
+
     G = cugraph.Graph()
     G.add_adj_list(sources, destinations, values)
-    
+
     t1 = time.time()
     df = cugraph.bfs(G, start_vertex)
     t2 = time.time() - t1
     print('Time : '+str(t2))
-    
+
     # Return distances as np.array()
     return df['vertex'].to_array(), df['distance'].to_array()
 
 
-def base_Call(M, start_vertex):
-    intMax = 2147483647
+def base_call(M, start_vertex):
+    int_max = 2**31 - 1
+
     M = M.tocsr()
+
     offsets = M.indptr
     indices = M.indices
     num_verts = len(offsets) - 1
     dist = np.zeros(num_verts, dtype=np.int32)
     vertex = list(range(num_verts))
 
     for i in range(num_verts):
-        dist[i] = intMax
-    import queue
+        dist[i] = int_max
+
     q = queue.Queue()
     q.put(start_vertex)
     dist[start_vertex] = 0
     while(not q.empty()):
         u = q.get()
-        for iCol in range(offsets[u],offsets[u + 1]):
-            v = indices[iCol]
-            if (dist[v] == intMax):
+        for i_col in range(offsets[u], offsets[u + 1]):
+            v = indices[i_col]
+            if (dist[v] == int_max):
                 dist[v] = dist[u] + 1
                 q.put(v)
+
     return vertex, dist
 
-datasets = ['/datasets/networks/dolphins.mtx',
+
+DATASETS = ['/datasets/networks/dolphins.mtx',
             '/datasets/networks/karate.mtx',
             '/datasets/networks/polbooks.mtx',
             '/datasets/golden_data/graphs/dblp.mtx']
 
-@pytest.mark.parametrize('graph_file', datasets)
+
+@pytest.mark.parametrize('graph_file', DATASETS)
 def test_bfs(graph_file):
+    M = read_mtx_file(graph_file)
+
+    base_vid, base_dist = base_call(M, 0)
+    cugraph_vid, cugraph_dist = cugraph_call(M, 0)
+
+    # Calculating mismatch
 
-    M = ReadMtxFile(graph_file)
-    base_v_id, base_dist = base_Call(M, 0)
-    v_id, dist = cugraph_Call(M, 0)
-
-    assert len(base_dist) == len(dist)
-    for i in range(len(dist)):
-        assert base_v_id[i] == v_id[i]
-        assert base_dist[i] == dist[i]
+    assert len(base_dist) == len(cugraph_dist)
+    for i in range(len(cugraph_dist)):
+        assert base_vid[i] == cugraph_vid[i]
+        assert base_dist[i] == cugraph_dist[i]

python/cugraph/graph/c_graph.pxd

@@ -2,7 +2,7 @@ from libcpp cimport bool
 
 cdef extern from "cudf.h":
 
-    ctypedef enum gdf_error: 
+    ctypedef enum gdf_error:
         pass
 
     ctypedef enum gdf_dtype:
@@ -13,9 +13,9 @@ cdef extern from "cudf.h":
         GDF_INT64,
         GDF_FLOAT32,
         GDF_FLOAT64,
-        GDF_DATE32,     
-        GDF_DATE64,     
-        GDF_TIMESTAMP,  
+        GDF_DATE32,
+        GDF_DATE64,
+        GDF_TIMESTAMP,
         GDF_CATEGORY,
         GDF_STRING,
         N_GDF_TYPES
@@ -24,18 +24,18 @@ cdef extern from "cudf.h":
     ctypedef size_t gdf_size_type
 
     struct gdf_column_:
-        void *data                       
+        void *data
         gdf_valid_type *valid
-        gdf_size_type size             
+        gdf_size_type size
         gdf_dtype dtype
         gdf_size_type null_count
 
     ctypedef gdf_column_ gdf_column
 
-    cdef gdf_error gdf_column_view_augmented(gdf_column *column, 
-                              void *data, 
+    cdef gdf_error gdf_column_view_augmented(gdf_column *column,
+                              void *data,
                               gdf_valid_type *valid,
-                              gdf_size_type size, 
+                              gdf_size_type size,
                               gdf_dtype dtype,
                               gdf_size_type null_count)
 
@@ -60,13 +60,13 @@ cdef extern from "cugraph.h":
         gdf_adj_list *transposedAdjList
 
 
-    cdef gdf_error gdf_edge_list_view(gdf_graph *graph, 
+    cdef gdf_error gdf_edge_list_view(gdf_graph *graph,
                              const gdf_column *source_indices,
                              const gdf_column *destination_indices,
                              const gdf_column *edge_data)
     cdef gdf_error gdf_add_edge_list(gdf_graph *graph)
     cdef gdf_error gdf_delete_edge_list(gdf_graph *graph)
-    cdef gdf_error gdf_adj_list_view (gdf_graph *graph, 
+    cdef gdf_error gdf_adj_list_view (gdf_graph *graph,
                              const gdf_column *offsets,
                              const gdf_column *indices,
                              const gdf_column *edge_data)