Add preset backend tests - #4838
Conversation
Greptile SummaryThis PR adds preset backend integration tests across Key issues found:
Confidence Score: 3/5
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| from isaaclab.renderers import Renderer | ||
| from isaaclab.renderers.base_renderer import BaseRenderer | ||
|
|
||
| from isaaclab_ov.renderers import OVRTXRendererCfg, OVRTXRenderer |
There was a problem hiding this comment.
Module-level import of OVRTXRenderer will cause a collection error when ovrtx is absent.
OVRTXRenderer is imported at module level here. Via lazy_export(), this resolves ovrtx_renderer.py, which immediately does from ovrtx import Device, PrimMode, Renderer, ... (a GPU/C-library). When ovrtx is not installed (e.g., in a CI environment without OV), this raises an ImportError at collection time, producing a hard collection error instead of a graceful skip.
The analogous test in isaaclab_physx/test/test_renderer_backends.py correctly guards the real-instantiation test with pytest.importorskip("omni.physics"). A similar guard is needed here:
| from isaaclab_ov.renderers import OVRTXRendererCfg, OVRTXRenderer | |
| import pytest | |
| from isaaclab.renderers import Renderer | |
| from isaaclab.renderers.base_renderer import BaseRenderer | |
| pytest.importorskip("ovrtx") | |
| from isaaclab_ov.renderers import OVRTXRendererCfg, OVRTXRenderer |
Or move the OVRTXRenderer import inside the test body behind the skip guard.
| @pytest.mark.parametrize( | ||
| "cfg_cls,expected_class_name", | ||
| [ | ||
| (IsaacRtxRendererCfg, "IsaacRtxRenderer"), | ||
| (OVRTXRendererCfg, "OVRTXRenderer"), | ||
| ], | ||
| ids=["IsaacRtxRendererCfg", "OVRTXRendererCfg"], | ||
| ) | ||
| def test_renderer_factory_instantiation_real_backends(cfg_cls, expected_class_name): | ||
| """Renderer(cfg) with real registry returns correct backend class (no mock). | ||
| NewtonWarpRenderer is excluded: its __init__ requires SimulationContext.instance(). | ||
| """ | ||
| cfg = cfg_cls() | ||
| renderer = Renderer(cfg) | ||
| assert type(renderer).__name__ == expected_class_name | ||
| assert isinstance(renderer, BaseRenderer) |
There was a problem hiding this comment.
Real-instantiation test missing per-renderer skip guards for C-library dependencies.
This test tries to instantiate both IsaacRtxRenderer (omni.physics) and OVRTXRenderer (ovrtx) for real, with no skip guards beyond the module-level importorskip("isaaclab_physx") / importorskip("isaaclab_ov"). Those guards only confirm that the Python packages are importable (they use lazy_export), not that their underlying native/C dependencies (omni.physics, ovrtx) are available.
When Renderer(OVRTXRendererCfg()) is called it dynamically imports isaaclab_ov.renderers.OVRTXRenderer, triggering from ovrtx import .... If ovrtx is absent this surfaces as an ImportError during the test run rather than a clean skip.
The isaaclab_physx/test/test_renderer_backends.py establishes the correct pattern — guard the real-instantiation path:
| @pytest.mark.parametrize( | |
| "cfg_cls,expected_class_name", | |
| [ | |
| (IsaacRtxRendererCfg, "IsaacRtxRenderer"), | |
| (OVRTXRendererCfg, "OVRTXRenderer"), | |
| ], | |
| ids=["IsaacRtxRendererCfg", "OVRTXRendererCfg"], | |
| ) | |
| def test_renderer_factory_instantiation_real_backends(cfg_cls, expected_class_name): | |
| """Renderer(cfg) with real registry returns correct backend class (no mock). | |
| NewtonWarpRenderer is excluded: its __init__ requires SimulationContext.instance(). | |
| """ | |
| cfg = cfg_cls() | |
| renderer = Renderer(cfg) | |
| assert type(renderer).__name__ == expected_class_name | |
| assert isinstance(renderer, BaseRenderer) | |
| @pytest.mark.parametrize( | |
| "cfg_cls,expected_class_name,skip_module", | |
| [ | |
| (IsaacRtxRendererCfg, "IsaacRtxRenderer", "omni.physics"), | |
| (OVRTXRendererCfg, "OVRTXRenderer", "ovrtx"), | |
| ], | |
| ids=["IsaacRtxRendererCfg", "OVRTXRendererCfg"], | |
| ) | |
| def test_renderer_factory_instantiation_real_backends(cfg_cls, expected_class_name, skip_module): | |
| pytest.importorskip(skip_module) | |
| cfg = cfg_cls() | |
| renderer = Renderer(cfg) | |
| assert type(renderer).__name__ == expected_class_name | |
| assert isinstance(renderer, BaseRenderer) |
1d58a58 to
d387e3f
Compare
d387e3f to
08a398d
Compare
031811b to
fe90744
Compare
| def test_ovrtx_renderer_cfg_yields_ovrtx_renderer(): | ||
| """OVRTXRendererCfg -> Renderer(cfg) returns OVRTXRenderer.""" | ||
| cfg = OVRTXRendererCfg() | ||
| renderer = Renderer(cfg) |
There was a problem hiding this comment.
This is instantiation of the renderer backend - this will not work. You need to have deps installed.
| from isaaclab.renderers import Renderer | ||
| from isaaclab.renderers.base_renderer import BaseRenderer | ||
|
|
||
| from isaaclab_ov.renderers import OVRTXRendererCfg, OVRTXRenderer |
There was a problem hiding this comment.
You should not import OVRTXRenderer class. An instance of this class is meant to be created through factory.
| class TestHydraPresetsIntegration: | ||
| """Test that preset string -> parse_overrides -> apply_overrides yields expected backends.""" | ||
|
|
||
| def test_parse_overrides_global_presets(self): |
There was a problem hiding this comment.
Those are more like rendering tests. What I would find valuable, is something similar to this:
You provide preset and do isinstance check if proper config has been instantiated.
There was a problem hiding this comment.
Hello, @bareya .
I have made changes to reflect this and also reused _resolve_with_presets from test_preset_kit_decision.py. It reused methods that I called previously, so I thought it would make the tests easier to follow.
Is the test test_resolve_renderer_backend_ovrtx for ovrtx proper? I wanted to make sure I instantiated OVRTXRendererCfg properly like you showed me from here .
…inila/presets-unit-tests
|
Hi @bdilinila — thanks for putting this one up! 🙏 We're doing a cleanup pass over the Isaac Lab PR backlog, which had grown past 400 open pull requests, and we're closing out the ones that have gone quiet so the queue is reviewable again. Why this PR is being closed: Here is exactly what we found on this PR when we reviewed the backlog:
It was picked up by the sweep because it has been open for about 6 months. It was then put in the "close" bucket because the author has been silent for about 6 months — which is the signal we used to tell apart pull requests that are still being worked on from ones that have genuinely been set aside. We deliberately did not close pull requests that were approved and ready to land, or that were small and clearly still fixing a live bug — there were 27 of those, and we are merging them rather than closing them. No judgement on the change itself — this is purely backlog hygiene. If this is still wanted, please reopen it or re-submit against 🤖 This comment was drafted with AI assistance as part of a maintainer-led sweep of the Isaac Lab pull request backlog. A maintainer is behind this cleanup — but if this closure looks wrong, it may well be, so please push back and we'll take another look. |
3 participants
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