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] Improve runtime performance of RF to Treelite conversion #3410

Merged
merged 16 commits into from
Jan 30, 2021
Merged

[REVIEW] Improve runtime performance of RF to Treelite conversion #3410

merged 16 commits into from
Jan 30, 2021

Conversation

wphicks
Copy link
Contributor

@wphicks wphicks commented Jan 25, 2021 •
edited
Loading

Speed up prediction time by an additional factor of 4.82
Build Treelite Trees directly to avoid deletion and CommitModel call for TreeBuilder (PoC by @canonizer)
Avoid use of stdlib queues where the growth of those queues is bounded and can be optimized via indexing into a vector of the appropriate size

@wphicks wphicks added feature request New feature or request 2 - In Progress Currenty a work in progress non-breaking Non-breaking change labels Jan 25, 2021
@wphicks wphicks changed the title [WIP] Improve runtime performance of RF to Treelite conversion [skip-ci] [WIP] Improve runtime performance of RF to Treelite conversion Jan 26, 2021
@wphicks wphicks marked this pull request as ready for review January 26, 2021 01:02
@wphicks wphicks requested a review from a team as a code owner January 26, 2021 01:02
@wphicks wphicks changed the title [WIP] Improve runtime performance of RF to Treelite conversion [REVIEW] Improve runtime performance of RF to Treelite conversion Jan 26, 2021
@wphicks wphicks added 3 - Ready for Review Ready for review by team and removed 2 - In Progress Currenty a work in progress labels Jan 26, 2021
canonizer
canonizer suggested changes Jan 26, 2021
View reviewed changes
cpp/src/decisiontree/decisiontree_impl.cuh Show resolved Hide resolved
cpp/src/decisiontree/decisiontree_impl.cuh Show resolved Hide resolved
cpp/src/randomforest/randomforest.cu Outdated Show resolved Hide resolved
cpp/src/randomforest/randomforest.cu Outdated Show resolved Hide resolved
canonizer
canonizer approved these changes Jan 27, 2021
View reviewed changes
Copy link
Contributor

@canonizer canonizer left a comment

Choose a reason for hiding this comment

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

Approved provided the comments are addressed.

@wphicks wphicks added 5 - Ready to Merge Testing and reviews complete, ready to merge and removed 3 - Ready for Review Ready for review by team labels Jan 27, 2021
@JohnZed
Copy link
Contributor

JohnZed commented Jan 27, 2021

rerun tests

@wphicks
Copy link
Contributor Author

wphicks commented Jan 28, 2021

Looking into test failures

@wphicks wphicks mentioned this pull request Jan 29, 2021
Closed
@wphicks
Copy link
Contributor Author

wphicks commented Jan 29, 2021

All right, I think this is good to go now. Merged in branch-0.18 and added in a call that got dropped somewhere along the line. Once this passes CI, I think we're all set.

@JohnZed
Copy link
Contributor

JohnZed commented Jan 30, 2021

@gpucibot merge

@rapids-bot rapids-bot bot merged commit dfcea4e into rapidsai:branch-0.18 Jan 30, 2021
@wphicks wphicks mentioned this pull request Feb 8, 2021
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JohnZed JohnZed JohnZed approved these changes

@canonizer canonizer canonizer approved these changes

@hcho3 hcho3 hcho3 approved these changes

Assignees
No one assigned
Labels
5 - Ready to Merge Testing and reviews complete, ready to merge feature request New feature or request libcuml non-breaking Non-breaking change
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@wphicks @JohnZed @canonizer @hcho3