Restore remote asset URLs in stage dumps - #7083
Conversation
Remote assets are handed to callers as a local cache copy, so a stage built from one records an absolute path that resolves only on the machine holding the cache. Flattening such a stage for a dump under ISAAC_LAB_SAVE_STAGES therefore writes texture and material paths that are meaningless anywhere else. Record the source URL of each cached copy as the copy is located, and add unmirror_file_path to look it up, so a dump can name the asset it was built from. Recording the pair is exact where recovering it from the path is not: a directory named after a URL scheme -- Omniverse is where Omniverse puts user projects by default -- would otherwise read as a cache layout and be rewritten to a URL that was never fetched. The map is populated even when nothing is downloaded, because retrieval walks the whole dependency tree before consulting the cache, so a warm cache still names its sources.
urlparse reports the drive letter of a path such as C:\assets as a URL scheme, so retrieve_git_asset_path took a local checkout on Windows for a remote repository and tried to clone it into the asset cache, failing with a missing-asset error. Require a scheme longer than one character, since no URL scheme is a single letter.
Greptile SummaryThe PR records the source URL associated with each locally mirrored asset and restores those URLs when writing flattened rendering-test stage dumps.
Confidence Score: 5/5The PR appears safe to merge, with no concrete changed-code failure identified. The new source mapping is populated during existing cache-path resolution, unmatched paths remain unchanged, and the Windows-specific guards cover the intended drive-letter ambiguity. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart LR
URL["Remote asset URL"] --> Mirror["_mirror_path"]
Mirror --> Cache["Local cached copy"]
Mirror --> Map["_MIRRORED_URLS"]
Cache --> Stage["Flattened stage"]
Stage --> Restore["_restore_remote_asset_paths"]
Map --> Restore
Restore --> Dump["Portable stage dump with source URLs"]
Reviews (1): Last reviewed commit: "Treat a Windows drive letter as a local ..." | Re-trigger Greptile |
![isaaclab-review-bot[bot]](https://avatars.githubusercontent.com/u/157846462?s=60&v=4){kind=small}
There was a problem hiding this comment.
Isaac Lab Review Bot
The cache-path-to-source-URL mapping and stage-dump rewrite are coherently integrated, but the changelog omits the user-visible Windows behavior fix in retrieve_git_asset_path.
- Design and architecture: Recording source URLs when mirror paths are derived avoids unreliable reconstruction from cache layout, and the rendering-test consumer preserves the existing dependency direction with lazy USD imports. No architectural issue was identified.
- API:
unmirror_file_pathis an additive, documented API with a clear empty-string miss contract. However,_is_git_remote_pathalso changes observable behavior of publicretrieve_git_asset_pathfor Windows local checkout paths, and that fix is absent from the package release note. - Implementation: The stage-dump callback preserves unrecognized asset paths and rewrites recognized cached copies to their recorded URLs. The Windows drive-letter guard is directly covered by tests, but its user-visible effect should be added as a
Fixedchangelog entry before merge.
Minor fixes needed. Posted 1 actionable finding inline.
Automated review; human maintainers own approval decisions.
| Added | ||
| ^^^^^ | ||
|
|
||
| * Added :func:`~isaaclab.utils.assets.unmirror_file_path`, which maps a locally cached asset copy |
There was a problem hiding this comment.
🔵 Suggestion · Api — Changelog omits git remote-path behavior change
The fragment documents only the new unmirror_file_path, but the _is_git_remote_path change also alters user-visible behavior of the public retrieve_git_asset_path: a Windows local checkout such as C:\assets was previously treated as a remote repository and cloned into the cache, and is now used in place. Repository rules require changed behavior to appear in the release note; add a Fixed entry covering it.
Keeps both parametrized lists identical to the ones under review in isaac-sim#7080 so the two implementations diff directly, and moves the cases that distinguish them -- an ordinary local directory named after a URL scheme -- into a test named for what it covers.
| """ | ||
| from pxr import UsdUtils # noqa: PLC0415 | ||
|
|
||
| from isaaclab.utils.assets import unmirror_file_path # noqa: PLC0415 |
There was a problem hiding this comment.
should this be moved to the top?
There was a problem hiding this comment.
We already have imports at the top of isaaclab.utils, I would assume yes? i wonder why would we want to delay this import?
4 participants
Description
When
ISAAC_LAB_SAVE_STAGESis used, assets aren't resolved properly: remote assets are handed to callers as a local cache copy, so a stage built from one records an absolute path that resolves only on the machine holding the cache. Flattening for the dump therefore writes texture and material paths that are meaningless anywhere else.This records the source URL of each cached copy as the copy is located, and adds
unmirror_file_pathto look it up, so a dump can name the asset it was built from.Recording the pair is exact where recovering it from the path is not. A directory named after a URL scheme is an ordinary local layout —
Omniverseis where Omniverse puts user projects by default — and inferring from the path rewrites it to a URL that was never fetched:C:/Users/user/Omniverse/MyProject/scene.usdomniverse://MyProject/scene.usd""(left alone)/data/omniverse/assets/robot.usdomniverse://assets/robot.usd""(left alone)/home/user/projects/https/site/logo.pnghttps://site/logo.png""(left alone)The map is populated even when nothing is downloaded, because retrieval walks the whole dependency tree before consulting the cache — so a warm cache still names its sources. Verified against the real DexCube asset on a fully warm cache in a fresh process: the texture is recovered as its S3 URL despite zero downloads and only the root USD being requested.
This supersedes the string-parsing approach in #7080. It is one line shorter in
assets.py(+31/-1 vs +32/-0), adds no module-level constants, and drops the scheme allowlist, the':'↔'_'port round-trip, and the coupling to the_mirror_pathlayout.Commits
unmirror_file_path, and the rendering-test consumer.C:/...must not be recorded as a cached copy; also covers USD reporting cached copies with forward slashes on Windows._is_git_remote_pathusedbool(urlparse(git_path).scheme), andurlparse("C:\assets").scheme == 'c', soretrieve_git_asset_pathtook a local checkout for a remote repository and tried to clone it into the cache. Independent of the rest; drop this commit if you'd rather it went separately.Type of change
Testing
source/isaaclab/test/utils/test_assets.py: 45 passed, run per-commit in a clean worktree.Commits 1 and 2 each carry one failure — the pre-existing Windows
retrieve_git_asset_pathbug, confirmed failing identically on pristinedevelop— which commit 3 fixes.The drive-letter regression test was verified to fail without its guard and pass with it.
Checklist
pre-commitchecks with./isaaclab.sh --formatsource/<pkg>/changelog.d/for every touched packageCONTRIBUTORS.mdor my name already exists there