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

Remove is_relationally_comparable for table device views #10342

Merged

Conversation

davidwendt
Copy link
Contributor

@davidwendt davidwendt commented Feb 22, 2022

Closes #10080

Removes is_relationally_comparable function for table_device_view and mutable_table_device_view. The existing is_relationally_comparable can still be used with non-device table_view and mutable_table_view.

Callers are updated as appropriate. The incorrect kernel call has been removed.

@davidwendt davidwendt added bug Something isn't working 2 - In Progress Currently a work in progress libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change labels Feb 22, 2022
@davidwendt davidwendt self-assigned this Feb 22, 2022
Copy link
Contributor

@jrhemstad jrhemstad left a comment

Choose a reason for hiding this comment

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

I am very confused about why this code needs a stream or run on the device at all.

@codecov
Copy link

codecov bot commented Feb 22, 2022

Codecov Report

Merging #10342 (66a3124) into branch-22.04 (289a6a1) will increase coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@               Coverage Diff                @@
##           branch-22.04   #10342      +/-   ##
================================================
+ Coverage         86.13%   86.15%   +0.01%     
================================================
  Files               139      139              
  Lines             22465    22460       -5     
================================================
  Hits              19351    19351              
+ Misses             3114     3109       -5     
Impacted Files Coverage Δ
python/cudf/cudf/core/column/datetime.py 89.07% <0.00%> (-0.05%) ⬇️
python/cudf/cudf/core/single_column_frame.py 97.01% <0.00%> (-0.03%) ⬇️
python/cudf/cudf/core/column/column.py 88.99% <0.00%> (-0.02%) ⬇️
python/cudf/cudf/core/frame.py 91.72% <0.00%> (-0.01%) ⬇️
python/cudf/cudf/core/column/string.py 88.39% <0.00%> (+0.10%) ⬆️
python/cudf/cudf/core/tools/datetimes.py 84.49% <0.00%> (+0.30%) ⬆️
python/cudf/cudf/core/groupby/groupby.py 91.92% <0.00%> (+0.43%) ⬆️
python/cudf/cudf/core/column/lists.py 90.56% <0.00%> (+0.47%) ⬆️

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 7ff1956...66a3124. Read the comment docs.

@davidwendt davidwendt added 3 - Ready for Review Ready for review by team and removed 2 - In Progress Currently a work in progress labels Feb 23, 2022
@davidwendt davidwendt marked this pull request as ready for review February 23, 2022 14:40
@davidwendt davidwendt requested a review from a team as a code owner February 23, 2022 14:40
@vyasr
Copy link
Contributor

vyasr commented Mar 2, 2022

@davidwendt just FYI I have avoided reviewing this PR so far because I'm not clear on whether @jrhemstad's question is resolved or whether to expect a real resolution within the scope of this PR, and it seems like a decision there could significantly alter this changeset. Please let me know if you think I should just go ahead and review.

@davidwendt
Copy link
Contributor Author

davidwendt commented Mar 2, 2022

@davidwendt just FYI I have avoided reviewing this PR so far because I'm not clear on whether @jrhemstad's question is resolved or whether to expect a real resolution within the scope of this PR, and it seems like a decision there could significantly alter this changeset. Please let me know if you think I should just go ahead and review.

Thanks @vyasr
I think the right approach here is to remove the troublesome is_relationally_comparable_functor for table device-views. This will remove the incorrect kernel/thrust call and solve the original issue. I believe @devavret will be wrapping the row_lexicographic_comparator with an owning class in a separate PR (#10164) which could re-enable the comparable checking on non-device table views there instead.

I welcome all and any reviews at this time.

@davidwendt davidwendt changed the title Add stream parameter to is_relationally_comparable for table device views Remove is_relationally_comparable for table device views Mar 2, 2022
@devavret
Copy link
Contributor

devavret commented Mar 7, 2022

I believe @devavret will be wrapping the row_lexicographic_comparator with an owning class in a separate PR (#10164) which could re-enable the comparable checking on non-device table views there instead.

That is correct. The check will then be on table_view. But #10164 will have the new comparator in namespace experimental and won't immediately replace the existing comparator. So there may be benefit in this PR going through now.

Copy link
Contributor

@vyasr vyasr left a comment

Choose a reason for hiding this comment

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

LGTM.

cpp/include/cudf/table/row_operators.cuh Outdated Show resolved Hide resolved
cpp/src/table/table_device_view.cu Outdated Show resolved Hide resolved
table_view const& rhs);

// Explicit extern template instantiation for a table of mutable views
extern template bool is_relationally_comparable<mutable_table_view>(mutable_table_view const& lhs,
Copy link
Contributor

@ttnghia ttnghia Mar 11, 2022

Choose a reason for hiding this comment

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

extern should not be used here. As I found https://stackoverflow.com/questions/8130602/using-extern-template-c11):

You should only use extern template to force the compiler to not instantiate a template when you know that it will be instantiated somewhere else. It is used to reduce compile time and object file size.

@davidwendt
Copy link
Contributor Author

@gpucibot merge

@rapids-bot rapids-bot bot merged commit cbc4d8b into rapidsai:branch-22.04 Mar 11, 2022
@davidwendt davidwendt deleted the bug-table-comparable-stream branch March 11, 2022 13: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 bug Something isn't working libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] cudaMalloc and cudaFree are being called during aggregations
5 participants