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

libcugraph conda package requires RMM, which is not used by the vast majority of users #1682

Closed
Tracked by #3256
rlratzel opened this issue Jun 21, 2021 · 4 comments
Closed
Tracked by #3256
Assignees

Comments

@rlratzel
Copy link
Contributor

rlratzel commented Jun 21, 2021

C++ users installing the libcugraph package require librmm headers since the cugraph headers expose the RMM includes. The librmm headers are technically a build-time dependency, and C++ users should not need those when developing with libcugraph.

libcugraph is a package containing both a runtime library (needed by all users), and a development package that installs headers (needed only by devs integrating libcugraph functionality into their application/library, which makes up a very small set of users). Because of the latter, RMM is installed as a dependency since RMM is part of the libcugraph API, yet RMM (being a header-only library) is not needed at runtime.

As with any dependency, this results in the vast majority of users having to install an extra package. This increases the risk of incompatibilities and makes the conda solver spend more time resolving dependencies. It also means there is a potential for users needing a different version of RMM for other reasons to end up with broken installs.

If RMM could be removed as a dependency (either change the libcugraph API to not use RMM types, create separate dev and runtime packages, or something else), many users would benefit.

This became a problem recently when packaging libcugraph for conda-forge, and the workaround was to create a separate librmm header-only package and have users install it as a runtime dependency. conda-forge/libcugraph-feedstock#10

@rlratzel rlratzel added bug Something isn't working ? - Needs Triage Need team to review and classify labels Jun 21, 2021
@rlratzel rlratzel added Fix and removed bug Something isn't working labels Jun 21, 2021
@rlratzel rlratzel changed the title [BUG] libcugraph leaks the RMM dependency libcugraph leaks the RMM dependency Jun 21, 2021
@BradReesWork BradReesWork removed the ? - Needs Triage Need team to review and classify label Aug 5, 2021
@BradReesWork BradReesWork added this to the 21.10 milestone Aug 5, 2021
@ChuckHastings
Copy link
Collaborator

Do we think this needs to be fixed in libcugraph, or just in libcugraph_c?

@ChuckHastings
Copy link
Collaborator

To follow up (as we discussed separately), there is an API dependency on rmm. Functions that allocate memory in C++ and return it to python return rmm::device_buffer and rmm::device_uvector objects.

The new API that Andrei has designed should completely hide this dependency. If we want to eliminate the dependency before that we will need to look at each API call that relies on RMM and identify how to eliminate that dependency. I will assess the amount of work required and identify when we can get to this change.

@rlratzel
Copy link
Contributor Author

rlratzel commented Sep 20, 2021

The most noticeable impact of this issue is the user installing our conda packages. Today, libcugraph installs librmm as a runtime dependency, since libcugraph is both a build-time package (it installs headers needed by developers that want to call cugraph C++ APIs, and those devs will need the RMM headers too since cugraph's headers pull them in) and runtime package (it installs a .so for the runtime lib needed by all users, but those users don't need RMM headers). This won't change as long as RMM is used in the cugraph C++ API. Note: we could release a separate libcugraph-devel package for developers, then libcugraph could be only for the runtime and RMM wouldn't be required, but we're not thinking that's what we want right now.

The C API can hide the RMM types, and therefore wouldn't need to depend on RMM for developer or runtime use cases. However, the C API is currently/will be installed by the same libcugraph conda package, so users would still get the RMM dependency. Instead, we could make a new libcugraph_c conda package that packages like pylibcugraph can depend on, which will not depend on RMM. For this to work, libcugraph_c cannot depend on libcugraph though, since that will still result in having RMM installed. Not depending on libcugraph may be complicated though, since it involves both libcugraph and libcugraph_c packaging the same libcugraph.so file, and may prevent a side-by-side install of both packages.

If we want to go back to the idea of just removing RMM from the C++ API, @ChuckHastings mentions:

If we want to remove RMM from the C++ API we would need to do the following:

  • Create a graph handle that wraps the raft handle and adds whatever RMM features we need to pass through the API
  • Create an analog to the rmm::device_uvector that we can pass as input and output from all of the APIs that require this type of mechanism
  • Modify our APIs to use these new features.

so this issue will require more research.

@rlratzel
Copy link
Contributor Author

rlratzel commented Sep 21, 2021

Not depending on libcugraph may be complicated though, since it involves both libcugraph and libcugraph_c packaging the same libcugraph.so file, and may prevent a side-by-side install of both packages.

Another option: instead of having libcugraph and libcugraph_c both install libcugraph.so, we could have libcugraph_c.so link all the same object files linked by libcugraph.so, plus the additional ones needed for the C API. That way, each conda package still includes only unique .so files. libcugraph_c.so ends up being a superset of libcugraph.so and neither of them depend on each other. This would be a change made in the CMakeLists.txt file. Some downsides, of course, are the larger package size and extra space on disk when both packages are installed, and the potentially longer build/link time.

Note that this only solves the problem for users of the libcugraph_c C API package - users requiring the libcugraph C++ package for only the runtime lib still have the extra RMM dependency...unless they depend on libcugraph_c just for the runtime lib (since it's a superset), but that could be confusing. actually, this probably won't work because the .so files are named differently in the different packages, so an app built with libcugraph could not have its runtime deps satisfied with the libcugraph_c package

@rlratzel rlratzel changed the title libcugraph leaks the RMM dependency libcugraph conda package requires RMM, which is not used by the vast majority of users Sep 21, 2021
@ChuckHastings ChuckHastings modified the milestones: 21.10, 21.12 Sep 22, 2021
@ChuckHastings ChuckHastings modified the milestones: 21.12, 22.02 Nov 10, 2021
@BradReesWork BradReesWork modified the milestones: 22.02, 22.06 Jan 26, 2022
@rapidsai rapidsai deleted a comment from github-actions bot Mar 10, 2022
@rapidsai rapidsai deleted a comment from github-actions bot Mar 10, 2022
@BradReesWork BradReesWork removed this from the 22.06 milestone Jun 1, 2022
@kingmesal kingmesal added bug Something isn't working and removed Fix labels Feb 9, 2023
@rapidsai rapidsai deleted a comment from github-actions bot Feb 22, 2023
@rapidsai rapidsai deleted a comment from github-actions bot Feb 22, 2023
@kingmesal kingmesal assigned rlratzel and unassigned ChuckHastings Aug 7, 2023
@kingmesal kingmesal removed the bug Something isn't working label Aug 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants