Preserve HDF5 format version when merging datasets - #7277
Preserve HDF5 format version when merging datasets#7277sylvesterkaczmarek wants to merge 9 commits into
Conversation
Greptile SummaryThe PR preserves the root HDF5
Confidence Score: 4/5The PR should not merge until mixed legacy/current inputs are rejected or reconciled, because the output can otherwise expose incorrect root-pose orientations. The merge accepts multiple datasets but assigns one file-level quaternion format from only the first input, causing legacy episodes in a current-labeled output to bypass required conversion. Files Needing Attention: scripts/tools/merge_hdf5_datasets.py Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart LR
A[First input metadata] --> M[Merge tool]
B[Later input episodes] --> M
M --> O[One output format_version]
O --> L[Dataset loader]
L --> Q[Quaternion interpretation for every episode]
Reviews (1): Last reviewed commit: "Preserve HDF5 format version when mergin..." | Re-trigger Greptile |
There was a problem hiding this comment.
Isaac Lab Review Bot
The merge tool now preserves the first input dataset’s root format_version, but this can incorrectly label merged episodes when input files use different format versions.
- Design and architecture: A single root-level format flag applies to every episode in the merged file. Copying that flag solely from the first input is incompatible with heterogeneous inputs because the output cannot represent multiple quaternion layouts.
- API: The CLI and public Python APIs are unchanged. However, the generated HDF5 contract can become incorrect: mixed legacy and version-1 inputs are labeled according to only the first file, causing consumers to apply the wrong quaternion conversion to some episodes.
- Implementation: The guarded attribute copy works for inputs sharing one format version and preserves the legacy no-attribute behavior. Before merging episodes, the tool should compare every input’s effective version, treating a missing attribute as legacy version 0, and reject mismatches with a clear error.
Minor fixes needed. Posted 1 actionable finding inline.
Automated review; human maintainers own approval decisions.
There was a problem hiding this comment.
AI review disclosure: This review was produced with assistance from multiple AI systems.
Thank you for putting this PR together.
Assessment: Preserving the root version is correct, but mixed-version rejection breaks the documented augmented-data workflow because mp4_to_hdf5.py drops the source format_version.
Findings: The documented original+Cosmos merge now rejects its generated files; invalid .patch.rst changelog filename.
Requested changes: Make mp4_to_hdf5.py preserve root format_version, test the documented merge path once, rename the fragment, and improve the mixed-version error remedy.
Changelog filename: Rename the current <slug>.patch.rst fragment to <slug>.rst for this patch-level change. IsaacLab accepts <slug>.rst (patch), <slug>.minor.rst, and <slug>.major.rst; it does not accept a .patch.rst suffix.
Test scope: Keep one preservation test and one documented-workflow regression; avoid a separate oversized script-test module if existing tool tests can host them.
Documentation / AI skills: No additional documentation update is required: existing workflow docs remain valid only after mp4_to_hdf5.py is fixed. No AI-skill update is required.
Description
Fixes
scripts/tools/merge_hdf5_datasets.pydropping the dataset rootformat_versionattribute.Current Isaac Lab HDF5 datasets use
format_version=1to mark root-pose quaternions as XYZW. The merge tool copied episode data andenv_args, but not this root attribute. A merged current-format dataset therefore looked like a legacy version-0 dataset toHDF5DatasetFileHandler, which can trigger an unnecessary WXYZ-to-XYZW conversion when episodes are loaded.The merge tool now preserves
format_versionwhen all inputs use the same effective dataset format. Before creating the output, it checks every input's effectiveformat_version(treating a missing attribute as legacy version 0) and rejects mixed-version merges with a clear error, because one root-level format flag cannot safely represent multiple quaternion layouts. Legacy-only datasets without the attribute remain legacy.Type of change
Validation
format_version=1.mp4_to_hdf5.pypreserves the sourceformat_version, and the original plus augmented datasets merge successfully with the same version.isaaclabchangelog fragment.Checklist