Initial cuGraph C API - biased RW, C tests, script updates, cmake files, C library helpers #1799

aschaffer · 2021-08-31T20:13:59Z

Initial cuGraph C API:

C-API for Biased RW and supporting C library helpers.
cmake updates to build the new object files and library using the appropriate compiler (g++ or gcc) accordingly.
C tests written without gtests to ensure a pure-C application build works.
updated CI scripts to build and run the new tests.

The Python API for the updated Biased RW will be done in a separate PR.

Merge latest release 0.17

Merge latest branch-0.18

Update forked branch-0.18

Update forked branch-0.18 from release

Update branch-0.19 from release

update forked from release branch-0.19

Merge from release branch-0.19

Merge latest rapidsai:branch-0.19 into aschaffer:branch-0.19

Create forked branch-0.20

merge latest release branch-0.20

Merge latest rapidsai/branch-0.20 into forked branch-0.20

aschaffer/21.08 <- rapidsai/21.08

…xt_binding_rw_biased

…to fea_ext_binding_rw_biased

…xt_binding_rw_biased

codecov-commenter · 2021-09-02T01:32:52Z

Codecov Report

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

❗ Current head a56fed6 differs from pull request most recent head bbadef7. Consider uploading reports for the commit bbadef7 to get more accurate results

@@               Coverage Diff               @@
##             branch-21.12    #1799   +/-   ##
===============================================
  Coverage                ?   70.10%           
===============================================
  Files                   ?      143           
  Lines                   ?     8827           
  Branches                ?        0           
===============================================
  Hits                    ?     6188           
  Misses                  ?     2639           
  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 1313e34...bbadef7. Read the comment docs.

…itor code flow for RW visitor.

seunghwak · 2021-09-08T20:14:53Z

cpp/CMakeLists.txt

@@ -205,6 +205,7 @@ add_library(cugraph SHARED
    src/visitors/graph_envelope.cpp
    src/visitors/visitors_factory.cpp
    src/visitors/bfs_visitor.cpp
+    src/visitors/rw_visitor.cpp


So, do we plan to place bindings related files under the cugrpah/cpp directory? Or we'd better separate the core libcugraph from the bindings related files? (i.e. placing bindings related files under something like cugraph/bindings)

The visitor implementation is not restricted to bindings only. It's meant to be (and it is) used for implementation purposes, too. These are cpp files. There will be a separate branch for the C-only API.

Then, what about creating a separate directory under the cpp (similar to tests)? Is this directly relevant to libcugraph users? (someone willing to directly use our C++ API). If not, I am not sure it is a good idea to include this in our core libcugraph_(sg|mg).so.

Yes, it is. As you probably know Visitor is useful to enrich the functionality of an object (visitee) without having to add new methods to it, which may inflate the code and cause merge conflicts on high traffic headers, like graph*.hpp .

The visitor abstraction is being used in RW, for example, for exactly such reasons.

Also, this structuring of this code has been in place for a while. And it has been extensively discussed before-hand (at least 2 presentations and demos have been done). Constantly shuffling code around after the fact is counterproductive, confusing, and wasteful. If new code restructuring is in the works, then this is not the PR for it. This PR has different goals. We've already agreed on small, targeted PRs as opposed to all-inclusive major code restructuring ones.

There's also the question of what exactly is the criterion for code cut-off to be accepted in cugraph. If this is to be discussed, then, again, maybe this is something to discuss before-fact, not after. I am starting to get the feeling that it's increasingly hard to get code accepted in cugraph and a lot of effort is potentially wasted with code shuffling and reshuffling on what appears to be quite ad-hoc and last-moment criteria.

rlratzel · 2021-09-28T18:41:11Z

cpp/tests/c_api/create_sg_graph_test.c

+   */
+  return 0;
+
+  G = cugraph_make_sg_graph(&handle,


I just realized I named the test and test file with the word "create" instead of "make". Similar to Chuck's comment re: func and file names matching, I'd make these match too (either change the API to "create" or the file and test to "make").

rlratzel · 2021-09-28T18:44:21Z

cpp/tests/c_api/random_walks_test.c

+
+  return (count_passed == num_paths);
+}
+int test_random_walks_1()


You may not be finished updating this test, but just so you don't forget: this was just meant to be a placeholder name - I'd change it to something more descriptive so the user can identify what failed from the test output.

rlratzel · 2021-09-28T20:46:44Z

rerun tests

…ates of the C-API.

rlratzel

Still reviewing but a few things jumped out (I've only looked at the tests) so I'll post what I have so far.
My biggest concern is that it looks like some tests are hardcoded to return success, which should be easy to fix so that CI will actually catch problems.

rlratzel · 2021-09-29T18:08:41Z

cpp/src/c_api/cugraph_api.cpp

+int data_type_sz[] = {4, 8, 4, 8};
+}
+
+bool_t runtime_assert(bool_t statement_truth_value, const char* error_msg)


I think the name "assert" here is misleading. When reading code, I expect execution to stop when a false value is passed to this function.

I don't necessarily agree, but you're free to change that.

rlratzel · 2021-09-29T18:09:52Z

cpp/tests/c_api/random_walks_test.c

+  cugraph_free_handle(p_handle);
+
+  /* because bools and err return codes from programs have opposite meaning...*/
+  if (flag_failed)


I think this is still inverted: tests need to return 0 on success, but this will return 0 if flag_failed is true, meaning it's a failure? Maybe I'm confused...

edit: oh, this flag is only used for the check above which must "fail". I would possibly rename the variable, or add something to the comment, or change the name of the test, or all of the above.

It's correct. This is a negative test.

rlratzel · 2021-09-29T18:10:28Z

cpp/tests/c_api/random_walks_test.c

+}
+
+/* negative test RW call flow*/
+int test_random_walks_3()


this tests and others still using the placeholder name need to have a more descriptive name.

You provided these names. You're free to change them in the next release.

rlratzel · 2021-09-29T18:13:32Z

cpp/tests/c_api/random_walks_test.c

+  31, 32, 34, 312, 3124, 3125, 31246, 31256, 324, 325,  3246, 3256, 346, 46,  56};
+
+/* linear search of `value` inside `p_cmprsd_path[max_num_paths]`*/
+bool_t is_one_of(int32_t value, int32_t* p_cmprsd_path, int max_num_paths)


I think some of these abbreviations are harder to type and read than the non-abbreviated words. Can this just say "compressed" instead of "cmprsd" (assuming that's what it's abbreviating)?

Again, question of style/taste. You're free to change them in the next release.

rlratzel · 2021-09-29T18:29:50Z

cpp/tests/c_api/random_walks_test.c

+
+  cugraph_free_handle(p_handle);
+
+  return 0;


Because the asserts don't fail the test and their return values aren't being captured and checked, this test will always pass, and therefore CI will not catch anything.

Yes, this is just debug info. We can modify this in the next release.

rlratzel · 2021-09-29T18:30:27Z

cpp/tests/c_api/random_walks_test.c

+  vertex_t h_start[]                        = {0, 2};
+
+  p_handle = cugraph_create_handle();
+  runtime_assert(p_handle != NULL, "raft handle creation failed.");


since this isn't a real assert (the implementation I saw above seems to just print to stderr), the final return value of the test needs to reflect if this fails, but it's just being ignored. Same for all other runtime_assert calls that aren't setting a flag of some sort.

We can modify this in the next release.

cpp/tests/c_api/create_sg_graph_test.c

BradReesWork · 2021-10-01T14:17:01Z

rerun tests

ChuckHastings

Created #1867 to remind us to address the review feedback that is being deferred.

…xt_binding_rw_biased

rlratzel

Approving now that #1867 has been created.

ChuckHastings · 2021-10-04T22:10:48Z

rerun tests

ChuckHastings · 2021-10-07T04:35:51Z

rerun tests

…ding_rw_biased

BradReesWork · 2021-10-07T22:10:24Z

@gpucibot merge

aschaffer and others added 20 commits November 30, 2020 15:00

Merge pull request #37 from rapidsai/branch-0.17

acf15bc

Merge latest release 0.17

Merge pull request #38 from rapidsai/branch-0.18

a584e0b

Merge latest branch-0.18

Merge pull request #39 from rapidsai/branch-0.18

338b2d4

Update forked branch-0.18

Merge pull request #40 from rapidsai/branch-0.18

efd7e9a

Update forked branch-0.18 from release

Merge pull request #41 from rapidsai/branch-0.19

a23ce0d

Update branch-0.19 from release

Merge pull request #42 from rapidsai/branch-0.19

22b32d8

update forked from release branch-0.19

Merge pull request #43 from rapidsai/branch-0.19

e5042fc

Merge from release branch-0.19

Merge pull request #44 from rapidsai/branch-0.19

ca02ee4

Merge latest rapidsai:branch-0.19 into aschaffer:branch-0.19

Merge pull request #45 from rapidsai/branch-0.20

79409e7

Create forked branch-0.20

Merge pull request #46 from rapidsai/branch-0.20

360c293

merge latest release branch-0.20

Merge pull request #47 from rapidsai/branch-0.20

e57f261

Merge latest rapidsai/branch-0.20 into forked branch-0.20

Merge pull request #48 from rapidsai/branch-21.08

a63cdea

aschaffer/21.08 <- rapidsai/21.08

Merge branch 'rapidsai:branch-21.08' into branch-21.08

7e6bf4c

Merge branch 'rapidsai:branch-21.08' into branch-21.08

6f3b342

Merge branch 'rapidsai:branch-21.10' into branch-21.10

fbf5f36

Merge branch 'rapidsai:branch-21.10' into branch-21.10

ffacad0

Merge branch 'branch-21.10' of github.com:rapidsai/cugraph into fea_e…

beaf0fb

…xt_binding_rw_biased

Merge branch 'branch-21.10' of https://github.com/rapidsai/cugraph in…

3b3131e

…to fea_ext_binding_rw_biased

Merge branch 'branch-21.10' of github.com:rapidsai/cugraph into fea_e…

16a998f

…xt_binding_rw_biased

Prepare extended API for RW.

03a2055

aschaffer requested a review from a team as a code owner August 31, 2021 20:14

Merge branch 'branch-21.10' of github.com:rapidsai/cugraph into fea_e…

b4fe88a

…xt_binding_rw_biased

aschaffer added 2 commits September 2, 2021 18:40

Preliminary RW type-erasure. Needs work.

b7b87a2

Fixed typed-erased return to work with move-only semantics. Fixed vis…

af0e21b

…itor code flow for RW visitor.

seunghwak reviewed Sep 8, 2021

View reviewed changes

RW visitor test.

08a6ee7

aschaffer requested a review from a team as a code owner September 8, 2021 20:48

Changed order of header inclusion to fix problematic warning.

0336827

rlratzel reviewed Sep 28, 2021

View reviewed changes

Fix#1 for graph_envelope factory crash.

1e17b33

aschaffer added 7 commits September 28, 2021 16:48

Fixed graph_envelope creation crash.

f9f1006

Clean-up.

b8dd310

Fixed some C-API inconsistencies.

bb5be4d

Added a graph_envelope_t unwrapper to be called from within C++ deleg…

2f5b4ee

…ates of the C-API.

Enabled negative tests on C-API graph creation.

a59402b

Fixed graph_envelope unpacking in C-API RW wrapper.

b82e006

RW back to back tests.

a78f678

rlratzel requested changes Sep 29, 2021

View reviewed changes

BradReesWork added this to the 21.10 milestone Sep 29, 2021

aschaffer added 2 commits September 29, 2021 15:32

Fixed one no-op test to actually test.

c64e235

Fixed failure handling in RW wrapper.

84c2377

BradReesWork modified the milestones: 21.10, 21.12 Sep 30, 2021

BradReesWork changed the base branch from branch-21.10 to branch-21.12 September 30, 2021 14:43

ChuckHastings approved these changes Oct 1, 2021

View reviewed changes

Merge branch 'branch-21.12' of github.com:rapidsai/cugraph into fea_e…

bbadef7

…xt_binding_rw_biased

rlratzel approved these changes Oct 1, 2021

View reviewed changes

Merge remote-tracking branch 'upstream/branch-21.12' into fea_ext_bin…

91251b5

…ding_rw_biased

rapids-bot bot merged commit 50158c6 into rapidsai:branch-21.12 Oct 7, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial cuGraph C API - biased RW, C tests, script updates, cmake files, C library helpers #1799

Initial cuGraph C API - biased RW, C tests, script updates, cmake files, C library helpers #1799

aschaffer commented Aug 31, 2021 •

edited by rlratzel

Loading

codecov-commenter commented Sep 2, 2021 •

edited

Loading

seunghwak Sep 8, 2021

aschaffer Sep 9, 2021

seunghwak Sep 9, 2021

aschaffer Sep 9, 2021

rlratzel Sep 28, 2021

rlratzel Sep 28, 2021

rlratzel commented Sep 28, 2021

rlratzel left a comment

rlratzel Sep 29, 2021

aschaffer Sep 30, 2021

rlratzel Sep 29, 2021

aschaffer Sep 30, 2021

rlratzel Sep 29, 2021

aschaffer Sep 30, 2021

rlratzel Sep 29, 2021

aschaffer Sep 30, 2021

rlratzel Sep 29, 2021

aschaffer Sep 30, 2021

rlratzel Sep 29, 2021

aschaffer Sep 30, 2021

BradReesWork commented Oct 1, 2021

ChuckHastings left a comment

rlratzel left a comment

ChuckHastings commented Oct 4, 2021

ChuckHastings commented Oct 7, 2021

BradReesWork commented Oct 7, 2021

Initial cuGraph C API - biased RW, C tests, script updates, cmake files, C library helpers #1799

Initial cuGraph C API - biased RW, C tests, script updates, cmake files, C library helpers #1799

Conversation

aschaffer commented Aug 31, 2021 • edited by rlratzel Loading

codecov-commenter commented Sep 2, 2021 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rlratzel commented Sep 28, 2021

rlratzel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BradReesWork commented Oct 1, 2021

ChuckHastings left a comment

Choose a reason for hiding this comment

rlratzel left a comment

Choose a reason for hiding this comment

ChuckHastings commented Oct 4, 2021

ChuckHastings commented Oct 7, 2021

BradReesWork commented Oct 7, 2021

aschaffer commented Aug 31, 2021 •

edited by rlratzel

Loading

codecov-commenter commented Sep 2, 2021 •

edited

Loading