Skip to content

[REVIEW] BUG Calling gunrock cmake using explicit -D options, re-enabling C++ tests - #1242

Merged
BradReesWork merged 2 commits into
rapidsai:branch-0.17from
rlratzel:branch-0.17-cmakegunrockfix
Oct 23, 2020
Merged

[REVIEW] BUG Calling gunrock cmake using explicit -D options, re-enabling C++ tests#1242
BradReesWork merged 2 commits into
rapidsai:branch-0.17from
rlratzel:branch-0.17-cmakegunrockfix

Conversation

@rlratzel

Copy link
Copy Markdown
Contributor
  • Changes to call gunrock cmake using explicit -D options for each arch, similar to a prior version of the CMakeLists.txt, since the update to expand a variable into the arch options was being expanded as a single quoted value with spaces that wasn't recognized, resulting in gunrock being built for only compute 60 support. This caused C++ test crashes.
  • Re-enabled C++ tests.

This was tested by building cugraph with this change and running the Pagerank C++ test 1000 times without failures/crashes. Prior to this, the Pagerank C++ test (and others) would crash after 1-50 test run iterations.

The issue and corresponding fix was verified with a standalone reproducer, which can be seen here: https://github.com/rlratzel/cugraph_build_problem

…s for each arch, similar to a prior version of the CMakeLists.txt, since the update to expand a variable into the arch options was being expanded as a single quoted value with spaces that wasn't recognized, resulting in gunrock being built for only compute 60 support, which caused C++ test crashes. Also re-enabled C++ tests.
@rlratzel
rlratzel requested a review from afender October 23, 2020 15:30
@rlratzel
rlratzel requested review from a team as code owners October 23, 2020 15:30
@rlratzel rlratzel self-assigned this Oct 23, 2020
@GPUtester

Copy link
Copy Markdown
Contributor

Please update the changelog in order to start CI tests.

View the gpuCI docs here.

@ajschmidt8

Copy link
Copy Markdown
Member

@rlratzel, nice job. I created an integration PR to revert these changes in that repo as well. Please let me know if that PR can be merged with this one.

rapidsai/integration#163

@seunghwak

Copy link
Copy Markdown
Contributor

Wow, so if I am not mistaken, gunrock (built for an outdated architecture) affected the cugraph pagerank/SSSP behavior outside gunrock? Very surprising and great catch!

@rlratzel

Copy link
Copy Markdown
Contributor Author

Wow, so if I am not mistaken, gunrock (built for an outdated architecture) affected the cugraph pagerank/SSSP behavior outside gunrock? Very surprising and great catch!

I admittedly don't fully understand why, but when the test binary loads a .so with gunrock code compiled for the old arch, the test binary will occasionally crash in a Thrust call. I image it may not have to be gunrock code, but gunrock exposed the issue with the incorrect arch option. I created a minimal reproducer here to verify the problem and the fix: https://github.com/rlratzel/cugraph_build_problem

I'd like to better understand why we see this particular crash even though the code being built for the old arch isn't even being called, but I figured we should move forward with this as-is if it allows us to re-enable C++ tests sooner.

@BradReesWork BradReesWork added this to the 0.17 milestone Oct 23, 2020
@codecov-io

codecov-io commented Oct 23, 2020

Copy link
Copy Markdown

Codecov Report

Merging #1242 into branch-0.17 will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##           branch-0.17    #1242   +/-   ##
============================================
  Coverage        56.59%   56.59%           
============================================
  Files               62       62           
  Lines             2564     2564           
============================================
  Hits              1451     1451           
  Misses            1113     1113           

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 60af376...3a5ba3f. Read the comment docs.

@BradReesWork
BradReesWork merged commit 6450729 into rapidsai:branch-0.17 Oct 23, 2020
@rlratzel
rlratzel deleted the branch-0.17-cmakegunrockfix branch December 9, 2020 21:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants