Raise error when explicitly requested visualizer fails

[rdsa-nv]

Summary

• When a visualizer is explicitly requested via CLI (--visualizer), raise a RuntimeError instead of silently logging the failure

Split from #4966 per reviewer feedback (simulation context change only).

Test plan

• Run with --visualizer <invalid> and verify RuntimeError is raised
• Run with --visualizer newton and verify normal initialization still works
• Run without --visualizer and verify silent fallback behavior is unchanged

[greptile-apps]

Greptile Summary

This PR improves failure-visibility for explicitly requested visualizers by raising a RuntimeError instead of silently swallowing failures when --visualizer is used on the CLI. Two guard points are added: one inside _resolve_visualizer_cfgs() that detects unknown types or missing-package failures before the create/initialize loop, and one inside initialize_visualizers() that re-raises exceptions from create_visualizer() / initialize() when cli_explicit=True.

Key changes:

• _resolve_visualizer_cfgs(): After building the resolved list, a new check compares every requested CLI type against the types that made it into resolved; any absent type raises RuntimeError with a helpful message.
• initialize_visualizers(): When cli_explicit=True, exceptions inside the per-visualizer try/except are re-raised as RuntimeError instead of being logged.
• New helper _make_context_with_settings consolidates boilerplate across test cases, and seven new tests cover explicit/non-explicit failure paths.

Issues found:

• initialize_visualizers() initializes self._scene_data_provider before the creation loop; when cli_explicit=True causes a RuntimeError to be thrown mid-loop, the cleanup block (if not self._visualizers … close_provider()) is never reached, leaving the provider open.
• test_explicit_missing_package_raises patches builtins.__import__ but _create_default_visualizer_configs uses importlib.import_module, which routes through CPython's internal bootstrap and does not call builtins.__import__. Additionally, isaaclab_visualizers.rerun is already cached in sys.modules from the file-level import isaaclab_visualizers.rerun.rerun_visualizer statement, so even a correct patch of importlib.import_module would be bypassed. The test therefore does not simulate the missing-package scenario it claims to test.

Confidence Score: 3/5

• Safe to merge with caveats — the core logic is sound but the provider cleanup leak and the broken missing-package test should be addressed before the PR is considered complete.
• The main production logic (detecting unknown types and re-raising on init failure) is correct. However, there is a genuine resource-cleanup gap: when an explicit RuntimeError is raised mid-loop, the SceneDataProvider initialized just before the loop is not closed. The test coverage gap (test_explicit_missing_package_raises) means the missing-package scenario is untested. These are real issues that reduce confidence in the PR being fully correct and verified.
• source/isaaclab/isaaclab/sim/simulation_context.py (provider cleanup on RuntimeError path) and source/isaaclab/test/sim/test_simulation_context_visualizers.py (broken missing-package test)

Important Files Changed

Filename	Overview
source/isaaclab/isaaclab/sim/simulation_context.py	Adds RuntimeError for explicitly requested visualizers that fail. The _resolve_visualizer_cfgs check correctly catches unknown types and missing packages. However, when cli_explicit=True causes a RuntimeError to be thrown mid-loop in initialize_visualizers, the scene data provider initialized just before the loop is not cleaned up.
source/isaaclab/test/sim/test_simulation_context_visualizers.py	Adds tests for the new explicit-error behavior. The _make_context_with_settings helper centralizes test context construction well. test_explicit_missing_package_raises has a fundamental flaw: monkeypatching builtins.import does not intercept importlib.import_module (which is what _create_default_visualizer_configs uses), and the rerun module is already in sys.modules from the module-level import, so the test will not actually simulate the missing-package scenario it claims to test.
source/isaaclab/config/extension.toml	Patch-level version bump from 4.5.20 to 4.5.21, consistent with the fix being added.
source/isaaclab/docs/CHANGELOG.rst	Adds a 4.5.21 changelog entry accurately describing the fix to initialize_visualizers error propagation.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[initialize_visualizers] --> B{self._visualizers non-empty?}
    B -- yes --> Z[return early]
    B -- no --> C[_resolve_visualizer_cfgs]
    C --> D{cli_disable_all?}
    D -- yes --> E[resolved = empty]
    D -- no --> F{cli_explicit?}
    F -- no --> G[resolved = cfg visualizer_cfgs]
    F -- yes --> H{cfg visualizer_cfgs empty?}
    H -- yes --> I[_create_default_visualizer_configs cli_requested]
    H -- no --> J[filter cfgs by cli_requested + create defaults for missing]
    I --> K
    J --> K
    G --> K
    E --> K
    K{cli_explicit AND cli_requested?} -- yes --> L{any cli_requested type missing from resolved?}
    L -- yes --> M[🔴 raise RuntimeError unknown/missing-package]
    L -- no --> N[return resolved]
    K -- no --> N
    N --> O{resolved empty?}
    O -- yes --> Z
    O -- no --> P[initialize_scene_data_provider]
    P --> Q[for each cfg in resolved]
    Q --> R{create_visualizer + initialize succeed?}
    R -- yes --> S[append to self._visualizers]
    S --> Q
    R -- no --> T{cli_explicit?}
    T -- yes --> U[🔴 raise RuntimeError ⚠️ provider not cleaned up]
    T -- no --> V[log exception]
    V --> Q
    Q -- done --> W{self._visualizers empty?}
    W -- yes --> X[close + clear _scene_data_provider]
    W -- no --> Y[done]

Loading

Comments Outside Diff (1)

1. source/isaaclab/isaaclab/sim/simulation_context.py, line 567-584 (link)

   SceneDataProvider not cleaned up on explicit RuntimeError

   When cli_explicit=True and a visualizer fails, the RuntimeError at line 569 exits the function early — bypassing the cleanup block at lines 580–584. This leaves self._scene_data_provider (initialized at line 559) open and self._scene_data_requirements set, even when self._visualizers ends up empty.

   In the non-explicit path the exception is logged and the loop continues normally, so the cleanup block is always reached. Only the explicit path is affected.

   To fix, the cleanup should be performed before (or instead of) re-raising:

           for cfg in visualizer_cfgs:
               try:
                   visualizer = cfg.create_visualizer()
                   visualizer.initialize(self._scene_data_provider)
                   self._visualizers.append(visualizer)
               except Exception as exc:
                   if cli_explicit:
                       # Clean up provider before propagating so we don't leak it.
                       if not self._visualizers and self._scene_data_provider is not None:
                           close_provider = getattr(self._scene_data_provider, "close", None)
                           if callable(close_provider):
                               close_provider()
                           self._scene_data_provider = None
                       raise RuntimeError(
                           f"Visualizer '{cfg.visualizer_type}' was explicitly requested "
                           f"but failed to create or initialize: {exc}"
                       ) from exc
                   logger.exception(
                       "Failed to initialize visualizer '%s' (%s): %s",
                       cfg.visualizer_type,
                       type(cfg).__name__,
                       exc,
                   )
   
           if not self._visualizers and self._scene_data_provider is not None:
               close_provider = getattr(self._scene_data_provider, "close", None)
               if callable(close_provider):
                   close_provider()
               self._scene_data_provider = None

_{Last reviewed commit: c90f7f1}

[greptile-apps]

Explicit-error guard has a gap for config-creation failures

The cli_explicit guard added here only covers failures that occur inside cfg.create_visualizer() or visualizer.initialize(). However, two earlier silent-failure paths in _create_default_visualizer_configs() are not covered:

1. Unknown type — if --visualizer <invalid> is passed, the type is not in _VISUALIZER_TYPES so _create_default_visualizer_configs logs a warning and returns an empty list. initialize_visualizers then hits the early-return at line 506 (if not visualizer_cfgs: return) — no RuntimeError is raised.

2. Package not installed — if --visualizer rerun is passed but isaaclab_visualizers is not installed, the ImportError is caught inside _create_default_visualizer_configs, a warning is logged, and again an empty list is returned — no RuntimeError is raised.

Because of this, the test-plan item "Run with --visualizer <invalid> and verify RuntimeError is raised" will not pass with the current code; the process exits silently with a log warning.

To fully cover explicit failures, _resolve_visualizer_cfgs (which already reads cli_explicit at line 471) could compare the set of successfully-created configs against cli_requested and raise when any explicitly-requested type is missing, e.g.:

if cli_explicit:
    resolved_types = {getattr(cfg, "visualizer_type", None) for cfg in resolved_cfgs}
    missing = [t for t in cli_requested if t not in resolved_types]
    if missing:
        raise RuntimeError(
            f"Explicitly requested visualizer(s) could not be configured: {missing}. "
            f"Valid types: {', '.join(_VISUALIZER_TYPES)}."
        )

[AntoineRichard]

@greptile

[greptile-apps]

Monkeypatching builtins.__import__ won't intercept importlib.import_module

_create_default_visualizer_configs loads the visualizer module with importlib.import_module(f"isaaclab_visualizers.{viz_type}") (line 376 of simulation_context.py). CPython's importlib.import_module routes through the internal _bootstrap._gcd_import machinery and does not call builtins.__import__. Patching builtins.__import__ therefore has no effect on this code path.

There is a second problem: because the test file contains a module-level import import isaaclab_visualizers.rerun.rerun_visualizer as rerun_visualizer, the module isaaclab_visualizers.rerun is already in sys.modules by the time this test runs. Even if importlib.import_module did honour the patched __import__, it would return the cached entry first and never call __import__ at all.

As a result this test does not actually simulate the "missing package" scenario and will either:

• silently pass for the wrong reason (the import succeeds from cache, a config is created, but resolved_types = {"rerun"} → missing = [] → no RuntimeError is raised → the pytest.raises block fails), or
• fail in CI rather than providing the intended coverage.

To reliably simulate a missing package, patch the importlib.import_module symbol that the production code imports, e.g.:

import importlib
import isaaclab.sim.simulation_context as sc_mod

monkeypatch.setattr(sc_mod, "importlib", ...)

or patch sc_mod._create_default_visualizer_configs directly to return an empty list, and separately verify that the outer guard in initialize_visualizers raises RuntimeError when resolved_types is empty.

[ooctipus]

@greptile