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

Refactor model wrapper #1056

Merged
merged 4 commits into from
Mar 24, 2023

Conversation

sakoush
Copy link
Member

@sakoush sakoush commented Mar 23, 2023

Move the runtime wrapper to a common utility file.

@sakoush sakoush requested a review from adriangonz March 23, 2023 20:17
)


class WrapperMLModel(MLModel):
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you remember why we needed this instead of just extending the MLModel class?

Copy link
Member Author

Choose a reason for hiding this comment

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

It is essentially so that we have different implementations of MLModel where inner MLModel in this wrapper is created by a factory method and allowing the user to have in the model-settings.json one reference in implementation to this WrapperMLModel. For example in mlserver-alibi-explain runtime it is going to be AlibiExplainRuntime for ally types of alibi models.

Copy link
Member Author

Choose a reason for hiding this comment

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

I simplified the code based on your suggestion to use __new__ instead

Copy link
Contributor

@adriangonz adriangonz left a comment

Choose a reason for hiding this comment

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

This looks great! I love when there are more lines removed than added 😁

image

@sakoush sakoush merged commit 137d5a6 into SeldonIO:master Mar 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants