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

Fix thrust failure when transfering data from device_vector to host_vector with vectors of size 1 #7382

Merged
merged 9 commits into from
Feb 17, 2021

Conversation

ttnghia
Copy link
Contributor

@ttnghia ttnghia commented Feb 12, 2021

This is a fix for the problem that popped up in the bug #6364.

I debug this problem and discovered that thrust crashes only when the vector size is 1. There should not be any problem with the vector size, thus the bug should be due to something else. Finally, I found a fix for this: adding qualifiers __host__ __device__ to the type transformer.

It really weird that without those qualifiers, there is runtime problem only when the vector size is 1.

@ttnghia ttnghia requested a review from a team as a code owner February 12, 2021 20:01
@ttnghia ttnghia requested review from vuule and codereport February 12, 2021 20:01
@ttnghia ttnghia self-assigned this Feb 12, 2021
@github-actions github-actions bot added the libcudf Affects libcudf (C++/CUDA) code. label Feb 12, 2021
@ttnghia ttnghia added non-breaking Non-breaking change 4 - Needs Review Waiting for reviewer to review or respond bug Something isn't working libcudf Affects libcudf (C++/CUDA) code. and removed libcudf Affects libcudf (C++/CUDA) code. labels Feb 12, 2021
cpp/include/cudf_test/column_wrapper.hpp Outdated Show resolved Hide resolved
@ttnghia ttnghia added the tests Unit testing for project label Feb 12, 2021
@ttnghia ttnghia force-pushed the branch-0.19-bug-6364 branch from 708cc10 to 7811ad4 Compare February 12, 2021 21:17
@vuule
Copy link
Contributor

vuule commented Feb 12, 2021

@ttnghia can you add a test for this fix?

@codecov
Copy link

codecov bot commented Feb 13, 2021

Codecov Report

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

Impacted file tree graph

@@              Coverage Diff               @@
##             branch-0.19    #7382   +/-   ##
==============================================
  Coverage               ?   81.79%           
==============================================
  Files                  ?      100           
  Lines                  ?    16610           
  Branches               ?        0           
==============================================
  Hits                   ?    13586           
  Misses                 ?     3024           
  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 d180213...9e47ec0. Read the comment docs.

@ttnghia
Copy link
Contributor Author

ttnghia commented Feb 13, 2021

@ttnghia can you add a test for this fix?

Sure. I've added that.

Copy link
Member

@harrism harrism left a comment

Choose a reason for hiding this comment

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

I think we could name things a bit better while we are at it, and I'm not sure about that test.

cpp/include/cudf_test/column_utilities.hpp Outdated Show resolved Hide resolved
cpp/include/cudf_test/column_utilities.hpp Outdated Show resolved Hide resolved
cpp/include/cudf_test/column_utilities.hpp Outdated Show resolved Hide resolved
cpp/include/cudf_test/column_utilities.hpp Outdated Show resolved Hide resolved
cpp/include/cudf_test/column_utilities.hpp Outdated Show resolved Hide resolved
cpp/tests/reshape/byte_cast_tests.cpp Outdated Show resolved Hide resolved
Copy link
Contributor Author

@ttnghia ttnghia left a comment

Choose a reason for hiding this comment

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

Thanks for the comments. I have made a couple of changes.

@harrism
Copy link
Member

harrism commented Feb 16, 2021

BTW, In my own unrelated work I caused some tests to fail and observed these CUDA errors (not crashes) as well. Adding the code from this PR fixed the errors and allowed me to see the differences. Let's get this merged ASAP!

Copy link
Contributor

@codereport codereport left a comment

Choose a reason for hiding this comment

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

lgtm 👍, copyright needs to be updated in both files though

Copy link
Contributor

@codereport codereport left a comment

Choose a reason for hiding this comment

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

👍

@ttnghia
Copy link
Contributor Author

ttnghia commented Feb 17, 2021

lgtm 👍, copyright needs to be updated in both files though

Thanks for reminding me about that. I have updated.

@harrism
Copy link
Member

harrism commented Feb 17, 2021

@gpucibot merge

@rapids-bot rapids-bot bot merged commit 4cd5f8d into rapidsai:branch-0.19 Feb 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4 - Needs Review Waiting for reviewer to review or respond bug Something isn't working 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.

6 participants