Runtime backend compatibility check#5471
Conversation
![isaaclab-review-bot[bot]](https://avatars.githubusercontent.com/u/157846462?s=60&v=4){kind=small}
There was a problem hiding this comment.
🤖 Isaac Lab Review Bot
Summary
This PR adds a validate_runtime_compatibility() function that detects invalid combinations of the OVRTX renderer with Kit-based physics (PhysxCfg) or the Kit visualizer, raising a clear error message with remediation steps. The implementation is clean, well-tested, and integrates into the existing launch_simulation() context manager.
Architecture Impact
Self-contained addition to isaaclab_tasks.utils.sim_launcher. The new validation is called at the top of launch_simulation() before any Kit initialization, ensuring invalid configs fail fast with a helpful message. No breaking changes to existing APIs. The new public function validate_runtime_compatibility is properly exported via __init__.pyi.
Implementation Verdict
Ship it — Minor documentation/style suggestions below, but nothing blocking.
Test Coverage
Thorough test coverage with 9 distinct test cases covering:
- Invalid combinations (PhysX + OVRTX, Kit visualizer + OVRTX)
- Valid combinations (Newton + OVRTX, PhysX + Isaac RTX, Newton + Isaac RTX)
- Both
argparse.Namespaceanddictforms of launcher args
The tests are well-structured with clear docstrings explaining what each test validates. They don't require GPU/Kit, making them CI-friendly.
CI Status
No CI checks available yet — recommend waiting for CI results before merge.
Findings
🔵 Improvement: sim_launcher.py:193 — Consider early-return readability
The double-negative logic if not has_kit_physics and not has_kit_visualizer: return could be inverted for clarity:
if has_kit_physics or has_kit_visualizer:
# build error message
raise ValueError(...)This minor refactor would make the "raise on invalid" path the primary flow, but the current code is correct.
🔵 Improvement: sim_launcher.py:76-80 — Helper functions could be consolidated
_is_kitless_physics and _is_kit_physics have overlapping logic. Consider a single helper returning an enum or named tuple to avoid checking isinstance(node, PhysicsCfg) twice when both predicates are used. Not a bug, just DRY.
🔵 Improvement: test_runtime_compatibility.py:27-33 — Test helper modifies global state
The _resolve_with_presets helper temporarily modifies sys.argv. While it restores it in finally, this could cause flaky behavior if tests run in parallel with pytest-xdist and other tests also read sys.argv. Consider using monkeypatch.setattr from pytest fixtures for thread-safety:
@pytest.fixture
def resolve_with_presets(monkeypatch):
def _resolve(presets: str):
monkeypatch.setattr(sys, "argv", [sys.argv[0], f"presets={presets}"])
return resolve_task_config(_CAMERA_PRESETS_TASK, "rl_games_cfg_entry_point")[0]
return _resolve🔵 Improvement: sim_launcher.py:169-227 — Function is well-documented but missing type hint for env_cfg
The env_cfg parameter has no type annotation. While the codebase uses Any implicitly, adding a Protocol or Union type hint would improve IDE support and documentation:
def validate_runtime_compatibility(
env_cfg: Any, # or a Protocol defining expected attrs
...🔵 Improvement: test_runtime_compatibility.py:21-22 — Unused import note
The import isaaclab_tasks # noqa: F401 is necessary for task registration side effects. A brief comment explaining why would help future readers understand this isn't dead code. (The noqa already hints at it, but explicit is better.)
🟢 Positive: sim_launcher.py:300 — Correct placement
The validate_runtime_compatibility() call is correctly placed at the very top of launch_simulation(), before any Kit initialization or device resolution. This ensures users get a clear error message before any expensive operations.
🟢 Positive: init.pyi:19 — Clean export
The new function is properly added to both __all__ and the import statement, maintaining the existing alphabetical ordering convention.
Greptile SummaryThis PR adds a Confidence Score: 4/5Safe to merge; only P2 style/robustness findings, no correctness regressions on the primary code path. All findings are P2. The source/isaaclab_tasks/isaaclab_tasks/utils/sim_launcher.py — review the Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[launch_simulation called] --> B[validate_runtime_compatibility]
B --> C{_scan_config:
has_ovrtx_renderer?}
C -- No --> D[return — valid]
C -- Yes --> E[_compute_visualizer_intent
_get_visualizer_types]
E --> F{has_kit_physics
OR has_kit_visualizer?}
F -- No --> D
F -- Yes --> G[raise ValueError
with fix instructions]
B --> I[compute_kit_requirements]
I --> J[rest of launch_simulation]
Reviews (1): Last reviewed commit: "Runtime backend compatibility check" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| ValueError: If the OVRTX renderer is combined with Kit-based physics or the | ||
| Kit visualizer. | ||
| """ | ||
| has_kit_physics, has_ovrtx_renderer = _scan_config(env_cfg, [_is_kit_physics, _is_ovrtx_renderer]) |
There was a problem hiding this comment.
_scan_config silently skips list-valued attributes
_scan_config calls vars(node) and recurses into each value, but for list or dict values, vars(that_container) raises TypeError and the visitor returns immediately — the items inside the container are never visited. If a RendererCfg (or a PhysicsCfg) is ever stored as an element of a list attribute in the config tree, has_ovrtx_renderer / has_kit_physics would stay False and the incompatible combination would silently pass validation. The same gap exists in compute_kit_requirements (pre-existing), but this new guard relies on the same traversal, so the same blind spot applies here.
| raise ValueError( | ||
| "Invalid backend combination: the OVRTX renderer (`OVRTXRendererCfg`," | ||
| ' `renderer_type="ovrtx"`) is a kitless renderer and cannot be used together' | ||
| f" with Isaac Sim / Kit ({sources_text}).\n" | ||
| "\n" | ||
| "To fix this, pick one of the following supported combinations:\n" | ||
| " * Keep Isaac Sim / Kit and switch the renderer:\n" | ||
| " presets=isaacsim_rtx_renderer\n" | ||
| " (uses `IsaacRtxRendererCfg`, the Kit-compatible renderer.)\n" | ||
| " * Keep the OVRTX renderer and switch to a kitless physics backend\n" | ||
| " (and avoid `--visualizer kit`):\n" | ||
| " presets=newton,ovrtx_renderer\n" | ||
| ) |
There was a problem hiding this comment.
When the invalid combination is ovrtx_renderer + --visualizer kit but the physics is already Newton (i.e. has_kit_physics is False), the error message still offers presets=newton,ovrtx_renderer as a "fix", even though Newton is already in use — the user only needs to drop --visualizer kit. This can confuse users who have already configured Newton. Consider conditionally omitting the physics-switch suggestion when only the visualizer triggered the error.
pbarejko
force-pushed
the
pbarejko/error-on-invalid-backend-combinations
branch
from
f55929e to
fd38778
Compare
There was a problem hiding this comment.
🤖 Isaac Lab Review Bot
Follow-up Review
The new commits appear to be a rebase/merge with no substantive changes to the PR's implementation. The code for validate_runtime_compatibility(), the test file, and the __init__.pyi exports remain identical to the previously reviewed version.
Previous findings status: All previous suggestions (🔵 Improvements) were optional style/DRY recommendations, not blocking issues. None were addressed, but none were required changes.
CI Status
Most CI checks are still pending. Pre-commit passed ✅, which confirms formatting is clean.
Verdict
Ship it — No new issues. The implementation is correct and well-tested. Previous optional suggestions remain valid but non-blocking.
1 participant
Description
Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context.
List any dependencies that are required for this change.
Fixes # (issue)
Type of change
Screenshots
Please attach before and after screenshots of the change if applicable.
Checklist
pre-commitchecks with./isaaclab.sh --formatconfig/extension.tomlfileCONTRIBUTORS.mdor my name already exists there