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

initial creation of libcugraph_etl.so #1885

Merged
merged 19 commits into from
Nov 16, 2021

Conversation

ChuckHastings
Copy link
Collaborator

The new renumbering implementation will require C++ integration directly with cudf. In order to facilitate that, but also support our customers that won't need cudf, this PR will create a separate library (libcugraph_etl.so) which will ultimately link with libcudf.so and contain the ETL portions of cugraph that require cudf features.

This way our other libcugraph customers that don't need to reference the new library will not need to install all of the cudf dependencies.

To seed this, the PR also includes a proposed API for the new renumbering capability.

@rlratzel rlratzel added improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Oct 14, 2021
cpp/CMakeLists.txt Outdated Show resolved Hide resolved
cpp/CMakeLists.txt Outdated Show resolved Hide resolved
cpp/include/cugraph_etl/functions.hpp Outdated Show resolved Hide resolved
@BradReesWork BradReesWork added this to the 21.12 milestone Oct 20, 2021
@ChuckHastings ChuckHastings marked this pull request as ready for review October 26, 2021 19:30
@ChuckHastings ChuckHastings requested review from a team as code owners October 26, 2021 19:30
@ChuckHastings ChuckHastings self-assigned this Oct 26, 2021
* and/or dst_table that correspond to the vertex id
*
*/
std::tuple<cudf::column, cudf::column, cudf::table> renumber_cudf_tables(
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this function should return unique_ptrs of each of the cudf::column and tables. Caller can take ownership of the pointers and release when done.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Agreed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

CUDA::curand
CUDA::cusolver
CUDA::cusparse
cugraph::cugraph
Copy link
Contributor

Choose a reason for hiding this comment

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

Does cugraph:cugraph adds libcudf as well? May want to ensure libcudf is linked as well.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Seems likely. Until we have code in there I'm inclined not to worry too much about what we need. We also might be able to drop some of these entirely since they might not be used.

@ChuckHastings
Copy link
Collaborator Author

rerun tests

@ChuckHastings
Copy link
Collaborator Author

rerun tests

@ChuckHastings
Copy link
Collaborator Author

I think I have addressed the suggestions. I'd like to merge this soon if there are no other concerns.

Copy link
Contributor

@chirayuG-nvidia chirayuG-nvidia left a comment

Choose a reason for hiding this comment

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

LGTM

@ChuckHastings
Copy link
Collaborator Author

rerun tests

3 similar comments
@rlratzel
Copy link
Contributor

rlratzel commented Nov 5, 2021

rerun tests

@BradReesWork
Copy link
Member

rerun tests

@ChuckHastings
Copy link
Collaborator Author

rerun tests

@codecov-commenter
Copy link

codecov-commenter commented Nov 8, 2021

Codecov Report

❗ No coverage uploaded for pull request base (branch-21.12@15be981). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@               Coverage Diff               @@
##             branch-21.12    #1885   +/-   ##
===============================================
  Coverage                ?   70.28%           
===============================================
  Files                   ?      143           
  Lines                   ?     8922           
  Branches                ?        0           
===============================================
  Hits                    ?     6271           
  Misses                  ?     2651           
  Partials                ?        0           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 15be981...69c94ae. Read the comment docs.

@ChuckHastings ChuckHastings added the DO NOT MERGE Hold off on merging; see PR for details label Nov 9, 2021
…fter the pattern used by cudf and libkafka_cudf), in order for libcugraph to not have a new dependency on libcudf. The new etl library/package will be used only by users needing ETL functions, and the added dependency will only be required by them.
@ChuckHastings ChuckHastings requested a review from a team as a code owner November 9, 2021 19:04
@ChuckHastings
Copy link
Collaborator Author

rerun tests

@rlratzel
Copy link
Contributor

rerun tests

Want to make sure the latest cupy is being used.

@ChuckHastings ChuckHastings removed the DO NOT MERGE Hold off on merging; see PR for details label Nov 16, 2021
@jjacobelli
Copy link
Contributor

Hey, I think you also need to update the upload script to upload the new package

@rlratzel
Copy link
Contributor

@gpucibot merge

@rapids-bot rapids-bot bot merged commit e5b024e into rapidsai:branch-21.12 Nov 16, 2021
@ChuckHastings ChuckHastings deleted the fea_cugraph_etl branch February 1, 2022 16:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement Improvement / enhancement to an existing function non-breaking Non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants