-
Notifications
You must be signed in to change notification settings - Fork 29
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
Add Forest Inference Operator #118
Merged
karlhigley
merged 15 commits into
NVIDIA-Merlin:main
from
oliverholworthy:ops-fil-wrapper
Jun 14, 2022
Merged
Changes from 9 commits
Commits
Show all changes
15 commits
Select commit
Hold shift + click to select a range
db5e6a9
Add backend param to export method of PipelineableInferenceOperator
oliverholworthy 564ad51
Add params optional keyword argument to export method of FIL class
oliverholworthy a2649c6
Add instance_group param to FIL op to allow choice of CPU or GPU
oliverholworthy e0d9a9d
Add Forest class for running inference with forest models
oliverholworthy 5f9402a
Add docstrings to op runner model
oliverholworthy 3b176dd
Allow tensors to be returned by inference operators
oliverholworthy f79faf2
Remove as_numpy from transform method of Forest operator
oliverholworthy 94a115a
Add copyright to the fil and test_forest module
oliverholworthy 36a7cdf
Add test for Ensemble with NVTabular Workflow and Forest Operator
oliverholworthy 46e8248
Correct config name in forest test
oliverholworthy 604b232
Rename Forest PredictForest and update docstring.
oliverholworthy 30c6878
Correct name of directory in tests
oliverholworthy 816abb7
Merge branch 'main' into ops-fil-wrapper
karlhigley eaf7e32
Merge branch 'main' into ops-fil-wrapper
oliverholworthy 96e6d80
Correct backend assertion for transform op in forest ensemble test
oliverholworthy File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is less an issue with this PR and more an issue for the library generally, which I don't think should block this PR but is probably a worthwhile point of discussion on which I'm open to suggestions:
We should probably come up with a consistent naming scheme for the operators that make actual model predictions in separate back-ends (like FIL and Tensorflow.) Originally I was thinking to make the convention
FrameworkNamePredict
, which I thought read pretty nicely in the operator graph definition. On second thought, I realized it might be nice for the prediction operators to be conceptually and alphabetically grouped together, so we changed the name toPredictTensorflow
, which would imply this operator might be calledPredictForest
orPredictFIL
. I'm not entirely happy with either convention though, so open to suggestions for how we might do this in a consistent way.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Also, as a point of order for interpreting my PR comments: I mean this as literally as you can possibly take it—and I usually do. This is not a polite way of saying something else; I am not making a veiled suggestion for changing the code herein. I honestly don't know what to do here, and could be persuaded to go in any direction someone makes a halfway compelling case for. I just think we should probably, at some point, not necessarily now or in this PR, figure out how to name these operators consistently but I can't myself make a halfway compelling case for any particular convention, so...I don't really have a suggestion here, just an issue to raise for your consideration and future pondering. Happy to address this someday, when we think of something smarter than I have so far.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For consistency with the current operators, I've updated to use the name
PredictForest
. There seems to be a similar pattern of<Type of Operatior><Type Of Data or Model>
(QueryFaiss, QueryFeast, PredictTensorflow, FilterCandidates, UnrollFeatures, TransformWorkflow)