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.

Sign up for GitHub

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

Jump to bottom

Add MG support to the C API #2110

Merged
merged 23 commits into from
Mar 16, 2022
Merged

Add MG support to the C API #2110

merged 23 commits into from
Mar 16, 2022

Conversation

ChuckHastings
Copy link
Collaborator

This PR adds multi-GPU support to the C API. The calls were already defined but if multi-GPU was defined they would return errors.

Closes #2091

@ChuckHastings ChuckHastings requested review from a team as code owners March 11, 2022 20:11
@ChuckHastings ChuckHastings requested a review from rlratzel March 11, 2022 20:11
@ChuckHastings ChuckHastings self-assigned this Mar 11, 2022
@ChuckHastings ChuckHastings added 3 - Ready for Review improvement Improvement / enhancement to an existing function breaking Breaking change labels Mar 11, 2022
@ChuckHastings ChuckHastings added this to the 22.04 milestone Mar 11, 2022
@ChuckHastings ChuckHastings mentioned this pull request Mar 11, 2022
Merged
@ChuckHastings ChuckHastings requested a review from a team as a code owner March 11, 2022 23:28
@ChuckHastings
Copy link
Collaborator Author

rerun tests

seunghwak
seunghwak reviewed Mar 14, 2022
View reviewed changes
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.

Except for few minor issues, looks good to me.

cpp/cmake/thirdparty/get_raft.cmake Show resolved Hide resolved
cpp/include/cugraph_c/graph.h Show resolved Hide resolved
cpp/include/cugraph_c/resource_handle.h Outdated Show resolved Hide resolved
cpp/include/cugraph_c/resource_handle.h Outdated Show resolved Hide resolved
cpp/tests/CMakeLists.txt Show resolved Hide resolved
cpp/tests/c_api/mg_create_graph_test.c Show resolved Hide resolved
cpp/tests/c_api/mg_pagerank_test.c

ret_code = cugraph_resource_handle_init_comms(handle, prows, &ret_error);
TEST_ASSERT(result, ret_code == CUGRAPH_SUCCESS, "handle create failed.");
TEST_ASSERT(result, ret_code == CUGRAPH_SUCCESS, cugraph_error_message(ret_error));
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here. We may consider adding a utility function to set prows from p in our C++ main codebase and call that function in MG C++ tests, MG C tests, and python tests.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ultimately we need a more complete design of this portion of the system. @rlratzel and I discussed this a bit. The problem is that in order to properly configure the raft handle in the C API we would need to replicate a bunch of code that RAFT has implemented in Python and have it callable from C.

Our short term solution is to allow the calling program to create and configure a raft handle and then pass it into the C API call. I think the right long term solution is to have RAFT migrate the dynamic configuration of the comms object (which requires doing a dlopen of the appropriate shared object library) from python into C++ so that we can call it from the C API.

Copy link
Contributor

@seunghwak seunghwak Mar 15, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Umm... not sure how all these are related to computing p_rows from p, but I get this is not directly relevant to this PR. We should discuss how to properly compute p_rows (& p_cols) from p, but I agree that this should be a separate discussion.

@rlratzel
Copy link
Contributor

Fix for gmock CI build problem has been merged.

rerun tests

@ChuckHastings ChuckHastings requested a review from a team as a code owner March 15, 2022 19:14
@codecov-commenter
Copy link

codecov-commenter commented Mar 15, 2022 •
edited
Loading

Codecov Report

❗ No coverage uploaded for pull request base (branch-22.04@77b2374). Click here to learn what that means.
The diff coverage is n/a.

❗ Current head 3cfd539 differs from pull request most recent head 25967b7. Consider uploading reports for the commit 25967b7 to get more accurate results

@@               Coverage Diff               @@
##             branch-22.04    #2110   +/-   ##
===============================================
  Coverage                ?   54.98%           
===============================================
  Files                   ?       12           
  Lines                   ?      662           
  Branches                ?        0           
===============================================
  Hits                    ?      364           
  Misses                  ?      298           
  Partials                ?        0

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 77b2374...25967b7. Read the comment docs.

@aschaffer aschaffer mentioned this pull request Mar 15, 2022
Merged
ajschmidt8
ajschmidt8 approved these changes Mar 15, 2022
View reviewed changes
Copy link
Member

@ajschmidt8 ajschmidt8 left a comment

Choose a reason for hiding this comment

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

Approving ops-codeowner file changes

@ChuckHastings
Copy link
Collaborator Author

rerun tests

@rlratzel
Copy link
Contributor

Latest dask-cudf packages are available which should fix a CI failure.

rerun tests

@BradReesWork
Copy link
Member

@gpucibot merge

2 similar comments
@BradReesWork
Copy link
Member

@gpucibot merge

@rlratzel
Copy link
Contributor

@gpucibot merge

@ajschmidt8 ajschmidt8 merged commit 08ab284 into rapidsai:branch-22.04 Mar 16, 2022
@ChuckHastings ChuckHastings deleted the fea_c_api_mg_implementation branch August 4, 2022 18:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cjnolet cjnolet cjnolet left review comments

@seunghwak seunghwak seunghwak approved these changes

@ajschmidt8 ajschmidt8 ajschmidt8 approved these changes

@rlratzel rlratzel rlratzel approved these changes

Assignees

@ChuckHastings ChuckHastings

Labels
breaking Breaking change improvement Improvement / enhancement to an existing function
Projects
None yet
Milestone
22.04
Development

Successfully merging this pull request may close these issues.

[FEA] Add MG support to the C API
7 participants
@ChuckHastings @rlratzel @codecov-commenter @BradReesWork @cjnolet @ajschmidt8 @seunghwak