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

CPU/GPU interop with RandomForest #6175

Merged
merged 20 commits into from
Feb 7, 2025

Conversation

hcho3
Copy link
Contributor

@hcho3 hcho3 commented Dec 11, 2024

First version for CPU/GPU interop with RandomForest. Note. This feature requires latest Treelite.

@hcho3 hcho3 requested a review from a team as a code owner December 11, 2024 00:20
@hcho3 hcho3 requested review from bdice and viclafargue December 11, 2024 00:20
@hcho3 hcho3 marked this pull request as draft December 11, 2024 00:20
Copy link

copy-pr-bot bot commented Dec 11, 2024

Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually.

Contributors can view more details about this message here.

@github-actions github-actions bot added the Cython / Python Cython or Python issue label Dec 11, 2024
@hcho3 hcho3 changed the title [WIP] CPU/GPU interop with RandomForest CPU/GPU interop with RandomForest Jan 9, 2025
@hcho3 hcho3 marked this pull request as ready for review January 9, 2025 13:33
@hcho3 hcho3 requested review from a team as code owners January 9, 2025 13:33
@hcho3 hcho3 requested a review from robertmaynard January 9, 2025 13:33
@hcho3 hcho3 added improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Jan 9, 2025
@betatim
Copy link
Member

betatim commented Jan 30, 2025

Does "latest treelite" mean we need #6212 to be merged for this to work?

@hcho3
Copy link
Contributor Author

hcho3 commented Jan 30, 2025

This PR actually contains duplicate commits from #6212. So yes, we need the latest Treelite to support the interop feature.

Comment on lines +866 to +869
# treelite does an internal isinstance check to detect an sklearn
# RF, which proxymodule interferes with. We work around that
# temporarily here just for treelite internal check and
# restore the __class__ at the end of the method.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In a future version of Treelite, we should add an alternative version of treelite.sklearn.import_model that takes in a second argument model_type? The caller would provide a string to uniquely identify the model type, so that we can remove the internal isinstance check.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe, depends on whether we do other changes in cuML itself to potentially avoid this

@dantegd
Copy link
Member

dantegd commented Feb 7, 2025

/merge

@rapids-bot rapids-bot bot merged commit f60b5f0 into rapidsai:branch-25.02 Feb 7, 2025
68 checks passed
Copy link
Contributor

@viclafargue viclafargue left a comment

Choose a reason for hiding this comment

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

Just realized I forgot to review this PR before it was merged, sorry about that! I just took a look at it. It looks like several workarounds were used, but all of them seemed necessary. Also, good testing. LGTM. Thanks for the great work @hcho3!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Cython / Python Cython or Python issue improvement Improvement / enhancement to an existing function non-breaking Non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants