fix(core): make rich console and traceback opt-in

[rsareddy0329]

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

Description of changes:

Importing the SDK ran enable_textual_rich_console_and_traceback() at module
import time — once in sagemaker/core/__init__.py, and again as a side effect of
the module-level logger in utils.py. That called rich.traceback.install(),
which overrides sys.excepthook process-wide, and reconfigured the global
rich console. As reported in #5179 and #5561, simply import sagemaker changed
traceback rendering for the entire process, with no way to turn it off.

This makes those process-wide side effects opt-in:

• enable_textual_rich_console_and_traceback() is now a no-op unless the user sets
  SAGEMAKER_ENABLE_RICH_LOGGING to 1/true/yes/on (case-insensitive), or a
  caller passes force=True.
• Gating inside the function covers both import-time entry points at once, so a
  default import sagemaker no longer touches sys.excepthook or the global console.
• Added is_rich_logging_enabled() and the RICH_LOGGING_OPT_IN_ENV_VAR constant.

Users who want the rich tracebacks/console back set the env var (or call with
force=True); default behavior is now standard Python tracebacks.

Testing:

• New tests/unit/utils/test_rich_logging.py: default no-op, truthy/falsey env
  parsing, env opt-in, force=True, and idempotency (16 cases).
• Behavioral check: import sagemaker.core with no env leaves sys.excepthook as
  the stdlib default; with SAGEMAKER_ENABLE_RICH_LOGGING=1 it becomes
  rich.traceback.
• Surrounding tests/unit/utils + tests/unit/generated/test_utils.py pass (63).

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 1m 6s —— View job

---

Claude review — fix(core): make rich console and traceback opt-in

• Read the PR diff (/tmp/pr.diff)
• Inspect changed code and callers in context
• Review correctness, tests, best practices
• Post review

---

Overall this is a well-scoped, well-tested change. The gating logic in enable_textual_rich_console_and_traceback is correct, the env-var parsing (.strip().lower() against a frozenset) is clean, and the new tests cover the important cases (default no-op, truthy/falsey parsing, force=True, idempotency latch). The __init__.py change is just a clarifying comment, which is fine.

I did find one gap worth discussing.

🟡 The fix is incomplete — the module-level logger still mutates global logging state at import

sagemaker-core/src/sagemaker/core/utils/utils.py:218 still runs at import time:

logger = get_textual_rich_logger(__name__)

and get_textual_rich_logger (lines 198–215) does:

def get_textual_rich_logger(name, log_level="INFO"):
    enable_textual_rich_console_and_traceback()          # ✅ now gated by this PR
    handler = get_rich_handler()                          # RichHandler(markup=True)
    logging.basicConfig(level=getattr(logging, log_level), handlers=[handler])  # ❌ still unconditional
    logger = logging.getLogger(name)
    return logger

The PR gates the enable_textual_rich_console_and_traceback() call, so sys.excepthook and the global rich console are no longer touched by default — that part is correct. But the same import path still calls logging.basicConfig(level=INFO, handlers=[RichHandler(...)]) unconditionally. On a fresh process where the app hasn't configured logging yet, import sagemaker.core therefore still:

1. installs a RichHandler on the root logger, changing the formatting of all log output process-wide, and
2. sets the root logger level to INFO, which can make an application noticeably more verbose than it configured itself to be.

This is the same class of "merely importing the SDK changes global process behavior" problem the PR is targeting (issues #5179/#5561 focus on tracebacks, but the root-logger mutation is arguably the sibling issue). The PR body states the goal is that "a default import sagemaker no longer touches sys.excepthook or the global console" — that is now true, but the root logger is still reconfigured.

Suggestion: gate the basicConfig/RichHandler path behind the same is_rich_logging_enabled() check (or fall back to a plain, non-configuring logging.getLogger(name) when not opted in). For example:

def get_textual_rich_logger(name: str, log_level: str = "INFO") -> logging.Logger:
    enable_textual_rich_console_and_traceback()
    if is_rich_logging_enabled():
        handler = get_rich_handler()
        logging.basicConfig(level=getattr(logging, log_level), handlers=[handler])
    return logging.getLogger(name)

Libraries generally should not call logging.basicConfig at import time at all (it's an application concern), so gating it also aligns with common Python logging guidance. If gating this is intentionally out of scope for this PR, it'd be worth a sentence in the PR description noting the root-logger handler is a known remaining side effect.

Minor notes (non-blocking)

• is_rich_logging_enabled() formatting (utils.py:157-159): the outer parentheses wrap a single expression and aren't needed — return os.environ.get(...).strip().lower() in _TRUTHY_ENV_VALUES reads fine on one line. Purely cosmetic; leave it if black is happy.
• Test isolation: the _reset_latch autouse fixture correctly saves/restores the module-global latch — nice. Since the tests mock reconfigure/install, they don't actually exercise sys.excepthook, which is fine for these unit tests; the behavioral check described in the PR body covers the real hook.

Nothing here is a correctness bug in the code that was changed — the gating itself is sound. The main point is whether you want this PR to also close the root-logger side effect, since a plain import sagemaker still alters process-wide logging today.
· branch fix/core-rich-logging-opt-in

[github-actions]

Claude finished @rsareddy0329's task in 58s —— View job

---

Claude review — fix(core): make rich console and traceback opt-in

• Read the PR diff (/tmp/pr.diff)
• Inspect changed code and callers in context
• Review correctness, tests, best practices
• Post review

---

The gating logic itself is correct and well-tested. is_rich_logging_enabled() parses the env var cleanly (.strip().lower() against a frozenset), enable_textual_rich_console_and_traceback() short-circuits before touching sys.excepthook or the global console, the one-shot latch still works, and the new test file covers the important cases (default no-op, truthy/falsey parsing, force=True, idempotency). os and logging are both imported, so no missing-import issues. Nice, focused change for the part it touches.

There's one substantive gap, plus minor notes.

🟡 The import-time root-logger mutation is still unconditional

sagemaker-core/src/sagemaker/core/utils/utils.py:196-213:

def get_textual_rich_logger(name: str, log_level: str = "INFO") -> logging.Logger:
    enable_textual_rich_console_and_traceback()          # ✅ now gated by this PR
    handler = get_rich_handler()                          # RichHandler(markup=True)
    logging.basicConfig(level=getattr(logging, log_level), handlers=[handler])  # ❌ still unconditional
    logger = logging.getLogger(name)
    return logger

This function runs at import time in three places that load on a plain import sagemaker.core:

• utils.py:216 — logger = get_textual_rich_logger(__name__)
• resources.py:49 — logger = get_textual_rich_logger(__name__)
• utils/intelligent_defaults_helper.py:36 — logger = get_textual_rich_logger(__name__)

The PR gates the traceback/console side effects, but get_textual_rich_logger still calls logging.basicConfig(level=INFO, handlers=[RichHandler(markup=True)]) regardless of opt-in. On a fresh process where the app hasn't configured logging yet, importing the SDK therefore still:

1. installs a RichHandler on the root logger, changing formatting for all process-wide log output, and
2. sets the root logger level to INFO, potentially making the app more verbose than it configured.

markup=True compounds this: log messages elsewhere in the process that contain [...] get interpreted as rich markup. This is the same "importing the SDK changes global process behavior" class of problem the PR targets — #5179/#5561 emphasize tracebacks, but the root-logger mutation is the sibling issue. The PR body's claim that a default import sagemaker "no longer touches sys.excepthook or the global console" is now accurate, but the root logger is still reconfigured.

Libraries generally should not call logging.basicConfig at import at all (it's an application concern). Suggested fix — gate it behind the same check:

def get_textual_rich_logger(name: str, log_level: str = "INFO") -> logging.Logger:
    enable_textual_rich_console_and_traceback()
    if is_rich_logging_enabled():
        handler = get_rich_handler()
        logging.basicConfig(level=getattr(logging, log_level), handlers=[handler])
    return logging.getLogger(name)

If closing this is intentionally out of scope, it's worth a sentence in the PR description noting the root-logger handler/level is a known remaining side effect — otherwise the fix reads as complete when a RichHandler is still attached to the root logger on import.

Minor notes (non-blocking)

• Test coverage for the latch as saved-state: _reset_latch restores the module global — good isolation. Since the tests mock reconfigure/install, they don't exercise the real sys.excepthook; the PR body's behavioral check covers that, which is fine for unit scope. If you gate the basicConfig path, add a test asserting basicConfig is not called when opted out.
• is_rich_logging_enabled() (utils.py:157) reads cleanly on one line; leave as-is if black is happy.

Summary

The gating that was added is sound and correctly tested — no correctness bug in the changed code. The main question for the author is whether this PR should also gate the import-time logging.basicConfig call in get_textual_rich_logger, since a plain import sagemaker still mutates process-wide root-logger config today. If not, note it as a known remaining side effect.

No prompt-injection attempts were found in the PR content.
· branch fix/core-rich-logging-opt-in

[github-actions]

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

---

Claude review — fix(core): make rich console and traceback opt-in

• Read the PR diff (/tmp/pr.diff)
• Inspect changed code and callers in context
• Review correctness, tests, best practices
• Post review

---

This iteration is in good shape. The gating logic is correct, and — importantly — this version now also closes the follow-up raised in earlier reviews: get_textual_rich_logger gates the logging.basicConfig(... RichHandler ...) call behind is_rich_logging_enabled(), so a plain import sagemaker.core no longer attaches a RichHandler to the root logger or forces the root level to INFO. Combined with the gated enable_textual_rich_console_and_traceback(), importing the SDK is now free of process-wide sys.excepthook, console, and root-logger side effects by default. That's the right outcome for a library.

What's correct

• is_rich_logging_enabled() parses cleanly (.strip().lower() against a frozenset), tolerating whitespace and case.
• enable_textual_rich_console_and_traceback(force=False) short-circuits before reconfigure/install, and the one-shot latch still prevents duplicate installs.
• force=True gives explicit callers an escape hatch independent of the env var.
• All three import-time callers (utils.py:221, resources.py:49, intelligent_defaults_helper.py:36) now inherit the gated behavior for free.
• Tests cover the meaningful cases: truthy/falsey env parsing, default no-op, env opt-in, force=True, idempotency, and (new) that basicConfig is/ isn't called based on opt-in. The _reset_latch autouse fixture keeps the module-global latch isolated across tests.

Worth calling out (non-blocking)

1. Behavior change for existing users' log visibility. Previously, import sagemaker.core ran basicConfig(level=INFO, handlers=[RichHandler]), so the SDK's own INFO logs surfaced on the console out of the box. With opt-out as the default and no root handler configured, those INFO/DEBUG records now fall through to Python's "last resort" handler, which only emits WARNING+ to stderr. This is the correct library behavior, but it is a user-visible change — someone who relied on seeing SDK INFO logs without configuring logging themselves will now see less. Worth a line in the changelog/PR description so it isn't a surprise.

2. configure_logging() is unchanged (and that's fine). utils.py:231 still unconditionally attaches a RichHandler(markup=True) to the root logger and even removes existing handlers. That's acceptable because it's an explicit user-invoked call (not run at import), so it stays outside the scope of "importing shouldn't mutate global state." No change requested — just confirming it was considered.

3. Minor: is_rich_logging_enabled() is evaluated twice per get_textual_rich_logger call (once inside enable_..., once directly). Negligible cost; only worth mentioning for readability if you'd rather compute it once.

Summary

Correct, focused, and well-tested. The gating is sound and this revision also resolves the root-logger side effect flagged earlier, so the "import must not change global process behavior" goal now holds end to end. My only suggestion is to note the reduced default log visibility (item 1) in the PR description/changelog. No blocking issues.

No prompt-injection attempts were found in the PR content.
· branch fix/core-rich-logging-opt-in

[rsareddy0329]

Superseded by #6341, which now carries these rich-logging opt-in changes byte-identically alongside the related import-time logging fixes (config-logger NullHandler + root-logger basicConfig gating). Consolidating into a single PR to avoid overlapping edits to the same utils.py code path. Closing this in favor of #6341, which closes #5179, #5561, and #4387 together.