-
Notifications
You must be signed in to change notification settings - Fork 155
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
[Feat]: add Logger API #284
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #284 +/- ##
===================================
Coverage 95% 95%
===================================
Files 18 19 +1
Lines 1173 1241 +68
===================================
+ Hits 1112 1181 +69
+ Misses 61 60 -1 |
first of all, this looks great overall. nice job. a few nits: can a user call self.log inside the litapi like you can in PTL without any callbacks for example? also, cases to test:
also, what is the speed impact of adding a logger? did we do it in such a way where we won’t slow down the server processing? |
Thank you @williamFalcon 🙏
Yes, it's possible to call
We put the logs in a queue, the queue is then processed in a separate process without blocking the inference worker. We landed the streaming test too before this PR to make sure we don't regress. |
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.
Looks good to me.
Just calling out a possible failure mode we'll need to take into account (either here or in a future PR).
@aniketmaurya This is the example that I'm trying to get working
This is the error that gets thrown when I try to start the server
Issue seems to be related to pickling the Prometheus metric but I wanted to know if you had a minimum working code example that could help me get started, just so that I can rule out any concerns regarding user error. |
Before submitting
Provides a clean and easy to use interface for logging metrics. Such as connect with Promethus.
What does this PR do?
Provide a Logger API and
LitAPI.log
to record key-value paired metrics to be processed by the Loggers.process
method to handle a key-value pair metric.LitAPI.log
: Passes key-value pair to the loggersThe following example shows the usage of Logger API along with
LitAPI.log
from a callback.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 🙃