Skip to content

[fix][5889686] AutoCast: Fix logger#890

Open
galagam wants to merge 1 commit intoNVIDIA:mainfrom
galagam:fix-autocast-logger
Open

[fix][5889686] AutoCast: Fix logger#890
galagam wants to merge 1 commit intoNVIDIA:mainfrom
galagam:fix-autocast-logger

Conversation

@galagam
Copy link
Contributor

@galagam galagam commented Feb 13, 2026

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:

  • Inherit format and level if called from onnx.quantization
  • Configure independent format and level if called from onnx.autocast

Before your PR is "Ready for review"

  • Make sure you read and follow Contributor guidelines and your commits are signed.
  • Is this change backward compatible?: Yes
  • Did you write any new necessary tests?: No
  • Did you add or update any necessary documentation?: No
  • Did you update Changelog?: No

Additional Information

Summary by CodeRabbit

  • Bug Fixes

    • Improved logging configuration to ensure consistent behavior across modules with enhanced file and console output management.
    • Fixed log file handling to automatically create required directories.
    • Enhanced logger propagation logic for more reliable output routing.
  • Chores

    • Refined logging initialization for better automatic configuration at startup.

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>
@galagam galagam requested a review from a team as a code owner February 13, 2026 11:26
@galagam galagam requested a review from gcunhase February 13, 2026 11:26
@galagam galagam self-assigned this Feb 13, 2026
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Feb 13, 2026

📝 Walkthrough

Walkthrough

Refactors 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

Cohort / File(s) Summary
Logging Configuration Refactor
modelopt/onnx/autocast/logging_config.py
Updated logger initialization to use namespaced identifier. Added conditional parent logger level inheritance, centralized formatter for file and console handlers, directory creation for log files, and dynamic propagation logic based on parent handler presence. Post-configuration now applies levels to all modelopt.onnx.autocast child loggers. Auto-configures on module import.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title directly addresses the main change: fixing the logger for AutoCast by making it context-aware and properly namespaced under modelopt.onnx.autocast instead of relying on a generic 'autocast' logger.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Merge Conflict Detection ✅ Passed ✅ No merge conflicts detected when merging into main

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 that loggerDict may contain PlaceHolder entries.

logging.root.manager.loggerDict is a semi-private API and can contain PlaceHolder objects for intermediate namespace nodes. Calling logging.getLogger(name) on line 90 safely returns a real Logger, 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)

Comment on lines 32 to 50
def configure_logging(level=logging.INFO, log_file=None):
"""Configure logging for all AutoCast components.

Args:
level: The logging level to use (default: logging.INFO).
level: The logging level to use. Can be a string (e.g., "DEBUG", "INFO") or
a logging constant (e.g., logging.DEBUG). Default: logging.INFO.
log_file: Optional path to a log file. If provided, logs will be written to this file
in addition to stdout (default: None).
"""
# Set level for the parent logger and all child loggers
# Check if parent logger (modelopt.onnx) already has handlers configured
parent_logger = logging.getLogger("modelopt.onnx")
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:
level = parent_logger.level

# Set level for the autocast logger (accepts both string and int)
logger.setLevel(level)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

level == logging.INFO is an unreliable sentinel for "no level provided".

If a caller explicitly passes level=logging.INFO and the parent logger is set to DEBUG, this condition on line 46 will still be True, causing the autocast logger to unexpectedly inherit DEBUG instead of honoring the explicit INFO.

Use None as the default to distinguish "not provided" from "explicitly INFO":

Proposed fix
-def configure_logging(level=logging.INFO, log_file=None):
+def configure_logging(level=None, log_file=None):
     """Configure logging for all AutoCast components.
 
     Args:
-        level: The logging level to use. Can be a string (e.g., "DEBUG", "INFO") or
-               a logging constant (e.g., logging.DEBUG). Default: logging.INFO.
+        level: The logging level to use. Can be a string (e.g., "DEBUG", "INFO") or
+               a logging constant (e.g., logging.DEBUG). Default: None (inherits from
+               parent logger if configured, otherwise logging.INFO).
         log_file: Optional path to a log file. If provided, logs will be written to this file
                  in addition to stdout (default: None).
     """
     # Check if parent logger (modelopt.onnx) already has handlers configured
     parent_logger = logging.getLogger("modelopt.onnx")
     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:
-        level = parent_logger.level
+    # Resolve level: inherit from parent if available, otherwise default to INFO
+    if level is None:
+        if parent_has_handlers:
+            level = parent_logger.level
+        else:
+            level = logging.INFO
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
def configure_logging(level=logging.INFO, log_file=None):
"""Configure logging for all AutoCast components.
Args:
level: The logging level to use (default: logging.INFO).
level: The logging level to use. Can be a string (e.g., "DEBUG", "INFO") or
a logging constant (e.g., logging.DEBUG). Default: logging.INFO.
log_file: Optional path to a log file. If provided, logs will be written to this file
in addition to stdout (default: None).
"""
# Set level for the parent logger and all child loggers
# Check if parent logger (modelopt.onnx) already has handlers configured
parent_logger = logging.getLogger("modelopt.onnx")
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:
level = parent_logger.level
# Set level for the autocast logger (accepts both string and int)
logger.setLevel(level)
def configure_logging(level=None, log_file=None):
"""Configure logging for all AutoCast components.
Args:
level: The logging level to use. Can be a string (e.g., "DEBUG", "INFO") or
a logging constant (e.g., logging.DEBUG). Default: None (inherits from
parent logger if configured, otherwise logging.INFO).
log_file: Optional path to a log file. If provided, logs will be written to this file
in addition to stdout (default: None).
"""
# Check if parent logger (modelopt.onnx) already has handlers configured
parent_logger = logging.getLogger("modelopt.onnx")
parent_has_handlers = len(parent_logger.handlers) > 0
# Resolve level: inherit from parent if available, otherwise default to INFO
if level is None:
if parent_has_handlers:
level = parent_logger.level
else:
level = logging.INFO
# Set level for the autocast logger (accepts both string and int)
logger.setLevel(level)
🤖 Prompt for AI Agents
In `@modelopt/onnx/autocast/logging_config.py` around lines 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).

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:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to check level == logging.INFO or should we just propagate the parent's level?

Copy link
Contributor

@gcunhase gcunhase left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added 1 comment, otherwise LGTM. Approving.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants