-
Notifications
You must be signed in to change notification settings - Fork 244
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
Logger improvements #2053
Logger improvements #2053
Conversation
Signed-off-by: Ketan Umare <kumare3@users.noreply.github.com>
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #2053 +/- ##
==========================================
- Coverage 85.98% 85.97% -0.02%
==========================================
Files 308 308
Lines 22946 22997 +51
Branches 3468 3479 +11
==========================================
+ Hits 19731 19771 +40
- Misses 2615 2616 +1
- Partials 600 610 +10 ☔ View full report in Codecov by Sentry. |
Signed-off-by: Ketan Umare <kumare3@users.noreply.github.com>
|
||
handler.setLevel(logging.DEBUG) |
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 is unfortunately a breaking change because we use this handler
for further customization. This is the second time this part breaks :(
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.
@honnix , sorry for the breaking change. The intent of this change was to simplify the logging infrastructure.
Can you talk about what sort of customizations you are applying to this? We might simply expose that as an escape hatch.
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.
No worries. We have adapted accordingly. So there are two needs:
- we need to customize the json formatter to remap a field (levelname) to satisfy stackdriver logging
- we use the same handler to set all existing loggers (things from other libs) so we can make sure everything is json logged, not just flytekit and user space loggers
Tracking issue
With this change, flytekit now has 2 loggers, one for framework and one for user code.
Logger is default initialized based on environment. This logger is usually a json logger.
But, when a user runs any pyflyte command, the logger is switched (upgraded) to a rich logger. this can be turned off, but the logging is way more readable. the format is also adjusted
using
pyflyte -vvv
orpyflyte -v
changes verbosity level of the logThe PR also simplifies the logging setup. Now, everywhere in the framework,
logger
should be used. There is work to be done to make the logging less verbose and more meaningful, that will be done as a follow upWhy are the changes needed?
Better logging when using the CLI
What changes were proposed in this pull request?
Logger setup for flytekit
How was this patch tested?
locally and remotely