Restore remote asset URLs in stage dumps - #7080
Conversation
Greptile SummaryThis PR adds a reverse mapping from locally cached asset paths to their original remote URLs and applies it when exporting flattened rendering-test stages.
Confidence Score: 4/5The path-classification defect should be fixed before merging because valid local stage dependencies can be rewritten into unrelated remote URLs. The new restoration helper does not verify cache provenance before rewriting paths, and its caller applies it to every flattened asset reference. Files Needing Attention: source/isaaclab/isaaclab/utils/assets.py, source/isaaclab_tasks/test/rendering_test_utils.py Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart LR
A[Saved USD stage] --> B[Flatten stage]
B --> C[Modify every asset path]
C --> D{Recognized as mirrored path?}
D -->|Yes| E[Restore remote URL]
D -->|No| F[Keep original path]
E --> G[Export stage dump or golden]
F --> G
Reviews (1): Last reviewed commit: "Restore remote asset URLs in stage dumps" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| for index, part in enumerate(parts[:-2]): | ||
| if part.lower() not in _MIRROR_URL_SCHEMES: | ||
| continue | ||
| netloc, *remainder = parts[index + 1 :] | ||
| # ``_mirror_path`` writes a port separator as '_', which is not valid in a host name | ||
| netloc = _MIRROR_NETLOC_PORT_RE.sub(r"\1:\2", netloc) | ||
| return f"{part.lower()}://{netloc}/{'/'.join(remainder)}" |
There was a problem hiding this comment.
When a locally authored asset path contains a directory named http, https, or omniverse, unmirror_file_path treats that component as the start of a cache layout, causing the exported stage to replace the local dependency with an unrelated remote URL that fails to resolve or loads the wrong resource.
Knowledge Base Used: Terrains and Shared Utilities
![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 stage-export path restoration addresses cached remote assets, but the inverse mapping can misclassify ordinary local paths and rewrite them as remote URLs.
- Design and architecture: The forward mapping is scoped by a caller-supplied download directory, while
unmirror_file_pathinfers the cache boundary from any component namedhttp,https, oromniverse. The inverse therefore lacks enough context to distinguish cached assets from similarly named local directories. - API: The documented fallback contract is violated for local paths containing a scheme-named directory. For example, a local path containing
/omniverse/Assets/foo.usdcan be returned asomniverse://Assets/foo.usdrather than"". Cache recognition should be anchored to a known cache root and use the cache layout's exact scheme representation. - Implementation: Applying
UsdUtils.ModifyAssetPathsafter flattening is appropriately placed, and unmatched paths are preserved throughor asset_path. However, the component scan inunmirror_file_pathdirectly causes locally authored asset paths to be rewritten when one of their directory names resembles a supported URL scheme.
Minor fixes needed. Posted 1 actionable finding inline.
Automated review; human maintainers own approval decisions.
| parts = path.replace(os.sep, "/").split("/") | ||
| # the last two components are the host and at least one path component, so a scheme found | ||
| # there cannot be the start of a cache layout | ||
| for index, part in enumerate(parts[:-2]): |
There was a problem hiding this comment.
🟡 Warning · Design Architecture — Cache detection not anchored to download directory
The scan accepts any path component named http/https/omniverse (case-insensitively) without verifying the path lies under a cache download directory, unlike _mirror_path which is parameterized by download_dir and writes a lowercase scheme. A locally authored path such as ~/Documents/Omniverse/Assets/foo.usd is rewritten to omniverse://Assets/foo.usd, so _restore_remote_asset_paths corrupts it in the exported stage despite both docstrings promising local paths are untouched. Anchor the match to a known cache root and match the scheme exactly.
|
|
||
| _GIT_SSH_RE = re.compile(r"^[^@/:]+@[^:]+:.+") | ||
|
|
||
| _MIRROR_URL_SCHEMES = frozenset({"http", "https", "omniverse"}) |
There was a problem hiding this comment.
should these schemes be hard coded. I think I may have seen other schemes as well like s3://
There was a problem hiding this comment.
should these schemes be hard coded
This sounds like a question, but I am not sure. Are you suggesting that we shouldn't hard code those and rely on other functionality? Have you had something specific in mind?
| @@ -305,6 +310,33 @@ def _mirror_path(url: str, download_dir: str) -> str: | |||
| return os.path.join(download_dir, parsed.scheme, netloc, *parsed.path.lstrip("/").split("/")) | |||
|
|
|||
There was a problem hiding this comment.
additional problem I found with _mirror_path from 6751 is that it treats a Windows drive letter as a URL scheme. this should also be resolved
There was a problem hiding this comment.
tests should be added for Windows paths like this
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.
# Description When `ISAAC_LAB_SAVE_STAGES` is 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_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 is an ordinary local layout — `Omniverse` is where Omniverse puts user projects by default — and inferring from the path rewrites it to a URL that was never fetched: | local path | recovered from path | this PR | | --- | --- | --- | | `C:/Users/user/Omniverse/MyProject/scene.usd` | `omniverse://MyProject/scene.usd` | `""` (left alone) | | `/data/omniverse/assets/robot.usd` | `omniverse://assets/robot.usd` | `""` (left alone) | | `/home/user/projects/https/site/logo.png` | `https://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_path` layout. ## Commits 1. **Restore remote asset URLs in stage dumps** — the map, `unmirror_file_path`, and the rendering-test consumer. 2. **Cover Windows path handling in cached asset URL lookup** — a drive letter parses as a URL scheme, so `C:/...` must not be recorded as a cached copy; also covers USD reporting cached copies with forward slashes on Windows. 3. **Treat a Windows drive letter as a local git asset path** — pre-existing bug found while running the suite on Windows. `_is_git_remote_path` used `bool(urlparse(git_path).scheme)`, and `urlparse("C:\assets").scheme == 'c'`, so `retrieve_git_asset_path` took 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 - Bug fix (non-breaking change which fixes an issue) - New feature (non-breaking change which adds functionality) ## 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_path` bug, confirmed failing identically on pristine `develop` — which commit 3 fixes. The drive-letter regression test was verified to fail without its guard and pass with it. ## Checklist - [x] I have read and understood the [contribution guidelines](https://isaac-sim.github.io/IsaacLab/main/source/refs/contributing.html) - [x] I have run the [`pre-commit` checks](https://pre-commit.com/) with `./isaaclab.sh --format` - [x] I have made corresponding changes to the documentation - [x] My changes generate no new warnings - [x] I have added tests that prove my fix is effective or that my feature works - [x] I have added a changelog fragment under `source/<pkg>/changelog.d/` for every touched package - [x] I have added my name to the `CONTRIBUTORS.md` or my name already exists there
2 participants
Description
When
ISAAC_LAB_SAVE_STAGESis used, then assets aren't resolved properlyFixes # (issue)
Type of change
Screenshots
Please attach before and after screenshots of the change if applicable.
Checklist
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 there