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

add ctest memcheck using cuda-sanitizer #9414

Merged
merged 4 commits into from
Oct 19, 2021

Conversation

karthikeyann
Copy link
Contributor

@karthikeyann karthikeyann commented Oct 11, 2021

addresses part of #904 and pre-requisite for https://github.com/rapidsai/ops/issues/1462

  • Adds cuda-sanitizer as memcheck tool in ctest.
  • Adds environment variable GTEST_CUDF_RMM_MODE in gtests fixture

This PR enables the unit tests to run under compute-sanitizer --tool memcheck in gpuCI nightly or weekly builds.

The main issue pending in this PR is to find suitable way for specifying "--rmm_mode=cuda" only in memcheck runs.
ctest doesn't allow passing arguments to tests yet (ctest feature request).

So, options are

  1. environment variable in cmake command (requires cmake rebuild_cache!)
    • export GTEST_CUDF_RMM_MODE=cuda; ninja rebuild_cache; ctest -T memcheck
  2. environment variable in ctest using RunTests script (has limitations and fails sometimes)
  3. ctest -S custom_script
  4. add custom target in cmake instead of inbuilt ctest memcheck
  5. Add environment variable in gtest for rmm mode. (Another easier option) (This is implemented in this PR)
    • export GTEST_CUDF_RMM_MODE=cuda; ctest -T memcheck

Which method is most suitable? implemented [5]

@karthikeyann karthikeyann added feature request New feature or request help wanted 2 - In Progress Currently a work in progress tests Unit testing for project CMake CMake build issue 4 - Needs Review Waiting for reviewer to review or respond non-breaking Non-breaking change labels Oct 11, 2021
@karthikeyann karthikeyann requested review from a team as code owners October 11, 2021 18:16
@karthikeyann karthikeyann self-assigned this Oct 11, 2021
@github-actions github-actions bot added libcudf Affects libcudf (C++/CUDA) code. and removed gpuCI labels Oct 11, 2021
@davidwendt
Copy link
Contributor

Just wanted add here that the gtests/ERROR_TEST should not be run with the memcheck tool since it is expected to fail.
I'm not sure if that effects which scripting option should be selected.

@karthikeyann
Copy link
Contributor Author

Right. It doesn't affect this script. ERROR_TEST can be excluded using ctest easily.
ctest -T memcheck -E ERROR_TEST

@codecov
Copy link

codecov bot commented Oct 11, 2021

Codecov Report

Merging #9414 (f7056cd) into branch-21.12 (ab4bfaa) will decrease coverage by 0.04%.
The diff coverage is 0.00%.

❗ Current head f7056cd differs from pull request most recent head 2e46e85. Consider uploading reports for the commit 2e46e85 to get more accurate results
Impacted file tree graph

@@               Coverage Diff                @@
##           branch-21.12    #9414      +/-   ##
================================================
- Coverage         10.79%   10.74%   -0.05%     
================================================
  Files               116      116              
  Lines             18869    19502     +633     
================================================
+ Hits               2036     2096      +60     
- Misses            16833    17406     +573     
Impacted Files Coverage Δ
python/cudf/cudf/__init__.py 0.00% <0.00%> (ø)
python/cudf/cudf/_lib/__init__.py 0.00% <ø> (ø)
python/cudf/cudf/_lib/strings/__init__.py 0.00% <0.00%> (ø)
python/cudf/cudf/io/csv.py 0.00% <0.00%> (ø)
python/cudf/cudf/io/hdf.py 0.00% <0.00%> (ø)
python/cudf/cudf/io/orc.py 0.00% <0.00%> (ø)
python/cudf/cudf/_version.py 0.00% <0.00%> (ø)
python/cudf/cudf/core/abc.py 0.00% <0.00%> (ø)
python/cudf/cudf/api/types.py 0.00% <0.00%> (ø)
python/cudf/cudf/io/dlpack.py 0.00% <0.00%> (ø)
... and 56 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 e6caaf5...2e46e85. Read the comment docs.

@davidwendt
Copy link
Contributor

Also, the compute-sanitizer should be from CUDA toolkit 11.4 (or higher).
Lower versions report false positives for some libcudf gtests.

@harrism
Copy link
Member

harrism commented Oct 12, 2021

CC @robertmaynard as resident cmake expert.

@robertmaynard
Copy link
Contributor

I am a fan of #5 as making include/cudf_test/base_fixture.hpp be aware of an environment variable gives everyone more options to how to run tests with cuda sanitizer enabled and doesn't tie the solution to be CTest / CMake specific.

@karthikeyann karthikeyann added 3 - Ready for Review Ready for review by team and removed 2 - In Progress Currently a work in progress help wanted labels Oct 13, 2021
@karthikeyann karthikeyann removed the 4 - Needs Review Waiting for reviewer to review or respond label Oct 15, 2021
@karthikeyann
Copy link
Contributor Author

@gpucibot merge

@rapids-bot rapids-bot bot merged commit a3a27c6 into rapidsai:branch-21.12 Oct 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 - Ready for Review Ready for review by team CMake CMake build issue feature request New feature or request libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change tests Unit testing for project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants