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

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

Merged
merged 4 commits into from
Dec 6, 2022

Conversation

bdice
Copy link
Contributor

@bdice bdice commented Dec 5, 2022

Description

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. This means that rmm pulls spdlog via CPM with rapids-cmake -- CPM: adding package [email protected] (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'
  1. 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. However, there's some uncertainty about how to handle the spdlog conda-forge package which no longer vendors fmt 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 and here. If you have insights that can help us with this quandary, please share!

Checklist

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

@bdice bdice requested a review from a team as a code owner December 5, 2022 23:48
@github-actions github-actions bot added the conda label Dec 5, 2022
@harrism harrism added bug Something isn't working non-breaking Non-breaking change labels Dec 5, 2022
@raydouglass
Copy link
Member

@gpucibot merge

@codecov-commenter
Copy link

Codecov Report

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

Additional details and impacted files
@@              Coverage Diff               @@
##             branch-23.02   #1176   +/-   ##
==============================================
  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.

@vyasr
Copy link
Contributor

vyasr commented Dec 6, 2022

@gpucibot merge

@rapids-bot rapids-bot bot merged commit 4d1dee1 into rapidsai:branch-23.02 Dec 6, 2022
rapids-bot bot pushed a commit to rapidsai/rapids-cmake that referenced this pull request Dec 6, 2022
@kkraus14
Copy link
Contributor

kkraus14 commented Dec 8, 2022

  1. rmm's meta.yaml doesn't correctly specify spdlog as a build dep. It's incorrectly listed as a runtime dep. This means that rmm pulls spdlog via CPM with rapids-cmake -- CPM: adding package [email protected] (v1.8.5) and then clobbers its own installed spdlog headers

You linked to librmm's meta.yaml here, was that intentional? I believe the original thinking was that librmm is header only so we didn't actually need spdlog as a host entry, but in hindsight that probably triggers CPM to add it nowadays. I'll submit a PR moving it to the host section which would then use the spdlog run_export to have it in the run section.

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. However, there's some uncertainty about how to handle the spdlog conda-forge package which no longer vendors fmt 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 and here. If you have insights that can help us with this quandary, please share!

The pinning has since been updated to 1.11.0 (https://github.com/conda-forge/conda-forge-pinning-feedstock/blob/main/recipe/conda_build_config.yaml#L680-L681) so we should be able to skip 1.10.0 and go straight to 1.11.0.

As of now, the 1.11.0 package in conda-forge uses external fmt, but as you said discussion is ongoing and I'm somewhat unclear on all of the implications of the different options. That being said, it should be straightforward enough to match the conda-forge package behavior in rapids-cmake by handling fmt via CPM as well if that would be desired.

It looks like RMM C++ code uses fmt:: APIs already where it would probably make sense to directly depend on fmt instead of relying on spdlog depending on fmt. Given this, maybe makes sense to use external fmt and then re-evaluate when rmm upgrades to C++20 and std::format could be considered?

@bdice
Copy link
Contributor Author

bdice commented Dec 8, 2022

@kkraus14 See #1177, I started a PR. Your review would be welcome!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working conda non-breaking Non-breaking change
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

6 participants