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 cmake command-line setting for spdlog logging level #6215

Merged
merged 5 commits into from
Sep 15, 2020

Conversation

jlowe
Copy link
Member

@jlowe jlowe commented Sep 11, 2020

This adds the ability to configure the spdlog logging level on the cmake command-line, defaulting to no logging.

@jlowe jlowe added libcudf Affects libcudf (C++/CUDA) code. CMake CMake build issue 4 - Needs Review Waiting for reviewer to review or respond Java Affects Java cuDF API. 4 - Needs cuDF (Java) Reviewer labels Sep 11, 2020
@jlowe jlowe requested a review from harrism September 11, 2020 20:24
@jlowe jlowe requested review from a team as code owners September 11, 2020 20:24
@jlowe jlowe self-assigned this Sep 11, 2020
@GPUtester
Copy link
Collaborator

Please update the changelog in order to start CI tests.

View the gpuCI docs here.

Copy link
Collaborator

@kkraus14 kkraus14 left a 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

@kkraus14 kkraus14 added 3 - Ready for Review Ready for review by team and removed 4 - Needs cuDF (Java) Reviewer 4 - Needs Review Waiting for reviewer to review or respond labels Sep 11, 2020
cpp/CMakeLists.txt Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Sep 11, 2020

Codecov Report

Merging #6215 into branch-0.16 will increase coverage by 0.32%.
The diff coverage is n/a.

Impacted file tree graph

@@               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     
Impacted Files Coverage Δ
python/cudf/cudf/io/feather.py 100.00% <0.00%> (ø)
python/cudf/cudf/comm/serialize.py 0.00% <0.00%> (ø)
python/custreamz/custreamz/_version.py 0.00% <0.00%> (ø)
python/dask_cudf/dask_cudf/_version.py 0.00% <0.00%> (ø)
python/dask_cudf/dask_cudf/io/tests/test_csv.py 100.00% <0.00%> (ø)
python/dask_cudf/dask_cudf/io/tests/test_orc.py 100.00% <0.00%> (ø)
python/dask_cudf/dask_cudf/io/tests/test_json.py 100.00% <0.00%> (ø)
...ython/custreamz/custreamz/tests/test_dataframes.py 100.00% <0.00%> (ø)
...ython/dask_cudf/dask_cudf/io/tests/test_parquet.py 100.00% <0.00%> (ø)
python/cudf/cudf/utils/applyutils.py 98.75% <0.00%> (+0.02%) ⬆️
... and 31 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f8f95d9...8c9877a. Read the comment docs.

@jlowe
Copy link
Member Author

jlowe commented Sep 13, 2020

The unit test failures are:

FAILED cudf/tests/test_csv.py::test_csv_writer_file_handle - NotImplementedError: Unsupported encoding ISO-8859-1
FAILED cudf/tests/test_csv.py::test_csv_writer_file_append - NotImplementedError: Unsupported encoding ISO-8859-1

which I believe is a known issue happening on some of the CI build machines. It does not seem related to this change.

Copy link
Member

@harrism harrism 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, just wondering if this can be inherited from the RMM cmake...

cpp/CMakeLists.txt Outdated Show resolved Hide resolved
java/src/main/native/CMakeLists.txt Outdated Show resolved Hide resolved
# Set a default logging level if none was specified
set(DEFAULT_LOGGING_LEVEL OFF)

if(NOT LOGGING_LEVEL)
Copy link
Member

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?

Copy link
Member Author

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.

Copy link
Collaborator

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.

@jlowe
Copy link
Member Author

jlowe commented Sep 15, 2020

@harrism thanks for the second round of review. I updated the default RMM logging level to INFO.

@kkraus14 kkraus14 added 5 - Ready to Merge Testing and reviews complete, ready to merge and removed 3 - Ready for Review Ready for review by team labels Sep 15, 2020
@kkraus14 kkraus14 merged commit c2d544b into rapidsai:branch-0.16 Sep 15, 2020
@jlowe jlowe deleted the spdlog-level-flag branch September 10, 2021 15:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
5 - Ready to Merge Testing and reviews complete, ready to merge CMake CMake build issue Java Affects Java cuDF API. libcudf Affects libcudf (C++/CUDA) code.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants