-
Notifications
You must be signed in to change notification settings - Fork 186
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 catboost classifier support #1403
Conversation
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.
Hey @krishanbhasin-gc ,
Apologies for the delay reviewing this one.
This is great work! Thanks a lot for jumping on top of this one! 🚀 From my point of view, this looks great as-is. Once the tests are all good, we should be able to merge this one.
On what you mention around classifiers vs regressors, I think we should land this one as a first iteration and then open an issue to add support for extra types of CatBoost models. It's a shame the framework itself doesn't give us any way to differentiate between artefacts - in that case, we may need to add some extra config field to the model-settings.json
to select between model types.
Actually, tests are already all green - so we should be good to go with this one. Great work @krishanbhasin-gc ! 👍 |
Reviving #529.
From the original PR description
I have only resolved merge conflicts and done a first pass of reshaping some of the configuration files to match the new poetry approach.
Looking ahead to adding support for the Regressor and Ranker models, given catboost/catboost#2504 we can't use the same approach as XGBoost (using a try-catch:
MLServer/runtimes/xgboost/mlserver_xgboost/xgboost.py
Lines 24 to 34 in d41f24e
It might be worth creating 3 separate
MLModel
child classes, one for each of Classifier, Regressor, and Ranker instead. Could this fit the design approach of the project?