Enabled OVRTX rendering tests on CI

[huidongc]

Description

Enabled OVRTX rendering tests on CI.

OVRTX 0.3 is not published yet, so we have to use OVRTX 0.2 render output as golden images. Some of them are incorrect, I will update those images when we pin to OVRTX 0.3.

Fixes # (issue)

Type of change

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (existing functionality will not work without user modification)
• Documentation update

Screenshots

Please attach before and after screenshots of the change if applicable.

Checklist

• I have read and understood the contribution guidelines
• I have run the pre-commit checks with ./isaaclab.sh --format
• I have made corresponding changes to the documentation
• My changes generate no new warnings
• I have added tests that prove my fix is effective or that my feature works
• I have updated the changelog and the corresponding version in the extension's config/extension.toml file
• I have added my name to the CONTRIBUTORS.md or my name already exists there

[isaaclab-review-bot]

🤖 Isaac Lab Review Bot

Summary

This PR enables OVRTX rendering tests on GitHub CI by removing skip markers that previously disabled these tests on GitHub Actions runners. It also removes an LD_PRELOAD workaround from the documentation, suggesting the underlying OVRTX segfault issue has been resolved upstream. The changes are straightforward and self-contained.

Architecture Impact

Self-contained. The changes affect only test parametrization markers and documentation. No runtime code paths, APIs, or core framework components are modified. The KITLESS_PHYSICS_RENDERER_AOV_COMBINATIONS list is consumed by test modules that parametrize rendering correctness tests.

Implementation Verdict

Ship it — assuming the underlying OVRTX stability issue is actually resolved (which should be verified by CI passing).

Test Coverage

This PR enables existing tests rather than adding new functionality. The OVRTX rendering tests already exist and were being skipped; they will now run in CI. No new tests are needed for this change.

CI Status

No CI checks available yet. This is critical to verify — the entire premise of this PR is that OVRTX tests now pass on GitHub Actions. The PR should not be merged until CI demonstrates the tests pass without segfaults.

Findings

🔵 Improvement: source/isaaclab_tasks/test/rendering_test_utils.py:66-71 — Dead code removal is correct
The removal of _SKIP_ON_GITHUB_ACTIONS and _SKIP_ON_GITHUB_ACTIONS_MARK is clean. These variables were only used by the removed marks= parameters. No orphaned references remain.

🔵 Improvement: docs/source/setup/installation/kitless_installation.rst:108 — Documentation cleanup is appropriate
Removing the LD_PRELOAD workaround improves UX if the underlying issue is fixed. However, consider adding a brief note or troubleshooting section mentioning this was previously required, in case users on older OVRTX versions encounter the issue.

🟡 Warning: PR Description — Missing root cause context
The PR description doesn't explain why these tests can now be enabled. Was there an OVRTX bug fix? A CI runner update? A change in how the tests are invoked? This context is valuable for future debugging if the issue resurfaces. Consider adding this to the PR description or changelog entry.

🟡 Warning: source/isaaclab_tasks/changelog.d/huidongc-enable-ovrtx-rendering-tests.rst:1-4 — Changelog entry is minimal
The changelog says what changed but not why. For future maintainers, it would be helpful to note that this was previously disabled due to segfaults and is now enabled due to [upstream fix / environment change / etc.].

🔵 Improvement: source/isaaclab_tasks/test/rendering_test_utils.py:175-220 — Consider conditional skip as fallback
If OVRTX tests become flaky on CI in the future, consider keeping the skip infrastructure but making it configurable via an environment variable (e.g., ISAAC_LAB_SKIP_OVRTX_TESTS). This would allow quick disabling without code changes if issues recur. This is optional — the current approach is valid.

[greptile-apps]

Greptile Summary

This PR enables OVRTX kitless rendering tests on CI by removing the _SKIP_ON_CI_MARK guard and replacing it with _FLAKY_MARK, installing ovrtx as an extra pip package in the test jobs, fixing a premature renderer teardown bug (keep_system_alive=True), and removing an obsolete LD_PRELOAD step from installation docs.

• Golden images for OVRTX 0.2 renders are committed for rgb, rgba, albedo (except cartpole), and simple-shading AOVs across three environments, with the author noting some are already known-incorrect pending an OVRTX 0.3 pin.
• ovrtx is installed without a version pin, so when OVRTX 0.3 is published the 0.2-based golden images will systematically fail across all CI shards.
• newton-ovrtx-depth, newton-ovrtx-semantic_segmentation (all 3 environments), and newton-ovrtx-albedo (cartpole) are now unblocked on CI but lack committed golden images — these cases will self-generate a golden on the first (failing) run and trivially compare against it on the _FLAKY_MARK retry, providing no regression signal.

Confidence Score: 3/5

Safe to merge for immediate CI enablement, but two structural gaps mean the new tests will silently break or become no-ops when OVRTX 0.3 ships.

The renderer fix and CI wiring are correct, but the ovrtx package is installed without any version bound. The PR description explicitly states OVRTX 0.3 is imminent and that the committed golden images are based on 0.2 output. An unguarded upgrade will cause all three shards to fail their OVRTX golden-image comparisons simultaneously. Additionally, seven test cases (depth across all environments, semantic_segmentation across all environments, cartpole albedo) are now live on CI without reference golden images — they self-generate a golden on the first run and compare against themselves on retry, providing no regression signal.

.github/workflows/build.yaml and .github/workflows/daily-compatibility.yml for the unpinned ovrtx version; the missing golden images for depth, semantic_segmentation, and cartpole albedo test cases.

Important Files Changed

Filename	Overview
.github/workflows/build.yaml	Adds unversioned extra-pip-packages: "ovrtx" to all three isaaclab_tasks shards; will break golden-image tests the moment OVRTX 0.3 is published.
.github/workflows/daily-compatibility.yml	Adds extra-pip-packages: "ovrtx" to the daily tasks compatibility job; same unpinned version concern as build.yaml.
source/isaaclab_ov/isaaclab_ov/renderers/ovrtx_renderer.py	Adds keep_system_alive=True to RendererConfig to prevent premature teardown during pytest sessions; targeted and safe change.
source/isaaclab_tasks/test/rendering_test_utils.py	Removes CI skip guard from all OVRTX test cases; depth and semantic_segmentation cases (and cartpole albedo) have no committed golden images, so they will self-generate and self-validate rather than test against a known reference.
tools/test_settings.py	Adds 2000-second timeouts for the three kitless rendering test files; comment acknowledges values may need revisiting after OVRTX 0.3 pin.
docs/source/setup/installation/kitless_installation.rst	Removes the now-unnecessary LD_PRELOAD export step from the OVRTX installation instructions.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[CI Job starts] --> B["pip install ovrtx (unpinned)"]
    B --> C{OVRTX version?}
    C -->|0.2.x| D[Tests run with 0.2 renderer]
    C -->|0.3.x published| E["Tests run with 0.3 renderer"]
    D --> F{Golden image exists?}
    E --> F
    F -->|Yes| G[Compare render vs golden]
    F -->|No - depth / segmentation / cartpole albedo| H["First run: save current as golden, FAIL"]
    H --> I["_FLAKY_MARK retry: compare against self, PASS"]
    G -->|Match| J[Test PASSES]
    G -->|Mismatch 0.3 vs 0.2 golden| K["Test FAILS - all shards affected"]
    I --> J

Loading

_{Reviews (3): Last reviewed commit: "Increase timeout." | Re-trigger Greptile}

[isaaclab-review-bot]

🤖 Isaac Lab Review Bot

Summary

The new commits add automatic OVRTX installation logic in tools/conftest.py that detects when kitless rendering tests are scheduled and installs ov[ovrtx] on-demand if not already present. This addresses the previous concern about missing root cause context — the PR now actively ensures the dependency is available rather than just removing skip markers.

Previous Concerns Status

• Missing root cause context — Partially addressed by the new installation logic, though the changelog could still note this infrastructure change.
• Minimal changelog entry — Still applies; the changelog doesn't mention the new auto-install mechanism.
• Other previous findings (dead code removal, doc cleanup) remain correctly implemented.

New Findings

🔴 Critical: tools/conftest.py:106-110 — subprocess.run missing timeout
The subprocess.run call for isaaclab.sh -i ov[ovrtx] has no timeout. If the install hangs (network issues, pip resolver loops), CI will block indefinitely until the job-level timeout kills it. Add timeout=600 (or similar reasonable value) to prevent this.

result = subprocess.run(
    [isaaclab_cli, "-i", "ov[ovrtx]"],
    cwd=workspace_root,
    capture_output=True,
    text=True,
    timeout=600,  # Add timeout to prevent indefinite hangs
)

🟡 Warning: tools/conftest.py:85-92 — Swallowing import exceptions may hide issues
The except (ValueError, ModuleNotFoundError): pass block silently continues if find_spec raises. This is probably fine, but a debug print would help diagnose edge cases where ovrtx exists but can't be introspected.

Implementation Verdict

Minor fixes needed — Add a timeout to the subprocess call to prevent CI hangs.

[isaaclab-review-bot]

🤖 Isaac Lab Review Bot

Summary

The latest commit (d44676a) removes the auto-install logic from tools/conftest.py entirely and instead adds ov[ovrtx] installation directly to the base Docker image. This is a cleaner approach that avoids runtime subprocess calls and ensures the dependency is always available in the CI environment.

Previous Concerns Status

• Missing timeout on subprocess.run — No longer applicable; the subprocess call was removed entirely.
• Swallowing import exceptions — No longer applicable; the auto-install logic was removed.
• The Dockerfile change is straightforward and correctly chains the install commands.

Architecture Impact

Self-contained. The Docker image now includes OVRTX by default, and the skip markers are removed from the test parameters. No cross-module concerns.

Implementation Verdict

Ship it — The changes are clean and correct. Moving OVRTX installation to the Docker build phase is the right approach for CI reliability.

Test Coverage

This PR enables existing tests to run; no new test logic is added. The rendering tests themselves are comprehensive.

CI Status

Pre-commit passes. Other checks are pending but unrelated to the code changes.

Findings

No new issues. The PR is ready to merge.

[isaaclab-review-bot]

🤖 Isaac Lab Review Bot

Summary

This is a follow-up review. The PR now adds OVRTX installation to the Docker base image and removes the LD_PRELOAD workaround from the kitless installation docs, indicating the underlying issue has been resolved upstream.

Previous Concerns Status

Previous review noted "Ship it" with no issues. That assessment remains valid.

Implementation Verdict

Ship it — The changes are clean: Docker installs OVRTX at build time, skip markers removed from tests, and the LD_PRELOAD hack is no longer needed in docs.

CI Status

The isaaclab_ov job is failing. This should be investigated to confirm it's unrelated to this PR before merging — the failure could indicate OVRTX tests are now running but encountering environment issues on the CI runner.

Findings

🟡 Warning: CI failure in isaaclab_ov — Verify this failure is pre-existing or unrelated to enabling OVRTX tests. If the newly-enabled tests are failing, this PR should not merge until they pass.

[isaaclab-review-bot]

🤖 Isaac Lab Review Bot

Summary

This follow-up adds three missing RenderBufferKind entries to the OVRTX renderer contract test and moves wp.init() before the skip check by using pytest.mark.skipif instead of pytest.skip() inside the test body.

Previous Concerns Status

Previous CI concern about isaaclab_ov failure — the test changes in this commit appear to address potential initialization ordering issues by calling wp.init() at test start and using proper skip markers.

Implementation Verdict

Ship it — The test corrections are valid: the three shading buffer kinds were missing from the expected set, and the warp initialization fix ensures wp.init() runs before any warp operations.

CI Status

CI pending — verify isaaclab_ov tests pass with these fixes before merging.

Findings

No new issues. The changes are correct and improve test robustness.