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

[FIX] Use rapids_find_package to get cugraph-ops #2148

Merged
merged 5 commits into from
Mar 27, 2022

Conversation

trxcllnt
Copy link
Collaborator

@trxcllnt trxcllnt commented Mar 22, 2022

Depends on rapidsai/raft#583 and https://github.com/rapidsai/cugraph-ops/pull/70

This PR uses CPMFindPackage to find (or build) cugraph-ops.

This enables building from source against a local cugraph-ops build via -D cugraph-ops_ROOT=/cugraph-ops/build.

@trxcllnt trxcllnt requested a review from a team as a code owner March 22, 2022 23:37
@trxcllnt trxcllnt added 3 - Ready for Review improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Mar 22, 2022
@rlratzel
Copy link
Contributor

Since cugraph-ops is closed-source, can the community member somehow get messaging to direct them to install cugraph-ops prior to building cugraph? I think this will result in a failed github login message which could be confusing.

@BradReesWork BradReesWork added this to the 22.04 milestone Mar 23, 2022
@ChuckHastings
Copy link
Collaborator

Since cugraph-ops is closed-source, can the community member somehow get messaging to direct them to install cugraph-ops prior to building cugraph? I think this will result in a failed github login message which could be confusing.

Following up on Rick's comment above, we certainly want to make the default behavior that an external community member can checkout cugraph source code, create a conda environment and build cugraph from source and have everything work smoothly.

If someone wants to also build from cugraph-ops source (and has the ability), there needs to be a way to support that.

It's not clear to me how this PR would change the build experience for a person that doesn't have access to the cugraph-ops source code when they try and build cugraph. Can we explain that?

@trxcllnt
Copy link
Collaborator Author

trxcllnt commented Mar 23, 2022

@rlratzel can the community member somehow get messaging to direct them to install cugraph-ops prior to building cugraph? I think this will result in a failed github login message which could be confusing.

If a user already has cugraph-ops installed (i.e. via conda), CPM will find and use it. If they don't, we're in the same boat as today (CMake fails to configure for some reason). We could change the URL to a github SSH URL so it'll download for users with SSH credentials and in the RAPIDS org, and fail for users without access.

@ChuckHastings It's not clear to me how this PR would change the build experience for a person that doesn't have access to the cugraph-ops source code when they try and build cugraph. Can we explain that?

Partially answered this just now, but I should also mention AJ and I are working on publishing CPack'd .tgz builds to GH releases. So this PR would also allow someone to download cugraph-ops.tgz, unzip it somewhere, then point cugraph's build at that directory.

@rlratzel
Copy link
Contributor

If they don't, we're in the same boat as today (CMake fails to configure for some reason).

I think the developer experience, especially for the community member, is slightly worse with this change vs. what's in place IMO, since the github auth error is much less precise in describing the actual problem (ie. for community members, providing credentials won't fix the problem).

@robertmaynard 's suggestion above is what I was thinking of.

One way to address this (as I think Robert is also describing) could be to make the default behavior that of the current implementation (fail with a message that cugraph-ops couldn't be found), but then make the auto-download from the closed source repo an option. This would make the developers intent obvious and a github auth error would be better understood.

@trxcllnt
Copy link
Collaborator Author

trxcllnt commented Mar 23, 2022

@rlratzel I updated the logic to use rapids_find_package instead of rapids_cpm_find. This won't provide an option to download cugraph-ops from GH, and instead will fail if cugraph-ops is not installed or the package root provided via -D cugraph-ops_ROOT.

Not using rapids_cpm_find also means devs can't use -D CPM_cugraph-ops_SOURCE= to provide a source directory that CPM should attempt to build, but I assume that's fine? If devs have access to cugraph-ops and want to use a local clone, they can build it first and pass the build dir via -D cugraph-ops_ROOT.

I'm still partial to using rapids_cpm_find only, because it would provide us with more options both locally (-D CPM_cugraph-ops_SOURCE=) and in CI (we could add the option for PRs to change the cugraph-ops branch and CI would clone and build that branch from source).

We could try to search for it first via rapids_find_package and fall back to rapids_cpm_find -- but in the case of an external contributor who can't clone rapidsai/cugraph-ops, that will still fail with the error of not having correct GH access rights, which IIUC is the error you want to avoid.

The state of cugraph to me seems to be this:

  1. External contributors must retrieve cugraph-ops (install via conda, .tgz eventually)
  2. CMake must be able to find cugraph-ops (either implicitly or explicitly)

If neither of the above conditions are met, an external contributor building from source will fail to configure. If they cannot install cugraph-ops via conda (e.g. if avoiding conda is the reason they're building cugraph from source), and there's no other way to get the cugraph-ops binaries today, what difference does the error message make? The end result is they cannot configure cugraph and have no recourse to resolve it.

@trxcllnt trxcllnt changed the title [FIX] Use CPMFindPackage to get cugraph-ops [FIX] Use rapids_find_package to get cugraph-ops Mar 23, 2022
@trxcllnt
Copy link
Collaborator Author

rerun tests

1 similar comment
@trxcllnt
Copy link
Collaborator Author

rerun tests

@zbjornson
Copy link

zbjornson commented Mar 26, 2022

Just wanted to chime in to add a voice of support for a non-conda option here. We can't use conda in our computing environment, so we manually download and unpack libcugraphops (and libcumlprims) from anaconda.org.

@BradReesWork
Copy link
Member

@gpucibot merge

@rapids-bot rapids-bot bot merged commit ad770d2 into rapidsai:branch-22.04 Mar 27, 2022
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.

6 participants