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

Move NDS-H examples into benchmarks #16663

Merged
merged 32 commits into from
Sep 11, 2024
Merged

Move NDS-H examples into benchmarks #16663

merged 32 commits into from
Sep 11, 2024

Conversation

JayjeetAtGithub
Copy link
Contributor

@JayjeetAtGithub JayjeetAtGithub commented Aug 26, 2024 •
edited
Loading

Description

Moving the TPC-H examples into benchmarks by converting each of them into NVBench's. The benchmarks can be built by

./build.sh libcudf benchmarks

Also, addresses #16711

Checklist

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

@github-actions github-actions bot added libcudf Affects libcudf (C++/CUDA) code. CMake CMake build issue labels Aug 26, 2024
@JayjeetAtGithub JayjeetAtGithub marked this pull request as ready for review August 26, 2024 23:21
@JayjeetAtGithub JayjeetAtGithub requested a review from a team as a code owner August 26, 2024 23:21
@JayjeetAtGithub JayjeetAtGithub added feature request New feature or request non-breaking Non-breaking change labels Aug 26, 2024
@GregoryKimball
Copy link
Contributor

@davidwendt @PointKernel Would you please review this changeset from Jayjeet?

@JayjeetAtGithub JayjeetAtGithub mentioned this pull request Aug 27, 2024
Merged
11 tasks
@PointKernel
Copy link
Member

Should we migrate at least one example or benchmark to NVBench in this PR to ensure that the build system and data generation work correctly in the new setup? Doing this now will make the follow-up PRs more straightforward.

@JayjeetAtGithub
Copy link
Contributor Author

Hi @PointKernel , The datagen PR #16294 is yet to be merged. So, we can't integrate the examples/benchmarks with the datagen yet. But, I can convert these to NVBenches regardless, but they would need local files to exist for now.

@JayjeetAtGithub
Copy link
Contributor Author

Hi @PointKernel , So, I will have to hardcode the dataset dir for my dev env for me to convert these to nvbenches now, which I don't think is a good idea. Can we just merge it without NVbench for now ?

@PointKernel
Copy link
Member

The datagen PR #16294 is yet to be merged. So, we can't integrate the examples/benchmarks with the datagen yet.

I could miss something obvious but can we can wait for #16294 to be merged first and then focus on NVbench migration in this PR? Moving existing code and outdated documentation to a new location without making actual changes can easily lead to overlooked details.

@JayjeetAtGithub
Copy link
Contributor Author

I could miss something obvious but can we can wait for #16294 to be merged first and then focus on NVbench migration in this PR? Moving existing code and outdated documentation to a new location without making actual changes can easily lead to overlooked details.

Sounds good ! Let's wait for #16294

@karthikeyann
Copy link
Contributor

karthikeyann commented Aug 28, 2024 •
edited
Loading

@JayjeetAtGithub to unblock yourself, you may work on with changes from #16294 in a new branch and create a PR, mention that #16294 and #16663 are dependent PRs in description.

bdice
bdice reviewed Sep 3, 2024
View reviewed changes
cpp/benchmarks/CMakeLists.txt Outdated Show resolved Hide resolved
cpp/benchmarks/CMakeLists.txt Outdated Show resolved Hide resolved
cpp/benchmarks/tpch/q1.cpp Outdated
{
auto stream = cudf::get_default_stream();
state.set_cuda_stream(nvbench::make_cuda_stream_view(stream.value()));
state.exec(nvbench::exec_tag::sync, [&](nvbench::launch& launch) { run_tpch_q1(state); });
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be better to pass the stream through to run_tpch_q1 and use stream-ordered APIs?

cpp/benchmarks/tpch/utils.hpp Outdated Show resolved Hide resolved
cpp/benchmarks/tpch/utils.hpp Outdated Show resolved Hide resolved
cpp/benchmarks/tpch/utils.hpp Outdated Show resolved Hide resolved
cpp/benchmarks/tpch/utils.hpp Outdated Show resolved Hide resolved
cpp/benchmarks/tpch/utils.hpp Outdated Show resolved Hide resolved
bdice
bdice reviewed Sep 5, 2024
View reviewed changes
Copy link
Contributor

@bdice bdice left a comment

Choose a reason for hiding this comment

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

A new README for these benchmarks would be helpful. I have a couple comments but this is looking close to ready.

cpp/benchmarks/ndsh/utils.cpp Outdated Show resolved Hide resolved
cpp/benchmarks/ndsh/utils.hpp Outdated Show resolved Hide resolved
karthikeyann
karthikeyann previously requested changes Sep 6, 2024
View reviewed changes
cpp/benchmarks/ndsh/q1.cpp Outdated Show resolved Hide resolved
@JayjeetAtGithub
Copy link
Contributor Author

/merge

@rapids-bot rapids-bot bot merged commit c3d323d into rapidsai:branch-24.10 Sep 11, 2024
98 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bdice bdice bdice left review comments

@harrism harrism harrism approved these changes

@vyasr vyasr Awaiting requested review from vyasr vyasr is a code owner automatically assigned from rapidsai/cudf-cpp-codeowners

@mythrocks mythrocks Awaiting requested review from mythrocks mythrocks is a code owner automatically assigned from rapidsai/cudf-cpp-codeowners

@PointKernel PointKernel Awaiting requested review from PointKernel

@davidwendt davidwendt Awaiting requested review from davidwendt

@karthikeyann karthikeyann Awaiting requested review from karthikeyann

Assignees

@JayjeetAtGithub JayjeetAtGithub

Labels
CMake CMake build issue feature request New feature or request libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@JayjeetAtGithub @GregoryKimball @PointKernel @karthikeyann @bdice @harrism @davidwendt