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

Change device_vector to device_uvector for strings_column_view #7798

Merged
merged 14 commits into from
Apr 8, 2021

Conversation

davidwendt
Copy link
Contributor

References #7287

This replaces device_vector with device_uvector in strings_column_view functions.
This cascaded to some dependent functions that needed to be updated as well.

@davidwendt davidwendt added 3 - Ready for Review Ready for review by team libcudf Affects libcudf (C++/CUDA) code. improvement Improvement / enhancement to an existing function breaking Breaking change labels Mar 31, 2021
@davidwendt davidwendt self-assigned this Mar 31, 2021
@davidwendt davidwendt requested a review from a team as a code owner March 31, 2021 22:01
@davidwendt davidwendt added this to PR-WIP in v21.06 Release via automation Mar 31, 2021
v21.06 Release automation moved this from PR-WIP to PR-Needs review Mar 31, 2021
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 there's a copy error. Also opportunity to create a useful uvector utility.

cpp/include/cudf_test/column_utilities.hpp Show resolved Hide resolved
cpp/tests/strings/factories_test.cu Outdated Show resolved Hide resolved
cpp/src/strings/strings_column_view.cu Outdated Show resolved Hide resolved
cpp/src/strings/strings_column_view.cu Show resolved Hide resolved
Copy link
Contributor

@rgsl888prabhu rgsl888prabhu left a comment

Choose a reason for hiding this comment

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

Don't have any other concerns apart from what have been already raised.

cpp/src/io/utilities/column_buffer.hpp Show resolved Hide resolved
@davidwendt davidwendt requested a review from harrism April 1, 2021 15:25
@harrism
Copy link
Member

harrism commented Apr 1, 2021

Can you please put make_std_vector in vector_factories.hpp and make _async and _sync versions to match make_device_uvector?

@davidwendt
Copy link
Contributor Author

Can you please put make_std_vector in vector_factories.hpp and make _async and _sync versions to match make_device_uvector?

Absolutely. Are you ok with the name?

Copy link
Contributor

@rgsl888prabhu rgsl888prabhu left a comment

Choose a reason for hiding this comment

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

Minor comment, rest looks good.

cpp/src/strings/strings_column_view.cu Outdated Show resolved Hide resolved
@harrism
Copy link
Member

harrism commented Apr 5, 2021

Is the async version of make_std_vector safe? Could the temp host array be freed before the copy completes?

@codecov
Copy link

codecov bot commented Apr 6, 2021

Codecov Report

Merging #7798 (f5b7eb2) into branch-0.20 (599f62d) will increase coverage by 0.04%.
The diff coverage is 87.60%.

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

@@               Coverage Diff               @@
##           branch-0.20    #7798      +/-   ##
===============================================
+ Coverage        82.30%   82.34%   +0.04%     
===============================================
  Files              101      103       +2     
  Lines            17053    17355     +302     
===============================================
+ Hits             14035    14291     +256     
- Misses            3018     3064      +46     
Impacted Files Coverage Δ
python/cudf/cudf/utils/dtypes.py 83.44% <46.66%> (-6.45%) ⬇️
python/cudf/cudf/core/groupby/groupby.py 92.25% <78.57%> (-1.21%) ⬇️
python/cudf/cudf/core/column/lists.py 86.95% <80.00%> (-0.27%) ⬇️
python/dask_cudf/dask_cudf/backends.py 89.28% <83.33%> (-0.35%) ⬇️
python/cudf/cudf/core/column/struct.py 96.15% <86.66%> (-3.85%) ⬇️
python/cudf/cudf/core/index.py 92.58% <88.09%> (-0.44%) ⬇️
python/cudf/cudf/core/column/decimal.py 92.92% <91.48%> (-0.92%) ⬇️
python/cudf/cudf/core/column/interval.py 90.69% <92.30%> (+0.07%) ⬆️
python/cudf/cudf/core/column/column.py 87.90% <92.59%> (+0.46%) ⬆️
python/cudf/cudf/core/join/join.py 96.72% <93.75%> (+0.20%) ⬆️
... and 18 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 acf155d...f5a73a3. Read the comment docs.

@davidwendt
Copy link
Contributor Author

Is the async version of make_std_vector safe? Could the temp host array be freed before the copy completes?

There should be no temp host array since it is returned.
It should work the same as make_device_uvector_async which returns a device_uvector.
https://godbolt.org/z/PhP7svnn3

v21.06 Release automation moved this from PR-Needs review to PR-Reviewer approved Apr 6, 2021
@davidwendt
Copy link
Contributor Author

@gpucibot merge

@rapids-bot rapids-bot bot merged commit 343a5e7 into rapidsai:branch-0.20 Apr 8, 2021
v21.06 Release automation moved this from PR-Reviewer approved to Done Apr 8, 2021
@davidwendt davidwendt deleted the strings-device-vector branch April 8, 2021 20:34
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 breaking Breaking change improvement Improvement / enhancement to an existing function libcudf Affects libcudf (C++/CUDA) code.
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

5 participants