Closed aniketmaurya closed 2 months ago
Attention: Patch coverage is 96.19048%
with 4 lines
in your changes missing coverage. Please review.
Project coverage is 95%. Comparing base (
cf78c85
) to head (4b39bac
). Report is 1 commits behind head on main.
@aniketmaurya
for the hook names for the litapi, remove the litapi words:
on_setup_start
on_litapi_setup_start
For the litserver, what do these 2 do?
Before submitting
- [ ] Was this discussed/agreed via a Github issue? (no need for typos and docs improvements) - [ ] Did you read the [contributor guideline](https://github.com/Lightning-AI/pytorch-lightning/blob/main/.github/CONTRIBUTING.md), Pull Request section? - [ ] Did you make sure to update the docs? - [ ] Did you write any new necessary tests?⚠️ How does this PR impact the user? ⚠️
Allow users to trigger arbitrary functions via a callback system. This enables to inject useful functionalities like logging inference time, metrics collection, etc.
What does this PR do?
Implements a callback system with certain hooks that get triggered on events.
A caveat of this implementation would be that even though the callbacks are classes, it might not be straightforward to save state between to hooks like
on_litapi_predict_end
andon_server_setup_start
because they run in different process and once the callback object is pickled then we don't sync the state.I think this is okay, considering that when in same context (
on_litapi_predict_start
andon_litapi_predict_end
)on_litapi_*_*
we will have the state in same process.As a result of this PR, we will be able to refactor the monitoring metrics PR as a callback.
TODO
PR review
Anyone in the community is free to review the PR once the tests have passed. If we didn't discuss your PR in GitHub issues there's a high chance it will not be merged.
Did you have fun?
Make sure you had fun coding 🙃