Test run time improvement: remove AppLauncher from tests that do not require it#5646
Conversation
There was a problem hiding this comment.
Code Review Summary
Overview
This PR removes AppLauncher initialization code from 26 test files across multiple test modules (actuators, controllers, devices, envs, sensors, terrains, utils), enabling these tests to run without requiring the full Isaac Sim/Kit environment.
Key Findings
⚠️ Areas Requiring Attention
1. PR Description Incomplete
The PR description uses the default template without explaining:
- The motivation behind this change
- Which issue this fixes (if any)
- Whether all affected tests have been verified to pass without AppLauncher
2. Tests with Potential Runtime Dependencies
Several of the modified tests import modules that may require the simulation context:
| File | Concern |
|---|---|
test_configclass.py |
Original comment mentioned warp dependency requiring running simulation |
test_dict.py |
Same warp dependency note in original code |
test_string.py |
Same warp dependency note in original code |
test_multi_mesh_ray_caster.py |
Original comment: "Used for warp" |
test_mock_sensors.py |
Imports and uses warp as wp |
test_math.py |
Original comment: "This is only needed because of warp dependency" |
If these tests now run without AppLauncher, please confirm warp initialization is handled elsewhere (e.g., pytest fixtures, conftest.py) or that these specific tests no longer require warp.
3. Empty Changelog Skip Files
Three .skip files are added with only a newline. While this may be intentional to bypass changelog requirements, consider adding a comment explaining why no changelog entry is needed.
✅ Positive Aspects
- Consistent Pattern Removal: The refactoring is systematic and touches only the AppLauncher boilerplate
- No Logic Changes: Test logic remains unchanged, reducing regression risk
- Faster Test Execution: Tests will run significantly faster without simulator initialization overhead
Checklist Items
Based on the PR checklist, these items appear incomplete:
- Description explains the change and motivation
- Pre-commit checks run
- Tests verified to pass
Recommendations
- Update PR description with the rationale for this change
- Confirm test execution - share CI results showing all modified tests pass
- Document warp handling - clarify how tests using warp will initialize it
- Consider adding a changelog entry for this test infrastructure improvement
Verdict
This appears to be a work-in-progress for "kitless" test suites. The approach is sound, but the PR needs documentation and verification before merge. Would recommend addressing the above items and marking the checklist appropriately.
🔄 Update (e6cdd7d)
New commits add significant additional changes beyond the original kitless test suite scope:
- Unified RL training/play entrypoints - New
isaaclab train/isaaclab playCLI commands with library dispatch (rsl_rl,sb3,skrl, etc.) - AppLauncher CUDA fix - Deferred
torch.cuda.set_device()to afterSimulationAppstarts to prevent OpenBLAS fork crash - Converter refactoring - URDF/MJCF converters now delegate to Isaac Sim importers; removed
urdf_utils.py - RSL-RL export test batching - Batch Kit launches to reduce CI churn
- Newton label renaming fix - Boundary-safe path matching for label arrays
- Teleop XCR replay automation - New
isaaclab_teleop.automationsubpackage for CI replay - Many changelog entries - Version bumps and documented fixes
Previous review concerns about kitless tests still apply.
🔄 Update (665eb34)
1. Camera/Sensor Warp Migration (High Impact)
CameraDatanow usesProxyArray(warp+torch dual views) instead of raw torch tensors- Added
create_buffers()to allocate pose/intrinsics as warp arrays - New warp kernels in
warp_math.pyfor camera orientation conversion - Tests updated to access
.torchon ProxyArray properties
2. Renderer Interface Changes (Breaking)
BaseRenderer.set_outputs()andupdate_camera()now acceptProxyArrayinstead oftorch.TensorRenderBufferSpecdtype changed fromtorch.dtypeto warp dtypes (wp.uint8,wp.float32, etc.)- Newton and OVRTX renderers updated to match
3. Import Refactoring (Breaking)
- Changed ~50+ files from
from isaaclab.utils import configclasstofrom isaaclab.utils.configclass import configclass - Affects sensors, spawners, terrains, envs, controllers, managers, etc.
4. More Kitless Tests
- Additional test files modified to remove AppLauncher requirement
- Added new tests:
test_stacked_image_mdp.py,test_stacked_image_obs_manager.py
5. Newton Updates
- Bumped newton pin from
v1.2.0rc2tov1.2.0(stable) - Added IAdapter version verification in cubric shim
NewtonWarpRendererupdated for ProxyArray interface
6. OVRTX Renderer Improvements
- Avoided disk I/O by using in-memory USD export
- Renamed
use_cloningtouse_ovrtx_cloning, defaultTrue - Fixed multi-GPU device handling
- Fixed scene partition attribute creation
7. Articulation Additions (isaaclab_ovphysx)
- Major new
ArticulationandArticulationDataimplementation - Dual
*_index/*_maskwrite/set methods
8. Other Notable Changes
ProxyArray.__getattr__now forwards unknown attributes to torch view- Pink IK test tolerance increased, rotation error formula fixed
- Kit startup performance test adjusted for CI environment
🛑 Recommendations:
- Split this PR - 300 files across core sensors, renderers, tests, newton, ovrtx, and ovphysx is too much for one review
- Add migration guide - The configclass import change and ProxyArray changes are breaking
- CI verification needed - These camera/renderer changes need thorough testing
Previous concerns about PR scope and documentation still apply.
Greptile SummaryThis PR removes the Isaac Sim
Confidence Score: 4/5Safe to merge; changes are additive removals of boilerplate with no logic alterations to any tested code path. The bulk of the changes are straightforward and correct — tests that only exercise pure-Python/torch utilities no longer spin up a simulation engine. The three files that still invoke warp APIs (test_wrench_composer.py, test_mock_sensors.py, test_multi_mesh_ray_caster.py) depend on warp's lazy initialization instead of the explicit wp.init() call used by other kit-less warp tests already in this repo. This is a minor consistency gap rather than a functional breakage. test_wrench_composer.py, test_mock_sensors.py, and test_multi_mesh_ray_caster.py — these are the three files that use warp APIs without calling wp.init() explicitly. Important Files Changed
|
a6179eb to
e6cdd7d
Compare
e6cdd7d to
665eb34
Compare
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