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 minor errors in CMake configuration #662

Merged
merged 5 commits into from
May 20, 2022

Conversation

vyasr
Copy link
Contributor

@vyasr vyasr commented May 19, 2022

This PR fixes a number of small errors that slipped through in #644 and #633. None of them materially affect the build (whether a preexisting libraft exists or not) since 1) benchmarks are currently turned off by default anyway in the C++ build, and 2) pyraft only depends on libraft for headers, not for compiled libraries.

@vyasr vyasr requested a review from a team as a code owner May 19, 2022 00:04
@cjnolet cjnolet added improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels May 19, 2022
@vyasr vyasr marked this pull request as draft May 19, 2022 01:29
@vyasr
Copy link
Contributor Author

vyasr commented May 19, 2022

I don't yet have permissions to add labels, otherwise I would have added DO NOT MERGE instead of marking this as draft.

The reason this issue didn't crop up before is that after getting the UCX/NCCL dependencies finalized in #644 I did not go back and rebuild/test pyraft without a precompiled C++ build. The typos I fixed here will help fix that case, but they aren't sufficient. I will also need to install the built libraft library into the install tree and modify the rpaths for the dask/common and common/ modules. I will make these changes locally and test before we merge.

@cjnolet
Copy link
Member

cjnolet commented May 19, 2022

And hopefully by the time this is ready to merge, you'll have the permissions to merge it yourself

@vyasr
Copy link
Contributor Author

vyasr commented May 20, 2022

@cjnolet so it turns out that I did in fact previously test this, and the current build will work even without the changes that I proposed in my last message. The important distinction between pyraft and pylibraft is that pyraft actually doesn't depend on raft compiled libraries at all, it just requires some of the headers. Those headers are already being inherited by linking in the raft::raft target, so we should be set. I've done a little more cleanup here, but aside from that I think we're good.

@vyasr vyasr changed the title Fix typos in project names. Fix minor errors in CMake configuration May 20, 2022
@vyasr vyasr marked this pull request as ready for review May 20, 2022 19:20
@cjnolet
Copy link
Member

cjnolet commented May 20, 2022

Want to go ahead and merge it?

@vyasr
Copy link
Contributor Author

vyasr commented May 20, 2022

@gpucibot merge

@rapids-bot rapids-bot bot merged commit 6e8e41d into rapidsai:branch-22.06 May 20, 2022
@vyasr
Copy link
Contributor Author

vyasr commented May 20, 2022

Done!

@vyasr vyasr deleted the fix/project_name branch May 20, 2022 19:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CMake improvement Improvement / enhancement to an existing function non-breaking Non-breaking change python
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants