Skip to content

Conversation

@AntoineRichard
Copy link
Collaborator

Description

Allows running more of our tests without Kit

Type of change

  • Bug fix (non-breaking change which fixes an issue)

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 therenot done this task

@github-actions github-actions bot added the asset New asset feature or request label Dec 12, 2025
@greptile-apps
Copy link
Contributor

greptile-apps bot commented Dec 12, 2025

Greptile Overview

Greptile Summary

This PR decouples Newton physics simulation from Kit/Omniverse dependencies, enabling tests to run without the full Isaac Sim application. The changes enable headless CI testing by setting LAUNCH_OV_APP=0 and creating USD stages using core APIs when omni.usd is unavailable.

Key Changes:

  • Enhanced SimulationContext with fallback logic to create in-memory USD stages when Kit is unavailable
  • Removed Kit/Omniverse imports and initialization from test files (test_environments.py, test_environments_standalone.py, test_solver_convergence.py)
  • Increased solver ls_iterations from 10-15 to 25-40 across robot configurations to improve convergence stability
  • Removed friction parameters from Anymal actuator configs
  • Commented out Anymal-D rough terrain environments (solver stability issues)
  • Added stage cache cleanup in SimulationContext.clear()
  • Enabled debug logging for solver convergence data

Issues Found:

  • Duplicate stage cache insertion at simulation_context.py:193 (logic issue - create_new_stage_in_memory() already inserts into cache)
  • Debug print statement left in test code at test_solver_convergence.py:101

Confidence Score: 4/5

  • This PR is mostly safe to merge with one critical bug that should be fixed
  • The PR successfully achieves Kit decoupling for CI testing, but contains a duplicate stage cache insertion bug that could cause issues in edge cases. The solver parameter tuning and test simplifications are well-reasoned. Score reduced from 5 to 4 due to the duplicate Insert() call.
  • Pay close attention to source/isaaclab/isaaclab/sim/simulation_context.py line 193 - remove the duplicate stage cache insertion

Important Files Changed

File Analysis

Filename Score Overview
source/isaaclab/isaaclab/sim/_impl/newton_manager.py 5/5 Uncommented debug print statement for solver convergence data
source/isaaclab/isaaclab/sim/simulation_context.py 4/5 Enhanced stage initialization with fallback logic and stage cache management for Kit-free operation, added logging and null checks
source/isaaclab_tasks/test/test_environments.py 5/5 Removed Kit/Omniverse dependencies, added LAUNCH_OV_APP=0 env var, simplified test setup for headless operation
source/isaaclab_tasks/test/test_solver_convergence.py 4/5 Removed Kit dependencies, added LAUNCH_OV_APP=0, relaxed convergence limits for Anymal/A1 robots (max 50 iterations), added debug print

Sequence Diagram

sequenceDiagram
    participant Test as Test Suite
    participant Env as Environment
    participant SimCtx as SimulationContext
    participant Cache as USD StageCache
    participant Newton as NewtonManager
    
    Note over Test: Set LAUNCH_OV_APP=0
    Test->>Env: Create environment
    Env->>SimCtx: Initialize simulation
    
    alt create_stage_in_memory=True
        SimCtx->>Cache: create_new_stage_in_memory()
        Cache-->>SimCtx: Return in-memory stage
    else create_stage_in_memory=False
        alt Stage exists in cache
            SimCtx->>Cache: GetAllStages()
            Cache-->>SimCtx: Return existing stage
        else No stage in cache
            alt omni.usd available
                SimCtx->>SimCtx: import omni.usd
                SimCtx->>SimCtx: get_context().get_stage()
                alt Stage is None
                    SimCtx->>Cache: create_new_stage_in_memory()
                    Cache-->>SimCtx: Return new stage
                end
            else omni.usd not available
                SimCtx->>Cache: create_new_stage_in_memory()
                Note over SimCtx,Cache: Duplicate Insert issue
                SimCtx->>Cache: Insert(stage)
                Cache-->>SimCtx: Return new stage
            end
        end
    end
    
    SimCtx->>Newton: Initialize Newton solver
    Newton-->>SimCtx: Solver ready
    
    loop Each simulation step
        Test->>Env: step(actions)
        Env->>Newton: simulate()
        Newton->>Newton: Check convergence (debug mode)
        Newton-->>Env: Return observations
        Env-->>Test: Return step result
    end
    
    Test->>Env: close()
    Env->>SimCtx: clear()
    SimCtx->>Cache: Erase(stage)
    Cache-->>SimCtx: Stage removed
Loading

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

Additional Comments (2)

  1. source/isaaclab/isaaclab/sim/simulation_context.py, line 191-193 (link)

    logic: stage is created in memory at line 192, then immediately inserted into cache at line 193, but this insertion happens inside the except block. However, create_new_stage_in_memory() already inserts the stage into the cache (as shown in stage.py:616). This causes a duplicate insertion.

  2. source/isaaclab_tasks/test/test_solver_convergence.py, line 101 (link)

    style: debug print left in test code

14 files reviewed, 2 comments

Edit Code Review Agent Settings | Greptile

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

asset New asset feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant