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

Clean up public api #2398

Merged
merged 3 commits into from
Jul 12, 2022

Conversation

ChuckHastings
Copy link
Collaborator

We are exposing some internal implementation details in our public header files. This PR addresses exposing cuco in the public header files. It is the start of the effort to clean up the public API for libcugraph.

Ultimately our plan is to migrate the graph primitives into a separate library. This PR does move a bunch of the primitive headers out of the libcugraph public API.

@ChuckHastings ChuckHastings requested a review from a team as a code owner July 11, 2022 16:11
@ChuckHastings ChuckHastings self-assigned this Jul 11, 2022
@ChuckHastings ChuckHastings added 3 - Ready for Review improvement Improvement / enhancement to an existing function breaking Breaking change labels Jul 11, 2022
@ChuckHastings ChuckHastings added this to the 22.08 milestone Jul 11, 2022
Copy link
Contributor

@seunghwak seunghwak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I am approving this, but is there really a fundamental reason we should not include cuco in the public space? (why yes for thrust but no for cuco?)

If later we move primitives and graph.hpp and graph_view.hpp (and few more files to create a graph/graph_view object) to a separate higher-level library, cuco should come back to the public space again.

Is this more of a temporary hack or we should never reference cuco in the public space in the future?

@ChuckHastings
Copy link
Collaborator Author

Fair question.

Anything we reference in the public API needs to be installed on the machine in order for someone to use our software as a C/C++ library. If a developer writes code and includes <cugraph/graph.hpp> or some other header from our library, then anything referenced in that header file needs to be installed. The smaller the list of requirements the easier it is for someone to compile with our code.

Obviously if they want to compile our code they will need all of our build-time dependencies. But if they install our code (say from conda) and then try and build, it would be nice to minimize the external things that we require them to have.

I'm inclined to say thrust might be a reasonable target to consider removing, since I don't think we use thrust across what I think of as our public API. I'm inclined to say that raft and rmm are going to be required, since we will want the raft::device_span and the rmm::device_uvector as inputs and outputs (respectively) of our functions.

I believe the primary motivation (from the scikit-build perspective) of removing dependency on cuco but not thrust is that thrust is already a requirement of other libraries, so it's not a burden.

@ChuckHastings
Copy link
Collaborator Author

@gpucibot merge

@rapids-bot rapids-bot bot merged commit 7a660e6 into rapidsai:branch-22.08 Jul 12, 2022
@vyasr
Copy link
Contributor

vyasr commented Jul 20, 2022

To clarify the current issue a little bit, the problem is that in the current setup cuco is in a hybrid state:

  • get_cuco.cmake does not include it in the INSTALL_EXPORT_SET, only the BUILD_EXPORT_SET, and it sets EXCLUDE_FROM_ALL=TRUE. That is how it should be treated if it were a private dependency, i.e. if it is required for building libcugraph but not for other libraries to build against libcugraph.
  • cpp/CMakeLists.txt, on the other hand, says that cuco is a public link dependency. That means that any library building against libcugraph is going to try to find cuco, but it won't succeed because libcugraph didn't actually install it.

The upshot is that any library building against libcugraph is going to think that it needs cuco, even though it actually doesn't. That applies to the Python build with scikit-build, but also any other CMake-based build that has libcugraph as a dependency. That leaves two solutions:

  1. Actually install cuco as part of the libcugraph build. This is probably the long-term solution.
  2. Remove cuco from the public APIs. That's the solution employed here, which is simpler in the short term. In addition to the changes in this PR, that will also require removing cuco from the parts of the CMake build where it's exposed as a public link dependency. I'm working on that in Make cuco a private dependency and leverage rapids-cmake #2432.

We can probably aim for 1 in the long term, but 2 is the expedient solution to enable scikit-build for cugraph Python.

rapids-bot bot pushed a commit that referenced this pull request Jul 25, 2022
After #2398 cuco is not actually part of libcugraph's public API and should not be required by consumers. Since it is already not being installed, this PR removes it from the public link interface of libcugraph. This PR also uses rapids-cmake to fetch cuco, ensuring that libcugraph remains in sync with the rest of RAPIDS.

There are a handful of tests that still rely on `libcugraph::detail` APIs that use cuco. To minimize the leakage of these dependencies, they are explicitly linked to cuco (rather than the current approach where they receive the cuco headers transitively from the libcugraph target).

Authors:
  - Vyas Ramasubramani (https://github.com/vyasr)

Approvers:
  - Rick Ratzel (https://github.com/rlratzel)
  - Chuck Hastings (https://github.com/ChuckHastings)

URL: #2432
@ChuckHastings ChuckHastings deleted the clean_up_public_api branch August 4, 2022 18:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking Breaking change improvement Improvement / enhancement to an existing function
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants