Fix the default log file path of OVRTX renderer#5749
Conversation
There was a problem hiding this comment.
🤖 Isaac Lab Review Bot
Summary
This PR fixes a cross-platform compatibility issue with the OVRTX renderer log file path. The fix replaces the hardcoded Linux-specific /tmp/ovrtx_renderer.log with a cross-platform solution using Python's tempfile.gettempdir(). Additionally, it redirects OVRTX renderer logs to stdout during CI tests for better visibility.
🏗️ Architecture & Design Review
Cross-platform temp directory (✅ Excellent)
- Using
tempfile.gettempdir()is the correct cross-platform approach - This returns
C:\\Users\\<user>\\AppData\\Local\\Tempon Windows and/tmpon Linux/macOS - The import statements are properly organized at module level
Stdout redirection for tests (✅ Well-designed)
- The
_redirect_ovrtx_renderer_log_to_stdout()helper is well-structured - Uses
/dev/stdouton Unix andCONon Windows - both are valid approaches for platform-specific stdout redirection - The function properly walks both direct env (
tiled_camera) and manager-based env (scene.base_camera,scene.wrist_camera) patterns
🔇 Silent Failure Analysis
Potential edge cases:
-
Renderer type check (Lines 304-305 in
rendering_test_utils.py): The checkgetattr(renderer_cfg, "renderer_type", None) == "ovrtx"is safe and will silently skip non-OVRTX renderers as intended. -
Missing attributes: The code uses
getattr(..., None)defensively throughout, which prevents AttributeError exceptions. This is appropriate for config inspection. -
Windows
CONdevice: UsingCONas a pseudo-file for stdout on Windows is a valid special device name. However, note that some environments (like certain CI runners) may have restrictions on console device access. If this becomes an issue, an alternative would beNULfor discarding logs or a real temp file.
🧪 Test Coverage Assessment
What's covered:
- The PR adds OVRTX log redirection to all three rendering test functions:
rendering_test_shadow_hand,rendering_test_cartpole, andrendering_test_dexsuite_kuka - The redirect is conditionally applied only when
renderer == "ovrtx_renderer"
What could be improved:
- No explicit unit test for
_redirect_ovrtx_renderer_log_to_stdout()itself, but this is acceptable given it's an internal test utility function - The existing rendering tests will exercise this code path when run with OVRTX renderer
📋 Minor Observations
-
Docstring clarity (Line 283-287): The docstring clearly explains the purpose and the platform-specific behavior. Well documented!
-
Changelog fragment: Present and correctly describes the fix.
-
Skip file for isaaclab_tasks: The
.skipfile is present as expected when changes don't require a changelog entry for that package.
✅ Verdict
This is a clean, well-thought-out fix for a genuine cross-platform compatibility issue. The implementation follows Python best practices and the test infrastructure changes improve CI observability.
CI Status: Several checks are still pending. Recommend waiting for CI to complete before merging.
🤖 Review generated by Isaac Lab Review Bot | Commit: 0add0a0
Greptile SummaryThis PR fixes the OVRTX renderer's default log file path to use
Confidence Score: 4/5Safe to merge; changes are narrowly scoped to the log file path default and a test-only stdout redirect helper. The config fix is correct and strictly improves portability. The new source/isaaclab_tasks/test/rendering_test_utils.py — the hardcoded camera attribute list in Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[rendering_test_shadow_hand / cartpole / dexsuite_kuka] --> B{renderer == ovrtx_renderer?}
B -- Yes --> C[_redirect_ovrtx_renderer_log_to_stdout]
B -- No --> E[Continue with default log_file_path]
C --> D{Walk camera cfgs\ntiled_camera / base_camera / wrist_camera}
D --> F{renderer_cfg.renderer_type == 'ovrtx'?}
F -- Yes --> G[Set log_file_path = /dev/stdout or CON]
F -- No --> H[Skip]
G --> I[OVRTX renderer writes logs to stdout]
I --> J[Logs visible in CI output]
E --> K[OVRTXRendererCfg default\nos.path.join tempfile.gettempdir, ovrtx_renderer.log]
Reviews (1): Last reviewed commit: "changelog fragment" | Re-trigger Greptile |
# Description Cherry-picked PR #5749 from `develop`. ## Type of change - Bug fix (non-breaking change which fixes an issue) ## Checklist - [x] I have read and understood the [contribution guidelines](https://isaac-sim.github.io/IsaacLab/main/source/refs/contributing.html) - [x] I have run the [`pre-commit` checks](https://pre-commit.com/) with `./isaaclab.sh --format` - [x] I have made corresponding changes to the documentation - [x] My changes generate no new warnings - [x] I have added tests that prove my fix is effective or that my feature works - [x] I have updated the changelog and the corresponding version in the extension's `config/extension.toml` file - [x] I have added my name to the `CONTRIBUTORS.md` or my name already exists there
Description
/tmp/ovrtx_renderer.logwhich is Linux specific. We need to use a cross-platform temp directory instead.rendering-correctnesstest job (https://github.com/isaac-sim/IsaacLab/actions/runs/26287973633/job/77380393480?pr=5749):Type of change
Checklist
pre-commitchecks with./isaaclab.sh --formatconfig/extension.tomlfileCONTRIBUTORS.mdor my name already exists there