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

Add stream parameter to Set Operations (Public List APIs) #14305

Conversation

SurajAralihalli
Copy link
Contributor

@SurajAralihalli SurajAralihalli commented Oct 19, 2023

This PR marks the conclusion of the List API series. The PR introduces the stream parameter to the Set operations (Public List Comparison and Intersection APIs).

Comparison and Intersection (set_operations.hpp)

have_overlap
intersect_distinct
union_distinct
difference_distinct

Reference 13744

Checklist

  • I am familiar with the Contributing Guidelines.
  • New or existing tests cover these changes.
  • The documentation is up to date with these changes.

Signed-off-by: Suraj Aralihalli <suraj.ara16@gmail.com>
Signed-off-by: Suraj Aralihalli <suraj.ara16@gmail.com>
@copy-pr-bot
Copy link

copy-pr-bot bot commented Oct 19, 2023

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@github-actions github-actions bot added the libcudf Affects libcudf (C++/CUDA) code. label Oct 19, 2023
@SurajAralihalli SurajAralihalli marked this pull request as ready for review October 19, 2023 20:54
@SurajAralihalli SurajAralihalli requested a review from a team as a code owner October 19, 2023 20:54
@ttnghia
Copy link
Contributor

ttnghia commented Oct 20, 2023

Those are not quite "list" operations. Indeed, they are set operations (set is part of list, but saying "list" means something else).

@SurajAralihalli SurajAralihalli changed the title Add stream parameter to List Comparison and Intersection APIs Add stream parameter to Set Operations (Public List APIs) Oct 20, 2023
@SurajAralihalli
Copy link
Contributor Author

Thanks @ttnghia. I have updated the PR title and description.

@davidwendt davidwendt added 3 - Ready for Review Ready for review by team improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Oct 20, 2023
@ttnghia
Copy link
Contributor

ttnghia commented Oct 20, 2023

/ok to test

Copy link
Contributor

@shrshi shrshi left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks!

@ttnghia
Copy link
Contributor

ttnghia commented Oct 23, 2023

/ok to test

@SurajAralihalli
Copy link
Contributor Author

The test for pr / conda-cpp-build / build (11.8.0, ubuntu22.04, amd64, 3.10) failed with the following error. This error didn't show up during local build and testing because it originates from the benchmarks suite. I'm moving this PR to draft and will investigate the issue further.

$SRC_DIR/cpp/benchmarks/lists/set_operations.cpp:53:10: error: use of deleted function 'constexpr rmm::cuda_stream_view::cuda_stream_view(int)'
   53 |     bfunc(cudf::lists_column_view{*lhs},
      |     ~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   54 |           cudf::lists_column_view{*rhs},
      |           ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   55 |           cudf::null_equality::EQUAL,
      |           ~~~~~~~~~~~~~~~~~~~~~~~~~~~
   56 |           cudf::nan_equality::ALL_EQUAL,
      |           ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   57 |           rmm::mr::get_current_device_resource());
      |           ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from $PREFIX/include/rmm/device_buffer.hpp:18,
                 from $SRC_DIR/cpp/include/cudf/utilities/span.hpp:19,
                 from $SRC_DIR/cpp/include/cudf/column/column_view.hpp:20,
                 from $SRC_DIR/cpp/include/cudf/column/column.hpp:18,
                 from $SRC_DIR/cpp/include/cudf/table/table.hpp:18,
                 from $SRC_DIR/cpp/benchmarks/common/generate_input.hpp:19,
                 from $SRC_DIR/cpp/benchmarks/lists/set_operations.cpp:17:
$PREFIX/include/rmm/cuda_stream_view.hpp:51:13: note: declared here
   51 |   constexpr cuda_stream_view(int)            = delete;  //< Prevent cast from 0

@SurajAralihalli SurajAralihalli marked this pull request as draft October 24, 2023 16:58
@davidwendt
Copy link
Contributor

To reproduce locally, add the -DBUILD_BENCHMARKS=ON option to your local cmake build.

@ttnghia
Copy link
Contributor

ttnghia commented Oct 24, 2023

Just to update the benchmark code to add stream param.
This is when CI/CD shows its advantages.

@SurajAralihalli
Copy link
Contributor Author

Thanks @davidwendt @ttnghia, I'll do that!

Signed-off-by: Suraj Aralihalli <suraj.ara16@gmail.com>
@SurajAralihalli SurajAralihalli marked this pull request as ready for review October 24, 2023 23:40
@davidwendt
Copy link
Contributor

/ok to test

@ttnghia
Copy link
Contributor

ttnghia commented Oct 27, 2023

/merge

@rapids-bot rapids-bot bot merged commit f6099ca into rapidsai:branch-23.12 Oct 27, 2023
57 checks passed
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 improvement Improvement / enhancement to an existing function libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants