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.

Sign up for GitHub

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

Jump to bottom

Replace raw streams with rmm::cuda_stream_view (part 3) #6744

Merged
merged 120 commits into from
Nov 24, 2020
Merged

Replace raw streams with rmm::cuda_stream_view (part 3) #6744

merged 120 commits into from
Nov 24, 2020

Conversation

harrism
Copy link
Member

@harrism harrism commented Nov 12, 2020 •
edited
Loading

Converting libcudf to use rmm::cuda_stream_view will require a LOT of changes, so I'm splitting it into multiple PRs to ease reviewing. This is the third PR in the series. This series of PRs will

Contributes to #6645 and #5119

Depends on #6646 and #6648 so this PR will look much bigger until they are merged.

This third PR converts:

  • remaining dictionary functionality
  • cuio
  • lists
  • scalar
  • strings
  • groupby
  • join
  • contiguous_split
  • get_element
  • datetime_ops
  • extract
  • merge
  • partitioning
  • minmax reduction
  • scan
  • byte_cast
  • clamp
  • interleave_columns
  • is_sorted
  • groupby
  • rank
  • tests
  • concurrent map classes

@harrism
Fix cast warning.
ff53e23
@harrism
Initial stream changes
8c44ade
@harrism
Revert commented out stuff.
7802d52
@harrism
Convert AST to cuda_stream_view
f0ca10c
@harrism
Convert column_device_view to rmm::cuda_stream_view
22a14dd
@harrism
Convert column to rmm::cuda_stream_view
2fab2ad
@harrism
Changelog for rapidsai#6646
984c7ca
@harrism
Merge branch 'branch-0.17' into fea-cuda_stream_view
953d133
@harrism
Convert column factories to cuda_stream_view
0ebf99e
@harrism
convert detail/aggregation headers and source to cuda_stream_view
31716db
@harrism
Use cuda_stream_view in groupby sort_helper
ba2fc0f
@harrism
Convert binops to cuda_stream_view
d8827e9
@harrism
Fix includes and copyright dates
6c88b59
@harrism
Merge branch 'branch-0.17' into fea-cuda_stream_view
a31c432
@harrism
Update round to use detail::copy_bitmask
325ca52
@harrism
Merge branch 'fea-cuda_stream_view' into fea-cuda_stream_view-2
eafb55c
@harrism
Use stream.synchronize()
ad24fb7
@harrism
Convert concatenate to cuda_stream_view
3b6b0aa
@harrism
Convert table and copy_if to cuda_stream_view
a2edf78
@harrism
Changelog for rapidsai#6648
94b1627
@harrism
Convert copy_range to cuda_stream_view
c497fcc
@harrism
Convert fill to cuda_stream_view
03aec6a
@harrism
Merge branch 'branch-0.17' into fea-cuda_stream_view
802fa76
@harrism
Merge branch 'fea-cuda_stream_view' into fea-cuda_stream_view-2
c7faec9
@harrism
Convert gather to cuda_stream_view
927378b
@harrism
rename type_conversion .cu->.cpp
243d2a1
@harrism
Rename structs_column_view .cu->.cpp
554a370
@harrism
Convert hash groupby to cuda_stream_view
a76e9ec
@harrism
Convert hashing to cuda_stream_view
817c715
@harrism
Merge branch 'branch-0.17' into fea-cuda_stream_view
e663d97
@harrism
Copy link
Member Author

harrism commented Nov 22, 2020

rerun tests

1 similar comment
@kkraus14
Copy link
Collaborator

rerun tests

@harrism harrism mentioned this pull request Nov 23, 2020
Closed
jlowe
jlowe approved these changes Nov 23, 2020
View reviewed changes
Copy link
Member

@jlowe jlowe left a comment

Choose a reason for hiding this comment

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

Java approval.

rgsl888prabhu
rgsl888prabhu approved these changes Nov 23, 2020
View reviewed changes
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.

LGTM, just small changes

cpp/include/cudf/strings/detail/merge.cuh Outdated Show resolved Hide resolved
cpp/src/io/csv/writer_impl.cu Outdated Show resolved Hide resolved
cpp/src/join/join.cu Outdated Show resolved Hide resolved
cpp/src/reshape/interleave_columns.cu Outdated Show resolved Hide resolved
cpp/src/strings/capitalize.cu Outdated Show resolved Hide resolved
cpp/src/strings/replace/multi_re.cu Outdated Show resolved Hide resolved
cpp/src/strings/replace/replace_re.cu Outdated Show resolved Hide resolved
cpp/src/strings/substring.cu Outdated Show resolved Hide resolved
cpp/src/strings/translate.cu Outdated Show resolved Hide resolved
cpp/src/text/generate_ngrams.cu Outdated Show resolved Hide resolved
devavret
devavret suggested changes Nov 23, 2020
View reviewed changes
Copy link
Contributor

@devavret devavret left a comment

Choose a reason for hiding this comment

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

Suggest making a debug build once and changing any CHECK_CUDA(stream) to CHECK_CUDA(stream.view()). There are several instances in this PR which I cannot request changes in because those lines are not part of diffs.

@harrism harrism mentioned this pull request Nov 23, 2020
Merged
@harrism
Copy link
Member Author

harrism commented Nov 23, 2020

Suggest making a debug build once and changing any CHECK_CUDA(stream) to CHECK_CUDA(stream.view()). There are several instances in this PR which I cannot request changes in because those lines are not part of diffs.

Thanks @devavret , done. Running a debug build in the background...

codereport
codereport approved these changes Nov 23, 2020
View reviewed changes
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 👍

@harrism
Copy link
Member Author

harrism commented Nov 24, 2020

@devavret I found and fixed an unrelated debug build issue. Can you approve?

@harrism harrism added 5 - Ready to Merge Testing and reviews complete, ready to merge and removed 3 - Ready for Review Ready for review by team labels Nov 24, 2020
@kkraus14 kkraus14 merged commit 8cc23bd into rapidsai:branch-0.17 Nov 24, 2020
This was referenced Dec 2, 2020
Closed
Closed
@davidwendt davidwendt mentioned this pull request Jan 26, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mike-wendt mike-wendt mike-wendt left review comments

@rgsl888prabhu rgsl888prabhu rgsl888prabhu approved these changes

@jlowe jlowe jlowe approved these changes

@devavret devavret devavret approved these changes

@codereport codereport codereport approved these changes

Assignees

@harrism harrism

Labels
5 - Ready to Merge Testing and reviews complete, ready to merge cuIO cuIO issue libcudf Affects libcudf (C++/CUDA) code.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@harrism @GPUtester @kkraus14 @jlowe @mike-wendt @devavret @codereport @rgsl888prabhu