Decouple rendering tests from task environments - #6976
Conversation
|
Too many files changed for review (621 files, 100 file limit). |
![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
The rendering-test decoupling and InteractiveScene.reset_to_default delegation are coherent, but two minor public-facing documentation issues should be corrected before merge: the changelog includes a test-internal Changed entry without migration guidance, and the new method’s docstring overstates which scene entity types it restores.
- Design and architecture: The centralized task-free rendering scene, shared case matrix, and thin manager-event adapter establish a clear dependency direction. No architectural defect was identified in the proposed scene ownership or delegation model.
- API:
reset_to_defaultis additive and preserves the prior manager-event behavior, but its public docstring claims restoration for each entity while the implementation only handles rigid objects, articulations, cables, and deformables, excluding rigid-object collections. Narrow the contract or extend the implementation. The changelog should retain the user-facing method addition while removing the internal test-refactorChangedentry. - Implementation: The manager event delegates the existing reset behavior to
InteractiveScene, including environment-origin offsets and optional joint-target restoration. The implementation concern is limited to the mismatch between the new method’s stated coverage and its actual entity loops; the changelog also needs to comply with repository guidance for user-facing entries.
Minor fixes needed. Posted 2 actionable findings inline.
Automated review; human maintainers own approval decisions.
| Changed | ||
| ^^^^^^^ | ||
|
|
||
| * Reworked rendering correctness tests around one deterministic task-free scene and bundled |
There was a problem hiding this comment.
🔵 Suggestion · Api — Changed entry describes test-only internals
This Changed entry describes an internal test refactor and provides no migration guidance, which the changelog rules require for Changed entries, and it exposes implementation detail users cannot act on. The sibling packages correctly use .skip fragments for the same work. Drop the Changed section and keep only the user-facing Added entry for reset_to_default.
| @@ -520,6 +520,47 @@ def reset(self, env_ids: Sequence[int] | None = None): | |||
| for sensor in self._sensors.values(): | |||
| sensor.reset(env_ids) | |||
|
|
|||
| def reset_to_default(self, env_ids: Sequence[int] | None = None, reset_joint_targets: bool = False) -> None: | |||
| """Restore asset state configured by each entity's initial-state configuration. | |||
There was a problem hiding this comment.
🔵 Suggestion · Api — Docstring overstates reset_to_default coverage
The docstring promises to restore state "configured by each entity's initial-state configuration", but the method only visits rigid objects, articulations, cables, and deformables. reset() a few lines above also iterates self._rigid_object_collections, so a scene using a RigidObjectCollection is silently left unrestored by this new public method. Either restore collection defaults too, or narrow the docstring to the supported entity types.
b555f1a to
3143848
Compare
bfd6f63 to
57d3802
Compare
| KITLESS_CASES = tuple( | ||
| (stage, case) | ||
| for case in _KITLESS_CASES | ||
| for stage in (("legacy", "ovstage") if case.renderer == "ovrtx" else ("legacy",)) |
There was a problem hiding this comment.
So this is only for ovphysx and ovrtx right? for newton, it should be empty?
| @@ -16,6 +16,72 @@ | |||
| """The default timeout for each test in seconds.""" | |||
|
|
|||
|
|
|||
| COLD_CACHE_TESTS = { | |||
There was a problem hiding this comment.
can this be based on pytest marker?
| "ShadowHandRenderingSceneCfg": {"ground", "key_light", "fill_light", "camera", "robot", "object"}, | ||
| } | ||
|
|
||
| required_signatures = ( |
There was a problem hiding this comment.
seem a bit verbose, what's the purpose?
| object: ArticulationCfg | RigidObjectCfg = _SHADOW_OBJECT_NEWTON.copy() | ||
|
|
||
|
|
||
| def make_rendering_scene_cfg( |
There was a problem hiding this comment.
Agent review: this returns six values that run_rendering_case destructures and re-threads as six keyword arguments into build_rendering_scene — a grouping that is real but unnamed, so a reader has to reconstruct it.
Naming it is also what would let one registry own the per-scene facts that are currently spread across this branch, three dicts in rendering_cases.py, two conditionals in rendering_runner.py, and five sets in the architecture gate. isaaclab.renderers.CameraRenderSpec is a nearby precedent for the shape.
| return cfg | ||
|
|
||
|
|
||
| def make_physics_cfg(physics_backend: str) -> PhysicsCfg: |
There was a problem hiding this comment.
Agent found bug: this shadows isaaclab.app.sim_launcher.make_physics_cfg, which already ships in this package, with different semantics — "physx" returns PhysxAutoCfg(...) there and PhysxCfg(...) here. Core also takes "newton_mjwarp" / "isaacsim_physx" / "newton_vbd" and rejects "newton"; this one is the reverse.
Two importable functions with one name where the same argument yields a different backend is easy to import wrongly. Develop's harness translated "newton" into core's vocabulary via _physics_preset_name(); this replaces the translation with a fork.
- rename to
make_rendering_physics_cfg, or take core's labels and layer the test-only determinism flags on its result
| "simple_shading_diffuse_mdl", | ||
| "simple_shading_full_mdl", | ||
| ) | ||
| RTX_AOVS = ( |
There was a problem hiding this comment.
Agent suggestion: these AOV names already exist as a typed enum.
isaaclab.renderers.output_contract.RenderBufferKind is a StrEnum enumerating exactly these strings — rgb, albedo, motion_vectors, instance_id_segmentation_fast, simple_shading_full_mdl and the rest — and its docstring states the values match CameraCfg.data_types.
Because it is a StrEnum it substitutes anywhere a str is accepted today, so this is a typing change rather than a refactor. The gain: a typo becomes an AttributeError at import instead of an AOV that silently matches nothing, and the four AOV tuples plus _NO_SSIM, _SEMANTIC_ONLY_AOVS and the inline alpha_only set all key off one declared vocabulary.
| "instance_id_segmentation_fast", | ||
| } | ||
| _SEMANTIC_ONLY_AOVS = {"motion_vectors"} | ||
| _MAX_DIFF_PCT = { |
There was a problem hiding this comment.
Agent suggestion: these tolerances read as properties of the scenes rather than of the runner.
_MAX_DIFF_PCT is keyed by scene name here, and two more scene-conditionals follow inside run_rendering_case — the canonical_rtx_rgb branch and 0.95 if case.scene == "kuka_heterogeneous". So the shared runner has to know every scene by name, which makes it harder to read as generic machinery.
If the scene configuration grows a home (see the comment on make_rendering_scene_cfg), these would sit next to the geometry that justifies them, where the measured-variance comments are checkable against the scene they describe.
| if implicit_camera_token in path.read_text() | ||
| } | ||
| assert former_implicit_callers <= assignments["COLD_CACHE_TESTS"] | ||
| assert assignments["PROCESS_ISOLATED_TESTS"] == { |
There was a problem hiding this comment.
Agent review: these assert the config equals a literal copy of itself — the only way to fail is to change the config, which is the intended operation.
Same pattern twice more in this file: len(KIT_CASES) == 8 / len(KITLESS_CASES) == 46 assert the length of module-level literals, and the "selector in target for selector in selectors..." check pins verbatim source text from tools/conftest.py, so reformatting that file breaks a test in another package. If the counts are meant as a cost tripwire, a budget says so: assert len(KIT_CASES) <= 10, "each case costs ~60 s of GPU time and one committed golden".
test_golden_inventory_matches_case_matrix shows the alternative — it computes its expectation from RenderCase.golden_id, so it needs no edit when the matrix grows.
Scope: the three above only, not the sum(map(len, partitions)) / set().union(*partitions) relations, which are derived and should not change.
| "test_contact_sensor.py": 2000, | ||
| } | ||
| """A dictionary of tests and their timeouts in seconds. | ||
|
|
||
| Note: Any tests not listed here will use the default timeout. | ||
| """ | ||
|
|
||
|
|
||
| PROCESS_ISOLATED_TESTS = { |
There was a problem hiding this comment.
Agent found bug: nothing asserts these partitions cover every case, so a future case can run in zero passes while the file reports green.
The runner invokes a partitioned file once per partition under -k, matched as a substring against the test ID. Add a scene whose name matches none of the selectors and its cases are collected by no pass — and the gate's PROCESS_ISOLATED_TESTS == {...} equality still passes, because this dict did not change. Latent rather than live: today's selectors do cover every case (CI: 10/10 and 4/4).
- assert every case in a partitioned file is selected by exactly one partition, computed from the case tuples — which also retires
tuple(map(len, partitions)) == (4, 10, 3, 0)in the gate
Agent suggestion, on the COLD_CACHE_TESTS question above: tools/_device_split.py already reads a file's module-level pytestmark from source without importing it, and run_individual_tests already loads test_content for that call — so markers work with the mechanism already here, and would drop the 60-path list.
|
Review follow-up is pushed in dc68b7a2c19.
The follow-up removes 326 net lines. Architecture 11/11, marker tests 8/8, representative Kit and Kit-less renders, and full pre-commit all pass. |
|
Addressed in 863ec8d0423. Both OVRTX execution paths remain in the 63-case Kit-less matrix; legacy still exercises the deprecated clone_usd/open_usd_from_string path and ovstage still exercises the replacement API, but both now resolve to the same stage-independent golden filename. I removed the 23 duplicate ovstage PNGs and dropped legacy-/ovstage- from all Kit-less baselines, including Newton-Warp. The retained RGB pairs are within the existing visual tolerances. OVRTX semantic IDs can receive different arbitrary colors from the two USD readers, so semantic images compare their rendered mask while the existing idToLabels assertions validate label presence and reject unexpected labels. Canonical means the broad synthetic scene used for the full AOV matrix; probe means a small task-faithful specialized scene. That distinction is used only in pytest IDs to form process-isolated groups under the 15-scene compilation budget and never participates in golden identity. |
|
Good catch. Case IDs are now scene-first:
The CI selectors and process partitions were updated, and a derived architecture gate now checks that IDs are unique and that these rejected labels cannot return. Validation: 13 targeted tests passed, all 63 Kit-less IDs collect, all 18 exact post-merge selectors resolve, pre-commit passes, and changelog coverage passes. The PR description and preview-image paths are updated too. |
| if physics_backend == "ovphysx": | ||
| from isaaclab_ovphysx.physics import OvPhysxCfg | ||
|
|
||
| return OvPhysxCfg(enable_enhanced_determinism=True, enable_external_forces_every_iteration=True) | ||
| if physics_backend == "newton": |
There was a problem hiding this comment.
It doesn't make a difference since each conditional returns, but i'd make this an elif anyway for best practices.
|
|
||
| pytestmark = [pytest.mark.isaacsim_ci, pytest.mark.cold_cache] | ||
|
|
||
| test_rendering = make_kit_test() |
There was a problem hiding this comment.
I'd re-name make_kit_test() to something like generate_kit_test_cases() for clarity that it is not a single test
|
|
||
| pytestmark = [pytest.mark.isaacsim_ci, pytest.mark.arm_ci, pytest.mark.cold_cache] | ||
|
|
||
| test_rendering = make_kitless_test() |
There was a problem hiding this comment.
I'd re-name make_kitless_test() to something like generate_kitless_test_cases() for clarity that it is not a single test
| ) | ||
|
|
||
|
|
||
| def maybe_validate_instance_segmentation( |
There was a problem hiding this comment.
@ooctipus I see that you ported over validation of semantic_segmentation it would be great to continue to also validate instance_segmentation
hujc7
left a comment
hujc7
left a comment
There was a problem hiding this comment.
Agent review: nothing blocking from me this round — the restructure landed and I re-checked it after the package move. One preference noted inline, freely skippable.
| _ALPHA_ONLY_AOVS = {RenderBufferKind.INSTANCE_SEGMENTATION, RenderBufferKind.INSTANCE_ID_SEGMENTATION_FAST} | ||
|
|
||
|
|
||
| def run_rendering_case( |
There was a problem hiding this comment.
Jichuan: Personal preference rather than a request — I'd like the harness to have a model that groups things into namespaces instead of free functions in a module. A file gives a location but not a scope: nothing declares what belongs together or what each part owns. As a general preference: where two or more functions only make sense together, or one exists only to serve another, I'd rather they lived on a type than sat side by side in a module. Genuine adapters with no shared context are fine as free functions — frame_image and camera_output_image read well exactly as they are. The goal is maintainability as this grows.
Agent suggestion: this function is the clearest place to see it. It reads as the orchestrator, but only about eight of its sixty-seven lines are sequencing; the rest is calculation belonging to the things being sequenced:
- twelve lines of tensor math for the motion-vector check —
amax, the 0.99 support-pixel ratio, the half-view bound - four threshold rules resolved at the call site:
scene.image_tolerance(...), thenewton_warpternary,_NO_SSIM, and_ALPHA_ONLY_AOVSnow or-ed with an OVRTX/semantic_segmentationspecial case - a three-line inline join to build
artifact_label - nine arguments handed to
build_rendering_scenethat the case and scene spec already know - two namespace keywords (
golden_namespace,artifact_namespace) threaded through 14 sites across three files, where one field on the case would do — and would also let the two test factories become one
case.golden_filename(aov, ...) is the counter-example and shows the direction already works — that identity used to be assembled here and now belongs to the case. A thin orchestrator would read as the sequence and nothing else:
def execute(self, request):
with self.case.build() as runtime: # 9 args -> the case already knows them
runtime.stabilize_camera()
outputs, info = runtime.camera_outputs()
if self.case.needs_motion_step: # stepping is sequence, stays here
outputs |= self._step_once_and_recapture(runtime)
self._assert_segmentation(outputs, info)
for aov in self.case.aovs:
self.case.judgement(aov).assert_ok(outputs[aov], request)On the motion math specifically: _SEMANTIC_ONLY_AOVS already declares that motion vectors are judged semantically rather than against a baseline — it just doesn't carry the check, which sits twenty lines above behind a separate if. Putting the two together makes both verdict kinds reachable the same way, and a second semantically-checked AOV becomes a table row instead of another branch here. The four threshold rules would sit naturally on RenderCase, which already holds the scene, renderer and AOV they depend on, and compare_to_golden with its four private helpers on the ImageComparison they already construct.
A sample from the current design, not a prescription. The data side has landed (RenderCase, SceneProbe, RenderingSceneSpec, and the gate even asserts this runner does not hardcode scene names); it is the behavior that has no owner yet — 25 module-level functions against 11 classes. Flow-wise the change is a no-op: same operations in the same order. It buys legibility, so it is worth it only if this keeps growing — adding one probe scene currently costs about seven edits across five files. Fine to note for later or skip.
|
Correction after checking both the exact develop images and develop's motion-vector behavior: Motion vectors Develop does not omit motion-vector goldens globally. It retains them for deterministic PhysX/OVPhysX cases and enables the corresponding determinism flags; only Newton motion-vector tests are skipped because their per-step magnitudes are not deterministic enough. My earlier wording generalized the Newton exception too far.
Validation: PhysX/Isaac RTX 1/1, legacy+ovstage OVPhysX/OVRTX 2/2, architecture/golden inventory 13/13, full pre-commit, and changelog coverage passed. Franka RGB comparison I downloaded and compared the exact PNGs rather than inferring from configuration:
The “same image” requirement applies to the deprecated legacy OVRTX reader versus the ovstage OVRTX reader. Those paths now share |
AntoineRichard
left a comment
AntoineRichard
left a comment
There was a problem hiding this comment.
Would it make sense to rename this from isaaclab_rendering_tests to _isaaclab_tests or something of the like?
|
Renamed the downstream project to the final requested name in a27cd508eb8:
Final-name validation passed: package build, 13 architecture/golden tests, 18 Kit and 63 kitless cases collected, full pre-commit, and changelog coverage. Representative real Kit and legacy OVRTX renders passed immediately before this final path-only rename. All 114 PNGs are exact 100% moves; this rename regenerated no goldens. |
|
Addressed these review notes in
The instance PNGs were already golden-compared, but the former Validation: 15 focused helper/architecture tests, full pre-commit, and real instance-segmentation renders through Isaac RTX, Newton-Warp, legacy OVRTX, and ovstage OVRTX. The Kuka heterogeneous and Shadow Hand real-render cases also passed. No golden images changed in this follow-up. |
2054709 to
103f77a
Compare
|
Rebased onto current The two failing jobs came from the restored instance-segmentation metadata validator being stricter than Newton-Warp's actual contract:
Isaac RTX and OVRTX still require exact pixel/color/path/class metadata. Newton-Warp now requires both maps to agree, every rendered color and reserved entry to be present, and every reported prim/class pair to be a non-empty subset of the scene declaration. This retains useful validation without asserting completeness that the renderer does not promise. The rebase also brought in four newly added registered-task Shadow goldens after this PR had removed their owning test. The architecture gate caught that duplicate ownership, so those four were removed; the downstream Shadow scene already owns the replacement coverage. Validated locally on the rebased tree:
No downstream golden image was regenerated. I am leaving the newly triggered CI run unmonitored. |
103f77a to
d1d6f30
Compare
64b9763 to
867555f
Compare
|
Pushed a follow-up for the latest I downloaded and compared the failing job's artifacts. Cartpole, Kuka, and all four failed Kit visualizer captures have the same camera, pose, geometry, and texture content as their goldens; the failures are low-amplitude RTX shading/color variation across GPU families. For example, using the existing per-pixel L2 threshold of 10 produced 14.10% Cartpole RGB and 17.97% Kuka RGB differences, while an L2 color-noise floor of 20 reduces those to 6.78% and 4.38% respectively. SSIM remains 0.9611 and 0.9765, so the structural signal is still strong. The fix is deliberately narrow:
Post-rebase local validation used the worktree's
|
|
Fixed the failed The direct MDL authoring code previously resolved built-in modules only from the pip layout ( The material boundary now checks the archive Kit root first and retains the pip fallback. Added a regression that forces the archive layout. Validation:
|
6 participants
Description
Rendering correctness tests previously instantiated complete registered task environments. That loaded manager stacks and task assets the tests did not exercise, coupled golden images to reset/task implementation changes, and repeated expensive setup for visually redundant scenes.
This PR makes rendering tests own their scenes and moves cross-package coverage to a private downstream testing composition root.
Architecture
source/_isaaclab_testingproject. It depends downstream on the runtime packages it integrates; production packages do not import or depend on it.isaaclab-testingdistribution non-public and non-importable. Rendering is one suite undertest/rendering, leaving room for future integration suites without renaming the project again.RenderingSceneSpecthe scene composition record. It owns the direct scene type, physics/runtime inputs, tolerances, and expected segmentation instances.RenderCaseowns only the backend/renderer/AOV combination.mdp.reset_scene_to_default.InteractiveScenegains only the lifecycle operation needed by direct scene owners to release callbacks before simulation teardown.The result removes the former per-task/per-visualizer test implementations and their duplicated helpers. Defining a new rendering scene now requires scene configuration and registry data, not another complete test.
Direct scenes and fidelity
No rendering test imports
isaaclab_tasksor constructs an RL environment. The suite locally declares the visible facts that rendering is meant to cover:develop.(0.0, -0.39, 0.6). The known backend-dependent hand/cube proximity remains tracked separately as issue 6593722.The full-AOV camera renders at 256×256. Task probes use their task camera composition with reviewable resolution. Direct PreviewSurface and MDL authoring preserves authored colors in Kitless OVRTX as well as Isaac RTX; Franka Cloth therefore has the same pink table and yellow cloth in both paths.
Golden and motion contracts
legacyandovstageare pytest/artifact identities only. They resolve to the same stage-independent golden filenames, so a disagreement is a failure rather than two accepted images.<scene>-<physics>-<renderer>[-<real-variant-or-single-AOV>]. The formercanonical,probe, and no-opstandardlabels are removed.RenderBufferKind; scene-specific tolerances remain beside the scene facts that justify them.CI organization
Rendering jobs target
_isaaclab_testing, preserving the dependency directionisaaclab -> isaaclab_ov -> _isaaclab_testing. Cold-cache selection is marker-driven. Process-isolated partitions are derived from the case matrices and gated so every case is selected exactly once; a future unmatched case cannot silently run in zero CI passes. Kitless USD parsing setsPXR_WORK_THREAD_LIMIT=1before USD import.Screenshots
These previews are pinned to commit
867555fe37170ef7487cad0dd79e03cb07b72d07.Validation
git diff --check: passed.developated1f83d939c; no unresolved files or upstream back-references remain.Type of change
Checklist
CONTRIBUTORS.md