Align Isaac Lab with Asset Region Profiles - #7549
Conversation
Greptile SummaryThis PR introduces US and China Asset Region Profiles, renames the profile selector, and synchronizes installation documentation, agent-skill guidance, tests, and changelog material.
Confidence Score: 4/5The released profile selector must retain a compatibility fallback before merging; the duplicated US root should also be aligned with the shipped experience configuration. Existing China-profile installations using the released environment variable are now silently routed through the default service, while explicit US selection uses a separately maintained root that already differs from the shipped experiences. Files Needing Attention: source/isaaclab/isaaclab/utils/assets.py, source/isaaclab/test/utils/test_assets.py Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
Env[Profile environment variables] --> Select[_selected_storage_profile]
Select --> Root[_resolve_asset_root]
Select --> Client[OmniClient routing]
Root --> Constants[Asset path constants]
Client --> Fetch[Regional asset retrieval]
Constants --> Fetch
Reviews (1): Last reviewed commit: "Remove legacy storage profile fallback" | Re-trigger Greptile |
There was a problem hiding this comment.
Isaac Lab Review Bot
The PR consistently introduces Asset Region Profile terminology, an explicit us profile, and the new initializer across implementation, tests, documentation, and install skills. However, it removes support for the previously documented ISAACSIM_STORAGE_PROFILE selector without a deprecation path, causing existing China-profile configurations to silently stop taking effect.
- Design and architecture: The named-profile table supports profiles with optional OmniClient routing and cleanly models the endpoint-less
usprofile. The main compatibility gap is that profile selection now reads only the new environment variable rather than accommodating the existing producer path during migration. - API:
configure_storage_profile()remains supported through the newconfigure_asset_region_profile()entry point, but the documentedISAACSIM_STORAGE_PROFILEenvironment interface is removed immediately. It should remain as a deprecated fallback with a warning, and the changelog should include migration guidance. - Implementation: Asset-root precedence, lazy OmniClient import, one-shot China routing configuration, the
uspath, documentation anchors, and focused tests are internally consistent. The tests replace the old selector entirely and therefore do not cover compatibility for existing environments that still export it.
Minor fixes needed. Posted 1 actionable finding inline.
Automated review; human maintainers own approval decisions.
AntoineRichard
left a comment
There was a problem hiding this comment.
Besides Kelly comments this looks good. Approving to unblock when it's ready
|
run-ci |
Description
Aligns Isaac Lab with Isaac Sim's Asset Region Profiles terminology and configuration. The installation guide now has a top-level Asset Region Profiles section immediately after Asset caching, with
usandchinaselection, precedence, switchback, kitless initialization, and manifest guidance.The implementation uses
ISAACSIM_ASSET_REGION_PROFILE, adds an explicitusprofile andconfigure_asset_region_profile(), and replaces the development-onlyISAACSIM_STORAGE_PROFILEselector. The installation skill and focused tests are updated with the same behavior.This is a follow-up to #7467. No additional dependencies are required.
Type of change
Release backport
developScreenshots
Rendered installation documentation was verified locally.
Checklist
Docker and GPU tests run on demand. Push the commits you want tested, then
comment
run-cion the pull request.pre-commitchecks with./isaaclab.sh --formatsource/<pkg>/changelog.d/for every touched package (do not editCHANGELOG.rstor bumpextension.toml— CI handles that)CONTRIBUTORS.mdor my name already exists thereValidation
uv run isaaclab -fpassed.