[fix][5889686] AutoCast: Fix logger#890
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughRefactors the autocast logging configuration to use a hierarchical logger namespace ("modelopt.onnx.autocast") with conditional parent logger inheritance, centralized formatter application, and hybrid propagation logic supporting both standalone and parent-backed usage patterns. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@modelopt/onnx/autocast/logging_config.py`:
- Around line 32-50: The function configure_logging currently treats
level=logging.INFO as "not provided" which breaks explicit INFO uses; change the
signature of configure_logging to use level=None as the default, update the
docstring to state None means "inherit parent or default", and implement logic
in configure_logging so: if parent_logger has handlers and level is None then
set level = parent_logger.level, else if level is None set level = logging.INFO;
also preserve support for string level names by mapping strings to logging
constants before calling logger.setLevel(level) (reference configure_logging,
parent_logger, logger.setLevel).
🧹 Nitpick comments (1)
modelopt/onnx/autocast/logging_config.py (1)
87-91: Consider thatloggerDictmay containPlaceHolderentries.
logging.root.manager.loggerDictis a semi-private API and can containPlaceHolderobjects for intermediate namespace nodes. Callinglogging.getLogger(name)on line 90 safely returns a realLogger, but it also materializes placeholders into full loggers as a side effect. This is unlikely to cause issues in practice, but a defensive check would make the intent clearer:Optional: skip non-Logger entries
for name in logging.root.manager.loggerDict: - if name.startswith("modelopt.onnx.autocast"): - logging.getLogger(name).setLevel(level) + if name.startswith("modelopt.onnx.autocast"): + child = logging.root.manager.loggerDict[name] + if isinstance(child, logging.Logger): + child.setLevel(level)
| parent_has_handlers = len(parent_logger.handlers) > 0 | ||
|
|
||
| # If parent is configured and no explicit level provided, inherit parent's level | ||
| if parent_has_handlers and level == logging.INFO: |
There was a problem hiding this comment.
Do we need to check level == logging.INFO or should we just propagate the parent's level?
There was a problem hiding this comment.
Refactored, the logic is clearer now. Please revisit.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #890 +/- ##
==========================================
- Coverage 73.74% 73.74% -0.01%
==========================================
Files 199 199
Lines 21163 21177 +14
==========================================
+ Hits 15606 15616 +10
- Misses 5557 5561 +4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Previously relied on quantization logger, which caused logs to be suppressed when onnx.autocast was used directly Instead: - Inherit format and level if called from onnx.quantization - Configure independent format and level if called from onnx.autocast Signed-off-by: Gal Hubara Agam <96368689+galagam@users.noreply.github.com>
Signed-off-by: Gal Hubara Agam <96368689+galagam@users.noreply.github.com>
22a5c15 to
f5bfb89
Compare
What does this PR do?
Type of change: Bug fix
Overview:
Previously relied on quantization logger, which caused logs to be suppressed when onnx.autocast was used directly
Instead:
Before your PR is "Ready for review"
Additional Information
Summary by CodeRabbit
Bug Fixes
Chores