Skip to content

Conversation

@ooctipus
Copy link
Collaborator

@ooctipus ooctipus commented Dec 4, 2025

Description

This is an alternative to PR #4134 which provide solution as client.py

here is a brief discussion around pros and cons between these choices

Created a new folder tools/installation as the first step to use modular python file for installation decoupling

USD Client Options

Option 1: client.py (pure Python) #4134

Pros

  • Fully transparent: if buggy, user can fix or submit issue
  • Relatively simple
  • No system config; only depends on Python stdlib

Cons

  • May not handle some user URLs robustly (even with urllib)
  • Need to re-implement:
    • stats(url) -> Result
      • Returns: OK, ERROR_NOT_FOUND, ERROR_PERMISSION_DENIED, ERROR_NETWORK, ERROR_UNKNOWN
    • read_file(url) -> data in memory
    • copy(url, local_path) → downloads file

Option 2: omni.client via Packman

Pros

  • No need to re-implement stats, read_file, copy
  • Should be robust, and has many nice future feature.

Cons

  • Recursive reference download still needs to be implemented
  • Not transparent / not easily editable if buggy
  • Must handle system differences
  • Needs install script (~100 lines) that:
    • Downloads omni_client.7z to temp folder and extract content.
    • Copies release/bindings-python/omni/... into _omni_client/ and writes omni_client.pth
    • Copies libomni*.so* into _omni_client/lib/
    • Symlinks (or copies) those libs into _omni_client/omni/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

@ooctipus ooctipus changed the title Enables omni-client be installed with packman [NEWTON]Enables omni-client be installed with packman Dec 4, 2025
@greptile-apps
Copy link
Contributor

greptile-apps bot commented Dec 4, 2025

Greptile Overview

Greptile Summary

This PR enables installation of omni.client as a standalone module via packman, replacing the previous dependency on Isaac Sim's bundled version. The implementation adds recursive USD dependency downloading to ensure all referenced assets are cached locally.

Major Changes:

  • Added install_omni_client_packman.py script that downloads omni.client library from CloudFront CDN and installs it into site-packages with proper pip dist-info
  • Enhanced assets.py with recursive USD reference resolution using Sdf API to download entire dependency trees (textures, sublayers, payloads)
  • Replaced dynamic carb.settings Nucleus root with hardcoded S3 URL (https://omniverse-content-production.s3-us-west-2.amazonaws.com/Assets/Isaac/5.0)
  • Simplified from_files.py by delegating file existence checks and downloads to utility functions
  • Removed hardcoded 4.5→5.0 version migration logic

Issues Found:

  • Critical: site.getsitepackages()[0] may not work correctly with virtual environments, potentially installing to wrong location
  • Error handling gaps: silent failures when force_download=False and copy operations fail
  • Hardcoded S3 URL may break custom nucleus server configurations

Confidence Score: 3/5

  • This PR introduces significant infrastructure changes with some logical issues that need resolution before merging.
  • The implementation is architecturally sound and solves a real dependency management problem. However, the virtual environment detection bug in the installer is a critical issue that could cause installation failures in common deployment scenarios. The error handling gaps in recursive download logic and the hardcoded S3 URL replacement also present risks. The code would benefit from more defensive error handling and testing with various Python environment configurations.
  • Pay close attention to tools/installation/install_omni_client_packman.py (line 69) for the site-packages detection issue, and source/isaaclab/isaaclab/utils/assets.py (lines 119-121) for the silent error handling.

Important Files Changed

File Analysis

Filename Score Overview
tools/installation/install_omni_client_packman.py 4/5 New installer script that downloads and installs omni.client from packman archive. Script has robust error handling and creates proper dist-info for pip uninstall.
source/isaaclab/isaaclab/utils/assets.py 3/5 Enhanced to support recursive USD dependency downloading with Sdf API parsing. Hardcoded cloud URL replaces carb settings. Contains complex reference resolution logic that needs testing.
source/isaaclab/isaaclab/sim/spawners/from_files/from_files.py 4/5 Simplified USD file path checking by delegating to check_file_path and retrieve_file_path utilities. Removed hardcoded 4.5 to 5.0 version migration logic.
isaaclab.sh 5/5 Added call to install_omni_client_packman.py during installation phase.

Sequence Diagram

sequenceDiagram
    participant User
    participant isaaclab.sh
    participant install_omni_client_packman.py
    participant CDN as CloudFront CDN
    participant Site as site-packages
    participant from_files.py
    participant assets.py as assets.py (utils)
    participant omni.client
    participant S3 as AWS S3
    
    User->>isaaclab.sh: ./isaaclab.sh --install
    isaaclab.sh->>install_omni_client_packman.py: Execute installer
    
    Note over install_omni_client_packman.py: Check if py7zr available
    install_omni_client_packman.py->>install_omni_client_packman.py: pip install py7zr if needed
    
    install_omni_client_packman.py->>CDN: Download omni_client_library.7z
    CDN-->>install_omni_client_packman.py: Return 7z archive
    
    install_omni_client_packman.py->>install_omni_client_packman.py: Extract to cache dir
    install_omni_client_packman.py->>Site: Copy Python bindings to _omni_client
    install_omni_client_packman.py->>Site: Copy native libs (.so files)
    install_omni_client_packman.py->>Site: Create symlinks for lib access
    install_omni_client_packman.py->>Site: Write .pth file
    install_omni_client_packman.py->>Site: Create dist-info for pip
    
    Note over User,from_files.py: Runtime - USD file spawning
    
    User->>from_files.py: spawn_from_usd(usd_path)
    from_files.py->>assets.py: check_file_path(usd_path)
    assets.py->>omni.client: stat(usd_path)
    omni.client-->>assets.py: Return status (local=1, remote=2, not found=0)
    assets.py-->>from_files.py: Return file_status=2 (remote)
    
    from_files.py->>assets.py: retrieve_file_path(usd_path)
    
    Note over assets.py: Recursive download loop
    loop For each USD file and dependencies
        assets.py->>omni.client: copy(remote_url, local_path)
        omni.client->>S3: Download file
        S3-->>omni.client: Return file data
        omni.client-->>assets.py: File copied
        
        assets.py->>assets.py: _find_usd_references(local_path)
        Note over assets.py: Parse USD with Sdf.Layer<br/>Extract references, payloads,<br/>sublayers, asset paths
        assets.py->>assets.py: _resolve_reference_url(base_url, ref)
        Note over assets.py: Add resolved refs to download queue
    end
    
    assets.py-->>from_files.py: Return local_root path
    from_files.py->>from_files.py: Create USD prim from local file
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.

4 files reviewed, 7 comments

Edit Code Review Agent Settings | Greptile

@kellyguo11 kellyguo11 merged commit 5e6429e into isaac-sim:dev/newton Dec 5, 2025
3 of 4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants