fix(core): stop hijacking stdout logging on import

[rsareddy0329]

Issue #, if available: Closes #4387

Description of changes:

get_sagemaker_config_logger() attached a StreamHandler(sys.stdout) to the
sagemaker.config logger and set propagate=False the first time it ran (which
happens during config resolution at import). A library forcing its own stdout
handler and disabling propagation overrides the application's logging
configuration and writes to stdout just from import sagemaker, as reported in
#4387.

This follows the standard library-logging pattern instead:

• Install a NullHandler on the top-level sagemaker logger at import (in
  sagemaker/core/__init__.py, the universal dependency), so SDK log records are
  safely discarded until the application configures logging.
• get_sagemaker_config_logger() now only sets a default INFO level when unset
  and lets records propagate to the application's handlers, rather than
  attaching a stdout handler or disabling propagation. Removed the now-unused
  import sys.

Behavior note: config-substitution INFO messages are still emitted on the
sagemaker.config logger; applications that configure logging (e.g.
logging.basicConfig(level=logging.INFO)) receive them through their own
handlers/formatting instead of an SDK-owned stdout handler.

Testing:

• New tests/unit/config/test_config_logging.py: asserts no stdout StreamHandler
  is attached, propagation is not disabled, the INFO default is applied, and the
  root sagemaker logger has a NullHandler after import.
• tests/unit/config shows no new failures from this change (the pre-existing
  environment-specific 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