-
Notifications
You must be signed in to change notification settings - Fork 3.8k
Fix isaaclab.* INFO logs silenced on kitless backends #6813
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| 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 | ||
| :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. | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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 | ||
|
|
||
|
|
@@ -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() | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
|
|
||
| def __init__(self, launcher_args: argparse.Namespace | dict | None = None, **kwargs): | ||
| """Create a `SimulationApp`_ instance based on the input settings. | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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: | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
| """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") | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. AI-generated review — Moderate: |
||
|
|
||
| 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) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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 | ||
|
|
@@ -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() | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. AI-generated review — Important: The existing |
||
| if level >= logging.WARNING: | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 Warning · Implementation — Root level override also matches ERROR/CRITICAL
|
||
| # 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) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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?
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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): | ||
|
|
||
There was a problem hiding this comment.
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.