-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
Introduce ServableModuleValidator Callback #13614
Conversation
I am not sure I am a fan of the sanityServe to be a callback.
I am bringing this up because I think it's important SanityServe provides value otherwise no one will use it. However, to provide value we should think about what can it provide that you would not have in the Serve module. |
Spoke with @tchaton async and we synced on it. |
Co-authored-by: Carlos Mocholí <carlossmocholi@gmail.com>
Co-authored-by: Carlos Mocholí <carlossmocholi@gmail.com>
Co-authored-by: Carlos Mocholí <carlossmocholi@gmail.com>
Co-authored-by: Carlos Mocholí <carlossmocholi@gmail.com>
Co-authored-by: Carlos Mocholí <carlossmocholi@gmail.com>
Co-authored-by: Carlos Mocholí <carlossmocholi@gmail.com>
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 looks great @tchaton !
Following up from our brief discussion in Slack, just added a couple comments below. It would be great to hear your thoughts!
the premise here is that every model needs to be served which is not true. for example a model to fold proteins does not need “serving”… this is a model that runs once in a while to generate a new protein sequence for a lab to synthesize. so, no, we can’t “force” every model to be “production ready.”. however, if a team opts for forcing their particular research code to always be production ready, they should have a mechanism to enforce that behavior and can opt in. |
@williamFalcon that is why this was designed to be a combination of an optional mixin + a callback instead of directly added to the lightning module and the trainer. So this is opt-in as it is currently implemented :) Meaning that all the hooks for the class |
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.
unblock
What does this PR do?
Fixes #13480
Context: Improve model mobility from training to serving. MLOps Engineers would argue that a production model shouldn't be trained and resources wasted if the model can't be served to provide value to customers. They would argue that the model should be unit tested as soon as possible to validate its conformity for its production usage. This PR investigates the addition of serving functionalities in PyTorch Lightning.
As a user, you would need to add the
SanityServing
callback to your Trainer and make your model subclassServableModule
.The
ServableModule
requires 3 hooks to be implemented in order to fully describe how the model behaves when being served.The Ideal API would rely only on the type to apply the deserialization and serialization and tensor shape could be added:
Extra functionalities for sanity serving
This would be even more impactful once https://github.com/pytorch/torchdynamo is available in PyTorch and models and their transforms served in pure python are further optimized.
Does your PR introduce any breaking changes? If yes, please list them.
Before submitting
PR review
Anyone in the community is welcome to review the PR.
Before you start reviewing, make sure you have read the review guidelines. In short, see the following bullet-list:
Did you have fun?
Make sure you had fun coding 🙃
cc @Borda @tchaton @justusschock @awaelchli @carmocca @ananthsub @ninginthecloud @jjenniferdai @rohitgr7 @akihironitta