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

[REVIEW] Moving cuML prims to RAFT #65

Merged
merged 19 commits into from
Oct 23, 2020

Conversation

divyegala
Copy link
Member

@divyegala divyegala commented Sep 9, 2020

This PR moves cuml prims to RAFT that will break the circular dependency between libcuml and libcumlprims. The changes made in these files are almost exactly the same as those mirrored by cuML, with only the header paths updated to reflect how raft does it:

  1. Like #include <common/cudart_utils.h> became #include <raft/cudart_utils.h>
  2. "#include "test_utils.h" became "../test_utils.h" (Our prims gtests are all in the same, root level tests/prims folder but I took the liberty of dividing it more as tests/linalg, tests/stats, etc.)

In any case, these changes will not affect when we finally delete prims from cuML. Only thing we would need to do there is also update the include path for these prims (files moved), as they are already in the correct formatting and namespaces.

@GPUtester
Copy link
Contributor

Please update the changelog in order to start CI tests.

View the gpuCI docs here.

@divyegala divyegala changed the title [WIP] Moving cuML prims to RAFT [WIP] Moving cuML rng prims to RAFT Sep 10, 2020
@divyegala divyegala changed the title [WIP] Moving cuML rng prims to RAFT [REVIEW] Moving cuML rng prims to RAFT Sep 30, 2020
@JohnZed
Copy link
Contributor

JohnZed commented Oct 1, 2020

We discussed offline that the build changes are new, but can you clarify the other files - are they all exact copies of the recently-updated cuml versions? Or are there additional changes?

@divyegala
Copy link
Member Author

@JohnZed updated the description of the PR to clarify

@raydouglass
Copy link
Member

rerun tests

@divyegala divyegala requested a review from a team as a code owner October 6, 2020 17:34
@divyegala divyegala changed the base branch from branch-0.16 to branch-0.17 October 6, 2020 17:34
@mike-wendt mike-wendt removed the request for review from a team October 6, 2020 19:39
@mike-wendt
Copy link
Contributor

Doesn't look like we're needed for code review, removing @rapidsai/ops-codeowners; add us back if necessary. Thanks!

@divyegala divyegala changed the title [REVIEW] Moving cuML rng prims to RAFT [REVIEW] Moving cuML prims to RAFT Oct 21, 2020
Copy link
Contributor

@JohnZed JohnZed left a comment

Choose a reason for hiding this comment

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

Looks straightforward to me! Thanks for making the move.

cpp/include/raft/cudart_utils.h Show resolved Hide resolved
@divyegala
Copy link
Member Author

@JohnZed added the leftover prims, as I mentioned on the call. Reference cuml PR is rapidsai/cuml#3044

@JohnZed
Copy link
Contributor

JohnZed commented Oct 23, 2020

GIven approvals from both ML and graph, I'll go ahead and merge. Excellent stuff, @divyegala !

@JohnZed JohnZed merged commit eee0193 into rapidsai:branch-0.17 Oct 23, 2020
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

Successfully merging this pull request may close these issues.

7 participants