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

Add support in column major distance metrics to use contractions_nt instead of cutlass #3691

Merged
merged 25 commits into from
Apr 9, 2021
Merged

Add support in column major distance metrics to use contractions_nt instead of cutlass #3691

merged 25 commits into from
Apr 9, 2021

Conversation

mdoijade
Copy link
Contributor

@mdoijade mdoijade commented Apr 1, 2021

This change depends on RAFT pull request - rapidsai/raft#188
-- Adds column major input support in prims_benchmark's Distance bench
-- Removes cutlass from all distance metrics in column major case.
-- Removes cutlass from all the distance metrics test cases.

mdoijade and others added 15 commits March 5, 2021 22:53
@mdoijade
Remove cutlass usage in row major input for euclidean exp/unexp dista…
c0ceb93 
…nce matrix calculation. make use of contraction kernels for gemm NT calculation. add PairwiseDistances class which will serve as common class for other distance metrics like cosine as well
@mdoijade
Remove cutlass usage in row major input for cosine distance matrix ca…
c3fbbac 
…lculation. instead use PairwiseDistances/contraction class kernel for it
@mdoijade
Remove cutlass usage in row major input for L1 distance matrix calcul…
707fdb0 
…ation. instead use PairwiseDistances/contraction class kernel for it
@mdoijade
fix errors reported by clang-format and copyright checker in cosine a…
b3a56a3 
…nd l1
@mdoijade
add review changes - move kernel to base class, move common epilog co…
3ff0707 
…de of l2,cosine to base class, move sqrt case from base class to epilog, make use of assert instead of if
@mdoijade
Merge branch 'branch-0.19' into branch-0.19
755da05
@mdoijade
add useNorms template arg instead of distanceType to select if norms …
a33fc96 
…are required, also update git tag of raft to check if it fixes CI issue
@mdoijade
Merge branch 'branch-0.19' of https://github.com/mdoijade/cuml into b…
1e841ae 
…ranch-0.19
@mdoijade
fix clang-format reported errors and make RAFT git tag to be ToT of c…
559a418 
…uml main repo
@mdoijade
Merge branch 'branch-0.19' of https://github.com/rapidsai/cuml into b…
03426d7 
…ranch-0.19
@mdoijade
Add column major contraction_nt kernels usage to all pairwise distanc…
707ed1a 
…e metrics
@mdoijade
fix incorrect veclen for column major by setting bytes based on lda, …
cc2b9b6 
…ldb. this allows column major support to run correctly with rectangular matrices.
@mdoijade
Remove cutlass outputile from distance metrics template arg, grammatr…
8361492 
…ix, scores and distance metrics test cases
@mdoijade
Remove cutlass outputile from prims benchmark and sg tests which got …
b4af5f3 
…missed out in previous commit
@mdoijade
Add column/row major input support to distance prims benchmark
2d3dae9
@mdoijade mdoijade requested a review from a team as a code owner April 1, 2021 07:19
@teju85 teju85 added breaking Breaking change improvement Improvement / enhancement to an existing function Tech Debt Issues related to debt labels Apr 1, 2021
@teju85
Copy link
Member

teju85 commented Apr 1, 2021

@mdoijade please add benchmark numbers just like how you did for the row-major PR.

@teju85
Copy link
Member

teju85 commented Apr 1, 2021

Also, please resolve the conflicts wrt 0.19 branch.

@mdoijade
Copy link
Contributor Author

mdoijade commented Apr 1, 2021

Also, please resolve the conflicts wrt 0.19 branch.

done!

teju85
teju85 requested changes Apr 1, 2021
View reviewed changes
Copy link
Member

@teju85 teju85 left a comment

Choose a reason for hiding this comment

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

I'm putting this note mostly for other reviewers...
Since this depends on the above raft PR, this PR in-turn should be reflected with the cmake change to update the commit version of raft too (once that raft-PR is merged)

@mdoijade
Copy link
Contributor Author

mdoijade commented Apr 1, 2021 •
edited
Loading

Attaching performance results with prims_benchmark for Distance* cases of changes in this PR for column & row major involving using contractions_nt vs old code with cutlass for row/column major inputs.
col_and_row_major_speedups.xlsx

@teju85 overall the results for column major is following similar pattern like row major where for large inputs perf is around 2-10% bad compared to cutlass for L2, cosine but for L1 all inputs sizes 2-3x faster than cutlass version.

@mdoijade mdoijade requested a review from a team as a code owner April 5, 2021 13:58
@github-actions github-actions bot added the CMake label Apr 5, 2021
@mdoijade mdoijade changed the title [WIP] Add support in column major distance metrics to use contractions_nt instead of cutlass Add support in column major distance metrics to use contractions_nt instead of cutlass Apr 5, 2021
cjnolet
cjnolet approved these changes Apr 6, 2021
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.

Approving on behalf of cmake codeowners.

@teju85 teju85 changed the base branch from branch-0.19 to branch-0.20 April 6, 2021 01:58
teju85
teju85 approved these changes Apr 6, 2021
View reviewed changes
Copy link
Member

@teju85 teju85 left a comment

Choose a reason for hiding this comment

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

Changes LGTM.

@teju85
Copy link
Member

teju85 commented Apr 6, 2021

rerun tests

1 similar comment
@JohnZed
Copy link
Contributor

JohnZed commented Apr 6, 2021

rerun tests

@mdoijade mdoijade mentioned this pull request Apr 6, 2021
Merged
@mdoijade
Copy link
Contributor Author

mdoijade commented Apr 7, 2021

blocked by #3699

@github-actions github-actions bot removed the CMake label Apr 7, 2021
@teju85
Copy link
Member

teju85 commented Apr 8, 2021

@gpucibot merge.

@teju85
Copy link
Member

teju85 commented Apr 8, 2021

rerun tests

@teju85
Copy link
Member

teju85 commented Apr 8, 2021

Can one of the admins verify this patch?

@JohnZed I think we need to whitelist this PR?

@JohnZed
Copy link
Contributor

JohnZed commented Apr 8, 2021

add to allowlist

@teju85
Copy link
Member

teju85 commented Apr 9, 2021

@cjnolet any ideas why the CI is failing on "test_min_max_axis[cupy-csr-True-1]" test? Seems to be unrelated to this change. (here's the log)

@teju85
Copy link
Member

teju85 commented Apr 9, 2021

rerun tests

@mdoijade
Copy link
Contributor Author

mdoijade commented Apr 9, 2021

@cjnolet any ideas why the CI is failing on "test_min_max_axis[cupy-csr-True-1]" test? Seems to be unrelated to this change. (here's the log)

seems that was an intermittent issue, all tests passed now. @teju85 can we merge this?

@cjnolet
Copy link
Member

cjnolet commented Apr 9, 2021

@gpucibot merge

@rapids-bot rapids-bot bot merged commit ca3cd6e into rapidsai:branch-0.20 Apr 9, 2021
rapids-bot bot pushed a commit that referenced this pull request Apr 14, 2021
@mdoijade
Remove cutlass dependencies from cmake and elsewhere as it is no long…
a0cec3e 
…er needed. (#3717)

This PR depends on #3691 
-- Remove cutlass dependencies from cmake and elsewhere as it is no longer needed.
-- also remove now redundant source files using cutlass

Authors:
  - Mahesh Doijade (https://github.com/mdoijade)

Approvers:
  - Thejaswi. N. S (https://github.com/teju85)
  - Dante Gama Dessavre (https://github.com/dantegd)

URL: #3717
vimarsh6739 pushed a commit to vimarsh6739/cuml that referenced this pull request Oct 9, 2023
@mdoijade
Add support in column major distance metrics to use contractions_nt i…
8401d0b 
…nstead of cutlass (rapidsai#3691)

This change depends on RAFT pull request - rapidsai/raft#188
-- Adds column major input support in prims_benchmark's Distance bench
-- Removes cutlass from all distance metrics in column major case.
-- Removes cutlass from all the distance metrics test cases.

Authors:
  - Mahesh Doijade (https://github.com/mdoijade)

Approvers:
  - Corey J. Nolet (https://github.com/cjnolet)
  - Thejaswi. N. S (https://github.com/teju85)

URL: rapidsai#3691
vimarsh6739 pushed a commit to vimarsh6739/cuml that referenced this pull request Oct 9, 2023
@mdoijade
Remove cutlass dependencies from cmake and elsewhere as it is no long…
b8476fb 
…er needed. (rapidsai#3717)

This PR depends on rapidsai#3691 
-- Remove cutlass dependencies from cmake and elsewhere as it is no longer needed.
-- also remove now redundant source files using cutlass

Authors:
  - Mahesh Doijade (https://github.com/mdoijade)

Approvers:
  - Thejaswi. N. S (https://github.com/teju85)
  - Dante Gama Dessavre (https://github.com/dantegd)

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

@cjnolet cjnolet cjnolet approved these changes

@teju85 teju85 teju85 approved these changes

Assignees
No one assigned
Labels
breaking Breaking change improvement Improvement / enhancement to an existing function Tech Debt Issues related to debt
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@mdoijade @teju85 @GPUtester @JohnZed @cjnolet