Skip to content

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

Merged
rapids-bot[bot] merged 9 commits into
NVIDIA:branch-0.19from
ttnghia:branch-0.19-bug-6364
Feb 17, 2021
Merged

Fix thrust failure when transfering data from device_vector to host_vector with vectors of size 1#7382
rapids-bot[bot] merged 9 commits into
NVIDIA:branch-0.19from
ttnghia:branch-0.19-bug-6364

Conversation

@ttnghia

@ttnghia ttnghia commented Feb 12, 2021

Copy link
Copy Markdown
Contributor

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 codereport and vuule 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
Comment thread cpp/include/cudf_test/column_wrapper.hpp Outdated
@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

vuule commented Feb 12, 2021

Copy link
Copy Markdown
Contributor

@ttnghia can you add a test for this fix?

@codecov

codecov Bot commented Feb 13, 2021

Copy link
Copy Markdown

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

ttnghia commented Feb 13, 2021

Copy link
Copy Markdown
Contributor Author

@ttnghia can you add a test for this fix?

Sure. I've added that.

@harrism harrism left a comment

Copy link
Copy Markdown
Contributor

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.

Comment thread cpp/include/cudf_test/column_utilities.hpp Outdated
Comment thread cpp/include/cudf_test/column_utilities.hpp Outdated
Comment thread cpp/include/cudf_test/column_utilities.hpp Outdated
Comment thread cpp/include/cudf_test/column_utilities.hpp Outdated
Comment thread cpp/include/cudf_test/column_utilities.hpp Outdated
Comment thread cpp/tests/reshape/byte_cast_tests.cpp Outdated

@ttnghia ttnghia left a comment

Copy link
Copy Markdown
Contributor Author

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

harrism commented Feb 16, 2021

Copy link
Copy Markdown
Contributor

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!

Comment thread cpp/include/cudf_test/column_utilities.hpp Outdated

@codereport codereport left a comment

Copy link
Copy Markdown
Contributor

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

@codereport codereport left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

👍

@ttnghia

ttnghia commented Feb 17, 2021

Copy link
Copy Markdown
Contributor Author

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

Thanks for reminding me about that. I have updated.

@harrism

harrism commented Feb 17, 2021

Copy link
Copy Markdown
Contributor

@gpucibot merge

@rapids-bot
rapids-bot Bot merged commit 4cd5f8d into NVIDIA: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