Skip to content

add catboost classifier support#529

Closed
theofpa wants to merge 6 commits into
SeldonIO:masterfrom
theofpa:catboost
Closed

add catboost classifier support#529
theofpa wants to merge 6 commits into
SeldonIO:masterfrom
theofpa:catboost

Conversation

@theofpa

@theofpa theofpa commented Mar 22, 2022

Copy link
Copy Markdown
Contributor

add catboost support:

  • add a new runtime by heavily copying lightgbm runtime
  • add test that builds a model, sends an inference request, validates the prediction
  • add example in docs
  • list in index/readme

adriangonz
adriangonz reviewed Mar 23, 2022
View reviewed changes

@adriangonz adriangonz left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This looks great @theofpa! Thanks a lot for submitting a new runtime! 🚀

It also seems to already tick most of the boxes. I've added a few questions, but it looks almost ready to go!

Comment thread docs/runtimes/index.md Outdated
Comment thread runtimes/catboost/mlserver_catboost/catboost.py Outdated
Comment thread runtimes/catboost/mlserver_catboost/catboost.py
@theofpa theofpa changed the title add catboost support add catboost classifier support Mar 24, 2022
@theofpa

theofpa commented Mar 24, 2022

Copy link
Copy Markdown
Contributor Author

Thanks for the review @adriangonz . Since we're adding only the classifier capability I've renamed the contribution and added a TODO for the regressor and ranker capabilities as a future improvement.

adriangonz
adriangonz approved these changes Mar 29, 2022
View reviewed changes

@adriangonz adriangonz left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nice one @theofpa! Thanks for addressing those comments.

Really great job! Once the tests are green, this one should be good to go 👍

@adriangonz

adriangonz commented Mar 29, 2022

Copy link
Copy Markdown
Contributor

Hey @theofpa, this PR should deal with the click / black issues that broke off the internet today 😬

#544

It has been merged already, so if you can rebase, all tests should look better 👍

@adriangonz

adriangonz commented Apr 8, 2022

Copy link
Copy Markdown
Contributor

Hey @theofpa ,

It seems that the linter is flagging a couple minor changes. The other failing test seems like a non-issue. So if you can update those linter changes, we should be good to go 👍

@adriangonz

adriangonz commented Aug 31, 2022

Copy link
Copy Markdown
Contributor

Hey @theofpa ,

Conscious it's been a while since this PR was opened, so we'll close the PR for now. However, if you get a chance to have a look in the near future, it would be great to pick it up again!

@adriangonz adriangonz closed this Aug 31, 2022
@krishanbhasin-gc krishanbhasin-gc mentioned this pull request Sep 20, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer
@adriangonz adriangonz adriangonz approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@theofpa @adriangonz