Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
Fixed
^^^^^

* Fixed ``logger.info`` calls in :class:`~isaaclab.renderers.RenderContext` and
:class:`~isaaclab.sensors.camera.Camera` (e.g. "Created new renderer for simulation" and
"Using renderer") being silenced on kitless backends (Newton, OvPhysX).
The ``isaaclab_info_stream`` log handler was only installed inside

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

AI-generated review — Moderate: Keep the changelog user-facing. These implementation details duplicate the PR description and helper docstring, and they advertise a helper that should remain internal. Reduce the entry to the observable outcome, for example: Fixed renderer and camera INFO messages being hidden when using kitless backends.

:class:`~isaaclab.app.AppLauncher`, which is skipped for kitless runs.
:func:`~isaaclab.app.logging_utils.ensure_isaaclab_info_stream_handler` is now a
shared utility called by both the Kit and kitless launch paths.
22 changes: 6 additions & 16 deletions source/isaaclab/isaaclab/app/app_launcher.py
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,11 @@
import isaacsim # noqa: F401
from isaacsim import SimulationApp

from isaaclab.app.logging_utils import apply_python_logging_level, resolve_python_logging_level
from isaaclab.app.logging_utils import (
apply_python_logging_level,
ensure_isaaclab_info_stream_handler,
resolve_python_logging_level,
)
from isaaclab.app.settings_manager import get_settings_manager, initialize_carb_settings
from isaaclab.utils._device import set_cuda_device

Expand Down Expand Up @@ -199,21 +203,7 @@ def _normalize_visualizer_intent(intent: Any) -> tuple[bool, bool]:
@staticmethod
def _ensure_isaaclab_info_stream_handler() -> None:
"""Add a stream handler for Isaac Lab INFO records hidden by Kit logging."""
handler_name = "isaaclab_info_stream"
root_logger = logging.getLogger()
if any(getattr(handler, "name", None) == handler_name for handler in root_logger.handlers):
return

class _IsaacLabInfoFilter(logging.Filter):
def filter(self, record: logging.LogRecord) -> bool:
return record.levelno == logging.INFO and record.name.startswith("isaaclab")

handler = logging.StreamHandler(sys.stdout)
handler.name = handler_name
handler.setLevel(logging.INFO)
handler.addFilter(_IsaacLabInfoFilter())
handler.setFormatter(logging.Formatter("[INFO]: %(message)s"))
root_logger.addHandler(handler)
ensure_isaaclab_info_stream_handler()

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

AI-generated review — Moderate: This private one-line delegator no longer owns any behavior and duplicates the shared helper's name. Remove it and invoke the module helper directly at the two _load_extensions call sites; that leaves one implementation and one functional contract to maintain.


def __init__(self, launcher_args: argparse.Namespace | dict | None = None, **kwargs):
"""Create a `SimulationApp`_ instance based on the input settings.
Expand Down
31 changes: 31 additions & 0 deletions source/isaaclab/isaaclab/app/logging_utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -58,3 +58,34 @@ def apply_python_logging_level(level: int) -> None:
root_logger.setLevel(level)
for handler in root_logger.handlers:
handler.setLevel(level)


def ensure_isaaclab_info_stream_handler() -> None:

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

AI-generated review — Moderate: This helper has only internal launcher callers, but its public name and changelog cross-reference turn an implementation detail into API that would later require deprecation. Please make it private, call it directly from both launcher modules, and keep its docstring to the functional contract. The current regardless of what Kit does claim also contradicts the root-level prerequisite documented immediately below it.

"""Add a stream handler that surfaces Isaac Lab INFO records on stdout.

Kit's Python log bridge suppresses INFO records on the console; this handler
compensates by routing ``isaaclab.*`` INFO records directly to stdout regardless
of what Kit does. The function is idempotent — calling it multiple times installs
the handler at most once.

After calling this, callers that want ``isaaclab.*`` INFO visible at the default
WARNING log level must also raise the root logger to INFO::

ensure_isaaclab_info_stream_handler()
logging.getLogger().setLevel(logging.INFO)
"""
handler_name = "isaaclab_info_stream"
root_logger = logging.getLogger()
if any(getattr(h, "name", None) == handler_name for h in root_logger.handlers):
return

class _IsaacLabInfoFilter(logging.Filter):
def filter(self, record: logging.LogRecord) -> bool:
return record.levelno == logging.INFO and record.name.startswith("isaaclab")

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

AI-generated review — Moderate: startswith("isaaclab") also accepts unrelated logger names such as isaaclab_plugin, although the contract says the isaaclab.* namespace. Match record.name == "isaaclab" or record.name.startswith("isaaclab."), and cover that boundary in the focused logging test.


handler = logging.StreamHandler(sys.stdout)
handler.name = handler_name
handler.setLevel(logging.INFO)
handler.addFilter(_IsaacLabInfoFilter())
handler.setFormatter(logging.Formatter("[INFO]: %(message)s"))
root_logger.addHandler(handler)
14 changes: 12 additions & 2 deletions source/isaaclab/isaaclab/app/sim_launcher.py
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,11 @@
from isaaclab_physx.physics import PhysxCfg
from isaaclab_physx.renderers import IsaacRtxRendererCfg

from isaaclab.app.logging_utils import apply_python_logging_level, resolve_python_logging_level
from isaaclab.app.logging_utils import (
apply_python_logging_level,
ensure_isaaclab_info_stream_handler,
resolve_python_logging_level,
)
from isaaclab.physics.physics_manager_cfg import PhysicsCfg
from isaaclab.renderers.renderer_cfg import RendererCfg
from isaaclab.sensors.camera.camera_cfg import CameraCfg
Expand Down Expand Up @@ -439,7 +443,13 @@ def launch_simulation(
# Kit-based backends apply the Python logging level inside AppLauncher; kitless backends
# never construct it, so honor --verbose / --info here to keep behavior consistent.
if not needs_kit:
apply_python_logging_level(resolve_python_logging_level(launcher_args))
level = resolve_python_logging_level(launcher_args)
apply_python_logging_level(level)
ensure_isaaclab_info_stream_handler()

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

AI-generated review — Important: The existing test_launch_simulation_kitless_applies_python_logging_level mocks only the resolver and applier. This new real call, together with the root-level branch below, mutates global logging state; the test's fake level 42 enters that branch without asserting or restoring the state. Mock this helper in the plumbing-only test, then add one focused standard-library logging test that covers two default launches, ERROR/CRITICAL, and an unrelated logger. No simulator integration test is needed.

if level >= logging.WARNING:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Warning · Implementation — Root level override also matches ERROR/CRITICAL

level >= logging.WARNING matches ERROR and CRITICAL too. A caller that set the root logger to ERROR gets that level applied by apply_python_logging_level(level) and then silently reset to INFO on the next line, re-enabling isaaclab.* INFO output through the scoped handler and record creation for all loggers. The comment states the intent is only the default WARNING case; use level == logging.WARNING, or raise only the isaaclab logger instead of root.

# Root logger is at WARNING; raise it to INFO so that isaaclab.* INFO
# records can propagate to the scoped stream handler above.
logging.getLogger().setLevel(logging.INFO)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

would this make all logging level as info for the process? i.e. we'd end up seeing a lot of prints in the terminal?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

AI-generated follow-up: Yes. The scoped handler filters what it prints, but setting the root logger to INFO changes effective levels process-wide. I reproduced a stronger two-launch case: the first default call left root at INFO; the second resolved that internal state as the requested level, lowered an existing root handler to INFO, and emitted an unrelated third-party INFO record. The bot is also correct that the current comparison overrides ERROR and CRITICAL. Please elevate only the isaaclab namespace and leave the root level unchanged.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

AI-generated suggestion: Centralize the policy in one private logging helper called by both AppLauncher and sim_launcher. It should: (1) apply the resolved level to the root logger and leave that root level unchanged; (2) set logging.getLogger("isaaclab") to INFO only when the resolved level is the default WARNING, otherwise use the resolved level; and (3) install or reconfigure one INFO handler filtered to record.name == "isaaclab" or record.name.startswith("isaaclab."). Remove the AppLauncher delegating wrapper. This surfaces Isaac Lab INFO records while Kit and third-party Python INFO records remain hidden by the WARNING root handlers. Carb or native stdout messages are outside Python logging and are unaffected. A focused standard-library test should cover two consecutive default configurations, ERROR/CRITICAL preservation, no duplicate handler, and rejection of a name such as isaaclab_plugin; no simulator integration test is needed.


if needs_kit and config_scan.has_kit_camera and launcher_args is not None:
if not _get_arg(launcher_args, "enable_cameras", False):
Expand Down