Skip to content

Preserve HDF5 format version when merging datasets - #7277

Open
sylvesterkaczmarek wants to merge 9 commits into
isaac-sim:developfrom
sylvesterkaczmarek:fix/merge-hdf5-format-version
Open

Preserve HDF5 format version when merging datasets#7277
sylvesterkaczmarek wants to merge 9 commits into
isaac-sim:developfrom
sylvesterkaczmarek:fix/merge-hdf5-format-version

Conversation

@sylvesterkaczmarek

@sylvesterkaczmarek sylvesterkaczmarek commented Aug 21, 2026

Copy link
Copy Markdown

Description

Fixes scripts/tools/merge_hdf5_datasets.py dropping the dataset root format_version attribute.

Current Isaac Lab HDF5 datasets use format_version=1 to mark root-pose quaternions as XYZW. The merge tool copied episode data and env_args, but not this root attribute. A merged current-format dataset therefore looked like a legacy version-0 dataset to HDF5DatasetFileHandler, which can trigger an unnecessary WXYZ-to-XYZW conversion when episodes are loaded.

The merge tool now preserves format_version when all inputs use the same effective dataset format. Before creating the output, it checks every input's effective format_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

  • Bug fix

Validation

  • Added a unit regression test that creates a current-format HDF5 dataset, runs the merge tool, and verifies the merged file retains format_version=1.
  • Added a regression for the documented augmentation workflow: mp4_to_hdf5.py preserves the source format_version, and the original plus augmented datasets merge successfully with the same version.
  • Runtime change is limited to preserving and validating the existing root metadata attribute.
  • Added the required isaaclab changelog fragment.

Checklist

  • I have read and understood the contribution guidelines
  • I have added a regression test
  • I have added the required changelog fragment
  • No new dependencies

@sylvesterkaczmarek
sylvesterkaczmarek requested a review from a team August 21, 2026 15:53
@github-actions github-actions Bot added bug Something isn't working isaac-lab Related to Isaac Lab team labels Aug 21, 2026
@greptile-apps

greptile-apps Bot commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

The PR preserves the root HDF5 format_version attribute during dataset merges and adds regression coverage plus a changelog entry. However, selecting that file-level version solely from the first input can mislabel mixed legacy/current merges.

  • Copies format_version alongside the existing first-input metadata.
  • Adds a unit test for a single current-format input.
  • Documents the corrected metadata preservation behavior.

Confidence Score: 4/5

The 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

Filename Overview
scripts/tools/merge_hdf5_datasets.py Preserves the first input's format version but does not validate that all merged inputs share that file-level format.
source/isaaclab/test/utils/test_merge_hdf5_datasets.py Correctly covers one current-format input, but does not exercise mixed-version multi-input merges.
source/isaaclab/changelog.d/sylvesterkaczmarek-merge-hdf5-format-version.patch.rst Accurately documents the single-format metadata preservation fix.

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]
Loading

Reviews (1): Last reviewed commit: "Preserve HDF5 format version when mergin..." | Re-trigger Greptile

Comment thread scripts/tools/merge_hdf5_datasets.py

@isaaclab-review-bot isaaclab-review-bot Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread scripts/tools/merge_hdf5_datasets.py

@AntoineRichard AntoineRichard left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working isaac-lab Related to Isaac Lab team

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants