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

Remove run-export of librmm. #1673

Draft
wants to merge 1 commit into
base: branch-24.10
Choose a base branch
from

Conversation

bdice
Copy link
Contributor

@bdice bdice commented Sep 9, 2024

Description

librmm has a run export on itself, which is not correct since it is header-only and therefore should impose no runtime requirement. See associated issue: rapidsai/build-planning#92

This self-run-export was originally added to make librmm suitable for use in a downstream "dev" package (which includes all of its "dev" dependencies) but in practice it has caused a lot of problems. For example, this indirectly requires spdlog and fmt to be present in all environments using rmm/librmm even if those headers are not needed in a runtime environment (only in a build environment). We plan to eventually expose RAPIDS -dev packages (see rapidsai/build-planning#46) which will solve this problem more cleanly.

Helps with rapidsai/build-planning#56.

We've discussed this as being a bug in rapidsai/build-planning#92, and it seems to have negative effects seen in rapidsai/cuspatial#1453 (comment) (despite us having other workarounds in that case). This change could be briefly disruptive to RAPIDS CI if we have downstream bugs (where we should've listed librmm in build/host requirements but did not do so) so I have marked it as breaking.

Checklist

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

@github-actions github-actions bot added the conda label Sep 9, 2024
@bdice bdice added breaking Breaking change improvement Improvement / enhancement to an existing function labels Sep 9, 2024
@bdice bdice self-assigned this Sep 9, 2024
@bdice bdice marked this pull request as ready for review September 9, 2024 13:11
@bdice bdice requested a review from a team as a code owner September 9, 2024 13:11
@msarahan
Copy link

msarahan commented Sep 9, 2024

It would be nice to switch stuff to the -dev packages at the same time we're doing this, but the timing just isn't right. I say it would be nice because we'd only have to touch the many recipes once, rather than with this, we may need to add librmm manually, then when we have the -dev packages, we should (to be as clean as possible, not necessary to be correct) remove the librmm manual dependency, and add the -dev package to the host dependencies.

Practicality beats purity, and we definitely should not wait on the -dev package work to do this change.

@bdice bdice added the 5 - DO NOT MERGE Hold off on merging; see PR for details label Sep 9, 2024
@bdice
Copy link
Contributor Author

bdice commented Sep 9, 2024

I am writing up some further concerns with this from conversations with @wence- — marking as do not merge for now.

@bdice
Copy link
Contributor Author

bdice commented Sep 9, 2024

I discussed this offline with @wence- and we had a few observations. Summarizing here:

  • RMM provides no ABI stability guarantees, so packages like rmm (Python) and libcudf (C++) are only guaranteed to have the same ABI
  • The librmm conda package is effectively acting as an ABI constraint package. Because librmm is run-exporting itself, all packages that were built with librmm in an environment must have the same librmm dependency, and therefore have the same ABI.
  • We have no such constraint in wheels (afaik) but seemingly have had no issues reported.
  • It would be possible to have a separate librmm-abi-version package indicating the ABI constraint without requiring other dependencies or shipping headers.
  • We could alternatively remove spdlog/fmt from the librmm run dependencies to reduce our downstream constraints (thereby achieving a similar goal to this PR's original intent), but there is at least one catch: we expose the spdlog::logger object that RMM uses (via rmm::logger()), implying an ABI-runtime requirement on spdlog from rmm. Maybe we could deprecate that or avoid the ABI leak from spdlog.
    • spdlog is also used in cudf, cuml, and raft but @wence- believes cuml and raft are "safe" (no spdlog ABI leakage)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
5 - DO NOT MERGE Hold off on merging; see PR for details breaking Breaking change conda improvement Improvement / enhancement to an existing function
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

2 participants