-
Notifications
You must be signed in to change notification settings - Fork 915
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 benchmarks suffix #10112
Remove benchmarks suffix #10112
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you rename join.cu
to left_joins.cu
so that we can rename join_nvbench.cu
to join.cu
.
That to me better captures what exact join routines each file is benchmarking.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
CI should build benchmarks. Are you certain it is not? I just fixed this in RMM, which was not. |
Last I checked this was a month ago: #9905. It's possible this has changed since then, but my comment indicated that CI did not build benchmarks. #9810 (comment) |
@harrism I see your changes to RMM rapidsai/rmm#941 and I think this line indicates we are not building benchmarks in cuDF CI (missing cudf/conda/recipes/libcudf/build.sh Line 7 in cfb6cbe
Shall I make a follow-up PR to build cuDF benchmarks in CI? There is a trade-off because this would cost additional CI time. |
Codecov Report
@@ Coverage Diff @@
## branch-22.04 #10112 +/- ##
================================================
+ Coverage 10.37% 10.42% +0.04%
================================================
Files 119 119
Lines 20149 20607 +458
================================================
+ Hits 2091 2148 +57
- Misses 18058 18459 +401
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fantastic! Please do the same for tests/*_tests.*
.
@gpucibot merge |
Would be good to discuss with the team. Not sure what CI time cost would be? |
I think the best way to find out is to open a draft PR. I'll work on this. |
Use build time metrics html report. |
This PR addresses suggestions from @robertmaynard, @karthikeyann, and @harrism in #10093 to remove the suffix
_benchmark
from the name of the benchmark files. The developer guide has been updated to reflect this change.I left a few shared files with the word "benchmark" in them, such as cpp/benchmarks/fixture/benchmark_fixture.hpp and cpp/benchmarks/fixture/templated_benchmark_fixture.hpp.
I built the benchmarks locally to ensure that nothing was broken (I'm noting this because I recall that our CI does not build the benchmarks).