[Test] Consolidate environment smoke tests - #7089
Conversation
Greptile SummaryThe PR splits core environment smoke coverage by physics backend and consolidates contributed environments into one default-backend suite.
Confidence Score: 3/5The PR is not safe to merge until the consolidated contrib suite restores Franka Pour’s artifact guard and isolates Factory environments. The replacement suite unconditionally runs a task known to require an external dataset and combines Factory tasks with other environments despite the removed code’s explicit requirement for separate execution. Files Needing Attention: source/isaaclab_tasks/test/contrib/test_contrib_environments.py and source/isaaclab_tasks/test/env_test_utils.py Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
Registry[Registered tasks] --> Tier{Task tier}
Tier -->|core| Preset{Physics preset}
Preset --> Isaac[Isaac Sim PhysX suite]
Preset --> OV[OV PhysX suite]
Preset --> Newton[Newton MJWarp suite]
Tier -->|contrib, single-agent| Contrib[Consolidated contrib suite]
Contrib --> Pour[Franka Pour without artifact guard]
Contrib --> Factory[Factory tasks without process isolation]
Reviews (1): Last reviewed commit: "Consolidate environment smoke tests" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| @pytest.mark.parametrize("task_name", setup_environment(multi_agent=False, tier="contrib")) | ||
| @pytest.mark.parametrize("num_envs, device", [(2, "cuda")]) | ||
| def test_contrib_environments(task_name, num_envs, device): | ||
| _run_environments(task_name, device, num_envs) |
There was a problem hiding this comment.
When the external reset dataset is unavailable, this suite now selects IsaacContrib-Franka-Pour and passes it directly to _run_environments, causing environment initialization to fail instead of skipping the task as the removed suite did.
| @pytest.mark.parametrize("task_name", setup_environment(multi_agent=False, tier="contrib")) | ||
| @pytest.mark.parametrize("num_envs, device", [(2, "cuda")]) | ||
| def test_contrib_environments(task_name, num_envs, device): | ||
| _run_environments(task_name, device, num_envs) |
There was a problem hiding this comment.
Factory and Forge tasks now run in the same Isaac Sim process as every other contributed environment, violating the documented requirement to collect them separately and causing the cross-environment failures that the dedicated Factory suite prevented.
![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 preset-scoped core smoke-test split and associated CI filename updates are coherent, but the consolidated contributor suite removes required task-specific setup, isolation, and skip behavior from the deleted suites. Restore these safeguards before relying on the consolidated job.
- Design and architecture: The
tierplusphysics_preset_nameselection model cleanly replaces the previous family-specific filtering for core backend coverage. However, combining Factory/Forge, teleop, and all other contributed tasks into one simulator session conflicts with the deleted suites’ explicit process-isolation constraints and can reintroduce cross-task interference. - API: The test-internal
setup_environmentsignature change is consistently reflected in the remaining callers, and the workflow and timeout registrations track the renamed test files. No public API compatibility issue is established by the accepted findings. - Implementation: The preset filter uses unresolved registry configurations and validates matching values as
PhysicsCfg. The contributor runner still needs three safeguards preserved from the removed tests: importpinocchiobeforeAppLauncherfor tasks using Pink IK, isolate Factory/Forge and teleop task families into separate simulator sessions, and skipIsaacContrib-Franka-Pourwhen its external reset dataset is unavailable.
Minor fixes needed. Posted 3 actionable findings inline.
Automated review; human maintainers own approval decisions.
|
|
||
| from isaaclab.app import AppLauncher | ||
|
|
||
| app_launcher = AppLauncher(headless=True, enable_cameras=True) |
There was a problem hiding this comment.
🟡 Warning · Implementation — Missing pinocchio pre-import before AppLauncher
The deleted teleop, pick-place/stack, SkillGen and AutoMate suites all imported pinocchio (guarded by sys.platform != "win32") before AppLauncher so the Isaac Lab build wins over Isaac Sim's bundled copy for the Pink IK controller. This file now collects exactly those contributed tasks but launches the app first and never performs that import. Restore the guarded pre-launch import.
| def _contrib_environment_params() -> list: | ||
| """Return each contributed environment with its supported test device.""" | ||
| params = [] | ||
| for task_param in setup_environment(multi_agent=False, tier="contrib"): |
There was a problem hiding this comment.
🟡 Warning · Design Architecture — Factory and teleop lose session isolation
The removed filters documented that Factory/Forge environments "cause tests to fail if run together with other envs" and that teleop environments "may interfere with other environment tests when run in the same process", which is why each had a dedicated file and simulator session. Collecting all contributed tasks into this single parametrized session reintroduces that interference; keep those families in separate modules or justify dropping the constraint.
|
|
||
| @pytest.mark.parametrize("task_name, device", _contrib_environment_params()) | ||
| def test_contrib_environments(task_name, device): | ||
| _run_environments(task_name, device, num_envs=2) |
There was a problem hiding this comment.
🟡 Warning · Implementation — Franka-Pour artifact skip was dropped
test_contrib_environments_smoke.py explicitly skipped IsaacContrib-Franka-Pour because it requires an external reset-dataset artifact, and no equivalent guard was moved into _run_environments. setup_environment(tier="contrib") still yields that task, so it is now collected and run unconditionally in the new job. Re-add the skip here or in the shared helper.
StafaH
changed the title
…ironment-tests # Conflicts: # source/isaaclab_tasks/test/benchmarking/test_environments_training.py # source/isaaclab_tasks/test/core/test_record_video.py
2 participants
Summary
num_envs=2.Validation
uv run python -m compileall -q source/isaaclab_tasks/test/env_test_utils.py source/isaaclab_tasks/test/core source/isaaclab_tasks/test/contrib/test_contrib_environments.pyuv run isaaclab -f