[NeoZng]

Description

MjcfConverter and UrdfConverter can return an earlier USD from a reused usd_dir. Their importers write a changed conversion to a numbered folder, but AssetConverterBase records only a hash and checks the original requested file on the next lazy call. For example, A writes robot/, B writes robot_1/, and a second request for B could return A from robot/.

The cache now stores one record in .asset_hash: the asset/configuration hash, requested USD file, and actual generated USD file. It reuses that file only when the hash and request match and the generated file still exists. A legacy hash-only record is a cache miss, since it cannot identify which output the importer wrote. The record is written only after conversion succeeds.

The cache retains only the most recent conversion. A → B → A converts again to a new numbered folder; existing outputs are not deleted. Multi-version reuse and cleanup are separate concerns. force_usd_conversion keeps its existing behavior.

Type of change

• Bug fix (non-breaking change which fixes an issue)

Release backport

• Backport this pull request to the active release branch after it merges into develop

Validation

• The extended URDF test_config_change fails on the old cache at the stale-output assertion and passes with the fix.
• source/isaaclab/test/sim/test_urdf_converter.py: 17 passed.
• source/isaaclab/test/sim/test_mjcf_converter.py: 11 passed.
• source/isaaclab/test/sim/test_mesh_converter.py -k 'no_change or config_change': 2 passed.
• uv run --no-sync isaaclab -f: passed with the PR base selected for changelog validation.

Checklist

• I have read and understood the contribution guidelines
• I have run the pre-commit checks
• I have validated the changed behavior with focused coverage and applied the test-audit criteria
• I have added a changelog fragment under source/isaaclab/changelog.d/
• My name already exists in CONTRIBUTORS.md

[greptile-apps]

Confidence Score: 4/5

[Medium risk] Fixes lazy conversion cache logic in asset converters.

The PR appears safe to merge, with a narrow filename edge case and useful cache-branch tests to address.

Findings

1.  Newlines break cache records ▶
2.  Cache recovery branches lack tests ▶

Summary

The PR records both requested and generated USD paths alongside the asset hash so lazy URDF and MJCF conversions reuse the numbered output actually produced by the importer.

• Updates converter documentation and the changelog to describe reuse and migration.
• Adds third-call regression checks for changed configurations.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart LR
  A[Requested USD name and asset hash] --> B{Record matches?}
  B -- Yes --> C{Recorded generated file exists?}
  C -- Yes --> D[Reuse generated USD]
  B -- No --> E[Convert asset]
  C -- No --> E
  E --> F[Record requested and generated names]

Loading

_{Reviews (1) · Last reviewed commit: "Fix lazy conversion loading a stale MJCF..."}

[isaaclab-review-bot]

Isaac Lab Review Bot

The cache-record expansion fixes stale lazy reuse for numbered URDF and MJCF outputs, with aligned documentation, changelog, and regression coverage. One implementation issue remains: a recorded generated path is adopted before deciding whether conversion is required, so forced and recovery conversions can use that generated path as their new request.

• Design and architecture: Keeping cache ownership in AssetConverterBase is the appropriate shared fix for both importers and avoids deleting outputs that may still be in use. However, loading record[2] into _usd_file_name before the conversion decision conflates the previously generated output with the requested destination; those states should remain separate until the code confirms a cache hit will be reused.
• API: The checked public surface—usd_dir, usd_file_name, usd_path, and the existing converter configuration fields—has no signature, symbol, or default changes. Lazy cache hits now return the previously generated numbered output as intended, but forced or missing-file reconversions can expose unexpectedly nested numbered paths because the prior generated name becomes the next requested name.
• Compatibility and deprecation: Breaking changes: none identified. Legacy single-line .asset_hash records are internal cache artifacts and cause the documented one-time reconversion. The implementation issue affects output naming during forced or recovery conversion rather than removing or renaming a public contract.
• Implementation: The record comparison and post-conversion write ordering preserve failed-variant retry behavior, but line 101 assigns record[2] before evaluating force_usd_conversion or a missing generated file. Consequently, _convert_asset can receive the previous generated path through usd_path and deduplicate from that path, producing nested names such as robot_1_1 rather than the documented sibling sequence. Apply the recorded generated name only when the conversion is actually skipped.
• Style consistency: The docstring, comments, encoding arguments, changelog fragment, and how-to notes otherwise follow adjacent conventions and repository guidance. No import, naming, typing, or member-order issue was identified; the required correction is behavioral rather than stylistic.
• Test quality: The changed MJCF and URDF test_config_change cases validly extend existing owner-boundary tests with a third lazy conversion and assert both generated-path reuse and unchanged modification time. They cover distinct importer paths and do not duplicate the mesh converter test. Additional focused coverage should exercise a cache hit followed by forced conversion or deletion of the recorded generated file, which would expose the accepted reconversion-path defect.

Minor fixes needed. Posted 1 actionable finding inline.

Automated review; human maintainers own approval decisions.

[AntoineRichard]

Thanks @NeoZng for raising this! Could we add an opt-in to allow overwriting previous conversions? My main concern with this PR is that nothing is cleaned-up.

If you switch between config A and config B, it doesn't reuse robot/ for A. Each switch converts again: robot, robot_1, robot_2, … The migration also forces one extra numbered folder for every existing cache.

[NeoZng]

Thanks! Some context: since #5394, every conversion into a reused usd_dir writes a new <name>_<N> folder. The Isaac Sim 6 importers never overwrite an earlier output, and #5394 removed the MJCF converter's deletion of <name>/ before the import. With one record per usd_dir, switching between configurations also converts again on develop. This PR keeps that behavior and only makes the lazy path return the folder that the importer actually wrote, instead of <name>/, which holds the first conversion. The one-time migration adds one folder per existing cache, because an old record does not name its folder.

Should we keep this, or delete the previous output and convert again in place? Deleting has caveats: it can remove a source folder named like the robot, it breaks processes that are reading that folder, and a process that has already opened the layer does not see a rewrite at the same path (on file systems with 1-second modification times, not even after Reload()). If you prefer deleting, I can make the converter delete only the folder recorded for its previous conversion and force-reload open layers, either always or behind an opt-in flag, in this PR or in a follow-up for develop (this PR is marked for backport). Reusing earlier outputs when switching between configurations would need a record with several entries, which also fits a follow-up.

[NeoZng]

@AntoineRichard I converted the changelog fragment to the towncrier format from #8118 (ec7f31e) and merged develop (db14a03); the code is unchanged. The question above, whether to keep the numbered output folders or to delete the previous output and convert again, still stands.

[AntoineRichard]

Thanks @NeoZng, and sorry for the slow answer. Let's keep the numbered folders and not delete anything by default. Your caveats about deleting are fair. Since this PR already changes the .asset_hash format, and every format change forces each existing cache to convert again, I'd like to settle the format here so we don't migrate twice (especially with the backport). Concretely:

1. Record every conversion, not just the last one

Keep the file name .asset_hash, so an older Isaac Lab reading the same usd_dir sees a record it can't parse and converts, instead of returning a stale folder. Make the content JSON:

{
  "version": 1,
  "entries": [
    {"hash": "<md5>", "requested": "robot/robot.usda", "generated": "robot/robot.usda"},
    {"hash": "<md5>", "requested": "robot/robot.usda", "generated": "robot_1/robot_1.usda"}
  ]
}

• Store requested and generated relative to usd_dir with / separators (PurePath(...).as_posix()), so a usd_dir shared between Windows and Linux still matches.
• Ignore an entry that isn't a dict of strings, or whose generated is absolute or contains ... That way a hand-edited record can't point usd_path outside usd_dir.

Lookup (skipped when force_usd_conversion=True): reuse the first entry where hash and requested match and os.path.isfile(os.path.join(usd_dir, generated)). Otherwise convert.

Write after a successful conversion (still after _select_physics_variant, as now):

1. Read the record again right before writing. This keeps entries that another process added while we were converting.
2. Drop entries whose generated file no longer exists.
3. Drop entries with the same (hash, requested) as the new one.
4. Drop entries whose generated equals the new generated. That file was just overwritten, and this case is real: MeshConverter always writes to the requested path. Without this rule, mesh A → B → A would return B's file for config A.
5. Append the new entry, write it to a temp file in usd_dir (tempfile.NamedTemporaryFile(dir=usd_dir, delete=False)), then os.replace it onto .asset_hash. A crash then can't leave a half-written record. Two concurrent writers can still lose an entry, which only means an extra conversion later.

With this, URDF/MJCF A → B → A reuses robot/ instead of writing robot_2/. Folders grow with the number of distinct configs, not with each switch.

2. Drop force_usd_conversion from the hash

In _config_to_hash, also pop force_usd_conversion. It controls whether a conversion runs, not what it produces. Today a forced run records a hash that no lazy run can match, so the next lazy run converts again. After this change, a lazy run reuses a forced run's output, and the "this script forces the conversion, so later lazy conversions convert again" sentence can go from the docs.

Note that this changes every hash, so the old-record handling below needs the old-style hash. Add a legacy: bool = False parameter that, instead of popping the key, sets config_dic["force_usd_conversion"] = False. Setting it in place keeps the key order of the old JSON dump, so the md5 is the same as before. Only lazy runs (False) could ever match an old record anyway. It would also be nice to read the asset file once and reuse its digest for both hashes.

3. Adopt old records instead of converting again

An old record is any content that doesn't parse as a JSON dict with version == 1 (check isinstance(data, dict); a hex md5 can sometimes parse as a JSON number). Its first line is the old hash. Adopt it when all of these hold:

• force_usd_conversion is false,
• the first line equals _config_to_hash(cfg, legacy=True),
• the requested file exists,
• the requested file has a folder component (robot/robot.usda), and usd_dir has no sibling folder matching ^{re.escape("robot")}_\d+$. If there's only robot/, only one conversion ever happened there, so it matches the hash. For a flat name (mesh), the old behavior was already correct, so skip the sibling check.

When adopted: reuse the requested file and try to rewrite the record as JSON with that single entry. Wrap the write in try/except OSError: pass, so a read-only usd_dir (Docker images, CI caches) keeps working. If any condition fails, convert as usual.

This removes the extra folder per existing cache in the common case, and the read-only caveat in the changelog.

4. Structure

Something like this in AssetConverterBase:

requested = pathlib.PurePath(self._usd_file_name).as_posix()
entries, legacy_hash = self._read_record()

cached = None
if not cfg.force_usd_conversion:
    cached = self._find_entry(entries, self._asset_hash, requested)
    if cached is None and self._can_adopt_legacy(legacy_hash, cfg, requested):
        cached = requested
        self._try_write_record(requested, requested)  # best effort, ignores OSError

if cached is not None:
    self._usd_file_name = cached
else:
    self._convert_asset(cfg)
    self._select_physics_variant(cfg.physics_variant)
    self._write_record(requested, pathlib.PurePath(self._usd_file_name).as_posix())
    # dump config.yaml as before

_is_same_asset / _usd_file_exists are private and nothing else uses them, so they can go or stay.

5. Tests

• URDF and MJCF: A → B → A. The third call returns the first usd_path, its mtime is unchanged, and no robot_2/ exists.
• Mesh: A → B → A. The third call converts again (the A entry was dropped when B overwrote the file).
• force_usd_conversion=True, then a lazy call with the same config: the lazy call reuses the forced output.
• Old one-line record, only robot/ exists: reused, no new folder, record rewritten as JSON.
• Old record with a robot_1/ sibling: converts.
• Old record in a read-only usd_dir (chmod, skip on Windows/root): reused without error.
• Recorded generated folder deleted: converts, and the stale entry is pruned.
• Corrupt record: converts.
• Keep test_lazy_conversion_converts_again_for_another_requested_file.

6. Docs and changelog

• The how-to notes become simpler: "A lazy conversion reuses any earlier output of the same asset and configuration in usd_dir; a new configuration is written to a new numbered folder." Plus the existing advice to delete stale folders.
• Update the AssetConverterBase docstring the same way.
• In the changelog fragment, drop the read-only caveat and mention the A → B → A reuse and the force_usd_conversion change.

Follow-up (develop only, not backported)

An opt-in flag to clean up older outputs. Since the record now lists every folder the converter created, the cleanup can delete exactly those entries' folders and never touch an unrelated folder with the same name.

[NeoZng]

@AntoineRichard Thanks for the detailed plan. Implemented in 0241117, with two small deviations:

• Rule 4 drops every entry in the new output's folder, not only the one for the same file. Instanceable mesh outputs in one folder share Props/instanceable_meshes.usd, so mesh A (a.usd) → B (b.usd) → A would otherwise return a.usd with B's geometry.
• The temp record is created with open(path, "x") instead of NamedTemporaryFile, whose 0600 mode would make it unreadable to other users of a shared usd_dir.

Instead of reading the asset once for both hashes, the legacy hash is only computed when an old record may be adopted. All tests from your list are in. Happy to follow up with the cleanup flag on develop.

[ooctipus]

run-ci

[isaaclab-bot]

Backported to release/3.0.0 as 52d4ab0.