Upgrade to spdlog 1.10 #1173

kkraus14 · 2022-12-01T18:01:22Z

Description

Updates spdlog dependency to 1.10 to match the conda-forge pinning. Depends on rapidsai/rapids-cmake#312

Also updates the flake8 pre-commit hook location since it's migrated from gitlab to github.

Checklist

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

rapids-bot · 2022-12-01T18:01:27Z

Pull requests from external contributors require approval from a rapidsai organization member with write or admin permissions before CI can begin.

kkraus14 · 2022-12-01T18:03:22Z

conda/recipes/rmm/meta.yaml

@@ -48,7 +48,6 @@ requirements:
    - python
    - scikit-build>=0.13.1
    - setuptools
-    - spdlog>=1.8.5,<2.0.0a0


As far as I could tell the RMM cython doesn't directly use spdlog in any way other than needing to find it during build time because of the librmm dependency. It will get spdlog from the run dependency of the librmm recipe.

kkraus14 · 2022-12-01T18:04:10Z

Apparently I no longer have permissions to set labels...?

kkraus14 · 2022-12-01T18:12:22Z

~~Should probably update this as well? https://github.com/rapidsai/rapids-cmake/blob/branch-23.02/rapids-cmake/cpm/versions.json#L35~~

EDIT: PR here: rapidsai/rapids-cmake#312

ajschmidt8 · 2022-12-01T19:38:11Z

/ok to test

ajschmidt8 · 2022-12-01T19:41:45Z

@kkraus14, I'm unaware of any permissions changes for your user account recently. I'll have to look into why you're unable to label PRs anymore.

Regarding CI, we are in the process of switching RAPIDS to using GitHub Actions. rmm is the first repository to have it fully enabled. With the GH Actions CI implementation, we do not currently have an allowlist setup for external contributors. We will be adding this as a future enhancement. Until then, every commit from every external contributor will need to be reviewed and approved by a RAPIDS org member before CI is run. You can find more details on the switch to GitHub Actions below.

https://docs.rapids.ai/resources/github-actions/

codecov-commenter · 2022-12-01T20:02:58Z

Codecov Report

❗ No coverage uploaded for pull request base (branch-23.02@8a01696). Click here to learn what that means.
Patch has no changes to coverable lines.

Additional details and impacted files

@@              Coverage Diff               @@
##             branch-23.02   #1173   +/-   ##
==============================================
  Coverage                ?   0.00%           
==============================================
  Files                   ?       5           
  Lines                   ?     406           
  Branches                ?       0           
==============================================
  Hits                    ?       0           
  Misses                  ?     406           
  Partials                ?       0

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

bdice

Looks good to me. Thanks @kkraus14!

bdice · 2022-12-02T21:01:51Z

@gpucibot merge

ttnghia · 2022-12-05T14:13:32Z

We have a reported compile issue today:

/home/scratch.junzhang_sw/workspace/github/cudf/cpp/build/_deps/spdlog-src/include/spdlog/fmt/bundled/core.h(426): error #186-D: pointless comparison of unsigned integer with zero
          detected during:
            instantiation of "auto fmt::v8::detail::to_unsigned(Int)->std::make_unsigned<Int>::type [with Int=size_t]" 
/home/scratch.junzhang_sw/workspace/github/cudf/cpp/build/_deps/spdlog-src/include/spdlog/fmt/bundled/format.h(458): here
            instantiation of "auto fmt::v8::detail::fill_n(T *, Size, char)->T * [with T=char, Size=size_t]" 
/home/scratch.junzhang_sw/workspace/github/cudf/cpp/build/_deps/spdlog-src/include/spdlog/fmt/bundled/format.h(702): here
            instantiation of "fmt::v8::basic_memory_buffer<T, SIZE, Allocator>::basic_memory_buffer(const Allocator &) [with T=char, SIZE=500UL, Allocator=std::allocator<char>]" 
/home/scratch.junzhang_sw/workspace/github/cudf/cpp/build/_deps/spdlog-src/include/spdlog/fmt/bundled/format-inl.h(85): here

/home/scratch.junzhang_sw/workspace/github/cudf/cpp/build/_deps/spdlog-src/include/spdlog/fmt/bundled/core.h(426): error #186-D: pointless comparison of unsigned integer with zero
          detected during:

Is that due to this PR?

Ref: https://nvidia.slack.com/archives/CDTANRCTT/p1670243383311319

dantegd · 2022-12-05T16:54:02Z

@wphicks @cjnolet this PR is what broke cuML's usage of spdlog, FYI in case RAFT/Treelite backend could be affected

This reverts commit 7bf7a76.

@kkraus14

This reverts commit 7bf7a76. cc: @kkraus14 There's a handful of issues this caused downstream. We'll be sorting through these and fixing them but need to quickly revert this for now. Currrent problems: 1. rmm's meta.yaml doesn't correctly specify spdlog as a build dep. It's [incorrectly listed as a runtime dep](https://github.com/rapidsai/rmm/blob/7bf7a763a3b6a32a7a57f900de35e2e5f30f17c0/conda/recipes/librmm/meta.yaml#L56). This means that rmm [pulls spdlog via CPM with rapids-cmake](https://github.com/rapidsai/rmm/actions/runs/3595940512/jobs/6056061203#step:6:595) `-- CPM: adding package spdlog@1.8.5 (v1.8.5)` and then clobbers its own installed spdlog headers: ``` 2022-12-01T19:46:46.2167513Z ClobberWarning: This transaction has incompatible packages due to a shared path. 2022-12-01T19:46:46.2168766Z packages: conda-forge/linux-64::spdlog-1.10.0-h924138e_0, file:///tmp/conda-bld-output/linux-64::librmm-23.02.00a-cuda11_gc328a18d_11 2022-12-01T19:46:46.2169686Z path: 'include/spdlog/async.h' ``` 2. spdlog 1.10.0 doesn't work with nvcc. We need spdlog 1.11.0 to get fmt 9.1.0, which contains [a necessary fix](fmtlib/fmt#2971). However, there's some uncertainty about how to handle the spdlog conda-forge package which [no longer vendors fmt](conda-forge/spdlog-feedstock#50) while rapids-cmake is still using the upstream repo which continues to vendor fmt. Does this inconsistency matter, as long as we pin `fmt=9.1.0` in the conda package cases? @kkraus14 It looks like you weighed in on this matter [here](conda-forge/spdlog-feedstock#53) and [here](conda-forge/conda-forge-pinning-feedstock#3654). If you have insights that can help us with this quandary, please share! Authors: - Bradley Dice (https://github.com/bdice) Approvers: - Mark Harris (https://github.com/harrism) - Ray Douglass (https://github.com/raydouglass) URL: #1176

kkraus14 · 2022-12-08T02:07:23Z

We have a reported compile issue today:

/home/scratch.junzhang_sw/workspace/github/cudf/cpp/build/_deps/spdlog-src/include/spdlog/fmt/bundled/core.h(426): error #186-D: pointless comparison of unsigned integer with zero
          detected during:
            instantiation of "auto fmt::v8::detail::to_unsigned(Int)->std::make_unsigned<Int>::type [with Int=size_t]" 
/home/scratch.junzhang_sw/workspace/github/cudf/cpp/build/_deps/spdlog-src/include/spdlog/fmt/bundled/format.h(458): here
            instantiation of "auto fmt::v8::detail::fill_n(T *, Size, char)->T * [with T=char, Size=size_t]" 
/home/scratch.junzhang_sw/workspace/github/cudf/cpp/build/_deps/spdlog-src/include/spdlog/fmt/bundled/format.h(702): here
            instantiation of "fmt::v8::basic_memory_buffer<T, SIZE, Allocator>::basic_memory_buffer(const Allocator &) [with T=char, SIZE=500UL, Allocator=std::allocator<char>]" 
/home/scratch.junzhang_sw/workspace/github/cudf/cpp/build/_deps/spdlog-src/include/spdlog/fmt/bundled/format-inl.h(85): here

/home/scratch.junzhang_sw/workspace/github/cudf/cpp/build/_deps/spdlog-src/include/spdlog/fmt/bundled/core.h(426): error #186-D: pointless comparison of unsigned integer with zero
          detected during:

Is that due to this PR?

Ref: https://nvidia.slack.com/archives/CDTANRCTT/p1670243383311319

This looks like spdlog is being treated as a user include instead of a system include which then makes it evaluate warnings as errors for code that is outside of your codebase's control. I would argue that's a downstream project cmake bug as opposed to an issue with the change introduced here.

Keith Kraus added 2 commits December 1, 2022 12:54

upgrade to spdlog 1.10

3e4b24d

update dependencies.yaml and regenerate

c328a18

kkraus14 requested a review from a team as a code owner December 1, 2022 18:01

github-actions bot added the conda label Dec 1, 2022

kkraus14 commented Dec 1, 2022

View reviewed changes

ajschmidt8 added non-breaking Non-breaking change improvement Improvement / enhancement to an existing function labels Dec 1, 2022

kkraus14 mentioned this pull request Dec 1, 2022

Rebuild for spdlog111 conda-forge/librmm-feedstock#37

Closed

harrism approved these changes Dec 1, 2022

View reviewed changes

bdice approved these changes Dec 2, 2022

View reviewed changes

bdice added this to PR-WIP in v23.02 Release via automation Dec 2, 2022

ajschmidt8 approved these changes Dec 2, 2022

View reviewed changes

v23.02 Release automation moved this from PR-WIP to PR-Reviewer approved Dec 2, 2022

robertmaynard mentioned this pull request Dec 2, 2022

Upgrade spdlog to 1.10.0 rapidsai/rapids-cmake#312

Merged

7 tasks

rapids-bot bot merged commit 7bf7a76 into rapidsai:branch-23.02 Dec 2, 2022

v23.02 Release automation moved this from PR-Reviewer approved to Done Dec 2, 2022

jlowe mentioned this pull request Dec 5, 2022

[submodule-sync] bot-submodule-sync-branch-23.02 to branch-23.02 [skip ci] [bot] NVIDIA/spark-rapids-jni#794

Merged

revans2 mentioned this pull request Dec 5, 2022

Make strip optional for castToInteger and castToDecimal NVIDIA/spark-rapids-jni#797

Merged

bdice added a commit to bdice/rmm that referenced this pull request Dec 5, 2022

Revert "Upgrade to spdlog 1.10 (rapidsai#1173)"

eb70815

This reverts commit 7bf7a76.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade to spdlog 1.10 #1173

Upgrade to spdlog 1.10 #1173

kkraus14 commented Dec 1, 2022 •

edited

Loading

rapids-bot bot commented Dec 1, 2022

kkraus14 Dec 1, 2022

kkraus14 commented Dec 1, 2022

kkraus14 commented Dec 1, 2022 •

edited

Loading

ajschmidt8 commented Dec 1, 2022

ajschmidt8 commented Dec 1, 2022

codecov-commenter commented Dec 1, 2022

bdice left a comment

bdice commented Dec 2, 2022

ttnghia commented Dec 5, 2022

dantegd commented Dec 5, 2022

kkraus14 commented Dec 8, 2022

Upgrade to spdlog 1.10 #1173

Upgrade to spdlog 1.10 #1173

Conversation

kkraus14 commented Dec 1, 2022 • edited Loading

Description

Checklist

rapids-bot bot commented Dec 1, 2022

kkraus14 Dec 1, 2022

Choose a reason for hiding this comment

kkraus14 commented Dec 1, 2022

kkraus14 commented Dec 1, 2022 • edited Loading

ajschmidt8 commented Dec 1, 2022

ajschmidt8 commented Dec 1, 2022

codecov-commenter commented Dec 1, 2022

Codecov Report

bdice left a comment

Choose a reason for hiding this comment

bdice commented Dec 2, 2022

ttnghia commented Dec 5, 2022

dantegd commented Dec 5, 2022

kkraus14 commented Dec 8, 2022

kkraus14 commented Dec 1, 2022 •

edited

Loading

kkraus14 commented Dec 1, 2022 •

edited

Loading