-
Notifications
You must be signed in to change notification settings - Fork 891
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 cmake command-line setting for spdlog logging level #6215
Conversation
Please update the changelog in order to start CI tests. View the gpuCI docs here. |
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, @harrism would be good for you to review as well
Codecov Report
@@ Coverage Diff @@
## branch-0.16 #6215 +/- ##
===============================================
+ Coverage 84.30% 84.63% +0.32%
===============================================
Files 82 82
Lines 13745 14087 +342
===============================================
+ Hits 11588 11922 +334
- Misses 2157 2165 +8
Continue to review full report at Codecov.
|
The unit test failures are:
which I believe is a known issue happening on some of the CI build machines. It does not seem related to this change. |
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.
Looks good, just wondering if this can be inherited from the RMM cmake...
# Set a default logging level if none was specified | ||
set(DEFAULT_LOGGING_LEVEL OFF) | ||
|
||
if(NOT LOGGING_LEVEL) |
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.
Is there a way we can just inherit this from the RMM cmake?
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.
cudf is currently picking up RMM via a conda package which does not ship any cmake files, so I don't know of a way to do this as RMM is located by libcudf today.
I think we'd either have to add a cmake snippet to the librmm conda package and have libcudf pick up the cmake settings from there, or we'd need to have libcudf pull RMM's source as it does Thrust/CUB today. However I don't know if avoiding the RMM conda package and pulling RMM source is something we want to do. @kkraus14
I suspect the RMM cmake source may also need to be refactored to make picking this up from other projects easier to do.
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.
Eventually the plan will be to export the cmake info in the librmm
conda package so someone can do find_package(rmm)
and get a nice target that allows inheriting options like these. We're still likely a bit away from that though.
@harrism thanks for the second round of review. I updated the default RMM logging level to INFO. |
This adds the ability to configure the spdlog logging level on the cmake command-line, defaulting to no logging.