-
Notifications
You must be signed in to change notification settings - Fork 2.9k
Unify Isaac Lab assets with Isaac Sim #3608
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
How was the asset verification done? Did you run training on a proper set of environments and made sure they all train exactly the same? |
Mayankm96
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Concerns with how asset generation was done and verifed. This is a big change.
Matthew helped me verify all these assets using a benchmark with 2 main metrics: rewards and duration. We verified that all the tasks have similar metrics as before @matthewtrepte to provide more details as well |
apps/isaaclab.python.headless.kit
Outdated
| persistent.isaac.asset_root.default = "https://omniverse-content-production.s3-us-west-2.amazonaws.com/Assets/Isaac/5.0" | ||
| persistent.isaac.asset_root.cloud = "https://omniverse-content-production.s3-us-west-2.amazonaws.com/Assets/Isaac/5.0" | ||
| persistent.isaac.asset_root.nvidia = "https://omniverse-content-production.s3-us-west-2.amazonaws.com/Assets/Isaac/5.0" | ||
| persistent.isaac.asset_root.default = "https://omniverse-content-staging.s3-us-west-2.amazonaws.com/Assets/Isaac/5.1" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we shouldn't be updating this on main, probably better to point this PR to release/2.3.0 branch instead
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, updated this to main
| import omni.client | ||
|
|
||
| NUCLEUS_ASSET_ROOT_DIR = carb.settings.get_settings().get("/persistent/isaac/asset_root/cloud") | ||
| NUCLEUS_ASSET_ROOT_DIR = (carb.settings.get_settings().get("/persistent/isaac/asset_root/cloud") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is this change needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this was from formatting, reverted it back
|
@matthewtrepte do you have any training curves to show the before/after for the impacted environments? @stevfeng please also run the formatter to address the linter errors. |
eed1139 to
1d3c102
Compare
Greptile OverviewGreptile SummaryThis PR migrates Isaac Lab robot assets from the Key changes:
Impact: Confidence Score: 5/5
Important Files ChangedFile Analysis
Sequence DiagramsequenceDiagram
participant Dev as Developer
participant Code as Isaac Lab Code
participant Assets as Asset Path Resolution
participant Nucleus as Isaac Sim Nucleus Server
Note over Dev,Nucleus: Asset Path Migration
Dev->>Code: Update robot configurations
Code->>Code: Replace ISAACLAB_NUCLEUS_DIR with ISAAC_NUCLEUS_DIR
Note over Code: Path structure changes:<br/>IsaacLab/Robots/* → Isaac/Robots/*
Code->>Assets: Request robot asset (e.g., Franka)
Assets->>Assets: Resolve path from ISAAC_NUCLEUS_DIR
Note over Assets: New path: Isaac/Robots/FrankaRobotics/FrankaEmika/
Assets->>Nucleus: Load USD file from new location
Nucleus-->>Assets: Return asset data
Assets-->>Code: Asset loaded successfully
Code->>Code: Update tests to match new paths
Code->>Code: Update documentation examples
Note over Dev,Nucleus: Result: Unified asset structure<br/>across Isaac Lab and Isaac Sim
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Additional Comments (3)
-
source/isaaclab_assets/isaaclab_assets/robots/anymal.py, line 47 (link)logic: actuator network file still references
ISAACLAB_NUCLEUS_DIRwhile robot USD paths were migrated toISAAC_NUCLEUS_DIR- verify this actuator network exists at this path in the new asset structure -
source/isaaclab_assets/isaaclab_assets/robots/unitree.py, line 32 (link)logic: actuator network file still references
ISAACLAB_NUCLEUS_DIRwhile robot USD paths were migrated toISAAC_NUCLEUS_DIR- verify this exists in the new asset structure -
apps/isaaclab.python.kit, line 218-220 (link)logic: hardcoded to Isaac 5.1 staging S3 bucket - this will break when Isaac 5.2+ is released unless updated
28 files reviewed, 8 comments
apps/isaaclab.python.headless.kit
Outdated
| persistent.isaac.asset_root.default = "https://omniverse-content-staging.s3-us-west-2.amazonaws.com/Assets/Isaac/5.1" | ||
| persistent.isaac.asset_root.cloud = "https://omniverse-content-staging.s3-us-west-2.amazonaws.com/Assets/Isaac/5.1" | ||
| persistent.isaac.asset_root.nvidia = "https://omniverse-content-staging.s3-us-west-2.amazonaws.com/Assets/Isaac/5.1" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: hardcoded to Isaac 5.1 staging S3 bucket - this will break when Isaac 5.2+ is released unless updated
| persistent.isaac.asset_root.default = "https://omniverse-content-staging.s3-us-west-2.amazonaws.com/Assets/Isaac/5.1" | ||
| persistent.isaac.asset_root.cloud = "https://omniverse-content-staging.s3-us-west-2.amazonaws.com/Assets/Isaac/5.1" | ||
| persistent.isaac.asset_root.nvidia = "https://omniverse-content-staging.s3-us-west-2.amazonaws.com/Assets/Isaac/5.1" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: hardcoded to Isaac 5.1 staging S3 bucket - this will break when Isaac 5.2+ is released unless updated
apps/isaaclab.python.rendering.kit
Outdated
| persistent.isaac.asset_root.default = "https://omniverse-content-staging.s3-us-west-2.amazonaws.com/Assets/Isaac/5.1" | ||
| persistent.isaac.asset_root.cloud = "https://omniverse-content-staging.s3-us-west-2.amazonaws.com/Assets/Isaac/5.1" | ||
| persistent.isaac.asset_root.nvidia = "https://omniverse-content-staging.s3-us-west-2.amazonaws.com/Assets/Isaac/5.1" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: hardcoded to Isaac 5.1 staging S3 bucket - this will break when Isaac 5.2+ is released unless updated
apps/isaaclab.python.xr.openxr.kit
Outdated
| persistent.isaac.asset_root.default = "https://omniverse-content-staging.s3-us-west-2.amazonaws.com/Assets/Isaac/5.1" | ||
| persistent.isaac.asset_root.cloud = "https://omniverse-content-staging.s3-us-west-2.amazonaws.com/Assets/Isaac/5.1" | ||
| persistent.isaac.asset_root.nvidia = "https://omniverse-content-staging.s3-us-west-2.amazonaws.com/Assets/Isaac/5.1" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: hardcoded to Isaac 5.1 staging S3 bucket - this will break when Isaac 5.2+ is released unless updated
| persistent.isaac.asset_root.default = "https://omniverse-content-staging.s3-us-west-2.amazonaws.com/Assets/Isaac/5.1" | ||
| persistent.isaac.asset_root.cloud = "https://omniverse-content-staging.s3-us-west-2.amazonaws.com/Assets/Isaac/5.1" | ||
| persistent.isaac.asset_root.nvidia = "https://omniverse-content-staging.s3-us-west-2.amazonaws.com/Assets/Isaac/5.1" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: hardcoded to Isaac 5.1 staging S3 bucket - this will break when Isaac 5.2+ is released unless updated
1d3c102 to
e631e17
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Additional Comments (3)
-
source/isaaclab/test/sim/test_utils.py, line 153-156 (link)logic: version check looks for "4.5" string in path but may not work correctly with Isaac Sim 5.1 staging URLs - the path might contain "5.1" instead
-
source/isaaclab/test/sim/test_schemas.py, line 115-116 (link)logic: hardcoded version replacement from 4.5 to 5.0 will fail with Isaac Sim 5.1+ - this logic needs to be updated or removed for compatibility with the new unified asset paths
-
source/isaaclab/test/sim/test_schemas.py, line 141-142 (link)logic: hardcoded version replacement from 4.5 to 5.0 will fail with Isaac Sim 5.1+ - this logic needs to be updated or removed for compatibility with the new unified asset paths
22 files reviewed, 3 comments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
22 files reviewed, no comments
|
@matthewtrepte @kellyguo11 could you please review the latest branch when you get a chance and hopefully we can get this merged soon. Thank you :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
22 files reviewed, no comments
d3f5754 to
1084d7e
Compare
Signed-off-by: Ji Yuan Feng <168472921+stevfeng@users.noreply.github.com>
Description
Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context.
List any dependencies that are required for this change.
Fixes # (issue)
Type of change
Screenshots
Please attach before and after screenshots of the change if applicable.
Checklist
pre-commitchecks with./isaaclab.sh --formatconfig/extension.tomlfileCONTRIBUTORS.mdor my name already exists there