Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ENH] Improve how we check for MG path at the C++ level #995

Closed
afender opened this issue Jul 14, 2020 · 4 comments
Closed

[ENH] Improve how we check for MG path at the C++ level #995

afender opened this issue Jul 14, 2020 · 4 comments
Assignees
Milestone

Comments

@afender
Copy link
Member

afender commented Jul 14, 2020

In several places we are using comms initialization as a way to specify whether to take the MG path or not:
if (handle.comms_initialized()) {

We need a better way to separate MG distributed from MG batch other than checking the handle communicator. We should also keep consolidation in mind. There are several things we should do :

  1. Add a batch parameter at the C++ API level, this is something we could leverage.
  2. template parameter to enables compile-time resolution of multi-GPU vs single-GPU.
template <bool multi_gpu = false, ...>
class Graph {
  static constexpr bool is_multi_gpu = multi_gpu;
  ...
};
  1. When a distributed graph is constructed we fill some specific structures. We should also set an "is_distributed" status in the graph that analytics can check this.

I think we should add both 1. and 2. first and consider 3. if 2. has complications at the python layer

@afender afender added this to the 0.15 milestone Jul 14, 2020
@afender afender added the OPG label Jul 15, 2020
@afender afender added this to P0 in v0.15 Release Jul 15, 2020
@BradReesWork BradReesWork removed this from P0 in v0.15 Release Aug 7, 2020
@BradReesWork BradReesWork modified the milestones: 0.15, 0.16 Aug 7, 2020
@BradReesWork
Copy link
Member

moving to 0.16 and the big push for better MG support

@BradReesWork BradReesWork added this to P1 in v0.16 Release Aug 7, 2020
@BradReesWork BradReesWork moved this from P1 to P0 in v0.16 Release Aug 24, 2020
@rlratzel rlratzel self-assigned this Aug 25, 2020
@seunghwak
Copy link
Contributor

I think this issue is obsolete now.

This is relevant to only 1D BFS & PageRank, but those will retire by the end of 0.16.

Addressing this requires updating the old graph class and can require sizable amount of effort, and I don't think this is really worth the effort.

This is already part of the new graph class and the pattern accelerator API based BFS & PageRank implementations.

@rlratzel
Copy link
Contributor

To summarize the effort so far, I added the bool template param to the graph classes in C++, but passing a non-type template arg in Cython was causing problems. I tried using a workaround described here, which made Cython happy, but the resulting C++ code looked suspicious and didn't compile, so I don't know if I used it correctly (still looking into that). Here's the generated code from the workaround for anyone interested:

/* "cugraph/structure/graph_new.pxd":24
 * from rmm._lib.device_buffer cimport device_buffer
 *
 * ctypedef bool falseParamVal "false"             # <<<<<<<<<<<<<<
 * ctypedef bool trueParamVal "true"
 *
 */
typedef bool false;

@Iroy30 pointed me to a thread with some additional ideas here, which I have yet to try. Unfortunately it appears that formal support for non-type template args in Cython still hasn't been added yet, based on this still-open PR

Another consideration is that since this is an additional template arg to the graph classes, it means that every Cython file will need to be updated, making it a fairly intrusive PR. I think that's normally okay, but as @seunghwak mentioned, it might be outside the intended scope of the issue if we already have it addressed in a new pattern accelerator-based approach to be applied soon.

@BradReesWork
Copy link
Member

This issue is really about the old 1D structure and is now OBE. Closing

v0.16 Release automation moved this from P0 to Done Sep 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
v0.16 Release
  
Done
Development

No branches or pull requests

4 participants