rapidsai / rmm

RAPIDS Memory Manager
https://docs.rapids.ai/api/rmm/stable/
Apache License 2.0
478 stars 195 forks source link

Remove run-export of librmm. #1673

Open bdice opened 3 weeks ago

bdice commented 3 weeks ago

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: https://github.com/rapidsai/build-planning/issues/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 https://github.com/rapidsai/build-planning/issues/46) which will solve this problem more cleanly.

Helps with https://github.com/rapidsai/build-planning/issues/56.

We've discussed this as being a bug in https://github.com/rapidsai/build-planning/issues/92, and it seems to have negative effects seen in https://github.com/rapidsai/cuspatial/pull/1453#issuecomment-2334885379 (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

msarahan commented 3 weeks ago

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 commented 3 weeks ago

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

bdice commented 3 weeks ago

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