Fix renderer backend INFO logs silenced on kitless backends - #6813
Conversation
|
Too many files changed for review. ( |
There was a problem hiding this comment.
Isaac Lab Review Bot
The shared isaaclab_info_stream handler extraction preserves the Kit path and enables INFO output for kitless Newton/OvPhysX runs. However, the kitless root-level adjustment unintentionally overrides explicitly stricter logging levels such as ERROR or CRITICAL.
- Design and architecture: Centralizing the idempotent handler installer in
logging_utilsand retainingAppLauncher._ensure_isaaclab_info_stream_handleras a delegator is a sound way to share logging behavior across Kit and kitless launch paths. - API: The existing
AppLauncherstatic method remains available, while the new helper is documented consistently with the neighboring logging utilities. The patch-tier changelog fragment follows the required format. - Implementation: The extracted handler retains the existing filter, formatter, and name-based idempotence. In the kitless path, however,
level >= logging.WARNINGalso matches ERROR and CRITICAL, so the subsequent root logger change to INFO defeats a caller's stricter configured level. Limit this adjustment to the intended default WARNING case or scope the INFO level to theisaaclablogger.
Minor fixes needed. Posted 1 actionable finding inline.
Automated review; human maintainers own approval decisions.
|
This shows how logging has changed: Run 1 — generic
|
| Log file | Command | Created new renderer |
Using renderer |
|---|---|---|---|
| cmd1_rtx_newton_mjwarp.log | renderer=rtx physics=newton_mjwarp |
✅ OVRTXRenderer |
✅ OVRTXRenderer |
| cmd2_rtx_ovphysx.log | renderer=rtx physics=ovphysx |
✅ OVRTXRenderer |
✅ OVRTXRenderer |
| cmd3_rtx_viz_kit.log | renderer=rtx --viz=kit |
✅ IsaacRtxRenderer |
✅ IsaacRtxRenderer |
Key log lines
cmd1 (renderer=rtx physics=newton_mjwarp — OVRTX kitless + Newton MJWarp):
[INFO]: Created new renderer for simulation: OVRTXRenderer
[INFO]: Using renderer: OVRTXRenderer
cmd2 (renderer=rtx physics=ovphysx — OVRTX kitless + OVPhysX):
[INFO]: Created new renderer for simulation: OVRTXRenderer
[INFO]: Using renderer: OVRTXRenderer
cmd3 (renderer=rtx --viz=kit — Isaac Sim Kit path):
[INFO]: Created new renderer for simulation: IsaacRtxRenderer
[INFO]: Using renderer: IsaacRtxRenderer
|
@ndahile-nvidia for review |
AntoineRichard
left a comment
There was a problem hiding this comment.
AI-generated review
Requesting changes.
The review bot's >= logging.WARNING finding is correct, and Kelly's process-wide logging concern is reproducible. With the sequence in this PR, a first default kitless launch resolves WARNING but leaves the root logger at INFO; a second launch then resolves that internally modified INFO level, lowers pre-existing root handlers to INFO, and allows unrelated third-party INFO records through. Explicit ERROR and CRITICAL levels are also reset to INFO. Please keep the INFO enablement scoped to the isaaclab logger namespace and preserve the caller's root level.
The inline comments cover the minimal regression coverage, exact namespace matching, unnecessary API/indirection, and duplicated changelog rationale.
Verification: source/isaaclab_tasks/test/core/test_sim_launcher_visualizer_intent.py passed (3 tests), but its current logging test does not exercise this behavior. ./isaaclab.sh -f passed all hooks. The PR currently conflicts with develop and will also need a rebase.
|
AI-generated implementation suggestion I recommend centralizing the complete logging policy in one private helper. The root logger should remain at the resolved level, while the In _ISAACLAB_INFO_HANDLER_NAME = "isaaclab_info_stream"
def _ensure_isaaclab_info_stream_handler() -> None:
"""Install the scoped Isaac Lab INFO handler if needed."""
root_logger = logging.getLogger()
for handler in root_logger.handlers:
if handler.name == _ISAACLAB_INFO_HANDLER_NAME:
# apply_python_logging_level() may have changed this.
handler.setLevel(logging.INFO)
return
class _IsaacLabInfoFilter(logging.Filter):
def filter(self, record: logging.LogRecord) -> bool:
is_isaaclab_logger = record.name == "isaaclab" or record.name.startswith("isaaclab.")
return record.levelno == logging.INFO and is_isaaclab_logger
handler = logging.StreamHandler(sys.stdout)
handler.name = _ISAACLAB_INFO_HANDLER_NAME
handler.setLevel(logging.INFO)
handler.addFilter(_IsaacLabInfoFilter())
handler.setFormatter(logging.Formatter("[INFO]: %(message)s"))
root_logger.addHandler(handler)
def _configure_python_logging(level: int) -> None:
"""Configure Python logging with default INFO output scoped to Isaac Lab."""
apply_python_logging_level(level)
isaaclab_logger = logging.getLogger("isaaclab")
isaaclab_logger.setLevel(logging.INFO if level == logging.WARNING else level)
if level <= logging.WARNING:
_ensure_isaaclab_info_stream_handler()Then both launch paths can use the same policy. In _configure_python_logging(self._python_logging_level)In if not needs_kit:
level = resolve_python_logging_level(launcher_args)
_configure_python_logging(level)This removes the process-wide A focused standard-library test should verify:
No simulator integration test is needed. Per the regression-test policy, temporarily restoring the old root-level override should make the new test fail. |
The two startup messages [INFO]: Created new renderer for simulation: <name> [INFO]: Using renderer: <name> were silenced on Newton / OvPhysX backends because the root logger and its handlers default to WARNING and no Kit logging bridge is present to surface them. Instead of permanently altering any logger or handler level, add a force_log_level(level) context manager to logging_utils that saves the root logger and handler levels, lowers them for the duration of the with-block, then restores them. The two call sites in RenderContext and Camera wrap their logger.info() calls with this shim so the messages always reach the console without affecting any other log output.
a44a6c7 to
75a682c
Compare
This comment was made on previous temp fix.
Summary
Test plan