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

Define and implement C API for RMAT generators #3285

Merged

Conversation

ChuckHastings
Copy link
Collaborator

@ChuckHastings ChuckHastings commented Feb 14, 2023

To get rid of cython.cu we need to be able add access to the RMAT generators through the C API.

This PR will define the C API, C unit tests and stub out the implementations so they can be reviewed.

Closes #2816

@ChuckHastings ChuckHastings self-assigned this Feb 14, 2023
@ChuckHastings ChuckHastings added improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Feb 14, 2023
@ChuckHastings ChuckHastings added this to the 23.04 milestone Feb 14, 2023
Copy link
Contributor

@seunghwak seunghwak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ChuckHastings ChuckHastings marked this pull request as ready for review March 1, 2023 17:52
@ChuckHastings ChuckHastings requested review from a team as code owners March 1, 2023 17:52
@ChuckHastings ChuckHastings changed the title Define C API for RMAT generators Define and implement C API for RMAT generators Mar 1, 2023
Copy link
Contributor

@seunghwak seunghwak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@rlratzel rlratzel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor change requests and some questions.

cpp/include/cugraph/graph_generators.hpp Outdated Show resolved Hide resolved
cpp/include/cugraph/graph_generators.hpp Show resolved Hide resolved
cugraph_error_t** error);

/**
* @brief Add edge ids to an RMAT COO
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it have to be an RMAT COO?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure the context of your question. The intention of this function is to add edge ids to the output of RMAT generator, which is an RMAT COO.

If your question is does the RMAT generator need to generate a COO, I think that's the easiest path. In SG you could imagine generating a CSR. But MG doesn't really use CSR, at least in the traditional sense. COO and MG COO are the common format that can be consistently used.

If your question is does this function need to have an RMAT COO, or could it operate on other inputs, yes it could. I chose the RMAT COO since it is the output from the RMAT generator, so it makes the API simple. The input information comes from the RMAT COO and the output information is added to the RMAT COO. However, generating these really only uses the size of the src array. We could either modify this function to just take (on each GPU) the number of local edges and return a device array, or we could add an additional C API function that operated on the number of local edges.

I'm inclined to the latter approach, since the C API generally has a pattern of:

  • Generate an output
  • Manipulate that output
  • Free that output

Making this a lower level function would require the caller to extract things from the original output, and would create a sequence of things (the COO, the edge ids, the edge list, the edge weights) that all need to be freed at the end.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does this function need to have an RMAT COO, or could it operate on other inputs,

This is closer to what I was wondering. IOW, the COO is a cugraph_coo_t and adding edge IDs and types to a graph seems generally useful outside of an RMAT call, so it wasn't obvious why this was tied to RMAT. I was expecting the type to be named cugraph_rmat_coo_t if it was intended to be used only for RMAT, but I suppose that's more of a comment on the type name.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't strictly have to be a cugraph_coo_t generated from the RMAT generator. When we get around to exposing other graph generators, they will each generate a cugraph_coo_t and these functions will be equally applicable.

At the moment, the only cugraph_coo_t is from the RMAT generator, and I don't have other generators on the road map yet. So the comment is correct for now. We probably will want to update the documentation when we add generators that aren't RMAT based.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed RMAT from the documentation block (since I had to merge the latest code and rerun through CI anyway).

cugraph_error_t** error);

/**
* @brief Generate random edge types, add them to an RMAT COO
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same question here: does it have to be an RMAT COO?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See above answer

cpp/include/cugraph/graph_generators.hpp Show resolved Hide resolved
Co-authored-by: Rick Ratzel <3039903+rlratzel@users.noreply.github.com>
@ChuckHastings
Copy link
Collaborator Author

/merge

@rapids-bot rapids-bot bot merged commit 60bd0eb into rapidsai:branch-23.04 Mar 23, 2023
@seunghwak seunghwak mentioned this pull request Mar 24, 2023
rapids-bot bot pushed a commit that referenced this pull request Mar 31, 2023
* Generate a SG test graph from the MG graph (instead of re-creating assuming that the underlying random number generator output is deterministic for the same seed).
* Add a utility function to collect MG results to compare with SG results.

Additional bug fix
* Recent PR (#3285) mistakenly updated the code to use the same seed for every GPU. Fix this.
f86c2c5

Authors:
  - Seunghwa Kang (https://github.com/seunghwak)

Approvers:
  - Naim (https://github.com/naimnv)
  - Joseph Nke (https://github.com/jnke2016)
  - Chuck Hastings (https://github.com/ChuckHastings)

URL: #3371
@ChuckHastings ChuckHastings deleted the c_api_for_rmat_generators branch September 27, 2023 21:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement Improvement / enhancement to an existing function non-breaking Non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants