Update graph partitioning scheme #1443
Conversation
seunghwak
added
2 - In Progress
DO NOT MERGE
…oning scheme, there can be more than one partition per GPU
seunghwak
changed the title
Codecov Report
@@ Coverage Diff @@
## branch-0.19 #1443 +/- ##
===============================================
+ Coverage 58.24% 60.26% +2.01%
===============================================
Files 71 70 -1
Lines 3281 3153 -128
===============================================
- Hits 1911 1900 -11
+ Misses 1370 1253 -117
Continue to review full report at Codecov.
|
|
rerun tests |
| row_comm.sync_stream(handle.get_stream()); // this is neessary as local_degrees will become | ||
| // out-of-scope once this function returns. | ||
| } | ||
| // FIXME: is this necessary? |
There was a problem hiding this comment.
The comments here are puzzling. The FIXME questions if this is necessary. The comments describe why this is necessary. Is the FIXME erroneous (as explained by the other comment), or is the FIXME challenging the assertion in the comment below about local_degrees going out-of-scope.
It is certainly true that local_degrees will go out of scope. Because the comms operations are asynchronous, it seems like the assertion in the comment is likely true.
| @@ -334,8 +309,6 @@ class graph_view_t<vertex_t, | |||
| bool sorted_by_global_degree_within_vertex_partition, | |||
| bool do_expensive_check = false); | |||
|
|
|||
| bool is_weighted() const { return adj_matrix_partition_weights_.size() > 0; } | |||
|
|
|||
| // FIXME: this should be removed once MNMG Louvain is updated to use graph primitives | |||
| partition_t<vertex_t> get_partition() const { return partition_; } | |||
There was a problem hiding this comment.
Should this be removed now? Or is it needed elsewhere and the FIXME is out of date or no longer required?
There was a problem hiding this comment.
Yes, this should be removed, and I will remove this.
There was a problem hiding this comment.
Lots of code in there. It's great but a bit challenging to review. Moving forward it would be better to have a finer PR granularity and a high-level description of the concrete steps taken to improve a given feature. We also need to start measuring improvements when it comes to performance and scalability optimizations.
Yeah... sorry for creating a huge PR. This was somewhat unavoidable as this changes the fundamental underlying data structure and multiple places get affected by this change. And I should fix bugs in multiple places to make this PR work. Yeah... but next time, I will try to plan ahead to avoid creating a huge PR; I will encounter a similar challenge when I need to bring DCSR/DCSC, and I will think about bringing this in multiple steps. |
|
I think I addressed all the comments and let me know if you want me to make additional changes. Once this PR gets merged, PR #1447 will become reviewable. |
|
rerun tests |
|
@gpucibot merge |
Partially addresses Issue #1442
Update graph partitioning scheme to better control memory footprint vs concurrency trade-offs for large-scale graph processing in large clusters. This new partitioning scheme also simplifies communication patterns among GPUs which can potentially improve scalability.