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

Fix primitives benchmarks #1389

Merged
merged 2 commits into from
Mar 31, 2023

Conversation

ahendriksen
Copy link
Contributor

Due to some typos in #1304, the benchmarks are not built any more.

I have fixed this in this PR. I think it is important that this PR goes into 23.04, otherwise we will not have benchmarks for two months in the stable release.

@ahendriksen ahendriksen requested a review from a team as a code owner March 31, 2023 08:34
@ahendriksen ahendriksen added bug Something isn't working improvement Improvement / enhancement to an existing function CMake 3 - Ready for Review and removed cpp CMake python labels Mar 31, 2023
@cjnolet cjnolet added non-breaking Non-breaking change and removed improvement Improvement / enhancement to an existing function labels Mar 31, 2023
@cjnolet
Copy link
Member

cjnolet commented Mar 31, 2023

Thanks for fixing this @ahendriksen. I'm not sure what happened in the ann benchmarks PR but I had originally fixed this in the ann benchmarks PR (see this file) and it looks like it was reverted. I think there was also a bad merge downstream at some point because the bench/CMakeLists.txt seems to have ended up back in the codebase. Can you merge the changes from bench/CMakeLists.txt in this PR as well and remove that file please?

@ahendriksen
Copy link
Contributor Author

I'm not sure what happened

No problem. Happens to me all the time as well. Merge commits can be quite huge..

Can you (1) merge the changes from bench/CMakeLists.txt in this PR as well and (2) remove that file please?

To (1): It looks like the changes from bench/CmakeLists.txt were properly merged into bench/prims/CmakeLists.txt (as in: the diff shows that the only difference is /prims/ added in paths).

To (2): This PR already deletes bench/CmakeLists.txt.

@ahendriksen
Copy link
Contributor Author

Unexpected build failure:

2023-03-31T09:08:07.8753008Z FAILED: CMakeFiles/NEIGHBORS_BENCH.dir/bench/prims/neighbors/knn/brute_force_float_int64_t.cu.o 
[... snip ..]
2023-03-31T09:08:07.8771095Z /opt/conda/conda-bld/work/cpp/bench/prims/neighbors/knn/../knn.cuh(319): error: identifier "CUDA_CHECK" is undefined

Will fix, but the tests should have caught this in the previous PR already right?

@cjnolet
Copy link
Member

cjnolet commented Mar 31, 2023

Will fix, but the tests should have caught this in the previous PR already right?

Unfortunately, I think those macros were removed after the file I linked above was reverted, which would explain why they weren't erroring in CI prior. For some background- these macros were deprecated over a year ago now so they shouldn't have been used in the first place.

To (2): This PR already deletes bench/CmakeLists.txt.

Thanks. For some reason that file isn't showing up in the file list on the "Files Changed" tab so I missed it.

@ahendriksen ahendriksen requested a review from a team as a code owner March 31, 2023 09:40
@ahendriksen
Copy link
Contributor Author

Unfortunately, I think those macros were removed after the file I linked above was reverted, which would explain why they weren't erroring in CI prior. For some background- these macros were deprecated over a year ago now so they shouldn't have been used in the first place.

I overlooked. The compilation error was actually in a bench. In any case, it is fixed :)

Copy link
Member

@cjnolet cjnolet left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for fixing this!

@cjnolet
Copy link
Member

cjnolet commented Mar 31, 2023

/merge

@rapids-bot rapids-bot bot merged commit 09720d8 into rapidsai:branch-23.04 Mar 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Development

Successfully merging this pull request may close these issues.

2 participants