-
Notifications
You must be signed in to change notification settings - Fork 154
[Logging] Switch from Standard Logging to Loguru #1521
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
base: main
Are you sure you want to change the base?
Conversation
👋 Hi! Thank you for contributing to llm-compressor. Please add the ready label when the PR is ready for review. Note: This is required to complete the testing suite, please only add the label once the PR is code complete and local testing has been performed. |
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.
Hello @dichn, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
Summary of Changes
Gemini here, providing a summary for this pull request authored by dichn. The main goal of this PR is to standardize the logging implementation across the codebase by switching from the standard Python logging
library to loguru
. This change is motivated by the fact that loguru
is already in use in some parts of the project, and this PR aims to achieve consistency in logging behavior. The changes primarily involve updating the metrics/logger.py
file to integrate loguru
, modifying how log levels are handled, and simplifying the SparsificationGroupLogger
class.
Highlights
- Logging Library Switch: The core change is replacing the standard
logging
library withloguru
in themetrics/logger.py
module to ensure a consistent logging approach. - PythonLogger Update: The
PythonLogger
class has been refactored to use the globalloguru.logger
instance. This includes updating the_create_default_logger
method to addloguru
sinks instead of standardlogging
handlers and removing the ability to pass an externallogging.Logger
instance. - Log Level Type Hinting: Type hints for logging level parameters across various methods (
log_hyperparams
,log_scalar
,log_scalars
,log_string
) have been updated fromOptional[int]
toOptional[Union[int, str]]
to accommodateloguru
's support for string-based log levels. - SparsificationGroupLogger Simplification: The
python
parameter inSparsificationGroupLogger
has been simplified from acceptingOptional[Union[bool, Logger]]
to justbool
, aligning its behavior with thelog_python
parameter inLoggerManager
and removing the option to inject a specificlogging.Logger
instance.
Changelog
Click here to see the changelog
- src/llmcompressor/metrics/logger.py
- Replaced
logging
imports withloguru.logger
andsys
(DIFF 1). - Removed the
LOGGING_LEVELS
dictionary (DIFF 2). - Updated type hints for
level
parameter inlog_hyperparams
,log_scalar
,log_scalars
, andlog_string
methods to acceptUnion[int, str]
(DIFF 3, 4, 5, 12, 13, 14, 18, 19, 20, 21, 22, 23, 24, 25, 31, 32, 33). - Updated docstrings to refer to
loguru.logger
instead oflogging.Logger
(DIFF 6, 15). - Removed the
logger
parameter fromPythonLogger.__init__
and removed the__getattr__
andlogger
property (DIFF 7). - Refactored
_create_default_logger
inPythonLogger
to useloguru.logger.add
for console and file sinks, tracking them in_logger_sink_registry
(DIFF 7, 8, 9). - Updated the level comparison logic in
_log_lambda
to work withloguru
levels (DIFF 10). - Changed the logging call in
_log_lambda
fromself._logger.log
tologger.log
(DIFF 11). - Simplified the
python
parameter inSparsificationGroupLogger.__init__
to a boolean and updated its default value and type hint (DIFF 16). - Removed the logic in
SparsificationGroupLogger.__init__
that handled passing alogging.Logger
instance toPythonLogger
(DIFF 17). - Updated the default level strings passed in helper methods (
debug
,info
,warning
,warn
,error
,critical
) to matchloguru
string levels (DIFF 26, 27, 28, 29, 30).
- Replaced
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command>
or @gemini-code-assist <command>
. Below is a summary of the supported commands.
Feature | Command | Description |
---|---|---|
Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/
folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
Link to issue #1223 |
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.
Code Review
This pull request effectively transitions the logging mechanism from Python's standard logging
module to Loguru
, which is a good step for consistency and leveraging Loguru's features. The changes to type hints for log levels and the update to PythonLogger
to use Loguru sinks are central to this effort. The simplification of SparsificationGroupLogger
's python
parameter is also a welcome improvement.
I've identified a few areas for attention, primarily concerning how PythonLogger
manages global Loguru sinks, a potential reduction in flexibility for PythonLogger
instantiation, and a minor point on code clarity in level comparison. Addressing the sink management issue is particularly important to prevent unexpected logging behavior.
Summary of Findings
- Duplicate Sink Addition Risk:
PythonLogger
instances may repeatedly add default stream and file sinks to the globalloguru.logger
due to instance-level checks for global resources, potentially causing duplicate log messages. - Reduced Flexibility in
PythonLogger
: ThePythonLogger
no longer accepts an external logger instance, which might be a breaking change for users who relied on this for custom logger configurations. - Log Level Comparison Clarity: The logic for comparing log levels in
PythonLogger._log_lambda
could be refactored for improved readability and maintainability.
Merge Readiness
The pull request makes good progress in migrating to Loguru. However, there's a high-severity issue regarding potential duplicate sink additions by PythonLogger
that should be addressed before merging. Additionally, a medium-severity concern about reduced flexibility in PythonLogger
and a suggestion for code clarity should be considered.
I am unable to approve pull requests. Please ensure these points are reviewed and addressed, and seek further reviews and approval from other maintainers before merging.
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, please take a look and update through Gemini's suggestions
Re-pushed for fixing the previous duplicate Hello @dsikka |
Re-pushed for a rebase. |
@dichn Thank you for your PR! |
As loguru is used in some places, we update other logging instances for consistency with its behavior * metrics/logger.py: Change `level: Optional[int]` to `level: Optional[Union[int, str]]` to better support loguru's logging level behavior * Update `PythonLogger._create_default_logger` to use loguru.logger * PythonLogger: Add class-level variable `_global_file_sink_id` to prevent duplicate file sink creations and remove manual stream handler setup since loguru adds a default `sys.stderr` sink * Simplify `PythonLogger` by no longer accepting an external logger instance, because the main concept of loguru is that there is one and only one `logger` * Simplify `SparsificationGroupLogger` by replacing the `python` union type with `bool`, aligning it with `log_python` in `LoggerManager` and removing ambiguity
Re-pushed for
|
SUMMARY: Community user PRs are still failing tracing test checks. This loosens the skip check so it resolves correctly After merge and rebase, this should also resolve issue for #1521 TEST PLAN: Confirmed with @dbarbuzzi that this resolved the issue for #1445 . Signed-off-by: Brian Dellabetta <[email protected]>
Purpose
As loguru is used in some places, we update other logging instances for consistency with its behavior
Changes
level: Optional[int]
tolevel: Optional[Union[int, str]]
to better support loguru's logging level behaviorPythonLogger._create_default_logger
to use loguru.loggerSparsificationGroupLogger
by replacing thepython
union type withbool
, aligning it withlog_python
inLoggerManager
and removing ambiguity