[REVIEW] Use chebyshev, canberra, hellinger and minkowski distance metrics by mdoijade · Pull Request #3990 · NVIDIA/cuml

mdoijade · 2021-06-15T15:43:51Z

This PR relies on RAFT PR NVIDIA/raft#276 which adds these distance metrics support.

…m raft. update test_metrics pytest accordingly

… dist branch

cjnolet · 2021-06-15T16:49:54Z

 find_and_configure_raft(VERSION    ${CUML_MIN_VERSION_raft}
-                        FORK       rapidsai
-                        PINNED_TAG branch-${CUML_BRANCH_VERSION_raft}
+                        FORK       mdoijade


Just a note here to refert this once merged

mdoijade · 2021-06-17T13:07:00Z

@dantegd why are these CI tests stuck since days? can you please rerun test?

teju85 · 2021-06-18T09:13:57Z

rerun tests

dantegd · 2021-06-21T21:37:27Z

@mdoijade is there something in this PR or the RAFT one that increases compile times dramatically? The CI log once again timed out during compilation, I haven't seen it happen in any other PR (will keep an eye on that), so it might be something particular to the changes here

mdoijade · 2021-06-22T08:32:51Z

@mdoijade is there something in this PR or the RAFT one that increases compile times dramatically? The CI log once again timed out during compilation, I haven't seen it happen in any other PR (will keep an eye on that), so it might be something particular to the changes here

@dantegd after adding these 4 distance metrics the compilation time on cuML increases by almost 4x because of which the build is getting timed out.
I am trying to see what I can do to reduce the build time. is there any suggestions here / std way in cuML? like instead of including pairwise distance as kernel headers in cuML building them separately and linking?

dantegd · 2021-06-22T15:05:44Z

@dantegd after adding these 4 distance metrics the compilation time on cuML increases by almost 4x because of which the build is getting timed out.

Does that mean 4 times the total compile time of libcuml++ or only for the particular files/targets related to the distances? If it is for the total amount of time and it is expected, then that is something we really need to solve before merging the PR

I am trying to see what I can do to reduce the build time. is there any suggestions here / std way in cuML? like instead of including pairwise distance as kernel headers in cuML building them separately and linking?

If the increase build time is due to template instantiations, an approach like the one here #3514 doing explicit instantiations, but it depends what is the reason behind the increase in compile time. I was wondering if you could provide some more explanation of what is causing the increase?

…avoid multiple recompilation

mdoijade · 2021-06-23T12:53:11Z

Does that mean 4 times the total compile time of libcuml++ or only for the particular files/targets related to the distances? If it is for the total amount of time and it is expected, then that is something we really need to solve before merging the PR

I think it is libcuml++ and also the tests ml/prims. I have now tried to use pairwise_distance from "<cuml/metrics/metrics.hpp>" instead of raft wherever feasible to reuse the compiled kernels from pairwise_distance.cu in cuML. that does reduce the compilation time by a good margin but still it takes time due to increase in number of pairwise_distance kernel.

…to enable parallel compilation. this helps reduce build time substantially with cuda 11.2 but with cuda 11.0 it is still slow due to perhaps some compiler issue with pow/powf

teju85 · 2021-07-08T02:22:31Z

rerun tests

codecov-commenter · 2021-07-08T07:07:05Z

Codecov Report

❗ No coverage uploaded for pull request base (branch-21.08@f71d369). Click here to learn what that means.
The diff coverage is n/a.

@@               Coverage Diff               @@
##             branch-21.08    #3990   +/-   ##
===============================================
  Coverage                ?   85.46%           
===============================================
  Files                   ?      230           
  Lines                   ?    18139           
  Branches                ?        0           
===============================================
  Hits                    ?    15502           
  Misses                  ?     2637           
  Partials                ?        0

Flag	Coverage Δ
dask	`48.14% <0.00%> (?)`
non-dask	`77.74% <0.00%> (?)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f71d369...32859ea. Read the comment docs.

cjnolet

LGTM

JohnZed · 2021-07-08T18:08:06Z

@gpucibot merge

…IDIA#3990) This PR relies on RAFT PR NVIDIA/raft#276 which adds these distance metrics support. Authors: - Mahesh Doijade (https://github.com/mdoijade) Approvers: - Dante Gama Dessavre (https://github.com/dantegd) - Corey J. Nolet (https://github.com/cjnolet) - AJ Schmidt (https://github.com/ajschmidt8) URL: NVIDIA#3990

mdoijade added 2 commits June 15, 2021 21:10

use canberra, minkowski, hellinger and chebyshev distance metrics fro…

4d05759

…m raft. update test_metrics pytest accordingly

Merge branch 'branch-21.08' into use_new_dist_metrics

638c821

mdoijade requested review from a team as code owners June 15, 2021 15:43

github-actions Bot added CUDA/C++ Cython / Python Cython or Python issue labels Jun 15, 2021

mdoijade added 2 commits June 15, 2021 21:24

fix clang format issues

81a9561

fix issues reported by flake check and pin raft repo to mdoijade fork…

3c25b7b

… dist branch

mdoijade requested a review from a team as a code owner June 15, 2021 16:42

github-actions Bot added the CMake label Jun 15, 2021

cjnolet reviewed Jun 15, 2021

View reviewed changes

cjnolet added 2 - In Progress Currenty a work in progress feature request New feature or request non-breaking Non-breaking change labels Jun 15, 2021

mdoijade changed the title ~~[WIP] Use chebyshev, canberra, hellinger and minkowski distance metrics~~ [REVIEW] Use chebyshev, canberra, hellinger and minkowski distance metrics Jun 17, 2021

mdoijade added 3 commits June 22, 2021 22:00

Merge branch 'branch-21.08' into use_new_dist_metrics

dad4f69

use pairwise distance from cuml instead of raft wherever feasible to …

1efac85

…avoid multiple recompilation

Merge branch 'branch-21.08' into use_new_dist_metrics

04470a8

mdoijade mentioned this pull request Jun 23, 2021

[REVIEW] Add chebyshev, canberra, minkowksi and hellinger distance metrics NVIDIA/raft#276

Merged

mdoijade added 5 commits June 29, 2021 19:01

split pairwise_distance.cu into individual dist metrics source files …

8525688

…to enable parallel compilation. this helps reduce build time substantially with cuda 11.2 but with cuda 11.0 it is still slow due to perhaps some compiler issue with pow/powf

Merge branch 'branch-21.08' into use_new_dist_metrics

77d81dc

fix headers formatting in canberra, chebyshev, hellinger and l1

f43e897

temp commit to trigger ci test

67e0b05

fix doc bug in metrics.hpp of new parameter metric_arg

c46d923

revert mdoijade raft fork use as those changes are merged

4e00e25

dantegd approved these changes Jul 6, 2021

View reviewed changes

teju85 mentioned this pull request Jul 8, 2021

Temporarily revert #276 NVIDIA/raft#288

Closed

install clang tools 8.0.1

32859ea

github-actions Bot added the gpuCI gpuCI issue label Jul 8, 2021

cjnolet approved these changes Jul 8, 2021

View reviewed changes

wphicks mentioned this pull request Jul 8, 2021

Updating Clang Version to 11.0.0 #4029

Merged

ajschmidt8 approved these changes Jul 8, 2021

View reviewed changes

rapids-bot Bot merged commit 954d7cb into NVIDIA:branch-21.08 Jul 8, 2021

Navigation Menu

Uh oh!

[REVIEW] Use chebyshev, canberra, hellinger and minkowski distance metrics - #3990

[REVIEW] Use chebyshev, canberra, hellinger and minkowski distance metrics#3990
rapids-bot[bot] merged 14 commits into
NVIDIA:branch-21.08from
mdoijade:use_new_dist_metrics

mdoijade commented Jun 15, 2021

Uh oh!

cjnolet Jun 15, 2021

Uh oh!

mdoijade commented Jun 17, 2021

Uh oh!

teju85 commented Jun 18, 2021

Uh oh!

dantegd commented Jun 21, 2021

Uh oh!

mdoijade commented Jun 22, 2021

Uh oh!

dantegd commented Jun 22, 2021

Uh oh!

mdoijade commented Jun 23, 2021

Uh oh!

teju85 commented Jul 8, 2021

Uh oh!

codecov-commenter commented Jul 8, 2021

Uh oh!

cjnolet left a comment

Uh oh!

JohnZed commented Jul 8, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Uh oh!

Conversation

mdoijade commented Jun 15, 2021

Uh oh!

cjnolet Jun 15, 2021

Choose a reason for hiding this comment

Uh oh!

mdoijade commented Jun 17, 2021

Uh oh!

teju85 commented Jun 18, 2021

Uh oh!

dantegd commented Jun 21, 2021

Uh oh!

mdoijade commented Jun 22, 2021

Uh oh!

dantegd commented Jun 22, 2021

Uh oh!

mdoijade commented Jun 23, 2021

Uh oh!

teju85 commented Jul 8, 2021

Uh oh!

codecov-commenter commented Jul 8, 2021

Codecov Report

Uh oh!

cjnolet left a comment

Choose a reason for hiding this comment

Uh oh!

JohnZed commented Jul 8, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants