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.

Sign up for GitHub

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

Jump to bottom

Unify use of common functors #1049

Merged
merged 24 commits into from
Dec 14, 2022
Merged

Unify use of common functors #1049

merged 24 commits into from
Dec 14, 2022

Conversation

Nyrio
Copy link
Contributor

@Nyrio Nyrio commented Nov 25, 2022

The same functors are redefined in many different files. I believe this change can help with compilation times, executable size, and consistency.

@Nyrio Nyrio requested a review from a team as a code owner November 25, 2022 18:59
@github-actions github-actions bot added the cpp label Nov 25, 2022
@Nyrio Nyrio added non-breaking Non-breaking change enhancement New feature or request improvement Improvement / enhancement to an existing function and removed enhancement New feature or request labels Nov 25, 2022
@Nyrio
Copy link
Contributor Author

Nyrio commented Nov 25, 2022

Notes regarding #1024 since it's related:

  • If I remove the template arguments of all these functors, it will be a breaking change. I also have the option of setting them to void and throwing a deprecation warning if set, then we can remove them later.
  • Any thoughts on whether there are cases where we don't want the types to be inferred? Some prims have different InT/OutT but many of these functors are just implicitly casting the output anyway.

@Nyrio Nyrio requested review from a team as code owners November 30, 2022 13:37
@Nyrio Nyrio changed the base branch from branch-22.12 to branch-23.02 November 30, 2022 13:37
@Nyrio
Copy link
Contributor Author

Nyrio commented Nov 30, 2022

(sorry for the irrelevant review triggers, I updated the target branch)

@github-actions github-actions bot added the CMake label Dec 9, 2022
@Nyrio Nyrio requested a review from achirkin December 9, 2022 13:59
@Nyrio Nyrio assigned cjnolet and achirkin and unassigned Nyrio Dec 9, 2022
@Nyrio Nyrio added 4 - Waiting on Reviewer Waiting for reviewer to review or respond and removed 4 - Waiting on Author Waiting for author to respond to review labels Dec 9, 2022
@Nyrio
Copy link
Contributor Author

Nyrio commented Dec 9, 2022

I've added full test coverage of the operators on both host and device.
I had to move some utils from test_utils.h to a new file test_utils.cuh in the process to compile host-only tests with a host compiler.

achirkin
achirkin approved these changes Dec 9, 2022
View reviewed changes
Copy link
Contributor

@achirkin achirkin left a comment

Choose a reason for hiding this comment

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

Thanks @Nyrio, for the tests! Looks good to me. Here's just one comment that does not really need to be addressed.

cpp/include/raft/core/operators.hpp Show resolved Hide resolved
@Nyrio Nyrio added 5 - Ready to Merge and removed 4 - Waiting on Reviewer Waiting for reviewer to review or respond labels Dec 12, 2022
@Nyrio
Copy link
Contributor Author

Nyrio commented Dec 13, 2022

I don't think the CI failures have much to do with my changes.

/opt/conda/envs/rapids/bin/gtests/libraft/CLUSTER_TEST:
error while loading shared libraries: libnvJitLink.so.12: cannot open shared object file: No such file or directory

@robertmaynard
Copy link
Contributor

I don't think the CI failures have much to do with my changes.

/opt/conda/envs/rapids/bin/gtests/libraft/CLUSTER_TEST:
error while loading shared libraries: libnvJitLink.so.12: cannot open shared object file: No such file or directory

Something is pulling in and linking to CUDA 12. libnvJitLink is a new library introduced in 12.

@Nyrio
Copy link
Contributor Author

Nyrio commented Dec 13, 2022

Perhaps this?:

libcusparse:      12.0.0.76-0                              nvidia

@robertmaynard
Copy link
Contributor

Perhaps this?:

libcusparse:      12.0.0.76-0                              nvidia

Yes that would do it. @cjnolet Is the presumption we wait on GHA or is there an open PR that fixes the current env?

@cjnolet
Copy link
Member

cjnolet commented Dec 13, 2022

@robertmaynard, the plan is to submit the fix that @dantegd has been testing in cuml to pin the libcusolver and libcusparse packages so the offending cuda 12 binary is never installed/linked.

@cjnolet
Copy link
Member

cjnolet commented Dec 13, 2022

rerun tests

@ajschmidt8 ajschmidt8 removed the request for review from a team December 14, 2022 00:58
@ajschmidt8
Copy link
Member

Removing ops-codeowners from the required reviews since it doesn't seem there are any file changes that we're responsible for. Feel free to add us back if necessary.

cjnolet
cjnolet approved these changes Dec 14, 2022
View reviewed changes
Copy link
Member

@cjnolet cjnolet left a comment

Choose a reason for hiding this comment

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

LGTM

@cjnolet
Copy link
Member

cjnolet commented Dec 14, 2022

Before this is merged, we might want to run a quick test PR through cuml's CI pinned to this branch just to make sure nothing has broken there.

@Nyrio Nyrio mentioned this pull request Dec 14, 2022
Closed
@cjnolet
Copy link
Member

cjnolet commented Dec 14, 2022

@gpucibot merge

@rapids-bot rapids-bot bot merged commit 0039f33 into rapidsai:branch-23.02 Dec 14, 2022
@Nyrio Nyrio mentioned this pull request Dec 15, 2022
Merged
@ahendriksen ahendriksen mentioned this pull request Jan 11, 2023
Merged
rapids-bot bot pushed a commit that referenced this pull request Jan 12, 2023
Add raft::void_op functor (#1136)
bbe0755 
Follow up on PR #1049. Adds a void_op functor for lambdas that are unused.

Authors:
  - Allard Hendriksen (https://github.com/ahendriksen)

Approvers:
  - Louis Sugy (https://github.com/Nyrio)
  - Artem M. Chirkin (https://github.com/achirkin)
  - Corey J. Nolet (https://github.com/cjnolet)

URL: #1136
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@achirkin achirkin achirkin approved these changes

@tfeher tfeher tfeher approved these changes

@cjnolet cjnolet cjnolet approved these changes

Assignees

@cjnolet cjnolet

Labels
5 - Ready to Merge CMake cpp improvement Improvement / enhancement to an existing function non-breaking Non-breaking change
Projects
VS/ML/DM Primitives Release Board
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@Nyrio @achirkin @robertmaynard @cjnolet @ajschmidt8 @tfeher