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] Use chebyshev, canberra, hellinger and minkowski distance metrics #3990

Merged
merged 14 commits into from
Jul 8, 2021

Conversation

mdoijade
Copy link
Contributor

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

@mdoijade mdoijade requested review from a team as code owners June 15, 2021 15:43
@github-actions github-actions bot added CUDA/C++ Cython / Python Cython or Python issue labels Jun 15, 2021
@mdoijade mdoijade requested a review from a team as a code owner June 15, 2021 16:42
@github-actions github-actions bot added the CMake label Jun 15, 2021
@@ -44,6 +44,6 @@ set(CUML_BRANCH_VERSION_raft "${CUML_VERSION_MAJOR}.${CUML_VERSION_MINOR}")
# To use a different RAFT locally, set the CMake variable
# CPM_raft_SOURCE=/path/to/local/raft
find_and_configure_raft(VERSION ${CUML_MIN_VERSION_raft}
FORK rapidsai
PINNED_TAG branch-${CUML_BRANCH_VERSION_raft}
FORK mdoijade
Copy link
Member

Choose a reason for hiding this comment

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

Just a note here to refert this once merged

@cjnolet 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
@cjnolet cjnolet added this to PR-WIP in v21.08 Release via automation Jun 15, 2021
@mdoijade 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
Copy link
Contributor Author

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

@teju85
Copy link
Member

teju85 commented Jun 18, 2021

rerun tests

@dantegd
Copy link
Member

dantegd commented Jun 21, 2021

@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
Copy link
Contributor Author

@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
Copy link
Member

dantegd commented Jun 22, 2021

@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?

@mdoijade
Copy link
Contributor Author

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
v21.08 Release automation moved this from PR-WIP to PR-Reviewer approved Jul 6, 2021
@teju85
Copy link
Member

teju85 commented Jul 8, 2021

rerun tests

v21.08 Release automation moved this from PR-Reviewer approved to PR-Needs review Jul 8, 2021
@github-actions github-actions bot added the gpuCI gpuCI issue label Jul 8, 2021
@codecov-commenter
Copy link

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.

Impacted file tree graph

@@               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.

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

@JohnZed
Copy link
Contributor

JohnZed commented Jul 8, 2021

@gpucibot merge

v21.08 Release automation moved this from PR-Needs review to PR-Reviewer approved Jul 8, 2021
@rapids-bot rapids-bot bot merged commit 954d7cb into rapidsai:branch-21.08 Jul 8, 2021
v21.08 Release automation moved this from PR-Reviewer approved to Done Jul 8, 2021
vimarsh6739 pushed a commit to vimarsh6739/cuml that referenced this pull request Oct 9, 2023
…pidsai#3990)

This PR relies on RAFT PR rapidsai/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: rapidsai#3990
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2 - In Progress Currenty a work in progress CMake CUDA/C++ Cython / Python Cython or Python issue feature request New feature or request gpuCI gpuCI issue non-breaking Non-breaking change
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

7 participants