From b9782794dfe9f56d69a13e19c4ea18cd7e22492b Mon Sep 17 00:00:00 2001 From: Zeng Qingcheng <60593302+NeoZng@users.noreply.github.com> Date: Tue, 29 Sep 2026 18:20:06 +0800 Subject: [PATCH 1/7] Fix lazy conversion loading a stale MJCF or URDF conversion The Isaac Sim importers do not overwrite an earlier conversion in a reused usd_dir; they write the new one to a _1 folder. The converters still looked for /.usda, so the next lazy conversion with a matching asset hash returned the first output and reverted a changed asset file or configuration. Record the requested and the generated USD file with the asset hash, and load the generated file only when both the hash and the requested file match. Records of earlier versions hold only the hash, so each existing usd_dir converts once more. --- docs/source/how-to/import_new_asset.rst | 24 ++++++++------ .../changelog.d/fix-converter-lazy-cache.rst | 10 ++++++ .../sim/converters/asset_converter_base.py | 32 +++++++++++-------- .../isaaclab/test/sim/test_mjcf_converter.py | 11 ++++++- .../isaaclab/test/sim/test_urdf_converter.py | 11 ++++++- 5 files changed, 62 insertions(+), 26 deletions(-) create mode 100644 source/isaaclab/changelog.d/fix-converter-lazy-cache.rst diff --git a/docs/source/how-to/import_new_asset.rst b/docs/source/how-to/import_new_asset.rst index 04e7e8f12d88..60861913223e 100644 --- a/docs/source/how-to/import_new_asset.rst +++ b/docs/source/how-to/import_new_asset.rst @@ -265,11 +265,13 @@ is derived automatically from the robot name in the URDF): .. note:: The URDF importer auto-deduplicates the per-robot subdirectory when it already exists. If you re-run the converter against the same ``usd_dir`` with a changed configuration - (for example, flipping ``fix_base``), the importer writes to a new numbered folder - (``anymal_1/``, ``anymal_2/``, …) rather than overwriting the previous output. - :attr:`~sim.converters.UrdfConverter.usd_path` reflects whichever folder the importer - actually used. Delete stale subdirectories manually (or wipe ``usd_dir``) if you do not - want them to accumulate on disk. + (for example, flipping ``fix_base``), or re-run this script, which always converts, the + importer writes to a new numbered folder (``anymal_1/``, ``anymal_2/``, …) rather than + overwriting the previous output. :attr:`~sim.converters.UrdfConverter.usd_path` reflects + whichever folder the importer actually used, and later lazy conversions with the same + configuration load that folder again until another conversion, of any asset or + configuration, is written into the same ``usd_dir``. Delete stale subdirectories manually + (or wipe ``usd_dir``) if you do not want them to accumulate on disk. .. _import-new-asset-preview: @@ -419,11 +421,13 @@ Executing the above script will create the USD file inside the .. note:: The MJCF importer auto-deduplicates the per-robot subdirectory when it already exists, matching the URDF importer's behavior. If you re-run the converter against the same - ``usd_dir`` with a changed configuration, the importer writes to a new numbered folder - (``h1_1/``, ``h1_2/``, …) rather than overwriting the previous output. - :attr:`~sim.converters.MjcfConverter.usd_path` reflects whichever folder the importer - actually used. Delete stale subdirectories manually (or wipe ``usd_dir``) if you do not - want them to accumulate on disk. + ``usd_dir`` with a changed configuration, or re-run this script, which always converts, the + importer writes to a new numbered folder (``h1_1/``, ``h1_2/``, …) rather than overwriting + the previous output. :attr:`~sim.converters.MjcfConverter.usd_path` reflects whichever + folder the importer actually used, and later lazy conversions with the same configuration + load that folder again until another conversion, of any asset or configuration, is written + into the same ``usd_dir``. Delete stale subdirectories manually (or wipe ``usd_dir``) if you + do not want them to accumulate on disk. See :ref:`Previewing converted assets ` for the ``--viz`` options. diff --git a/source/isaaclab/changelog.d/fix-converter-lazy-cache.rst b/source/isaaclab/changelog.d/fix-converter-lazy-cache.rst new file mode 100644 index 000000000000..1817bfaf7a51 --- /dev/null +++ b/source/isaaclab/changelog.d/fix-converter-lazy-cache.rst @@ -0,0 +1,10 @@ +Fixed +^^^^^ + +* Fixed lazy conversion in :class:`~isaaclab.sim.converters.MjcfConverter` and + :class:`~isaaclab.sim.converters.UrdfConverter` loading the first output again after the asset file or the + configuration changed in a reused ``usd_dir``. The Isaac Sim importers write each new conversion to a numbered + folder such as ``_1`` instead of overwriting ````, so the converters recorded the requested and the + generated USD file next to the asset hash and loaded the generated one. Records written by earlier versions do + not name these files, so each existing ``usd_dir`` was converted once more, into a new numbered folder. A + read-only ``usd_dir`` with such a record has to be converted again where it is writable. diff --git a/source/isaaclab/isaaclab/sim/converters/asset_converter_base.py b/source/isaaclab/isaaclab/sim/converters/asset_converter_base.py index 753601334829..5829603c807f 100644 --- a/source/isaaclab/isaaclab/sim/converters/asset_converter_base.py +++ b/source/isaaclab/isaaclab/sim/converters/asset_converter_base.py @@ -29,11 +29,11 @@ class AssetConverterBase(abc.ABC): :meth:`_convert_asset` method to provide the actual conversion. The file conversion is lazy if the output directory (:obj:`AssetConverterBaseCfg.usd_dir`) is provided. - In the lazy conversion, the USD file is re-generated only if: + The output directory records its last conversion, and the USD file is re-generated only if: - * The asset file is modified. - * The configuration parameters are modified. - * The USD file does not exist. + * The asset file or the configuration parameters were modified since that conversion. + * The requested USD file name differs from that conversion. + * The USD file that the conversion generated does not exist. To override this behavior to force conversion, the flag :obj:`AssetConverterBaseCfg.force_usd_conversion` can be set to True. @@ -45,9 +45,8 @@ class AssetConverterBase(abc.ABC): triggered conversions try to use the same directory for reading/writing the generated files. .. note:: - Changes to the parameters :obj:`AssetConverterBaseCfg.asset_path`, :obj:`AssetConverterBaseCfg.usd_dir`, and - :obj:`AssetConverterBaseCfg.usd_file_name` are not considered as modifications in the configuration instance - that trigger the USD file re-generation. + Changes to the parameters :obj:`AssetConverterBaseCfg.asset_path` and :obj:`AssetConverterBaseCfg.usd_dir` + are not considered as modifications in the configuration instance that trigger the USD file re-generation. """ @@ -87,15 +86,20 @@ def __init__(self, cfg: AssetConverterBaseCfg): self._usd_file_name = usd_file_name os.makedirs(self.usd_dir, exist_ok=True) - self._usd_file_exists = os.path.isfile(self.usd_path) - # the recorded hash tells whether the cached USD was generated from this asset and config + # the record holds the hash of the asset and config, the requested USD file, and the file that the + # conversion generated, which differs when an importer does not overwrite an earlier output self._dest_hash_path = os.path.join(self.usd_dir, ".asset_hash") self._asset_hash = self._config_to_hash(cfg) + requested_usd_file_name = self._usd_file_name try: - with open(self._dest_hash_path) as f: - self._is_same_asset = f.readline() == self._asset_hash + with open(self._dest_hash_path, encoding="utf-8") as f: + record = f.read().splitlines() except FileNotFoundError: - self._is_same_asset = False + record = [] + self._is_same_asset = len(record) == 3 and record[:2] == [self._asset_hash, requested_usd_file_name] + if self._is_same_asset: + self._usd_file_name = record[2] + self._usd_file_exists = os.path.isfile(self.usd_path) # convert the asset to USD if the hash is different or USD file does not exist if cfg.force_usd_conversion or not self._usd_file_exists or not self._is_same_asset: @@ -106,8 +110,8 @@ def __init__(self, cfg: AssetConverterBaseCfg): self._select_physics_variant(cfg.physics_variant) # record the hash only now: writing it earlier would let a conversion that raised # still count as cached, so an identical retry would skip it and return the asset - with open(self._dest_hash_path, "w") as f: - f.write(self._asset_hash) + with open(self._dest_hash_path, "w", encoding="utf-8") as f: + f.write(f"{self._asset_hash}\n{requested_usd_file_name}\n{self._usd_file_name}") # dump the configuration next to the asset, stamped with the converter that produced it config_path = os.path.join(self.usd_dir, "config.yaml") dump_yaml(config_path, to_dict(cfg)) diff --git a/source/isaaclab/test/sim/test_mjcf_converter.py b/source/isaaclab/test/sim/test_mjcf_converter.py index 7a0d05435d41..ee2eaca6584a 100644 --- a/source/isaaclab/test/sim/test_mjcf_converter.py +++ b/source/isaaclab/test/sim/test_mjcf_converter.py @@ -105,7 +105,11 @@ def test_no_change(test_setup_teardown): def test_config_change(test_setup_teardown): - """Call conversion twice but change the config in the second call. This should generate a new USD file.""" + """Call conversion twice but change the config in the second call. This should generate a new USD file. + + A third call with the changed config must load that file, although the importer writes it next to the + first one instead of overwriting it. + """ sim, mjcf_config = test_setup_teardown mjcf_converter = MjcfConverter(mjcf_config) @@ -122,6 +126,11 @@ def test_config_change(test_setup_teardown): assert time_usd_file_created != new_time_usd_file_created + # convert again with the changed config, which must not generate a new USD file + lazy_mjcf_converter = MjcfConverter(new_config) + assert lazy_mjcf_converter.usd_path == new_mjcf_converter.usd_path + assert os.stat(lazy_mjcf_converter.usd_path).st_mtime_ns == new_time_usd_file_created + def test_create_prim_from_usd(test_setup_teardown): """Call conversion and create a prim from it.""" diff --git a/source/isaaclab/test/sim/test_urdf_converter.py b/source/isaaclab/test/sim/test_urdf_converter.py index 1a49377c1f87..dc29d7344a24 100644 --- a/source/isaaclab/test/sim/test_urdf_converter.py +++ b/source/isaaclab/test/sim/test_urdf_converter.py @@ -97,7 +97,11 @@ def test_no_change(sim_config): @pytest.mark.isaacsim_ci def test_config_change(sim_config, tmp_path): - """Call conversion twice but change the config in the second call. This should generate a new USD file.""" + """Call conversion twice but change the config in the second call. This should generate a new USD file. + + A third call with the changed config must load that file, although the importer writes it next to the + first one instead of overwriting it. + """ sim, config = sim_config output_dir = os.path.join(str(tmp_path), "urdf_config_change") @@ -118,6 +122,11 @@ def test_config_change(sim_config, tmp_path): assert time_usd_file_created != new_time_usd_file_created + # convert again with the changed config, which must not generate a new USD file + lazy_urdf_converter = UrdfConverter(new_config) + assert lazy_urdf_converter.usd_path == new_urdf_converter.usd_path + assert os.stat(lazy_urdf_converter.usd_path).st_mtime_ns == new_time_usd_file_created + @pytest.mark.isaacsim_ci def test_create_prim_from_usd(sim_config): From 1ed22a0829b01c331cc39897d94e2b6941bf8e15 Mon Sep 17 00:00:00 2001 From: Zeng Qingcheng <60593302+NeoZng@users.noreply.github.com> Date: Wed, 30 Sep 2026 00:53:13 +0800 Subject: [PATCH 2/7] Adopt the recorded USD file only when a lazy conversion is reused Keep the requested file name until the converter decides to reuse the recorded conversion, so that every conversion starts from the requested file. Test that a record of an earlier version and a missing generated file each convert again, into a folder next to the earlier ones. --- .../sim/converters/asset_converter_base.py | 11 ++++---- .../isaaclab/test/sim/test_urdf_converter.py | 28 +++++++++++++++++++ 2 files changed, 34 insertions(+), 5 deletions(-) diff --git a/source/isaaclab/isaaclab/sim/converters/asset_converter_base.py b/source/isaaclab/isaaclab/sim/converters/asset_converter_base.py index 5829603c807f..63264745f744 100644 --- a/source/isaaclab/isaaclab/sim/converters/asset_converter_base.py +++ b/source/isaaclab/isaaclab/sim/converters/asset_converter_base.py @@ -97,12 +97,13 @@ def __init__(self, cfg: AssetConverterBaseCfg): except FileNotFoundError: record = [] self._is_same_asset = len(record) == 3 and record[:2] == [self._asset_hash, requested_usd_file_name] - if self._is_same_asset: - self._usd_file_name = record[2] - self._usd_file_exists = os.path.isfile(self.usd_path) + cached_usd_file_name = record[2] if self._is_same_asset else requested_usd_file_name + self._usd_file_exists = os.path.isfile(os.path.join(self.usd_dir, cached_usd_file_name)) - # convert the asset to USD if the hash is different or USD file does not exist - if cfg.force_usd_conversion or not self._usd_file_exists or not self._is_same_asset: + # reuse the recorded USD file, or convert the asset if the hash differs or the file does not exist + if self._is_same_asset and self._usd_file_exists and not cfg.force_usd_conversion: + self._usd_file_name = cached_usd_file_name + else: # convert the asset to USD self._convert_asset(cfg) # importers put the physics payloads behind a "Physics" variant set and disagree on diff --git a/source/isaaclab/test/sim/test_urdf_converter.py b/source/isaaclab/test/sim/test_urdf_converter.py index dc29d7344a24..28f2b2890318 100644 --- a/source/isaaclab/test/sim/test_urdf_converter.py +++ b/source/isaaclab/test/sim/test_urdf_converter.py @@ -24,6 +24,7 @@ import math import os +import shutil import warnings from types import SimpleNamespace @@ -128,6 +129,33 @@ def test_config_change(sim_config, tmp_path): assert os.stat(lazy_urdf_converter.usd_path).st_mtime_ns == new_time_usd_file_created +@pytest.mark.isaacsim_ci +def test_lazy_conversion_converts_again_for_stale_record(sim_config, tmp_path): + """Convert again when the record comes from an earlier version or its generated file is missing. + + Each new output is a numbered folder next to the earlier ones, and later lazy conversions reuse it. + """ + sim, config = sim_config + config.usd_dir = os.path.join(str(tmp_path), "urdf_stale_record") + first_path = UrdfConverter(config).usd_path + + # a record of an earlier version holds only the hash + record_path = os.path.join(config.usd_dir, ".asset_hash") + with open(record_path, encoding="utf-8") as f: + asset_hash = f.readline().strip() + with open(record_path, "w", encoding="utf-8") as f: + f.write(asset_hash) + second_path = UrdfConverter(config).usd_path + assert second_path != first_path + assert UrdfConverter(config).usd_path == second_path + + # a missing generated file converts again, into a folder next to the first one + shutil.rmtree(os.path.dirname(second_path)) + converter = UrdfConverter(config) + assert os.path.isfile(converter.usd_path) + assert os.path.dirname(os.path.dirname(converter.usd_path)) == converter.usd_dir + + @pytest.mark.isaacsim_ci def test_create_prim_from_usd(sim_config): """Call conversion and create a prim from it.""" From 5b13b6b98187a698bd7937c18790f1168e288511 Mon Sep 17 00:00:00 2001 From: Zeng Qingcheng <60593302+NeoZng@users.noreply.github.com> Date: Wed, 30 Sep 2026 01:15:17 +0800 Subject: [PATCH 3/7] Check the output path instead of file times in test_config_change The first and second conversions write different files, and on a file system with 1-second modification times both can share the same time. The importer writes the second conversion to a new folder, so compare the paths. --- source/isaaclab/test/sim/test_mjcf_converter.py | 4 ++-- source/isaaclab/test/sim/test_urdf_converter.py | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/source/isaaclab/test/sim/test_mjcf_converter.py b/source/isaaclab/test/sim/test_mjcf_converter.py index ee2eaca6584a..158bd94fa5a6 100644 --- a/source/isaaclab/test/sim/test_mjcf_converter.py +++ b/source/isaaclab/test/sim/test_mjcf_converter.py @@ -113,7 +113,6 @@ def test_config_change(test_setup_teardown): sim, mjcf_config = test_setup_teardown mjcf_converter = MjcfConverter(mjcf_config) - time_usd_file_created = os.stat(mjcf_converter.usd_path).st_mtime_ns # change the config new_config = mjcf_config @@ -124,7 +123,8 @@ def test_config_change(test_setup_teardown): new_mjcf_converter = MjcfConverter(new_config) new_time_usd_file_created = os.stat(new_mjcf_converter.usd_path).st_mtime_ns - assert time_usd_file_created != new_time_usd_file_created + # the importer writes the new conversion next to the first one; file times may share a second + assert new_mjcf_converter.usd_path != mjcf_converter.usd_path # convert again with the changed config, which must not generate a new USD file lazy_mjcf_converter = MjcfConverter(new_config) diff --git a/source/isaaclab/test/sim/test_urdf_converter.py b/source/isaaclab/test/sim/test_urdf_converter.py index 28f2b2890318..4ba3bd4988e1 100644 --- a/source/isaaclab/test/sim/test_urdf_converter.py +++ b/source/isaaclab/test/sim/test_urdf_converter.py @@ -110,7 +110,6 @@ def test_config_change(sim_config, tmp_path): config.usd_dir = output_dir urdf_converter = UrdfConverter(config) - time_usd_file_created = os.stat(urdf_converter.usd_path).st_mtime_ns # change the config new_config = config @@ -121,7 +120,8 @@ def test_config_change(sim_config, tmp_path): new_urdf_converter = UrdfConverter(new_config) new_time_usd_file_created = os.stat(new_urdf_converter.usd_path).st_mtime_ns - assert time_usd_file_created != new_time_usd_file_created + # the importer writes the new conversion next to the first one; file times may share a second + assert new_urdf_converter.usd_path != urdf_converter.usd_path # convert again with the changed config, which must not generate a new USD file lazy_urdf_converter = UrdfConverter(new_config) From 7c9a28d929bf3bcd737e5095beaa7ec755c0ef4b Mon Sep 17 00:00:00 2001 From: Zeng Qingcheng <60593302+NeoZng@users.noreply.github.com> Date: Wed, 30 Sep 2026 02:32:29 +0800 Subject: [PATCH 4/7] Test the requested-file check and clarify forced conversions Check that a lazy conversion converts again when the record matches the hash but names another requested file, and that a missing generated file is not replaced by the first output. Note in the how-to that the conversion scripts force the conversion, so later lazy conversions do not reuse their output, and that only URDF and MJCF reconvert into a new numbered folder. --- docs/source/how-to/import_new_asset.rst | 10 ++++++---- .../changelog.d/fix-converter-lazy-cache.rst | 3 ++- source/isaaclab/test/sim/test_urdf_converter.py | 14 ++++++++++++++ 3 files changed, 22 insertions(+), 5 deletions(-) diff --git a/docs/source/how-to/import_new_asset.rst b/docs/source/how-to/import_new_asset.rst index 60861913223e..ec8c1faa3c61 100644 --- a/docs/source/how-to/import_new_asset.rst +++ b/docs/source/how-to/import_new_asset.rst @@ -270,8 +270,9 @@ is derived automatically from the robot name in the URDF): overwriting the previous output. :attr:`~sim.converters.UrdfConverter.usd_path` reflects whichever folder the importer actually used, and later lazy conversions with the same configuration load that folder again until another conversion, of any asset or - configuration, is written into the same ``usd_dir``. Delete stale subdirectories manually - (or wipe ``usd_dir``) if you do not want them to accumulate on disk. + configuration, is written into the same ``usd_dir``. Because this script forces the + conversion, later lazy conversions convert again rather than reuse its output. Delete stale + subdirectories manually (or wipe ``usd_dir``) if you do not want them to accumulate on disk. .. _import-new-asset-preview: @@ -426,8 +427,9 @@ Executing the above script will create the USD file inside the the previous output. :attr:`~sim.converters.MjcfConverter.usd_path` reflects whichever folder the importer actually used, and later lazy conversions with the same configuration load that folder again until another conversion, of any asset or configuration, is written - into the same ``usd_dir``. Delete stale subdirectories manually (or wipe ``usd_dir``) if you - do not want them to accumulate on disk. + into the same ``usd_dir``. Because this script forces the conversion, later lazy conversions + convert again rather than reuse its output. Delete stale subdirectories manually (or wipe + ``usd_dir``) if you do not want them to accumulate on disk. See :ref:`Previewing converted assets ` for the ``--viz`` options. diff --git a/source/isaaclab/changelog.d/fix-converter-lazy-cache.rst b/source/isaaclab/changelog.d/fix-converter-lazy-cache.rst index 1817bfaf7a51..eea62f28e2ad 100644 --- a/source/isaaclab/changelog.d/fix-converter-lazy-cache.rst +++ b/source/isaaclab/changelog.d/fix-converter-lazy-cache.rst @@ -6,5 +6,6 @@ Fixed configuration changed in a reused ``usd_dir``. The Isaac Sim importers write each new conversion to a numbered folder such as ``_1`` instead of overwriting ````, so the converters recorded the requested and the generated USD file next to the asset hash and loaded the generated one. Records written by earlier versions do - not name these files, so each existing ``usd_dir`` was converted once more, into a new numbered folder. A + not name these files, so each existing ``usd_dir`` was converted once more, by URDF and MJCF into a new numbered + folder. A read-only ``usd_dir`` with such a record has to be converted again where it is writable. diff --git a/source/isaaclab/test/sim/test_urdf_converter.py b/source/isaaclab/test/sim/test_urdf_converter.py index 4ba3bd4988e1..0ebe963279d9 100644 --- a/source/isaaclab/test/sim/test_urdf_converter.py +++ b/source/isaaclab/test/sim/test_urdf_converter.py @@ -153,9 +153,23 @@ def test_lazy_conversion_converts_again_for_stale_record(sim_config, tmp_path): shutil.rmtree(os.path.dirname(second_path)) converter = UrdfConverter(config) assert os.path.isfile(converter.usd_path) + assert converter.usd_path != first_path assert os.path.dirname(os.path.dirname(converter.usd_path)) == converter.usd_dir +@pytest.mark.isaacsim_ci +def test_lazy_conversion_converts_again_for_another_requested_file(sim_config, tmp_path): + """Convert again when the record matches the hash but names another requested USD file.""" + sim, config = sim_config + config.usd_dir = os.path.join(str(tmp_path), "urdf_requested_file") + # the same bytes under another file name hash the same but request another USD file + for name in ("robot_a", "robot_b"): + config.asset_path = os.path.join(str(tmp_path), f"{name}.urdf") + shutil.copy(_MERGE_JOINTS_URDF, config.asset_path) + usd_path = UrdfConverter(config).usd_path + assert os.path.relpath(usd_path, config.usd_dir) == os.path.join("robot_b", "robot_b.usda") + + @pytest.mark.isaacsim_ci def test_create_prim_from_usd(sim_config): """Call conversion and create a prim from it.""" From ec7f31e9896ec5d4345eed019df8bd2c565a2f00 Mon Sep 17 00:00:00 2001 From: Zeng Qingcheng <60593302+NeoZng@users.noreply.github.com> Date: Thu, 1 Oct 2026 18:39:36 +0800 Subject: [PATCH 5/7] Write the changelog fragment in the towncrier format --- ...er-lazy-cache.rst => fix-converter-lazy-cache.fixed.rst} | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) rename source/isaaclab/changelog.d/{fix-converter-lazy-cache.rst => fix-converter-lazy-cache.fixed.rst} (84%) diff --git a/source/isaaclab/changelog.d/fix-converter-lazy-cache.rst b/source/isaaclab/changelog.d/fix-converter-lazy-cache.fixed.rst similarity index 84% rename from source/isaaclab/changelog.d/fix-converter-lazy-cache.rst rename to source/isaaclab/changelog.d/fix-converter-lazy-cache.fixed.rst index eea62f28e2ad..d5009c0556f2 100644 --- a/source/isaaclab/changelog.d/fix-converter-lazy-cache.rst +++ b/source/isaaclab/changelog.d/fix-converter-lazy-cache.fixed.rst @@ -1,11 +1,7 @@ -Fixed -^^^^^ - * Fixed lazy conversion in :class:`~isaaclab.sim.converters.MjcfConverter` and :class:`~isaaclab.sim.converters.UrdfConverter` loading the first output again after the asset file or the configuration changed in a reused ``usd_dir``. The Isaac Sim importers write each new conversion to a numbered folder such as ``_1`` instead of overwriting ````, so the converters recorded the requested and the generated USD file next to the asset hash and loaded the generated one. Records written by earlier versions do not name these files, so each existing ``usd_dir`` was converted once more, by URDF and MJCF into a new numbered - folder. A - read-only ``usd_dir`` with such a record has to be converted again where it is writable. + folder. A read-only ``usd_dir`` with such a record has to be converted again where it is writable. From 0241117e76d9f1a8067afc22529abe5abd136fbe Mon Sep 17 00:00:00 2001 From: Zeng Qingcheng <60593302+NeoZng@users.noreply.github.com> Date: Tue, 6 Oct 2026 19:28:04 +0800 Subject: [PATCH 6/7] Reuse any recorded output of the same asset and configuration Record every conversion in .asset_hash, a JSON document with one entry per conversion that holds the hash of the asset file and configuration, the requested USD file and the generated one. A lazy conversion reuses the generated file of a matching entry while it exists, so switching from configuration A to B and back to A reuses the first output. A new entry replaces entries whose file is gone, the entry of the same hash and requested file, and the entries in the folder it wrote to, since outputs in one folder can share files such as the mesh converter's Props/instanceable_meshes.usd. The record is replaced in one step. Leave force_usd_conversion out of the hash, so lazy conversions reuse a forced output. Adopt a record of an earlier version when it matches the hash that version recorded for a lazy conversion, the requested file exists and no numbered folder sits next to it. --- docs/source/how-to/import_new_asset.rst | 29 ++- .../fix-converter-lazy-cache.changed.rst | 3 + .../fix-converter-lazy-cache.fixed.rst | 9 +- .../sim/converters/asset_converter_base.py | 190 +++++++++++++++--- .../converters/asset_converter_base_cfg.py | 4 +- .../isaaclab/test/sim/test_mesh_converter.py | 28 ++- .../isaaclab/test/sim/test_mjcf_converter.py | 12 +- .../isaaclab/test/sim/test_urdf_converter.py | 174 ++++++++++++++-- 8 files changed, 378 insertions(+), 71 deletions(-) create mode 100644 source/isaaclab/changelog.d/fix-converter-lazy-cache.changed.rst diff --git a/docs/source/how-to/import_new_asset.rst b/docs/source/how-to/import_new_asset.rst index ec8c1faa3c61..bfeb18c13132 100644 --- a/docs/source/how-to/import_new_asset.rst +++ b/docs/source/how-to/import_new_asset.rst @@ -264,15 +264,13 @@ is derived automatically from the robot name in the URDF): .. note:: The URDF importer auto-deduplicates the per-robot subdirectory when it already exists. - If you re-run the converter against the same ``usd_dir`` with a changed configuration - (for example, flipping ``fix_base``), or re-run this script, which always converts, the - importer writes to a new numbered folder (``anymal_1/``, ``anymal_2/``, …) rather than - overwriting the previous output. :attr:`~sim.converters.UrdfConverter.usd_path` reflects - whichever folder the importer actually used, and later lazy conversions with the same - configuration load that folder again until another conversion, of any asset or - configuration, is written into the same ``usd_dir``. Because this script forces the - conversion, later lazy conversions convert again rather than reuse its output. Delete stale - subdirectories manually (or wipe ``usd_dir``) if you do not want them to accumulate on disk. + A lazy conversion reuses any earlier output of the same asset and configuration in + ``usd_dir``; a new configuration (for example, flipping ``fix_base``), or a conversion + that this script forces, is written to a new numbered folder (``anymal_1/``, + ``anymal_2/``, …) rather than overwriting the previous output. + :attr:`~sim.converters.UrdfConverter.usd_path` reflects whichever folder holds the + conversion. Delete stale subdirectories manually (or wipe ``usd_dir``) if you do not want + them to accumulate on disk. .. _import-new-asset-preview: @@ -421,14 +419,11 @@ Executing the above script will create the USD file inside the .. note:: The MJCF importer auto-deduplicates the per-robot subdirectory when it already exists, - matching the URDF importer's behavior. If you re-run the converter against the same - ``usd_dir`` with a changed configuration, or re-run this script, which always converts, the - importer writes to a new numbered folder (``h1_1/``, ``h1_2/``, …) rather than overwriting - the previous output. :attr:`~sim.converters.MjcfConverter.usd_path` reflects whichever - folder the importer actually used, and later lazy conversions with the same configuration - load that folder again until another conversion, of any asset or configuration, is written - into the same ``usd_dir``. Because this script forces the conversion, later lazy conversions - convert again rather than reuse its output. Delete stale subdirectories manually (or wipe + matching the URDF importer's behavior. A lazy conversion reuses any earlier output of the + same asset and configuration in ``usd_dir``; a new configuration, or a conversion that this + script forces, is written to a new numbered folder (``h1_1/``, ``h1_2/``, …) rather than + overwriting the previous output. :attr:`~sim.converters.MjcfConverter.usd_path` reflects + whichever folder holds the conversion. Delete stale subdirectories manually (or wipe ``usd_dir``) if you do not want them to accumulate on disk. See :ref:`Previewing converted assets ` for the ``--viz`` options. diff --git a/source/isaaclab/changelog.d/fix-converter-lazy-cache.changed.rst b/source/isaaclab/changelog.d/fix-converter-lazy-cache.changed.rst new file mode 100644 index 000000000000..26ee4ec18968 --- /dev/null +++ b/source/isaaclab/changelog.d/fix-converter-lazy-cache.changed.rst @@ -0,0 +1,3 @@ +* Changed lazy conversion with :class:`~isaaclab.sim.converters.AssetConverterBase` to reuse the output of a + conversion forced by :attr:`~isaaclab.sim.converters.AssetConverterBaseCfg.force_usd_conversion`. The flag + decides whether a conversion runs, not what it produces, so it is no longer part of the recorded hash. diff --git a/source/isaaclab/changelog.d/fix-converter-lazy-cache.fixed.rst b/source/isaaclab/changelog.d/fix-converter-lazy-cache.fixed.rst index d5009c0556f2..efbda14c5212 100644 --- a/source/isaaclab/changelog.d/fix-converter-lazy-cache.fixed.rst +++ b/source/isaaclab/changelog.d/fix-converter-lazy-cache.fixed.rst @@ -1,7 +1,8 @@ * Fixed lazy conversion in :class:`~isaaclab.sim.converters.MjcfConverter` and :class:`~isaaclab.sim.converters.UrdfConverter` loading the first output again after the asset file or the configuration changed in a reused ``usd_dir``. The Isaac Sim importers write each new conversion to a numbered - folder such as ``_1`` instead of overwriting ````, so the converters recorded the requested and the - generated USD file next to the asset hash and loaded the generated one. Records written by earlier versions do - not name these files, so each existing ``usd_dir`` was converted once more, by URDF and MJCF into a new numbered - folder. A read-only ``usd_dir`` with such a record has to be converted again where it is writable. + folder such as ``_1`` instead of overwriting ````, so ``.asset_hash`` now records the output of each + asset and configuration in ``usd_dir``, and switching from configuration A to B and back to A reuses the first + output. A record of an earlier version names only its last conversion. That output is still reused if it came from + a lazy conversion, no numbered folder such as ``_1`` lies next to it, and it is requested before anything else + is converted into ``usd_dir``; other outputs are converted once more. diff --git a/source/isaaclab/isaaclab/sim/converters/asset_converter_base.py b/source/isaaclab/isaaclab/sim/converters/asset_converter_base.py index 63264745f744..6e346287f0dc 100644 --- a/source/isaaclab/isaaclab/sim/converters/asset_converter_base.py +++ b/source/isaaclab/isaaclab/sim/converters/asset_converter_base.py @@ -4,13 +4,16 @@ # SPDX-License-Identifier: BSD-3-Clause import abc +import contextlib import hashlib import json import logging import os import pathlib import random +import re import tempfile +import uuid from datetime import datetime from ...utils import to_dict, validate @@ -29,14 +32,14 @@ class AssetConverterBase(abc.ABC): :meth:`_convert_asset` method to provide the actual conversion. The file conversion is lazy if the output directory (:obj:`AssetConverterBaseCfg.usd_dir`) is provided. - The output directory records its last conversion, and the USD file is re-generated only if: - - * The asset file or the configuration parameters were modified since that conversion. - * The requested USD file name differs from that conversion. - * The USD file that the conversion generated does not exist. + The output directory records its conversions, and a lazy conversion reuses an earlier output of the same asset + file, configuration parameters and requested USD file name, as long as the output exists and no later + conversion wrote to its folder. Otherwise, the asset is converted again. The URDF and MJCF importers do not + overwrite earlier outputs and write a new configuration to a new numbered folder, so each of their outputs stays + reusable. To override this behavior to force conversion, the flag :obj:`AssetConverterBaseCfg.force_usd_conversion` - can be set to True. + can be set to True. Later lazy conversions with the same configuration reuse the forced output. When no output directory is defined, lazy conversion is deactivated and the generated USD file is stored in folder ``/IsaacLab/usd_{date}_{time}_{random}``, where ```` is the system @@ -86,33 +89,41 @@ def __init__(self, cfg: AssetConverterBaseCfg): self._usd_file_name = usd_file_name os.makedirs(self.usd_dir, exist_ok=True) - # the record holds the hash of the asset and config, the requested USD file, and the file that the - # conversion generated, which differs when an importer does not overwrite an earlier output - self._dest_hash_path = os.path.join(self.usd_dir, ".asset_hash") + # the record lists the conversions in the output directory, see _read_record() + self._record_path = os.path.join(self.usd_dir, ".asset_hash") self._asset_hash = self._config_to_hash(cfg) - requested_usd_file_name = self._usd_file_name - try: - with open(self._dest_hash_path, encoding="utf-8") as f: - record = f.read().splitlines() - except FileNotFoundError: - record = [] - self._is_same_asset = len(record) == 3 and record[:2] == [self._asset_hash, requested_usd_file_name] - cached_usd_file_name = record[2] if self._is_same_asset else requested_usd_file_name - self._usd_file_exists = os.path.isfile(os.path.join(self.usd_dir, cached_usd_file_name)) - - # reuse the recorded USD file, or convert the asset if the hash differs or the file does not exist - if self._is_same_asset and self._usd_file_exists and not cfg.force_usd_conversion: - self._usd_file_name = cached_usd_file_name + requested_usd_file_name = pathlib.PurePath(self._usd_file_name).as_posix() + + # reuse an earlier output of the same asset file and configuration if it still exists + cached_usd_file_name = None + if not cfg.force_usd_conversion: + entries, legacy_hash = self._read_record() + cached_usd_file_name = next( + ( + entry["generated"] + for entry in entries + if (entry["hash"], entry["requested"]) == (self._asset_hash, requested_usd_file_name) + and os.path.isfile(os.path.join(self.usd_dir, entry["generated"])) + ), + None, + ) + if cached_usd_file_name is None and self._can_reuse_legacy_record(legacy_hash, requested_usd_file_name): + cached_usd_file_name = requested_usd_file_name + # a read-only output directory keeps the record of the earlier version + with contextlib.suppress(OSError): + self._write_record(requested_usd_file_name, requested_usd_file_name) + + if cached_usd_file_name is not None: + self._usd_file_name = str(pathlib.PurePath(cached_usd_file_name)) else: # convert the asset to USD self._convert_asset(cfg) # importers put the physics payloads behind a "Physics" variant set and disagree on # which variant to select, so settle it here self._select_physics_variant(cfg.physics_variant) - # record the hash only now: writing it earlier would let a conversion that raised + # record the conversion only now: recording it earlier would let a conversion that raised # still count as cached, so an identical retry would skip it and return the asset - with open(self._dest_hash_path, "w", encoding="utf-8") as f: - f.write(f"{self._asset_hash}\n{requested_usd_file_name}\n{self._usd_file_name}") + self._write_record(requested_usd_file_name, pathlib.PurePath(self._usd_file_name).as_posix()) # dump the configuration next to the asset, stamped with the converter that produced it config_path = os.path.join(self.usd_dir, "config.yaml") dump_yaml(config_path, to_dict(cfg)) @@ -204,15 +215,135 @@ def _select_physics_variant(self, variant: str): variant_set.SetVariantSelection(variant) stage.GetRootLayer().Save() + def _read_record(self) -> tuple[list[dict[str, str]], str | None]: + """Read the record of the conversions in the output directory. + + The record is a JSON document with one entry per conversion. An entry holds the hash of the asset file + and configuration (see :meth:`_config_to_hash`), the requested USD file, and the USD file that the + conversion generated, both relative to the output directory with ``/`` separators. Entries that are + malformed or point outside the output directory are ignored. Earlier versions wrote only the hash of + the last conversion, on the first line. + + Returns: + The valid entries, and the hash of a record written by an earlier version or None. + """ + try: + with open(self._record_path, encoding="utf-8") as f: + content = f.read() + except (OSError, UnicodeDecodeError): + return [], None + try: + record = json.loads(content) + except ValueError: + record = None + # a hash of an earlier version can also parse as a JSON number + if not isinstance(record, dict) or record.get("version") != 1: + lines = content.splitlines() + return [], lines[0].strip() if lines else None + recorded_entries = record.get("entries") + if not isinstance(recorded_entries, list): + recorded_entries = [] + keys = ("hash", "requested", "generated") + entries = [] + for entry in recorded_entries: + if not isinstance(entry, dict) or not all(isinstance(entry.get(key), str) for key in keys): + continue + generated = pathlib.PurePath(entry["generated"]) + # a record edited by hand must not point outside the output directory + if generated.anchor or ".." in generated.parts: + continue + entries.append({key: entry[key] for key in keys}) + return entries, None + + def _can_reuse_legacy_record(self, legacy_hash: str | None, requested_usd_file_name: str) -> bool: + """Check whether a record of an earlier version can be reused for the requested USD file. + + Earlier versions recorded only the hash of the last conversion, not its USD file, and loaded the requested + USD file when the hash matched. The record is not reused while a numbered folder, such as ``_1`` next + to ````, shows that a later conversion went elsewhere. As in earlier versions, the requested file may + still hold an earlier conversion if such a folder was deleted, or if the last conversion had this hash but + wrote another requested file. + + Args: + legacy_hash: The hash of a record written by an earlier version, or None. + requested_usd_file_name: The requested USD file, relative to the output directory with ``/`` + separators. + + Returns: + True if the requested USD file exists, the record holds the hash of this asset file and configuration, + and nothing shows that the requested file holds another conversion. + """ + if legacy_hash is None or not os.path.isfile(os.path.join(self.usd_dir, requested_usd_file_name)): + return False + folder = pathlib.PurePosixPath(requested_usd_file_name).parent + if folder.name: + parent_dir = os.path.join(self.usd_dir, folder.parent) + numbered_folder = re.compile(rf"{re.escape(folder.name)}_\d+") + try: + names = os.listdir(parent_dir) + except OSError: + # without a listing, a numbered folder cannot be ruled out + return False + for name in names: + if numbered_folder.fullmatch(name) and os.path.isdir(os.path.join(parent_dir, name)): + return False + # only a lazy conversion could have recorded a hash that a lazy conversion matches + return legacy_hash == self._config_to_hash(self.cfg, legacy=True) + + def _write_record(self, requested_usd_file_name: str, generated_usd_file_name: str) -> None: + """Add a conversion to the record of the output directory. + + The record is read again right before writing, to keep the entries that another process added in the + meantime. Entries whose USD file no longer exists or that this conversion replaces are dropped, and so are + the entries in the folder this conversion wrote to: outputs in one folder can share files, such as the + ``Props/instanceable_meshes.usd`` that every output of the mesh converter references. The URDF and MJCF + importers give each conversion its own folder. The record is replaced in one step, so a crash cannot leave + it half written, but writes are not serialized: two concurrent writers can still lose an entry, or keep one + that the other writer dropped. + + Args: + requested_usd_file_name: The requested USD file, relative to the output directory with ``/`` + separators. + generated_usd_file_name: The generated USD file, relative to the output directory with ``/`` + separators. + """ + entries, _ = self._read_record() + folder = pathlib.PurePosixPath(generated_usd_file_name).parent + entries = [ + entry + for entry in entries + if os.path.isfile(os.path.join(self.usd_dir, entry["generated"])) + and (entry["hash"], entry["requested"]) != (self._asset_hash, requested_usd_file_name) + and pathlib.PurePosixPath(entry["generated"]).parent != folder + ] + entries.append( + {"hash": self._asset_hash, "requested": requested_usd_file_name, "generated": generated_usd_file_name} + ) + # unlike the tempfile module, open() honors the umask, so other users of the directory can read the record + temp_path = f"{self._record_path}.{uuid.uuid4().hex}.tmp" + try: + with open(temp_path, "x", encoding="utf-8") as f: + json.dump({"version": 1, "entries": entries}, f, indent=2) + os.replace(temp_path, self._record_path) + except BaseException: + with contextlib.suppress(OSError): + os.remove(temp_path) + raise + @staticmethod - def _config_to_hash(cfg: AssetConverterBaseCfg) -> str: + def _config_to_hash(cfg: AssetConverterBaseCfg, legacy: bool = False) -> str: """Converts the configuration object and asset file to an MD5 hash of a string. + The hash leaves out :attr:`AssetConverterBaseCfg.force_usd_conversion`, which decides whether a + conversion runs, not what it produces. + .. warning:: It only checks the main asset file (:attr:`cfg.asset_path`). Args: - config : The asset converter configuration object. + cfg: The asset converter configuration object. + legacy: Whether to compute the hash that earlier versions recorded for a lazy conversion, which + included the flag. Returns: An MD5 hash of a string. @@ -223,6 +354,11 @@ def _config_to_hash(cfg: AssetConverterBaseCfg) -> str: _ = config_dic.pop("asset_path") _ = config_dic.pop("usd_dir") _ = config_dic.pop("usd_file_name") + if legacy: + # set the flag in place, which keeps the key order and so the hash of earlier versions + config_dic["force_usd_conversion"] = False + else: + _ = config_dic.pop("force_usd_conversion") # convert config dic to bytes config_bytes = json.dumps(config_dic).encode() # hash config diff --git a/source/isaaclab/isaaclab/sim/converters/asset_converter_base_cfg.py b/source/isaaclab/isaaclab/sim/converters/asset_converter_base_cfg.py index 2c63ba05bc15..5acbc91fa7bf 100644 --- a/source/isaaclab/isaaclab/sim/converters/asset_converter_base_cfg.py +++ b/source/isaaclab/isaaclab/sim/converters/asset_converter_base_cfg.py @@ -53,7 +53,9 @@ class PhysicsVariant(StrEnum): force_usd_conversion: bool = False """Force the conversion of the asset file to usd. Defaults to False. - If True, then the USD file is always generated. It will overwrite the existing USD file if it exists. + If True, then the USD file is always generated, and later lazy conversions with the same configuration reuse + it. Importers that do not overwrite earlier outputs, such as the URDF and MJCF importers, write it to a new + numbered folder. """ make_instanceable: bool = True diff --git a/source/isaaclab/test/sim/test_mesh_converter.py b/source/isaaclab/test/sim/test_mesh_converter.py index fb0d2bf8a428..36122008f31a 100644 --- a/source/isaaclab/test/sim/test_mesh_converter.py +++ b/source/isaaclab/test/sim/test_mesh_converter.py @@ -176,7 +176,10 @@ def test_no_change(assets): def test_config_change(assets): - """Call conversion twice but change the config in the second call. This should generate a new USD file.""" + """Call conversion twice but change the config in the second call. This should generate a new USD file. + + Changing the config back must generate the USD file again, since the second call overwrote it. + """ # create an initial USD file from asset mesh_config = MeshConverterCfg(asset_path=assets["obj"]) mesh_converter = MeshConverter(mesh_config) @@ -193,6 +196,29 @@ def test_config_change(assets): assert time_usd_file_created != new_time_usd_file_created + # change the config back, which must generate the USD file again + new_config.make_instanceable = not new_config.make_instanceable + first_mesh_converter = MeshConverter(new_config) + assert os.stat(first_mesh_converter.usd_path).st_mtime_ns != new_time_usd_file_created + + +def test_lazy_conversion_converts_again_after_another_output(assets, tmp_path): + """Convert again after another output was converted into the same directory. + + Every output in the directory references the same ``Props/instanceable_meshes.usd``, which the other conversion + replaced. + """ + usd_dir = str(tmp_path / "meshes") + mesh_config = MeshConverterCfg(asset_path=assets["obj"], usd_dir=usd_dir, usd_file_name="first.usd") + MeshConverter(mesh_config) + MeshConverter( + MeshConverterCfg( + asset_path=assets["obj"], usd_dir=usd_dir, usd_file_name="second.usd", translation=(1.0, 0.0, 0.0) + ) + ) + + check_mesh_conversion(MeshConverter(mesh_config)) + def test_convert_obj(assets, tmp_path): """Convert an OBJ file whose name has extra dots; the prim name is made a valid identifier.""" diff --git a/source/isaaclab/test/sim/test_mjcf_converter.py b/source/isaaclab/test/sim/test_mjcf_converter.py index 158bd94fa5a6..f609d566e160 100644 --- a/source/isaaclab/test/sim/test_mjcf_converter.py +++ b/source/isaaclab/test/sim/test_mjcf_converter.py @@ -107,12 +107,13 @@ def test_no_change(test_setup_teardown): def test_config_change(test_setup_teardown): """Call conversion twice but change the config in the second call. This should generate a new USD file. - A third call with the changed config must load that file, although the importer writes it next to the - first one instead of overwriting it. + The importer writes the new conversion next to the first one instead of overwriting it, so later calls with + either config must load their own file without generating another one. """ sim, mjcf_config = test_setup_teardown mjcf_converter = MjcfConverter(mjcf_config) + time_usd_file_created = os.stat(mjcf_converter.usd_path).st_mtime_ns # change the config new_config = mjcf_config @@ -131,6 +132,13 @@ def test_config_change(test_setup_teardown): assert lazy_mjcf_converter.usd_path == new_mjcf_converter.usd_path assert os.stat(lazy_mjcf_converter.usd_path).st_mtime_ns == new_time_usd_file_created + # change the config back, which must load the first USD file instead of generating a third one + new_config.self_collision = not new_config.self_collision + first_mjcf_converter = MjcfConverter(new_config) + assert first_mjcf_converter.usd_path == mjcf_converter.usd_path + assert os.stat(first_mjcf_converter.usd_path).st_mtime_ns == time_usd_file_created + assert not os.path.exists(f"{os.path.dirname(mjcf_converter.usd_path)}_2") + def test_create_prim_from_usd(test_setup_teardown): """Call conversion and create a prim from it.""" diff --git a/source/isaaclab/test/sim/test_urdf_converter.py b/source/isaaclab/test/sim/test_urdf_converter.py index 0ebe963279d9..f61c15eceb87 100644 --- a/source/isaaclab/test/sim/test_urdf_converter.py +++ b/source/isaaclab/test/sim/test_urdf_converter.py @@ -22,8 +22,11 @@ allow_module_level=True, ) +import hashlib +import json import math import os +import pathlib import shutil import warnings from types import SimpleNamespace @@ -34,6 +37,7 @@ import isaaclab.sim as sim_utils from isaaclab.sim import SimulationCfg, SimulationContext from isaaclab.sim.converters import UrdfConverter, UrdfConverterCfg +from isaaclab.utils import to_dict # The Kit-less container mounts the checkout read-only, so ``usd_dir`` goes under ``tmp_path``. pytestmark = [pytest.mark.integration, pytest.mark.kitless] @@ -100,8 +104,8 @@ def test_no_change(sim_config): def test_config_change(sim_config, tmp_path): """Call conversion twice but change the config in the second call. This should generate a new USD file. - A third call with the changed config must load that file, although the importer writes it next to the - first one instead of overwriting it. + The importer writes the new conversion next to the first one instead of overwriting it, so later calls with + either config must load their own file without generating another one. """ sim, config = sim_config @@ -110,6 +114,7 @@ def test_config_change(sim_config, tmp_path): config.usd_dir = output_dir urdf_converter = UrdfConverter(config) + time_usd_file_created = os.stat(urdf_converter.usd_path).st_mtime_ns # change the config new_config = config @@ -128,33 +133,164 @@ def test_config_change(sim_config, tmp_path): assert lazy_urdf_converter.usd_path == new_urdf_converter.usd_path assert os.stat(lazy_urdf_converter.usd_path).st_mtime_ns == new_time_usd_file_created + # change the config back, which must load the first USD file instead of generating a third one + new_config.fix_base = not new_config.fix_base + first_urdf_converter = UrdfConverter(new_config) + assert first_urdf_converter.usd_path == urdf_converter.usd_path + assert os.stat(first_urdf_converter.usd_path).st_mtime_ns == time_usd_file_created + assert not os.path.exists(f"{os.path.dirname(urdf_converter.usd_path)}_2") + + +def _write_legacy_record(config: UrdfConverterCfg): + """Replace the record in ``config.usd_dir`` with the one that earlier versions wrote for a lazy conversion. + + It held only the MD5 of the configuration without its paths, followed by the asset file. + """ + config_dict = to_dict(config) + for key in ("asset_path", "usd_dir", "usd_file_name"): + config_dict.pop(key) + md5 = hashlib.md5(json.dumps(config_dict).encode()) + with open(config.asset_path, "rb") as f: + md5.update(f.read()) + with open(os.path.join(config.usd_dir, ".asset_hash"), "w", encoding="utf-8") as f: + f.write(md5.hexdigest()) + + +def _recorded_conversions(usd_dir: str) -> list[tuple[str, str]]: + """Return the requested and the generated USD file of each conversion that ``usd_dir`` records.""" + with open(os.path.join(usd_dir, ".asset_hash"), encoding="utf-8") as f: + record = json.load(f) + assert record["version"] == 1 + return [(entry["requested"], entry["generated"]) for entry in record["entries"]] + + +@pytest.mark.isaacsim_ci +def test_lazy_conversion_reuses_forced_conversion(sim_config, tmp_path): + """Load the output of a forced conversion for later lazy conversions with the same config.""" + sim, config = sim_config + config.usd_dir = os.path.join(str(tmp_path), "urdf_forced_conversion") + first_path = UrdfConverter(config).usd_path + config.force_usd_conversion = True + usd_path = UrdfConverter(config).usd_path + time_usd_file_created = os.stat(usd_path).st_mtime_ns + # the importer writes the forced conversion next to the first one + assert usd_path != first_path + + config.force_usd_conversion = False + assert UrdfConverter(config).usd_path == usd_path + assert os.stat(usd_path).st_mtime_ns == time_usd_file_created + @pytest.mark.isaacsim_ci -def test_lazy_conversion_converts_again_for_stale_record(sim_config, tmp_path): - """Convert again when the record comes from an earlier version or its generated file is missing. +def test_lazy_conversion_reuses_legacy_record(sim_config, tmp_path): + """Load the requested USD file for a matching record of an earlier version, and migrate the record. - Each new output is a numbered folder next to the earlier ones, and later lazy conversions reuse it. + Without a numbered folder next to the requested one, the importer wrote only one conversion to ``usd_dir``. """ sim, config = sim_config - config.usd_dir = os.path.join(str(tmp_path), "urdf_stale_record") + config.usd_dir = os.path.join(str(tmp_path), "urdf_legacy_record") + usd_path = UrdfConverter(config).usd_path + time_usd_file_created = os.stat(usd_path).st_mtime_ns + _write_legacy_record(config) + + # the second call loads the file through the migrated record + for _ in range(2): + assert UrdfConverter(config).usd_path == usd_path + assert os.stat(usd_path).st_mtime_ns == time_usd_file_created + requested = pathlib.PurePath(os.path.relpath(usd_path, config.usd_dir)).as_posix() + assert _recorded_conversions(config.usd_dir) == [(requested, requested)] + + +@pytest.mark.isaacsim_ci +def test_lazy_conversion_converts_again_for_legacy_record_with_numbered_folder(sim_config, tmp_path): + """Convert again for a record of an earlier version when a numbered folder lies next to the requested one. + + Earlier versions recorded only the last conversion, here the one the importer wrote to the numbered folder, + while the requested USD file holds the first conversion. + """ + sim, config = sim_config + config.usd_dir = os.path.join(str(tmp_path), "urdf_legacy_record_numbered_folder") + first_path = UrdfConverter(config).usd_path + config.fix_base = not config.fix_base + UrdfConverter(config) + _write_legacy_record(config) + + assert UrdfConverter(config).usd_path != first_path + + +@pytest.mark.isaacsim_ci +@pytest.mark.skipif(os.name == "nt" or os.geteuid() == 0, reason="Needs file permissions that bind the user.") +def test_lazy_conversion_reuses_legacy_record_in_read_only_dir(sim_config, tmp_path): + """Load the requested USD file for a matching record of an earlier version that cannot be migrated.""" + sim, config = sim_config + config.usd_dir = os.path.join(str(tmp_path), "urdf_legacy_record_read_only") + usd_path = UrdfConverter(config).usd_path + _write_legacy_record(config) + os.chmod(config.usd_dir, 0o555) + try: + assert UrdfConverter(config).usd_path == usd_path + finally: + # let pytest remove the directory + os.chmod(config.usd_dir, 0o755) + + +@pytest.mark.isaacsim_ci +@pytest.mark.parametrize("legacy", [False, True], ids=["record", "legacy_record"]) +def test_lazy_conversion_converts_again_for_deleted_output(sim_config, tmp_path, legacy): + """Convert again when the recorded USD file was deleted, also for a record of an earlier version, and drop + deleted files from the record.""" + sim, config = sim_config + config.usd_dir = os.path.join(str(tmp_path), "urdf_deleted_output") + first_path = UrdfConverter(config).usd_path + config.fix_base = not config.fix_base + second_path = UrdfConverter(config).usd_path + for deleted_path in (first_path, second_path): + shutil.rmtree(os.path.dirname(deleted_path)) + + config.fix_base = not config.fix_base + if legacy: + _write_legacy_record(config) + usd_path = UrdfConverter(config).usd_path + assert os.path.isfile(usd_path) + generated = pathlib.PurePath(os.path.relpath(usd_path, config.usd_dir)).as_posix() + assert [conversion[1] for conversion in _recorded_conversions(config.usd_dir)] == [generated] + + +@pytest.mark.isaacsim_ci +def test_lazy_conversion_converts_again_for_corrupt_record(sim_config, tmp_path): + """Convert again when the record cannot be parsed, such as one cut short.""" + sim, config = sim_config + config.usd_dir = os.path.join(str(tmp_path), "urdf_corrupt_record") first_path = UrdfConverter(config).usd_path + record_path = os.path.join(config.usd_dir, ".asset_hash") + with open(record_path, encoding="utf-8") as f: + record = f.read() + with open(record_path, "w", encoding="utf-8") as f: + f.write(record[: len(record) // 2]) + + assert UrdfConverter(config).usd_path != first_path + - # a record of an earlier version holds only the hash +@pytest.mark.isaacsim_ci +@pytest.mark.parametrize("outside", ["absolute", "parent"]) +def test_lazy_conversion_ignores_entry_outside_usd_dir(sim_config, tmp_path, outside): + """Convert again when a hand-edited record names the generated USD file by a path outside ``usd_dir``.""" + sim, config = sim_config + config.usd_dir = os.path.join(str(tmp_path), "urdf_entry_outside") + first_path = UrdfConverter(config).usd_path record_path = os.path.join(config.usd_dir, ".asset_hash") with open(record_path, encoding="utf-8") as f: - asset_hash = f.readline().strip() + record = json.load(f) + # name the same USD file by an absolute path, or by one that leaves usd_dir and enters it again + entry = record["entries"][0] + if outside == "absolute": + entry["generated"] = pathlib.PurePath(first_path).as_posix() + else: + entry["generated"] = f"../{os.path.basename(config.usd_dir)}/{entry['generated']}" with open(record_path, "w", encoding="utf-8") as f: - f.write(asset_hash) - second_path = UrdfConverter(config).usd_path - assert second_path != first_path - assert UrdfConverter(config).usd_path == second_path - - # a missing generated file converts again, into a folder next to the first one - shutil.rmtree(os.path.dirname(second_path)) - converter = UrdfConverter(config) - assert os.path.isfile(converter.usd_path) - assert converter.usd_path != first_path - assert os.path.dirname(os.path.dirname(converter.usd_path)) == converter.usd_dir + json.dump(record, f) + + assert not os.path.samefile(UrdfConverter(config).usd_path, first_path) @pytest.mark.isaacsim_ci From 741ba87c2396b8a8714eb8cc64ec3d00d8300bc6 Mon Sep 17 00:00:00 2001 From: Octi Zhang Date: Mon, 12 Oct 2026 16:46:15 -0700 Subject: [PATCH 7/7] Simplify lazy converter cache to the generated output --- docs/source/how-to/import_new_asset.rst | 25 +-- .../fix-converter-lazy-cache.changed.rst | 3 - .../fix-converter-lazy-cache.fixed.rst | 11 +- .../sim/converters/asset_converter_base.py | 209 ++++-------------- .../converters/asset_converter_base_cfg.py | 5 +- .../isaaclab/test/sim/test_mesh_converter.py | 28 +-- .../isaaclab/test/sim/test_mjcf_converter.py | 17 +- .../isaaclab/test/sim/test_urdf_converter.py | 188 +--------------- 8 files changed, 69 insertions(+), 417 deletions(-) delete mode 100644 source/isaaclab/changelog.d/fix-converter-lazy-cache.changed.rst diff --git a/docs/source/how-to/import_new_asset.rst b/docs/source/how-to/import_new_asset.rst index bfeb18c13132..04e7e8f12d88 100644 --- a/docs/source/how-to/import_new_asset.rst +++ b/docs/source/how-to/import_new_asset.rst @@ -264,13 +264,12 @@ is derived automatically from the robot name in the URDF): .. note:: The URDF importer auto-deduplicates the per-robot subdirectory when it already exists. - A lazy conversion reuses any earlier output of the same asset and configuration in - ``usd_dir``; a new configuration (for example, flipping ``fix_base``), or a conversion - that this script forces, is written to a new numbered folder (``anymal_1/``, - ``anymal_2/``, …) rather than overwriting the previous output. - :attr:`~sim.converters.UrdfConverter.usd_path` reflects whichever folder holds the - conversion. Delete stale subdirectories manually (or wipe ``usd_dir``) if you do not want - them to accumulate on disk. + If you re-run the converter against the same ``usd_dir`` with a changed configuration + (for example, flipping ``fix_base``), the importer writes to a new numbered folder + (``anymal_1/``, ``anymal_2/``, …) rather than overwriting the previous output. + :attr:`~sim.converters.UrdfConverter.usd_path` reflects whichever folder the importer + actually used. Delete stale subdirectories manually (or wipe ``usd_dir``) if you do not + want them to accumulate on disk. .. _import-new-asset-preview: @@ -419,12 +418,12 @@ Executing the above script will create the USD file inside the .. note:: The MJCF importer auto-deduplicates the per-robot subdirectory when it already exists, - matching the URDF importer's behavior. A lazy conversion reuses any earlier output of the - same asset and configuration in ``usd_dir``; a new configuration, or a conversion that this - script forces, is written to a new numbered folder (``h1_1/``, ``h1_2/``, …) rather than - overwriting the previous output. :attr:`~sim.converters.MjcfConverter.usd_path` reflects - whichever folder holds the conversion. Delete stale subdirectories manually (or wipe - ``usd_dir``) if you do not want them to accumulate on disk. + matching the URDF importer's behavior. If you re-run the converter against the same + ``usd_dir`` with a changed configuration, the importer writes to a new numbered folder + (``h1_1/``, ``h1_2/``, …) rather than overwriting the previous output. + :attr:`~sim.converters.MjcfConverter.usd_path` reflects whichever folder the importer + actually used. Delete stale subdirectories manually (or wipe ``usd_dir``) if you do not + want them to accumulate on disk. See :ref:`Previewing converted assets ` for the ``--viz`` options. diff --git a/source/isaaclab/changelog.d/fix-converter-lazy-cache.changed.rst b/source/isaaclab/changelog.d/fix-converter-lazy-cache.changed.rst deleted file mode 100644 index 26ee4ec18968..000000000000 --- a/source/isaaclab/changelog.d/fix-converter-lazy-cache.changed.rst +++ /dev/null @@ -1,3 +0,0 @@ -* Changed lazy conversion with :class:`~isaaclab.sim.converters.AssetConverterBase` to reuse the output of a - conversion forced by :attr:`~isaaclab.sim.converters.AssetConverterBaseCfg.force_usd_conversion`. The flag - decides whether a conversion runs, not what it produces, so it is no longer part of the recorded hash. diff --git a/source/isaaclab/changelog.d/fix-converter-lazy-cache.fixed.rst b/source/isaaclab/changelog.d/fix-converter-lazy-cache.fixed.rst index efbda14c5212..3139615d720a 100644 --- a/source/isaaclab/changelog.d/fix-converter-lazy-cache.fixed.rst +++ b/source/isaaclab/changelog.d/fix-converter-lazy-cache.fixed.rst @@ -1,8 +1,3 @@ -* Fixed lazy conversion in :class:`~isaaclab.sim.converters.MjcfConverter` and - :class:`~isaaclab.sim.converters.UrdfConverter` loading the first output again after the asset file or the - configuration changed in a reused ``usd_dir``. The Isaac Sim importers write each new conversion to a numbered - folder such as ``_1`` instead of overwriting ````, so ``.asset_hash`` now records the output of each - asset and configuration in ``usd_dir``, and switching from configuration A to B and back to A reuses the first - output. A record of an earlier version names only its last conversion. That output is still reused if it came from - a lazy conversion, no numbered folder such as ``_1`` lies next to it, and it is requested before anything else - is converted into ``usd_dir``; other outputs are converted once more. +* Fixed lazy MJCF and URDF conversion returning an older USD after the importer wrote a changed asset to a numbered + folder. The cache now records the generated file and reuses only the most recent matching conversion. Old + hash-only cache records cause one conversion because they do not identify the generated file. diff --git a/source/isaaclab/isaaclab/sim/converters/asset_converter_base.py b/source/isaaclab/isaaclab/sim/converters/asset_converter_base.py index 6e346287f0dc..0e4623e73905 100644 --- a/source/isaaclab/isaaclab/sim/converters/asset_converter_base.py +++ b/source/isaaclab/isaaclab/sim/converters/asset_converter_base.py @@ -4,16 +4,13 @@ # SPDX-License-Identifier: BSD-3-Clause import abc -import contextlib import hashlib import json import logging import os import pathlib import random -import re import tempfile -import uuid from datetime import datetime from ...utils import to_dict, validate @@ -32,14 +29,19 @@ class AssetConverterBase(abc.ABC): :meth:`_convert_asset` method to provide the actual conversion. The file conversion is lazy if the output directory (:obj:`AssetConverterBaseCfg.usd_dir`) is provided. - The output directory records its conversions, and a lazy conversion reuses an earlier output of the same asset - file, configuration parameters and requested USD file name, as long as the output exists and no later - conversion wrote to its folder. Otherwise, the asset is converted again. The URDF and MJCF importers do not - overwrite earlier outputs and write a new configuration to a new numbered folder, so each of their outputs stays - reusable. + The most recent conversion in that directory is reused unless: + + * The asset file is modified. + * The configuration parameters are modified. + * The requested USD file name changes. + * The generated USD file does not exist. + + The importer may generate a numbered file instead of the requested one. The cache records the generated path + so a later conversion cannot return an older file at the requested path. Old hash-only records are cache misses + because they do not identify the generated file. To override this behavior to force conversion, the flag :obj:`AssetConverterBaseCfg.force_usd_conversion` - can be set to True. Later lazy conversions with the same configuration reuse the forced output. + can be set to True. When no output directory is defined, lazy conversion is deactivated and the generated USD file is stored in folder ``/IsaacLab/usd_{date}_{time}_{random}``, where ```` is the system @@ -48,8 +50,9 @@ class AssetConverterBase(abc.ABC): triggered conversions try to use the same directory for reading/writing the generated files. .. note:: - Changes to the parameters :obj:`AssetConverterBaseCfg.asset_path` and :obj:`AssetConverterBaseCfg.usd_dir` - are not considered as modifications in the configuration instance that trigger the USD file re-generation. + The configuration hash excludes :obj:`AssetConverterBaseCfg.asset_path`, + :obj:`AssetConverterBaseCfg.usd_dir`, and :obj:`AssetConverterBaseCfg.usd_file_name`. The asset contents are + hashed, and the requested USD file name is checked separately. """ @@ -89,41 +92,40 @@ def __init__(self, cfg: AssetConverterBaseCfg): self._usd_file_name = usd_file_name os.makedirs(self.usd_dir, exist_ok=True) - # the record lists the conversions in the output directory, see _read_record() - self._record_path = os.path.join(self.usd_dir, ".asset_hash") - self._asset_hash = self._config_to_hash(cfg) - requested_usd_file_name = pathlib.PurePath(self._usd_file_name).as_posix() - - # reuse an earlier output of the same asset file and configuration if it still exists - cached_usd_file_name = None - if not cfg.force_usd_conversion: - entries, legacy_hash = self._read_record() - cached_usd_file_name = next( - ( - entry["generated"] - for entry in entries - if (entry["hash"], entry["requested"]) == (self._asset_hash, requested_usd_file_name) - and os.path.isfile(os.path.join(self.usd_dir, entry["generated"])) - ), - None, - ) - if cached_usd_file_name is None and self._can_reuse_legacy_record(legacy_hash, requested_usd_file_name): - cached_usd_file_name = requested_usd_file_name - # a read-only output directory keeps the record of the earlier version - with contextlib.suppress(OSError): - self._write_record(requested_usd_file_name, requested_usd_file_name) - - if cached_usd_file_name is not None: - self._usd_file_name = str(pathlib.PurePath(cached_usd_file_name)) + record_path = os.path.join(self.usd_dir, ".asset_hash") + asset_hash = self._config_to_hash(cfg) + requested = pathlib.PurePath(self._usd_file_name).as_posix() + try: + with open(record_path, encoding="utf-8") as f: + record = json.load(f) + except (FileNotFoundError, ValueError): + record = {} + if not isinstance(record, dict): + record = {} + generated = record.get("generated") + + if ( + not cfg.force_usd_conversion + and record.get("hash") == asset_hash + and record.get("requested") == requested + and isinstance(generated, str) + and not pathlib.PurePath(generated).is_absolute() + and ".." not in pathlib.PurePath(generated).parts + and os.path.isfile(os.path.join(self.usd_dir, generated)) + ): + self._usd_file_name = generated else: # convert the asset to USD self._convert_asset(cfg) # importers put the physics payloads behind a "Physics" variant set and disagree on # which variant to select, so settle it here self._select_physics_variant(cfg.physics_variant) - # record the conversion only now: recording it earlier would let a conversion that raised + # record the generated path only now: writing it earlier would let a conversion that raised # still count as cached, so an identical retry would skip it and return the asset - self._write_record(requested_usd_file_name, pathlib.PurePath(self._usd_file_name).as_posix()) + generated = pathlib.PurePath(self._usd_file_name).as_posix() + record = {"hash": asset_hash, "requested": requested, "generated": generated} + with open(record_path, "w", encoding="utf-8") as f: + json.dump(record, f) # dump the configuration next to the asset, stamped with the converter that produced it config_path = os.path.join(self.usd_dir, "config.yaml") dump_yaml(config_path, to_dict(cfg)) @@ -215,135 +217,15 @@ def _select_physics_variant(self, variant: str): variant_set.SetVariantSelection(variant) stage.GetRootLayer().Save() - def _read_record(self) -> tuple[list[dict[str, str]], str | None]: - """Read the record of the conversions in the output directory. - - The record is a JSON document with one entry per conversion. An entry holds the hash of the asset file - and configuration (see :meth:`_config_to_hash`), the requested USD file, and the USD file that the - conversion generated, both relative to the output directory with ``/`` separators. Entries that are - malformed or point outside the output directory are ignored. Earlier versions wrote only the hash of - the last conversion, on the first line. - - Returns: - The valid entries, and the hash of a record written by an earlier version or None. - """ - try: - with open(self._record_path, encoding="utf-8") as f: - content = f.read() - except (OSError, UnicodeDecodeError): - return [], None - try: - record = json.loads(content) - except ValueError: - record = None - # a hash of an earlier version can also parse as a JSON number - if not isinstance(record, dict) or record.get("version") != 1: - lines = content.splitlines() - return [], lines[0].strip() if lines else None - recorded_entries = record.get("entries") - if not isinstance(recorded_entries, list): - recorded_entries = [] - keys = ("hash", "requested", "generated") - entries = [] - for entry in recorded_entries: - if not isinstance(entry, dict) or not all(isinstance(entry.get(key), str) for key in keys): - continue - generated = pathlib.PurePath(entry["generated"]) - # a record edited by hand must not point outside the output directory - if generated.anchor or ".." in generated.parts: - continue - entries.append({key: entry[key] for key in keys}) - return entries, None - - def _can_reuse_legacy_record(self, legacy_hash: str | None, requested_usd_file_name: str) -> bool: - """Check whether a record of an earlier version can be reused for the requested USD file. - - Earlier versions recorded only the hash of the last conversion, not its USD file, and loaded the requested - USD file when the hash matched. The record is not reused while a numbered folder, such as ``_1`` next - to ````, shows that a later conversion went elsewhere. As in earlier versions, the requested file may - still hold an earlier conversion if such a folder was deleted, or if the last conversion had this hash but - wrote another requested file. - - Args: - legacy_hash: The hash of a record written by an earlier version, or None. - requested_usd_file_name: The requested USD file, relative to the output directory with ``/`` - separators. - - Returns: - True if the requested USD file exists, the record holds the hash of this asset file and configuration, - and nothing shows that the requested file holds another conversion. - """ - if legacy_hash is None or not os.path.isfile(os.path.join(self.usd_dir, requested_usd_file_name)): - return False - folder = pathlib.PurePosixPath(requested_usd_file_name).parent - if folder.name: - parent_dir = os.path.join(self.usd_dir, folder.parent) - numbered_folder = re.compile(rf"{re.escape(folder.name)}_\d+") - try: - names = os.listdir(parent_dir) - except OSError: - # without a listing, a numbered folder cannot be ruled out - return False - for name in names: - if numbered_folder.fullmatch(name) and os.path.isdir(os.path.join(parent_dir, name)): - return False - # only a lazy conversion could have recorded a hash that a lazy conversion matches - return legacy_hash == self._config_to_hash(self.cfg, legacy=True) - - def _write_record(self, requested_usd_file_name: str, generated_usd_file_name: str) -> None: - """Add a conversion to the record of the output directory. - - The record is read again right before writing, to keep the entries that another process added in the - meantime. Entries whose USD file no longer exists or that this conversion replaces are dropped, and so are - the entries in the folder this conversion wrote to: outputs in one folder can share files, such as the - ``Props/instanceable_meshes.usd`` that every output of the mesh converter references. The URDF and MJCF - importers give each conversion its own folder. The record is replaced in one step, so a crash cannot leave - it half written, but writes are not serialized: two concurrent writers can still lose an entry, or keep one - that the other writer dropped. - - Args: - requested_usd_file_name: The requested USD file, relative to the output directory with ``/`` - separators. - generated_usd_file_name: The generated USD file, relative to the output directory with ``/`` - separators. - """ - entries, _ = self._read_record() - folder = pathlib.PurePosixPath(generated_usd_file_name).parent - entries = [ - entry - for entry in entries - if os.path.isfile(os.path.join(self.usd_dir, entry["generated"])) - and (entry["hash"], entry["requested"]) != (self._asset_hash, requested_usd_file_name) - and pathlib.PurePosixPath(entry["generated"]).parent != folder - ] - entries.append( - {"hash": self._asset_hash, "requested": requested_usd_file_name, "generated": generated_usd_file_name} - ) - # unlike the tempfile module, open() honors the umask, so other users of the directory can read the record - temp_path = f"{self._record_path}.{uuid.uuid4().hex}.tmp" - try: - with open(temp_path, "x", encoding="utf-8") as f: - json.dump({"version": 1, "entries": entries}, f, indent=2) - os.replace(temp_path, self._record_path) - except BaseException: - with contextlib.suppress(OSError): - os.remove(temp_path) - raise - @staticmethod - def _config_to_hash(cfg: AssetConverterBaseCfg, legacy: bool = False) -> str: + def _config_to_hash(cfg: AssetConverterBaseCfg) -> str: """Converts the configuration object and asset file to an MD5 hash of a string. - The hash leaves out :attr:`AssetConverterBaseCfg.force_usd_conversion`, which decides whether a - conversion runs, not what it produces. - .. warning:: It only checks the main asset file (:attr:`cfg.asset_path`). Args: - cfg: The asset converter configuration object. - legacy: Whether to compute the hash that earlier versions recorded for a lazy conversion, which - included the flag. + config : The asset converter configuration object. Returns: An MD5 hash of a string. @@ -354,11 +236,6 @@ def _config_to_hash(cfg: AssetConverterBaseCfg, legacy: bool = False) -> str: _ = config_dic.pop("asset_path") _ = config_dic.pop("usd_dir") _ = config_dic.pop("usd_file_name") - if legacy: - # set the flag in place, which keeps the key order and so the hash of earlier versions - config_dic["force_usd_conversion"] = False - else: - _ = config_dic.pop("force_usd_conversion") # convert config dic to bytes config_bytes = json.dumps(config_dic).encode() # hash config diff --git a/source/isaaclab/isaaclab/sim/converters/asset_converter_base_cfg.py b/source/isaaclab/isaaclab/sim/converters/asset_converter_base_cfg.py index 5acbc91fa7bf..96d47fa93924 100644 --- a/source/isaaclab/isaaclab/sim/converters/asset_converter_base_cfg.py +++ b/source/isaaclab/isaaclab/sim/converters/asset_converter_base_cfg.py @@ -53,9 +53,8 @@ class PhysicsVariant(StrEnum): force_usd_conversion: bool = False """Force the conversion of the asset file to usd. Defaults to False. - If True, then the USD file is always generated, and later lazy conversions with the same configuration reuse - it. Importers that do not overwrite earlier outputs, such as the URDF and MJCF importers, write it to a new - numbered folder. + If True, the USD file is always generated. Some importers write a new numbered output instead of overwriting + an existing one. """ make_instanceable: bool = True diff --git a/source/isaaclab/test/sim/test_mesh_converter.py b/source/isaaclab/test/sim/test_mesh_converter.py index 36122008f31a..fb0d2bf8a428 100644 --- a/source/isaaclab/test/sim/test_mesh_converter.py +++ b/source/isaaclab/test/sim/test_mesh_converter.py @@ -176,10 +176,7 @@ def test_no_change(assets): def test_config_change(assets): - """Call conversion twice but change the config in the second call. This should generate a new USD file. - - Changing the config back must generate the USD file again, since the second call overwrote it. - """ + """Call conversion twice but change the config in the second call. This should generate a new USD file.""" # create an initial USD file from asset mesh_config = MeshConverterCfg(asset_path=assets["obj"]) mesh_converter = MeshConverter(mesh_config) @@ -196,29 +193,6 @@ def test_config_change(assets): assert time_usd_file_created != new_time_usd_file_created - # change the config back, which must generate the USD file again - new_config.make_instanceable = not new_config.make_instanceable - first_mesh_converter = MeshConverter(new_config) - assert os.stat(first_mesh_converter.usd_path).st_mtime_ns != new_time_usd_file_created - - -def test_lazy_conversion_converts_again_after_another_output(assets, tmp_path): - """Convert again after another output was converted into the same directory. - - Every output in the directory references the same ``Props/instanceable_meshes.usd``, which the other conversion - replaced. - """ - usd_dir = str(tmp_path / "meshes") - mesh_config = MeshConverterCfg(asset_path=assets["obj"], usd_dir=usd_dir, usd_file_name="first.usd") - MeshConverter(mesh_config) - MeshConverter( - MeshConverterCfg( - asset_path=assets["obj"], usd_dir=usd_dir, usd_file_name="second.usd", translation=(1.0, 0.0, 0.0) - ) - ) - - check_mesh_conversion(MeshConverter(mesh_config)) - def test_convert_obj(assets, tmp_path): """Convert an OBJ file whose name has extra dots; the prim name is made a valid identifier.""" diff --git a/source/isaaclab/test/sim/test_mjcf_converter.py b/source/isaaclab/test/sim/test_mjcf_converter.py index f609d566e160..74673dca41a3 100644 --- a/source/isaaclab/test/sim/test_mjcf_converter.py +++ b/source/isaaclab/test/sim/test_mjcf_converter.py @@ -105,15 +105,10 @@ def test_no_change(test_setup_teardown): def test_config_change(test_setup_teardown): - """Call conversion twice but change the config in the second call. This should generate a new USD file. - - The importer writes the new conversion next to the first one instead of overwriting it, so later calls with - either config must load their own file without generating another one. - """ + """Reuse the importer's numbered output when the changed configuration is requested again.""" sim, mjcf_config = test_setup_teardown mjcf_converter = MjcfConverter(mjcf_config) - time_usd_file_created = os.stat(mjcf_converter.usd_path).st_mtime_ns # change the config new_config = mjcf_config @@ -124,21 +119,11 @@ def test_config_change(test_setup_teardown): new_mjcf_converter = MjcfConverter(new_config) new_time_usd_file_created = os.stat(new_mjcf_converter.usd_path).st_mtime_ns - # the importer writes the new conversion next to the first one; file times may share a second assert new_mjcf_converter.usd_path != mjcf_converter.usd_path - - # convert again with the changed config, which must not generate a new USD file lazy_mjcf_converter = MjcfConverter(new_config) assert lazy_mjcf_converter.usd_path == new_mjcf_converter.usd_path assert os.stat(lazy_mjcf_converter.usd_path).st_mtime_ns == new_time_usd_file_created - # change the config back, which must load the first USD file instead of generating a third one - new_config.self_collision = not new_config.self_collision - first_mjcf_converter = MjcfConverter(new_config) - assert first_mjcf_converter.usd_path == mjcf_converter.usd_path - assert os.stat(first_mjcf_converter.usd_path).st_mtime_ns == time_usd_file_created - assert not os.path.exists(f"{os.path.dirname(mjcf_converter.usd_path)}_2") - def test_create_prim_from_usd(test_setup_teardown): """Call conversion and create a prim from it.""" diff --git a/source/isaaclab/test/sim/test_urdf_converter.py b/source/isaaclab/test/sim/test_urdf_converter.py index f61c15eceb87..6c02c0c3f42c 100644 --- a/source/isaaclab/test/sim/test_urdf_converter.py +++ b/source/isaaclab/test/sim/test_urdf_converter.py @@ -22,11 +22,9 @@ allow_module_level=True, ) -import hashlib import json import math import os -import pathlib import shutil import warnings from types import SimpleNamespace @@ -37,7 +35,6 @@ import isaaclab.sim as sim_utils from isaaclab.sim import SimulationCfg, SimulationContext from isaaclab.sim.converters import UrdfConverter, UrdfConverterCfg -from isaaclab.utils import to_dict # The Kit-less container mounts the checkout read-only, so ``usd_dir`` goes under ``tmp_path``. pytestmark = [pytest.mark.integration, pytest.mark.kitless] @@ -102,11 +99,7 @@ def test_no_change(sim_config): @pytest.mark.isaacsim_ci def test_config_change(sim_config, tmp_path): - """Call conversion twice but change the config in the second call. This should generate a new USD file. - - The importer writes the new conversion next to the first one instead of overwriting it, so later calls with - either config must load their own file without generating another one. - """ + """Reuse the importer's numbered output, and reject old records that cannot identify that output.""" sim, config = sim_config output_dir = os.path.join(str(tmp_path), "urdf_config_change") @@ -114,7 +107,6 @@ def test_config_change(sim_config, tmp_path): config.usd_dir = output_dir urdf_converter = UrdfConverter(config) - time_usd_file_created = os.stat(urdf_converter.usd_path).st_mtime_ns # change the config new_config = config @@ -125,185 +117,19 @@ def test_config_change(sim_config, tmp_path): new_urdf_converter = UrdfConverter(new_config) new_time_usd_file_created = os.stat(new_urdf_converter.usd_path).st_mtime_ns - # the importer writes the new conversion next to the first one; file times may share a second assert new_urdf_converter.usd_path != urdf_converter.usd_path - - # convert again with the changed config, which must not generate a new USD file lazy_urdf_converter = UrdfConverter(new_config) assert lazy_urdf_converter.usd_path == new_urdf_converter.usd_path assert os.stat(lazy_urdf_converter.usd_path).st_mtime_ns == new_time_usd_file_created - # change the config back, which must load the first USD file instead of generating a third one - new_config.fix_base = not new_config.fix_base - first_urdf_converter = UrdfConverter(new_config) - assert first_urdf_converter.usd_path == urdf_converter.usd_path - assert os.stat(first_urdf_converter.usd_path).st_mtime_ns == time_usd_file_created - assert not os.path.exists(f"{os.path.dirname(urdf_converter.usd_path)}_2") - - -def _write_legacy_record(config: UrdfConverterCfg): - """Replace the record in ``config.usd_dir`` with the one that earlier versions wrote for a lazy conversion. - - It held only the MD5 of the configuration without its paths, followed by the asset file. - """ - config_dict = to_dict(config) - for key in ("asset_path", "usd_dir", "usd_file_name"): - config_dict.pop(key) - md5 = hashlib.md5(json.dumps(config_dict).encode()) - with open(config.asset_path, "rb") as f: - md5.update(f.read()) - with open(os.path.join(config.usd_dir, ".asset_hash"), "w", encoding="utf-8") as f: - f.write(md5.hexdigest()) - - -def _recorded_conversions(usd_dir: str) -> list[tuple[str, str]]: - """Return the requested and the generated USD file of each conversion that ``usd_dir`` records.""" - with open(os.path.join(usd_dir, ".asset_hash"), encoding="utf-8") as f: - record = json.load(f) - assert record["version"] == 1 - return [(entry["requested"], entry["generated"]) for entry in record["entries"]] - - -@pytest.mark.isaacsim_ci -def test_lazy_conversion_reuses_forced_conversion(sim_config, tmp_path): - """Load the output of a forced conversion for later lazy conversions with the same config.""" - sim, config = sim_config - config.usd_dir = os.path.join(str(tmp_path), "urdf_forced_conversion") - first_path = UrdfConverter(config).usd_path - config.force_usd_conversion = True - usd_path = UrdfConverter(config).usd_path - time_usd_file_created = os.stat(usd_path).st_mtime_ns - # the importer writes the forced conversion next to the first one - assert usd_path != first_path - - config.force_usd_conversion = False - assert UrdfConverter(config).usd_path == usd_path - assert os.stat(usd_path).st_mtime_ns == time_usd_file_created - - -@pytest.mark.isaacsim_ci -def test_lazy_conversion_reuses_legacy_record(sim_config, tmp_path): - """Load the requested USD file for a matching record of an earlier version, and migrate the record. - - Without a numbered folder next to the requested one, the importer wrote only one conversion to ``usd_dir``. - """ - sim, config = sim_config - config.usd_dir = os.path.join(str(tmp_path), "urdf_legacy_record") - usd_path = UrdfConverter(config).usd_path - time_usd_file_created = os.stat(usd_path).st_mtime_ns - _write_legacy_record(config) - - # the second call loads the file through the migrated record - for _ in range(2): - assert UrdfConverter(config).usd_path == usd_path - assert os.stat(usd_path).st_mtime_ns == time_usd_file_created - requested = pathlib.PurePath(os.path.relpath(usd_path, config.usd_dir)).as_posix() - assert _recorded_conversions(config.usd_dir) == [(requested, requested)] - - -@pytest.mark.isaacsim_ci -def test_lazy_conversion_converts_again_for_legacy_record_with_numbered_folder(sim_config, tmp_path): - """Convert again for a record of an earlier version when a numbered folder lies next to the requested one. - - Earlier versions recorded only the last conversion, here the one the importer wrote to the numbered folder, - while the requested USD file holds the first conversion. - """ - sim, config = sim_config - config.usd_dir = os.path.join(str(tmp_path), "urdf_legacy_record_numbered_folder") - first_path = UrdfConverter(config).usd_path - config.fix_base = not config.fix_base - UrdfConverter(config) - _write_legacy_record(config) - - assert UrdfConverter(config).usd_path != first_path - - -@pytest.mark.isaacsim_ci -@pytest.mark.skipif(os.name == "nt" or os.geteuid() == 0, reason="Needs file permissions that bind the user.") -def test_lazy_conversion_reuses_legacy_record_in_read_only_dir(sim_config, tmp_path): - """Load the requested USD file for a matching record of an earlier version that cannot be migrated.""" - sim, config = sim_config - config.usd_dir = os.path.join(str(tmp_path), "urdf_legacy_record_read_only") - usd_path = UrdfConverter(config).usd_path - _write_legacy_record(config) - os.chmod(config.usd_dir, 0o555) - try: - assert UrdfConverter(config).usd_path == usd_path - finally: - # let pytest remove the directory - os.chmod(config.usd_dir, 0o755) - - -@pytest.mark.isaacsim_ci -@pytest.mark.parametrize("legacy", [False, True], ids=["record", "legacy_record"]) -def test_lazy_conversion_converts_again_for_deleted_output(sim_config, tmp_path, legacy): - """Convert again when the recorded USD file was deleted, also for a record of an earlier version, and drop - deleted files from the record.""" - sim, config = sim_config - config.usd_dir = os.path.join(str(tmp_path), "urdf_deleted_output") - first_path = UrdfConverter(config).usd_path - config.fix_base = not config.fix_base - second_path = UrdfConverter(config).usd_path - for deleted_path in (first_path, second_path): - shutil.rmtree(os.path.dirname(deleted_path)) - - config.fix_base = not config.fix_base - if legacy: - _write_legacy_record(config) - usd_path = UrdfConverter(config).usd_path - assert os.path.isfile(usd_path) - generated = pathlib.PurePath(os.path.relpath(usd_path, config.usd_dir)).as_posix() - assert [conversion[1] for conversion in _recorded_conversions(config.usd_dir)] == [generated] - - -@pytest.mark.isaacsim_ci -def test_lazy_conversion_converts_again_for_corrupt_record(sim_config, tmp_path): - """Convert again when the record cannot be parsed, such as one cut short.""" - sim, config = sim_config - config.usd_dir = os.path.join(str(tmp_path), "urdf_corrupt_record") - first_path = UrdfConverter(config).usd_path - record_path = os.path.join(config.usd_dir, ".asset_hash") - with open(record_path, encoding="utf-8") as f: - record = f.read() - with open(record_path, "w", encoding="utf-8") as f: - f.write(record[: len(record) // 2]) - - assert UrdfConverter(config).usd_path != first_path - - -@pytest.mark.isaacsim_ci -@pytest.mark.parametrize("outside", ["absolute", "parent"]) -def test_lazy_conversion_ignores_entry_outside_usd_dir(sim_config, tmp_path, outside): - """Convert again when a hand-edited record names the generated USD file by a path outside ``usd_dir``.""" - sim, config = sim_config - config.usd_dir = os.path.join(str(tmp_path), "urdf_entry_outside") - first_path = UrdfConverter(config).usd_path - record_path = os.path.join(config.usd_dir, ".asset_hash") + # A hash-only record cannot identify B's missing numbered output while A still exists. + record_path = os.path.join(output_dir, ".asset_hash") with open(record_path, encoding="utf-8") as f: - record = json.load(f) - # name the same USD file by an absolute path, or by one that leaves usd_dir and enters it again - entry = record["entries"][0] - if outside == "absolute": - entry["generated"] = pathlib.PurePath(first_path).as_posix() - else: - entry["generated"] = f"../{os.path.basename(config.usd_dir)}/{entry['generated']}" + old_hash = json.load(f)["hash"] + shutil.rmtree(os.path.dirname(new_urdf_converter.usd_path)) with open(record_path, "w", encoding="utf-8") as f: - json.dump(record, f) - - assert not os.path.samefile(UrdfConverter(config).usd_path, first_path) - - -@pytest.mark.isaacsim_ci -def test_lazy_conversion_converts_again_for_another_requested_file(sim_config, tmp_path): - """Convert again when the record matches the hash but names another requested USD file.""" - sim, config = sim_config - config.usd_dir = os.path.join(str(tmp_path), "urdf_requested_file") - # the same bytes under another file name hash the same but request another USD file - for name in ("robot_a", "robot_b"): - config.asset_path = os.path.join(str(tmp_path), f"{name}.urdf") - shutil.copy(_MERGE_JOINTS_URDF, config.asset_path) - usd_path = UrdfConverter(config).usd_path - assert os.path.relpath(usd_path, config.usd_dir) == os.path.join("robot_b", "robot_b.usda") + f.write(old_hash) + assert UrdfConverter(new_config).usd_path != urdf_converter.usd_path @pytest.mark.isaacsim_ci