Skip to content

Conversation

@ooctipus
Copy link
Collaborator

@ooctipus ooctipus commented Dec 3, 2025

Description

This PR replaces omni.client with light weight implementation of isaaclab.utils.client.

Type of change

  • New feature (non-breaking change which adds functionality)

Screenshots

Please attach before and after screenshots of the change if applicable.

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 there

@github-actions github-actions bot added enhancement New feature or request isaac-lab Related to Isaac Lab team labels Dec 3, 2025
@ooctipus ooctipus changed the title replaces omni.client with isaaclab implementation of client Replaces omni.client with isaaclab implementation of client Dec 3, 2025
@github-actions github-actions bot added the documentation Improvements or additions to documentation label Dec 3, 2025
@greptile-apps
Copy link
Contributor

greptile-apps bot commented Dec 3, 2025

Greptile Overview

Greptile Summary

Replaced omni.client dependency with lightweight isaaclab.utils.client implementation supporting HTTP(S), S3, and local file operations. This significantly reduces dependency footprint and provides custom USD asset downloading with reference resolution.

Major changes:

  • Created new isaaclab/utils/client.py with stat, read_file, copy, and download_usd_with_references functions
  • Updated assets.py to use new client and handle USD dependency graph traversal
  • Removed omni.client imports from nucleus.py and tutorial scripts
  • Hardcoded NUCLEUS_ASSET_ROOT_DIR to S3 URL instead of reading from carb settings

Critical issues found:

  • boto3 dependency not declared in project dependencies
  • Deprecated asyncio.get_event_loop() used in download_usd_with_references_sync
  • Global S3 client initialization at module import will crash if boto3 unavailable
  • Hardcoded S3 URL breaks custom Nucleus server configurations

Confidence Score: 2/5

  • This PR has critical dependency and compatibility issues that will cause runtime failures
  • Score reflects multiple blocking issues: missing boto3 dependency will crash on import, deprecated asyncio API, global S3 client without error handling, and hardcoded S3 URL breaks existing configurations that rely on carb settings
  • source/isaaclab/isaaclab/utils/client.py needs boto3 dependency declaration and S3 client initialization fixes; source/isaaclab/isaaclab/utils/assets.py needs configuration flexibility restored

Important Files Changed

File Analysis

Filename Score Overview
source/isaaclab/isaaclab/utils/client.py 2/5 New lightweight client implementation replaces omni.client with HTTP/S3 support, but has critical issues: boto3 dependency not declared, deprecated asyncio API, global S3 client initialization
source/isaaclab/isaaclab/sim/utils/nucleus.py 4/5 Updated imports to use new isaaclab.utils.client, removed set_hang_detection_time_ms call (no longer needed)
source/isaaclab/isaaclab/utils/assets.py 4/5 Replaced omni.client with isaaclab.utils.client, changed hardcoded NUCLEUS_ASSET_ROOT_DIR from carb settings to S3 URL, added USD dependency resolution

Sequence Diagram

sequenceDiagram
    participant App as Application Code
    participant Assets as isaaclab.utils.assets
    participant Client as isaaclab.utils.client
    participant S3 as S3/HTTP Storage
    participant USD as USD Parser (Sdf)

    App->>Assets: retrieve_file_path(path)
    Assets->>Client: stat(path)
    Client->>S3: HEAD request / S3 head_object
    S3-->>Client: Result + metadata
    Client-->>Assets: Result.OK / NOT_FOUND
    
    alt Remote USD file
        Assets->>Client: download_usd_with_references_sync()
        Client->>Client: _normalize_url()
        loop For each asset in dependency graph
            Client->>S3: copy_async(url, local_path)
            S3-->>Client: Download file
            Client->>USD: _find_usd_references(local_usd)
            USD-->>Client: List of referenced assets
            Client->>Client: _resolve_reference_url()
        end
        Client-->>Assets: mapping (remote URLs → local paths)
        Assets-->>App: local_root_path
    else Remote non-USD file
        Assets->>Client: copy(url, local_path)
        Client->>S3: Download file
        S3-->>Client: File content
        Client-->>Assets: Result.OK
        Assets-->>App: local_path
    else Local file
        Assets-->>App: absolute_path
    end
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 (1)

  1. source/isaaclab/isaaclab/utils/client.py, line 614 (link)

    syntax: asyncio.get_event_loop() is deprecated in Python 3.10+, use asyncio.new_event_loop() or asyncio.get_running_loop() for running loops

7 files reviewed, 4 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

documentation Improvements or additions to documentation enhancement New feature or request isaac-lab Related to Isaac Lab team

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant