Add SSSP API, test and implementation #2016

ChuckHastings · 2022-01-18T17:50:43Z

@rlratzel needed the SSSP C API squeezed into version 22.02. Skipped the usual API PR and implementation PR to squeeze these together in the interest of time. Note that SSSP is nearly identical (semantically) to BFS which is already in the C API using a similar API.

This PR provides:

C API definition of SSSP
C unit test for SSSP
Implementation of the C API of SSSP (calling the underlying C++ implementation)

codecov-commenter · 2022-01-18T20:42:07Z

Codecov Report

Merging #2016 (6764ad5) into branch-22.02 (c92087b) will increase coverage by 0.59%.
The diff coverage is 100.00%.

❗ Current head 6764ad5 differs from pull request most recent head 031ecd8. Consider uploading reports for the commit 031ecd8 to get more accurate results

@@               Coverage Diff                @@
##           branch-22.02    #2016      +/-   ##
================================================
+ Coverage         70.16%   70.75%   +0.59%     
================================================
  Files               142      142              
  Lines              8671     8861     +190     
================================================
+ Hits               6084     6270     +186     
- Misses             2587     2591       +4

Impacted Files	Coverage Δ
python/cugraph/cugraph/utilities/utils.py	`70.37% <100.00%> (+0.18%)`	⬆️
python/cugraph/cugraph/tests/test_ecg.py	`100.00% <0.00%> (ø)`
python/cugraph/cugraph/tests/test_paths.py	`100.00% <0.00%> (ø)`
python/cugraph/cugraph/tests/test_egonet.py	`100.00% <0.00%> (ø)`
python/pylibcugraph/pylibcugraph/_version.py	`0.00% <0.00%> (ø)`
python/cugraph/cugraph/tests/test_renumber.py	`100.00% <0.00%> (ø)`
python/cugraph/cugraph/tests/test_wjaccard.py	`100.00% <0.00%> (ø)`
python/cugraph/cugraph/tests/test_hungarian.py	`100.00% <0.00%> (ø)`
python/cugraph/cugraph/tests/test_wsorensen.py	`100.00% <0.00%> (ø)`
python/cugraph/cugraph/tests/dask/mg_context.py	`0.00% <0.00%> (ø)`
... and 50 more

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 c92087b...031ecd8. Read the comment docs.

rlratzel · 2022-01-19T16:58:19Z

Looks like there was an undetected CI failure with the new SSSP C API:

Full log is here
The TEST_ASSERT macro used by the C API tests should also be updated to properly pass along the failure.

… match Knuth

seunghwak

Looks good to me in general...

Some minor cosmetic issues to consider.

seunghwak · 2022-01-19T19:04:07Z

cpp/include/cugraph_c/algorithms.h

+ * @return error code
+ */
+cugraph_error_code_t cugraph_sssp(const cugraph_resource_handle_t* handle,
+                                  cugraph_graph_t* graph,


Can't graph here be const?

C api will transpose the graph if the input is opposite from what the algorithm requires. This will modify the graph.

C api will transpose the graph if the input is opposite from what the algorithm requires. This will modify the graph.

Would it be better to set/return an error in that case? I'm envisioning a case where we or a user has a bug that isn't transposing data if an algo requires it, it gets silently transposed anyway, then we wonder why our benchmarks are slower than expected.

Fair question. I think we've discussed this (or else I had this argument with myself... entirely possible).

From an API perspective, we have two choices:

Do what the user is asking us to do regardless of the performance ramifications. The documentation should provide sufficient detail for the user to correct their bad choices.

Require the user to understand the performance ramifications and make all of the right calls explicitly

The C++ layer has always opted for the latter and I believe it should. Thus far I have opted for the former in the C API. Now that @rlratzel is building the python layer it would make sense for us to have this conversation. Perhaps the python layer should worry about providing the intelligence and the C layer should (like the C++ layer) simply fail fast if the input is in the wrong orientation.

That would be an easy change, although I think it should not be rushed into this release.

seunghwak · 2022-01-19T19:05:13Z

cpp/include/cugraph_c/algorithms.h

+                                  cugraph_graph_t* graph,
+                                  size_t source,
+                                  double cutoff,
+                                  bool_t do_expensive_check,


In the C++ API, do_expensive_check is the last parameter, any reason to deviate from this convention?

I missed that when we did bfs :-(. The fact that we can't default parameters in the C API made me a little careless on this issue.

The current C API "convention" has been to mimic the input and have the last two parameters be the return type and the error code. I can swap do_expensive_check and compute_predecessors to make that more consistent. In the C++ API, compute_predecessors is not a necessary parameter since we pass in the predecessors array and pass in a null pointer if we don't want to compute predecessors.

Swapped these in latest push.

ChuckHastings · 2022-01-20T16:59:25Z

rerun tests

rlratzel · 2022-01-20T18:13:57Z

cpp/include/cugraph_c/algorithms.h

+ * @return error code
+ */
+cugraph_error_code_t cugraph_sssp(const cugraph_resource_handle_t* handle,
+                                  cugraph_graph_t* graph,


C api will transpose the graph if the input is opposite from what the algorithm requires. This will modify the graph.

Would it be better to set/return an error in that case? I'm envisioning a case where we or a user has a bug that isn't transposing data if an algo requires it, it gets silently transposed anyway, then we wonder why our benchmarks are slower than expected.

rlratzel · 2022-01-20T18:23:29Z

cpp/tests/c_api/c_test_utils.h

+    if (!(RETURN_VALUE)) {                                                 \
+      (RETURN_VALUE) = !(STATEMENT);                                       \
+      if ((RETURN_VALUE)) { printf("ASSERTION FAILED: %s\n", (MESSAGE)); } \
+    }                                                                      \


This is better than before since it won't clobber a prior failing return value, but maybe it's worthwhile to see all failing tests instead of just the first one. Would this work for that?

Suggested change

if (!(RETURN_VALUE)) { \

(RETURN_VALUE) = !(STATEMENT); \

if ((RETURN_VALUE)) { printf("ASSERTION FAILED: %s\n", (MESSAGE)); } \

} \

if (!(STATEMENT)) { \

printf("ASSERTION FAILED: %s\n", (MESSAGE)); \

(RETURN_VALUE) |= 1; \

} \

Maybe. Isn't the gtest convention to bail out of a particular test at the first failure and move on to the next test? For a larger test data set you could get thousands of errors from a particular test which is probably not desired.

gtest supports either behavior, based on if you use the EXPECT_* or ASSERT_* assertions, where EXPECT will fail the test but continue running it to see everything (the behavior I described), and ASSERT will cause it to fail and return immediately. I wasn't thinking we'd have that many failures typically, but I don't feel strongly either way. Since this is named TEST_ASSERT, I guess the current behavior is indeed closer to gtest's ASSERT_*.

If you wanted to follow the gtest convention further, you could also have two different versions of the macros with the above names. Just another idea. My main concern was addressed (failures not reflected in the binary's exit code), so feel free to ignore this suggestion if you'd rather keep the current behavior.

BradReesWork · 2022-01-20T21:45:39Z

rerun tests

BradReesWork · 2022-01-21T18:58:45Z

rerun tests

ChuckHastings · 2022-01-22T05:16:05Z

@gpucibot merge

add SSSP API, test and implementation

895eba3

ChuckHastings requested review from a team as code owners January 18, 2022 17:50

ChuckHastings self-assigned this Jan 18, 2022

ChuckHastings added 3 - Ready for Review feature request New feature or request non-breaking Non-breaking change labels Jan 18, 2022

ChuckHastings added this to the 22.02 milestone Jan 18, 2022

ChuckHastings added 3 commits January 18, 2022 12:52

update copyright years

7d8bb7c

fix clang-format issues

edf272f

missed implementation file :-)

148df0d

BradReesWork requested review from rlratzel and seunghwak January 19, 2022 18:36

BradReesWork approved these changes Jan 19, 2022

View reviewed changes

ChuckHastings added 4 commits January 19, 2022 14:13

update C API testing to handle multiple asserts without losing failure

c3bc393

use nearlyEqual since SSSP distances are floats... fix nearlyEqual to…

c93ddd4

… match Knuth

fix clang-format issues

303390a

revert to a failing test to validate CI

cfa35b1

seunghwak approved these changes Jan 19, 2022

View reviewed changes

a few edits based on PR comments

b48637e

update copyright on bfs.cpp

5fee4b8

rlratzel reviewed Jan 20, 2022

View reviewed changes

ChuckHastings added 2 commits January 21, 2022 00:30

fix BFS and SSSP parameter order, flip SSSP test back to passing

e6cfaeb

fix copyright

354ba8a

flipped parameters... forgot to fix in this test

031ecd8

ChuckHastings mentioned this pull request Jan 22, 2022

Mods for pylibcugraph #2030

Closed

rlratzel approved these changes Jan 22, 2022

View reviewed changes

rapids-bot bot merged commit f4b5f3d into rapidsai:branch-22.02 Jan 22, 2022

ChuckHastings deleted the add_sssp_to_c_api branch February 1, 2022 16:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add SSSP API, test and implementation #2016

Add SSSP API, test and implementation #2016

ChuckHastings commented Jan 18, 2022

codecov-commenter commented Jan 18, 2022 •

edited

Loading

rlratzel commented Jan 19, 2022

seunghwak left a comment

seunghwak Jan 19, 2022

ChuckHastings Jan 20, 2022

rlratzel Jan 20, 2022 •

edited

Loading

ChuckHastings Jan 20, 2022 •

edited

Loading

seunghwak Jan 19, 2022

ChuckHastings Jan 20, 2022

ChuckHastings Jan 20, 2022

ChuckHastings commented Jan 20, 2022

rlratzel Jan 20, 2022 •

edited

Loading

rlratzel Jan 20, 2022

ChuckHastings Jan 20, 2022

rlratzel Jan 21, 2022

BradReesWork commented Jan 20, 2022

BradReesWork commented Jan 21, 2022

ChuckHastings commented Jan 22, 2022

Add SSSP API, test and implementation #2016

Add SSSP API, test and implementation #2016

Conversation

ChuckHastings commented Jan 18, 2022

codecov-commenter commented Jan 18, 2022 • edited Loading

Codecov Report

rlratzel commented Jan 19, 2022

seunghwak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rlratzel Jan 20, 2022 • edited Loading

Choose a reason for hiding this comment

ChuckHastings Jan 20, 2022 • edited Loading

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

ChuckHastings commented Jan 20, 2022

rlratzel Jan 20, 2022 • edited Loading

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 Jan 20, 2022

BradReesWork commented Jan 21, 2022

ChuckHastings commented Jan 22, 2022

codecov-commenter commented Jan 18, 2022 •

edited

Loading

rlratzel Jan 20, 2022 •

edited

Loading

ChuckHastings Jan 20, 2022 •

edited

Loading

rlratzel Jan 20, 2022 •

edited

Loading