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

[REVIEW] NVTX Markers for RF and RF-backend #3014

Merged
merged 47 commits into from
Feb 25, 2021
Merged

[REVIEW] NVTX Markers for RF and RF-backend #3014

merged 47 commits into from
Feb 25, 2021

Conversation

venkywonka
Copy link
Contributor

  • This PR adds NVTX Markers to major time-consuming function calls of the regressors and classifiers of RF and DecisionTrees.
  • They span both RandomForest and DecisionTree code-bases

@venkywonka
added tests for generateNextColor functionality of nvtx.cu wrapper
8d5aa33 
* this is to troubleshoot the bug in the `generateNextColor` function
@venkywonka
fixing the bug to generate colors for nvtx markers
535442c
@venkywonka
updated CHANGELOG.md
df4d65a
@venkywonka
Merge branch 'branch-0.16' into rf-nvtx-marking
71fa0e6
@venkywonka
Merge branch 'branch-0.16' of https://github.com/rapidsai/cuml into r…
72eb8d6 
…f-nvtx-marking
@venkywonka
adding nvtx markers
1c6c0e2
@venkywonka
adding NVTX markers to RFClassifier functions
1cf88c6
@venkywonka
Merge branch 'branch-0.16' of https://github.com/rapidsai/cuml into r…
09106f5 
…f-nvtx-marking
@venkywonka
nvtx fix
2ef51d7
@venkywonka
Merge branch 'branch-0.16' into rf-nvtx-marking
d0f6fef
@venkywonka
adding more markers and fixing imbalanced ones
1d7103d
@venkywonka
NVTX Markers added to classifier & regressor components of RF & decis…
00d943e 
…iontree
@venkywonka
Merge branch 'branch-0.16' of https://github.com/rapidsai/cuml into r…
a70f2b9 
…f-nvtx-marking
@venkywonka
undoing CMakeLists and .gitignore modifications, deleting raft symlinks
d07b984
@venkywonka
FIX clang-format fixes
c0b2e7a
@venkywonka
modified CHANGELOG.md
470a41c
@venkywonka
flake8 fix
032c25f
@venkywonka
flake8 fix rapidsai#2'
986ec9d
@venkywonka
Merge branch 'branch-0.16' of https://github.com/rapidsai/cuml into e…
fb936fe 
…nh-fea-rf-nvtx-markers
@venkywonka
deleted PR#2954 line from CHANGELOG
14e593d
@venkywonka venkywonka requested review from a team as code owners October 19, 2020 05:33
@GPUtester
Copy link
Contributor

Can one of the admins verify this patch?

@GPUtester
Copy link
Contributor

Please update the changelog in order to start CI tests.

View the gpuCI docs here.

@codecov-io
Copy link

codecov-io commented Oct 19, 2020 •
edited
Loading

Codecov Report

Merging #3014 (beb2027) into branch-0.18 (550121b) will increase coverage by 0.26%.
The diff coverage is n/a.

Impacted file tree graph

@@               Coverage Diff               @@
##           branch-0.18    #3014      +/-   ##
===============================================
+ Coverage        71.48%   71.75%   +0.26%     
===============================================
  Files              207      212       +5     
  Lines            16748    17089     +341     
===============================================
+ Hits             11973    12262     +289     
- Misses            4775     4827      +52
Impacted Files Coverage Δ
python/cuml/neighbors/ann.pxd 68.23% <0.00%> (-18.83%) ⬇️
python/cuml/fil/fil.pyx 91.87% <0.00%> (-1.88%) ⬇️
python/cuml/dask/neighbors/nearest_neighbors.py 28.75% <0.00%> (-1.74%) ⬇️
...ython/cuml/dask/neighbors/kneighbors_classifier.py 21.35% <0.00%> (-0.98%) ⬇️
python/cuml/svm/svm_base.pyx 94.27% <0.00%> (-0.63%) ⬇️
python/cuml/svm/svc.pyx 95.16% <0.00%> (-0.62%) ⬇️
python/cuml/ensemble/randomforestclassifier.pyx 73.66% <0.00%> (-0.06%) ⬇️
python/cuml/__init__.py 100.00% <0.00%> (ø)
python/cuml/cluster/dbscan.pyx 100.00% <0.00%> (ø)
python/cuml/neighbors/__init__.py 100.00% <0.00%> (ø)
... and 33 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 c9c8619...beb2027. Read the comment docs.

@dantegd dantegd added 2 - In Progress Currenty a work in progress CUDA / C++ CUDA issue labels Oct 19, 2020
@JohnZed
Copy link
Contributor

JohnZed commented Feb 5, 2021

@venkywonka can you take a look at the merge conflicts?

@github-actions github-actions bot added Cython / Python Cython or Python issue libcuml labels Feb 5, 2021
@venkywonka
Copy link
Contributor Author

@venkywonka can you take a look at the merge conflicts?

done @JohnZed ✌🏾

@venkywonka
Copy link
Contributor Author

Will update copyrights shortly

@JohnZed JohnZed changed the base branch from branch-0.18 to branch-0.19 February 10, 2021 05:55
@JohnZed JohnZed requested a review from a team as a code owner February 10, 2021 05:55
@ajschmidt8
Copy link
Member

@venkywonka, can you try updating your branch? The PR below just merged which should get branch-0.19 up-to-date and remove some of these extraneous commits from your PR.

#3444

ajschmidt8
ajschmidt8 requested changes Feb 11, 2021
View reviewed changes
CHANGELOG.md Outdated
@@ -136,6 +136,7 @@ Please see https://github.com/rapidsai/cuml/releases/tag/branch-0.18-latest for
- PR #2916: Add SKLearn multi-class GBDT model support in FIL

## Improvements
- PR #3014: NVTX Markers for RF and RF-backend
Copy link
Member

Choose a reason for hiding this comment

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

please make sure to remove this entry since 0.16 has already been released. Manual changelog entries are no longer required, so you can just omit any changes to this file entirely.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

my bad @ajschmidt8 !, taken it out ✌🏾

JohnZed
JohnZed reviewed Feb 25, 2021
View reviewed changes
python/cuml/ensemble/randomforestregressor.pyx
@@ -418,6 +419,8 @@ class RandomForestRegressor(BaseRandomForestModel, RegressorMixin):
Perform Random Forest Regression on the input data

"""
nvtx_range_push("Fit RF-Regressor @randomforestregressor.pyx")
Copy link
Contributor

Choose a reason for hiding this comment

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

We should really add a context manager so we can do: ```
with nvtx_range("fit"):
.. code here


and not need to remember to pop (or have to manage pop correctly in the case of exceptions or early returns)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

agreed @JohnZed ✌🏾

@JohnZed
Copy link
Contributor

JohnZed commented Feb 25, 2021

@gpucibot merge

@rapids-bot rapids-bot bot merged commit 8fa2b90 into rapidsai:branch-0.19 Feb 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ajschmidt8 ajschmidt8 ajschmidt8 approved these changes

@JohnZed JohnZed JohnZed approved these changes

@teju85 teju85 teju85 approved these changes

Assignees
No one assigned
Labels
2 - In Progress Currenty a work in progress 3 - Ready for Review Ready for review by team CUDA / C++ CUDA issue Cython / Python Cython or Python issue improvement Improvement / enhancement to an existing function libcuml non-breaking Non-breaking change
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@venkywonka @GPUtester @codecov-io @teju85 @JohnZed @ajschmidt8 @dantegd