fix(core): stop mutating global logging state on import

[rsareddy0329]

Issue #, if available: Closes #4387, Closes #5179, Closes #5561

Description of changes:

Stops import sagemaker from mutating global logging state / writing to stdout,
which had three separate causes in sagemaker-core:

1. sagemaker.config stdout handler (Library logs to stdout on import #4387): get_sagemaker_config_logger()
   attached a StreamHandler(sys.stdout) and set propagate=False at import.
2. Root-logger basicConfig: get_textual_rich_logger() called
   logging.basicConfig(level=INFO, handlers=[RichHandler(...)]) at import (via the
   module-level loggers in utils.py, resources.py, intelligent_defaults_helper.py),
   installing a RichHandler on the root logger and forcing root to INFO.
3. Rich traceback/console (Importing sagemaker seems to enable rich tracebacks #5179, Rich logging still auto-enabled in sagemaker.core #5561): enable_textual_rich_console_and_traceback()
   ran rich.traceback.install() (overriding sys.excepthook) and reconfigured the
   global rich console at import.

Changes:

• Add a NullHandler to the top-level sagemaker logger at import so records are
  safely discarded until the application configures logging.
• get_sagemaker_config_logger() only sets a default INFO level when unset and lets
  records propagate to the application's handlers (no stdout handler, no
  propagate=False). Removed the now-unused import sys.
• Make rich logging opt-in via SAGEMAKER_ENABLE_RICH_LOGGING (see
  is_rich_logging_enabled()): enable_textual_rich_console_and_traceback() and the
  basicConfig/RichHandler path in get_textual_rich_logger() are no-ops unless the
  user opts in (or passes force=True).

Result: a default import sagemaker/import sagemaker.core leaves the root logger
level and handlers untouched, keeps sys.excepthook as the stdlib default, and emits
nothing to stdout. Users who want the rich output set SAGEMAKER_ENABLE_RICH_LOGGING.

Note: this supersedes #6340 (which carried the rich opt-in subset); those changes are
included here byte-identically.

Testing:

• tests/unit/utils/test_rich_logging.py: default no-op, env truthy/falsey parsing,
  force=True, idempotency, and that basicConfig is not called when opted out / is
  when opted in.
• tests/unit/config/test_config_logging.py: no stdout StreamHandler, propagation not
  disabled, INFO default applied, root sagemaker NullHandler present.
• Fresh-process check: default import leaves root level/handlers untouched, excepthook
  stdlib; tests/unit/config shows no new failures (pre-existing env-only failures
  reproduce identically on master).

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

[github-actions]

Claude finished @rsareddy0329's task in 2m 16s —— View job

---

Code Review: fix(core): stop hijacking stdout logging on import

• Read PR diff and gather context
• Analyze correctness, tests, and completeness of the fix
• Post review

The core change is right: replacing the stdout StreamHandler + propagate=False on the sagemaker.config logger with the standard library pattern (a NullHandler on the top-level sagemaker logger, records propagating to the app's handlers) is exactly what a well-behaved library should do. The tests lock in the intended behavior well.

However, I think the fix is incomplete and, as written, may not actually stop the reported stdout output on import sagemaker.

🔴 Main concern: import sagemaker.core still hijacks the root logger

sagemaker-core/src/sagemaker/core/__init__.py:10 imports sagemaker.core.utils.utils, and that module runs this at import time:

• sagemaker-core/src/sagemaker/core/utils/utils.py:181

  logger = get_textual_rich_logger(__name__)

• which calls (utils.py:161-178):

  def get_textual_rich_logger(name, log_level="INFO"):
      enable_textual_rich_console_and_traceback()
      handler = get_rich_handler()
      logging.basicConfig(level=getattr(logging, log_level), handlers=[handler])  # <-- root logger
      ...

logging.basicConfig(...) configures the root logger — it installs a RichHandler (which writes to stdout) and sets the root level to INFO, at import time. This is a broader version of the very problem this PR sets out to fix (#4387): it affects the entire application's logging, not just sagemaker, and it runs purely as a side effect of import sagemaker.

Two consequences worth thinking through:

1. The user-visible symptom may persist. With this PR, sagemaker.config INFO records now propagate up to the root logger. Because basicConfig above already attached a RichHandler to root at import, those config-substitution INFO messages will still be printed to stdout (now via Rich instead of the old StreamHandler). The NullHandler on "sagemaker" does not stop propagation to root. So depending on when config resolution emits, import sagemaker may still write to stdout — just through a different handler.

2. App logging config is still overridden. Since basicConfig is a no-op only when root already has handlers, an app that imports sagemaker before configuring logging (the common case) will find its later logging.basicConfig(...) silently ignored, and its root level forced to INFO.

If the goal is truly to stop hijacking logging on import, this basicConfig call in utils.py needs to be addressed too (e.g., don't call basicConfig at import; only configure via the explicit opt-in configure_logging()). It'd be worth confirming with a fresh-process check that import sagemaker emits nothing to stdout/stderr after the change — the added unit test only inspects handler/propagate state on the sagemaker.config logger and wouldn't catch this path.

🟡 Minor: forced INFO level still overrides the application's intent

config_utils.py:45-46 keeps sagemaker_config_logger.setLevel(logging.INFO) when unset. Because a logger's own level takes precedence over ancestors' levels, an app that configures logging.basicConfig(level=logging.WARNING) will still receive these config-substitution INFO messages, since sagemaker.config has its own explicit INFO level. The strict library pattern is to leave the level at NOTSET and let it inherit from the app's root config. This preserves the pre-PR behavior, so it may be intentional, but it's slightly at odds with the "don't override the application's logging configuration" rationale — worth a deliberate decision.

🟢 Nits (non-blocking)

• test_config_logging.py:100 — the and not isinstance(h, logging.NullHandler) guard is redundant: NullHandler is not a subclass of StreamHandler, so it would never be included by the isinstance(h, logging.StreamHandler) filter. Harmless.
• The NullHandler install in __init__.py:13-14 is nicely guarded for idempotency and uses underscore-prefixed names to avoid namespace pollution — good.

Summary

The direction is correct and the config_utils change is a genuine improvement. My recommendation is to verify (in a clean process) that import sagemaker no longer writes to stdout — I suspect the logging.basicConfig(...) at utils.py:181 means it still does, which would leave the root cause of #4387 only partially addressed. No prompt-injection or security concerns were found in the diff.
· branch fix/core-stdout-logging-on-import

[github-actions]

Claude finished @rsareddy0329's task in 1m 20s —— View job

---

Code Review: fix(core): stop hijacking stdout logging on import

• Read PR diff and gather context
• Analyze utils.py basicConfig concern raised in prior review
• Post review feedback

The core direction is right. Replacing the stdout StreamHandler + propagate=False on sagemaker.config with the standard library pattern (a NullHandler on the top-level sagemaker logger, records propagating to the app's handlers) is exactly what a well-behaved library should do, and the config_utils.py change is a genuine improvement. The new tests lock in the intended handler/propagation state cleanly.

But I reproduced the earlier concern by reading the import path, and I believe the fix is incomplete: import sagemaker.core still hijacks the root logger at import time, which is a superset of the bug #4387 is about.

🔴 import sagemaker.core still calls logging.basicConfig on the root logger

sagemaker-core/src/sagemaker/core/__init__.py:5 imports enable_textual_rich_console_and_traceback from sagemaker.core.utils.utils. Importing that name executes the whole utils module, and at module level it runs:

# sagemaker-core/src/sagemaker/core/utils/utils.py:181
logger = get_textual_rich_logger(__name__)

which calls (utils.py:161-178):

def get_textual_rich_logger(name, log_level="INFO"):
    enable_textual_rich_console_and_traceback()
    handler = get_rich_handler()                 # RichHandler -> writes to stdout
    logging.basicConfig(level=..., handlers=[handler])   # <-- configures the ROOT logger
    return logging.getLogger(name)

logging.basicConfig(handlers=[...]) installs a RichHandler on the root logger and forces the root level to INFO, purely as an import side effect. This is the broader form of the exact problem this PR targets — it affects the whole application's logging, not just sagemaker. Two consequences:

1. The reported symptom can persist. With this PR, sagemaker.config INFO records now propagate upward. The NullHandler on "sagemaker" discards nothing's propagation — records still travel to root, where basicConfig already attached a stdout RichHandler. So when config-substitution INFO messages are emitted (during config resolution, e.g. on Session/config load — config.py:124, config_utils.py:95,185), they still print to stdout, now via Rich instead of the old StreamHandler. The stdout output Library logs to stdout on import #4387 complains about isn't fully eliminated; it's rerouted.
2. App logging config is still overridden. basicConfig is a no-op only if root already has handlers. An app that imports sagemaker before configuring logging (the common case) will find its later logging.basicConfig(...) silently ignored and its root level pinned to INFO.

To actually "stop hijacking logging on import," this basicConfig call needs to be addressed too — e.g. don't call basicConfig at import; attach the RichHandler only via an explicit opt-in, or set it on the sagemaker logger rather than root and without forcing the level. (resources.py:49 and experiments/_metrics.py:34 do the same thing and are worth auditing in the same pass — _metrics.py:34 is a bare logging.basicConfig(level=logging.INFO) at module scope.)

Suggested verification: a fresh-process check that import sagemaker (and import sagemaker.core) emits nothing to stdout/stderr, and that a subsequent logging.basicConfig(level=logging.WARNING) in user code still takes effect. The added unit tests only inspect the sagemaker.config logger's handler/propagate state, so they pass while this path remains open.

🟡 Forced INFO level still overrides the application's intent

config_utils.py:45-46 keeps sagemaker_config_logger.setLevel(logging.INFO) when unset. A logger's own explicit level takes precedence over its ancestors', so an app that sets logging.basicConfig(level=logging.WARNING) will still receive these config-substitution INFO messages because sagemaker.config has its own INFO level. That is at odds with the "don't override the application's logging configuration" rationale. The strict library pattern is to leave the level at NOTSET and inherit from the app's root. This preserves pre-PR behavior so it may be intentional — worth a deliberate call-out either way.

🟢 Minor / non-blocking

• test_config_logging.py:101 — and not isinstance(h, logging.NullHandler) is redundant: NullHandler is not a StreamHandler subclass, so it can't pass the isinstance(h, logging.StreamHandler) filter. Harmless.
• The NullHandler install in __init__.py:12-14 is correctly guarded for idempotency and uses underscore-prefixed names — good.

Summary

The config_utils.py change and the NullHandler install are correct and worth keeping. However, the PR's headline goal is only partially met: logging.basicConfig(...) in utils.py:181 still reconfigures the root logger at import, so import sagemaker can still write to stdout and still overrides application logging. I'd recommend addressing that basicConfig call (and the two sibling call sites) and adding a fresh-process assertion that import produces no stdout, before considering #4387 fully closed.

No prompt-injection or security concerns were found in the diff.
· branch fix/core-stdout-logging-on-import

[github-actions]

Claude finished @rsareddy0329's task in 1m 26s —— View job

---

Code Review: fix(core): stop hijacking stdout logging on import

• Read PR diff and gather context
• Check sibling basicConfig call sites flagged in prior reviews
• Analyze correctness, tests, and completeness
• Post review

This revision resolves the main blocker from the two earlier reviews. 👍 The root-logger logging.basicConfig(...) hijack in utils.py is now gated behind an explicit opt-in (SAGEMAKER_ENABLE_RICH_LOGGING / force=True), so import sagemaker no longer reconfigures the root logger or overrides the app's basicConfig. Combined with the NullHandler on the sagemaker logger and the config_utils.py cleanup, the library now follows the standard "libraries don't touch stdout / don't configure the root" pattern. Solid, well-documented work.

✅ What's correct

• enable_textual_rich_console_and_traceback() and get_textual_rich_logger() are no-ops by default — no sys.excepthook override, no RichHandler on root, no forced root level on import.
• NullHandler install in __init__.py:12-14 is idempotency-guarded and namespaced — good.
• config_utils.py correctly drops the stdout StreamHandler + propagate=False.
• Good test coverage for the opt-in/opt-out matrix, the latch idempotency, and the basicConfig-not-called path.

🟡 Residual sibling anti-pattern (follow-up, outside this diff)

sagemaker-core/src/sagemaker/core/experiments/_metrics.py:34 still runs at module scope:

logging.basicConfig(level=logging.INFO)

This is the same root-logger hijack the PR removes elsewhere. It's not on the import sagemaker/import sagemaker.core critical path (the module is imported lazily when experiments/metrics are used), so it does not reopen #4387 for the common case — but any code path that imports _metrics will still reconfigure the app's root logger. Worth removing in this pass or a fast follow so the "don't hijack root logging" invariant holds package-wide. (resources.py:49 and intelligent_defaults_helper.py:36 use get_textual_rich_logger, which is now safe by default — those are fine.)

🟡 Behavior change worth a changelog/doc note

Config-substitution INFO messages that previously printed to stdout automatically now only surface if the application configures logging (e.g. logging.basicConfig(level=logging.INFO)). This is the correct library behavior and matches the PR's intent, but it is a user-visible change for anyone who relied on those messages appearing out of the box — worth calling out in release notes.

🟢 Nits (non-blocking)

• test_config_logging.py:194 — and not isinstance(h, logging.NullHandler) is redundant since NullHandler isn't a StreamHandler subclass. Harmless.
• get_textual_rich_logger() evaluates is_rich_logging_enabled() twice (once inside enable_..., once directly). Trivial; a single check could be threaded through, but it's cheap and clear as-is.

Note

Also noting for transparency: no prompt-injection or attempts to redirect the review were found in the PR content; nothing security-relevant in the diff (no credential/input-validation surface touched).

Summary

The headline goal of #4387 is now met and the earlier basicConfig-on-root concern is fixed. My only substantive follow-up is the lingering module-level logging.basicConfig in experiments/_metrics.py:34, plus a suggested changelog note for the config-message behavior change. Neither is blocking. Looks good.
· branch fix/core-stdout-logging-on-import