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.
Fixes # (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 there

@greptile-apps
Copy link
Contributor

greptile-apps bot commented Dec 3, 2025

Greptile Overview

Greptile Summary

This PR replaces omni.client with a lightweight isaaclab implementation (isaaclab.utils.client) that supports local files, HTTP/HTTPS URLs, and S3 paths. The new client provides essential functionality: path normalization, stat/read operations, file copying, and USD dependency resolution.

Key Changes

  • New client module (source/isaaclab/isaaclab/utils/client.py): 574 lines implementing stat(), read_file(), copy(), and USD reference downloading
  • Updated asset utilities to use new client for cloud storage access and USD dependency management
  • Simplified file spawning logic in from_files.py by removing version-specific path fallback logic
  • Added boto3/botocore dependencies for S3 support
  • Removed omni.client dependency across the codebase

Issues Found

  • Critical: S3 client initialized at module import (line 64 of client.py) will fail in environments without AWS credentials
  • Critical: Event loop creation in download_usd_with_references_sync() (line 567) can conflict with existing event loops

Recommendation

The migration is well-structured but needs the two critical issues fixed before merge to avoid runtime failures in certain environments.

Confidence Score: 3/5

  • This PR has critical issues that will cause runtime failures in certain environments and should not be merged without fixes.
  • Score of 3 reflects that while the overall architecture and migration approach is sound, there are two critical bugs in the new client implementation that will cause failures: (1) S3 client initialization at import time without credentials handling, and (2) unsafe event loop creation that can conflict with existing loops. These issues need to be resolved before merging.
  • Pay close attention to source/isaaclab/isaaclab/utils/client.py - contains critical bugs that need fixing

Important Files Changed

File Analysis

Filename Score Overview
source/isaaclab/isaaclab/utils/client.py 3/5 New lightweight client implementation replacing omni.client with support for local/HTTP/S3 access. Has critical issue with S3 client initialization and event loop handling that needs fixing.
source/isaaclab/isaaclab/utils/assets.py 4/5 Updated to use new client module instead of omni.client. Implements USD dependency downloading and cloud asset retrieval. Clean migration with proper error handling.
source/isaaclab/isaaclab/sim/spawners/from_files/from_files.py 5/5 Simplified USD file path checking logic to use new check_file_path and retrieve_file_path utilities. Cleaner implementation than before.

Sequence Diagram

sequenceDiagram
    participant User as User Code
    participant Spawner as from_files.py
    participant Assets as assets.py
    participant Client as client.py
    participant Storage as Cloud Storage (S3/HTTP)
    participant USD as USD Layer (pxr.Sdf)

    User->>Spawner: spawn_from_usd(prim_path, usd_path, cfg)
    Spawner->>Assets: check_file_path(usd_path)
    Assets->>Client: stat(usd_path)
    
    alt Local file
        Client-->>Assets: Result.OK (status=1)
        Assets-->>Spawner: 1 (local)
    else Remote file (HTTP/S3)
        Client->>Storage: HEAD request / head_object()
        Storage-->>Client: metadata
        Client-->>Assets: Result.OK (status=2)
        Assets-->>Spawner: 2 (remote)
        
        Spawner->>Assets: retrieve_file_path(usd_path)
        
        alt USD file
            Assets->>Client: download_usd_with_references_sync()
            Client->>Storage: download root USD
            Storage-->>Client: USD file
            Client->>USD: _find_usd_references(local_path)
            USD-->>Client: list of referenced assets
            
            loop For each reference
                Client->>Storage: download referenced asset
                Storage-->>Client: asset file
                alt Reference is USD
                    Client->>USD: _find_usd_references()
                    USD-->>Client: nested references
                end
            end
            
            Client-->>Assets: mapping{remote_url: local_path}
            Assets-->>Spawner: local_usd_path
        else Non-USD file
            Assets->>Client: copy(url, local_path)
            Client->>Storage: download file
            Storage-->>Client: file data
            Client-->>Assets: Result.OK
            Assets-->>Spawner: local_file_path
        end
    else File not found
        Client-->>Assets: Result.ERROR_NOT_FOUND (status=0)
        Assets-->>Spawner: 0 (not found)
        Spawner-->>User: FileNotFoundError
    end
    
    Spawner->>Spawner: create_prim(prim_path, usd_path)
    Spawner-->>User: prim
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.

6 files reviewed, 2 comments

Edit Code Review Agent Settings | Greptile

return UrlParts(parsed.scheme or "", parsed.netloc or "", parsed.path or "")


_s3 = boto3.client("s3")
Copy link
Contributor

Choose a reason for hiding this comment

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

logic: boto3 client initialized at module import without configuration. This may fail in environments without AWS credentials or when custom S3 endpoints are needed (like LocalStack).

Suggested change
_s3 = boto3.client("s3")
# Initialize S3 client lazily to allow configuration
_s3 = None
def _get_s3_client():
global _s3
if _s3 is None:
_s3 = boto3.client("s3")
return _s3

kellyguo11 pushed a commit that referenced this pull request Dec 5, 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

<!-- As you go through the list, delete the ones that are not
applicable. -->

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

## Screenshots

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

<!--
Example:

| Before | After |
| ------ | ----- |
| _gif/png before_ | _gif/png after_ |

To upload images to a PR -- simply drag and drop an image while in edit
mode and it should upload the image directly. You can then paste that
source into the above before/after sections.
-->

## Checklist

- [ ] I have read and understood the [contribution
guidelines](https://isaac-sim.github.io/IsaacLab/main/source/refs/contributing.html)
- [ ] I have run the [`pre-commit` checks](https://pre-commit.com/) 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

<!--
As you go through the checklist above, you can mark something as done by
putting an x character in it

For example,
- [x] I have done this task
- [ ] I have not done this task
-->
@kellyguo11 kellyguo11 marked this pull request as draft December 5, 2025 04:39
@kellyguo11
Copy link
Contributor

keeping this one on hold for now in favor of omni.client

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