Skip to content

[Test] Cover contributed environments in the shared smoke test - #7068

Closed
StafaH wants to merge 11 commits into
isaac-sim:developfrom
StafaH:mh/test-contrib-environments
Closed

[Test] Cover contributed environments in the shared smoke test#7068
StafaH wants to merge 11 commits into
isaac-sim:developfrom
StafaH:mh/test-contrib-environments

Conversation

@StafaH

@StafaH StafaH commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

Adds contributed task coverage to the consolidated environment smoke test.\n\n- Runs eligible contrib tasks through each task’s configured default backend only.\n- Keeps dedicated Factory, Showcase, PickPlace/Stack, Teleop, AutoMate, and Skillgen suites separate.\n- Includes the shared core three-backend matrix, obsolete Play/MJWarp-discovery utility cleanup, and teardown reliability cleanup.\n\nThe shared test commit is intentionally identical to the Newton-default shards; rebase/drop it after the first shard merges.\n\nExtracted from #6980.

@greptile-apps

greptile-apps Bot commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

The PR consolidates core environment smoke coverage into a shared three-backend matrix, moves eligible contributed environments into that test module, removes obsolete Play/Newton discovery filters, and adds explicit garbage collection before simulation teardown.

  • Replaces the dedicated Newton smoke-test file with backend parameterization in the shared core suite.
  • Moves general contributed-task coverage into the consolidated test module while retaining specialized suites.
  • Simplifies environment discovery and updates workflow cache warming.
  • Collects unreachable environment objects before clearing the simulation singleton.

Confidence Score: 3/5

This PR is not safe to merge until the incomplete contributed test and preset-incompatible core matrix are corrected.

The consolidated module cannot be imported because its final test function lacks a body, and after that is repaired the unconditional preset matrix reaches core task configurations that do not define the requested backends.

Files Needing Attention: source/isaaclab_tasks/test/core/test_environments.py, source/isaaclab_tasks/test/env_test_utils.py

Important Files Changed

Filename Overview
source/isaaclab_tasks/test/core/test_environments.py Consolidates backend and contributed smoke coverage, but the contributed test has no body and the core matrix includes preset-incompatible tasks.
source/isaaclab_tasks/test/env_test_utils.py Removes obsolete discovery arguments and adds teardown collection, while also removing the compatibility filter needed by the new unconditional backend matrix.
.github/workflows/build.yaml Updates cache warming to target the consolidated test file while preserving task-family filtering.
source/isaaclab_tasks/test/contrib/test_contrib_environments_smoke.py Deletes the standalone contributed smoke suite, whose replacement in the shared file is currently incomplete.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart LR
    Registry[Gym task registry] --> Filter[setup_environment filters]
    Filter --> Core[Core task list]
    Filter --> Contrib[Contrib task list]
    Core --> Matrix[Three physics presets]
    Matrix --> Runner[_run_environments]
    Contrib --> ContribRunner[Default-backend rollout]
    Runner --> Teardown[Close env and collect garbage]
    ContribRunner --> Teardown
    Teardown --> Clear[Clear SimulationContext]
Loading

Reviews (1): Last reviewed commit: "Revert "Update preset tests for Newton d..." | Re-trigger Greptile

),
)
@pytest.mark.isaacsim_ci
def test_contrib_environments(task_name, num_envs, device):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Contrib test has no body

When pytest imports this module, the empty test_contrib_environments definition raises an IndentationError, preventing collection of both the core backend matrix and the contributed smoke test.

Suggested change
def test_contrib_environments(task_name, num_envs, device):
def test_contrib_environments(task_name, num_envs, device):
# run a short rollout with random actions to verify the environment loads and steps
_run_environments(task_name, device, num_envs, create_stage_in_memory=False)

# Local imports should be imported last
from env_test_utils import _run_environments, setup_environment # isort: skip

@pytest.mark.parametrize("physics_preset_name", ["newton_mjwarp", "physx", "isaacsim_physx"])

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Backend matrix ignores preset support

When the full suite reaches core tasks such as Fourbar Pole or Franka Soft/Cloth, it applies preset names their physics configurations do not define, causing preset resolution to raise ValueError before the rollout starts.

@isaaclab-review-bot isaaclab-review-bot Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isaac Lab Review Bot

The consolidated smoke coverage and three-preset core matrix are coherent, but the moved contributed-environment test no longer skips IsaacContrib-Franka-Pour, despite its documented external reset-dataset requirement.

  • Design and architecture: Consolidating core and eligible contributed smoke coverage into the shared environment test is consistent with the PR’s stated design. The preset axis replaces the dedicated Newton test while contributed tasks continue to use their configured default backend.
  • API: All changed callers of the test-local setup_environment helper were updated consistently after removing include_play and newton_mjwarp_envs. No unsupported public API change is evident.
  • Implementation: The teardown collection occurs before SimulationContext.clear_instance(), preserving the intended Warp finalizer ordering. However, the replacement contributed test parametrizes IsaacContrib-Franka-Pour without carrying over its artifact-dependent skip or excluding it during selection, so environments without that external dataset will now run a known-ineligible case.

Minor fixes needed. Posted 1 actionable finding inline.

Automated review; human maintainers own approval decisions.

),
)
@pytest.mark.isaacsim_ci
def test_contrib_environments(task_name, num_envs, device):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Warning · Implementation — Contrib move drops Franka-Pour artifact skip

The deleted test_contrib_environments_smoke.py skipped IsaacContrib-Franka-Pour because it requires an external reset-dataset artifact. test_contrib_environments reuses the same setup_environment filters (none of which exclude that task) but omits the skip, and XFAIL_TASKS is empty, so the task is now parametrized and will fail wherever the dataset is absent. Re-add the pytest.skip guard or exclude the task during selection.

…-for-tests

[Test] Generate Franka Pour resets for contrib smoke test
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

infrastructure isaac-lab Related to Isaac Lab team

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants