Skip to content

Adds release version to benchmark output#5188

Merged
kellyguo11 merged 7 commits into
developfrom
feature/benchmark-version-attribute
Apr 9, 2026
Merged

Adds release version to benchmark output#5188
kellyguo11 merged 7 commits into
developfrom
feature/benchmark-version-attribute

Conversation

@kellyguo11
Copy link
Copy Markdown
Contributor

@kellyguo11 kellyguo11 commented Apr 6, 2026

Description

Adds the Isaac Lab release version (from the root VERSION file) to the benchmark output files.

Motivation

The VersionInfoRecorder currently collects package versions via importlib.metadata (pip) and module __version__ attributes. These reflect what is installed in the current environment, but do not capture the release version of Isaac Lab itself (e.g. 3.0.0 from the root VERSION file).

This makes it hard to correlate benchmark results with specific releases, especially when the installed pip version does not match the source tree (e.g. editable installs during development).

Changes

  • source/isaaclab/isaaclab/test/benchmark/recorders/record_version_info.py:
    • Added reading of the top-level VERSION file → recorded as isaaclab_release

Example Output

Before (existing fields preserved):

isaaclab: 4.5.25
isaaclab_tasks: 1.5.15

After (new field added):

isaaclab_release: 3.0.0        # ← from VERSION file

Testing

Verified the recorder produces correct output with the expected field populated. No existing fields are modified — this is purely additive.

…tput

The VersionInfoRecorder now reads:
1. The top-level VERSION file (e.g. '3.0.0') and records it as
   'isaaclab_release' — this is the overall Isaac Lab release version.
2. Each sub-package's config/extension.toml version, recorded as
   '<package>_ext' (e.g. 'isaaclab_tasks_ext: 1.5.18').

This supplements the existing pip metadata versions (which reflect
what's installed in the current environment) with the source-of-truth
versions from the repository files. This is useful for:
- Tracking benchmark results against specific releases
- Detecting version mismatches between installed packages and source
- Reproducing benchmark results from a specific codebase state

The extension.toml versions are read using tomllib (Python 3.11+)
with a fallback to the tomli backport for older Python versions.
@github-actions github-actions Bot added enhancement New feature or request isaac-lab Related to Isaac Lab team labels Apr 6, 2026
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented Apr 6, 2026

Greptile Summary

This PR adds the Isaac Lab release version (from the root VERSION file) and per-sub-package extension.toml versions to the VersionInfoRecorder benchmark output. The implementation inlines both additions into _get_version_info() (note: the PR description mentions a separate _get_release_version() method that does not appear in the final code), wraps all I/O in try/except for graceful degradation, and correctly computes _REPO_ROOT as 6 levels up from the recorder file. CHANGELOG and version bump (4.5.27) are consistent across both CHANGELOG.rst and extension.toml.

Confidence Score: 4/5

Mostly safe to merge, but the bare import tomllib at module-level will raise ModuleNotFoundError on Python 3.10, which is within the declared python_requires=">=3.10" range.

The prior review threads already flagged the tomllib/tomli import strategy. The current code removed the tomli fallback but left a plain import tomllib, which is stdlib only from Python 3.11. Since setup.py declares python_requires=">=3.10", this remains an unresolved P1-level compatibility gap. All other changes (VERSION file read, source/ dir iteration, CHANGELOG, version bump) are correct and robust.

source/isaaclab/isaaclab/test/benchmark/recorders/record_version_info.py — top-level import tomllib incompatible with Python 3.10.

Vulnerabilities

No security concerns identified.

Important Files Changed

Filename Overview
source/isaaclab/isaaclab/test/benchmark/recorders/record_version_info.py Adds VERSION file and extension.toml version collection; uses top-level import tomllib (Python 3.11+ only) with no fallback, while setup.py sets python_requires=">=3.10", breaking module import on Python 3.10.
source/isaaclab/docs/CHANGELOG.rst New 4.5.27 version entry added with correct date, category, and RST formatting; consistent with extension.toml bump.
source/isaaclab/config/extension.toml Version bumped to 4.5.27 matching the new CHANGELOG entry; no other changes.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[VersionInfoRecorder.__init__] --> B[_get_version_info]
    B --> C[Module imports and importlib.metadata]
    B --> D[Open root VERSION file]
    B --> E[Iterate source subdirectories]
    E --> F[_get_ext_version for each package]
    F --> G{tomllib.load binary mode}
    G -->|success| H[record entry_ext key]
    G -->|exception| I[return None and skip]
    D -->|success| J[record isaaclab_release]
    D -->|exception| I
    C --> K[record isaaclab, warp, torch, etc.]
    H --> L[_version_info dict]
    J --> L
    K --> L
    L --> M[get_data converts to StringMetadata list]
Loading

Reviews (2): Last reviewed commit: "Add changelog entry and bump version to ..." | Re-trigger Greptile

Comment on lines +10 to +13
try:
import tomllib
except ModuleNotFoundError:
import tomli as tomllib # Python < 3.11
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.

P1 Unguarded tomli fallback crashes module import

If running Python < 3.11 and tomli is not installed (it is not declared in any dependency manifest — setup.py, pyproject.toml, or config/extension.toml), the bare import tomli as tomllib raises ModuleNotFoundError at module load time. This makes the entire record_version_info module unimportable, silently disabling all benchmark recording — not just the new version fields. The toml package is already a declared required dependency and parses the same format; a nested try/except using it as a final fallback would fix this.

Suggested change
try:
import tomllib
except ModuleNotFoundError:
import tomli as tomllib # Python < 3.11
try:
import tomllib
except ModuleNotFoundError:
try:
import tomli as tomllib # Python < 3.11 with tomli installed
except ModuleNotFoundError:
import toml as tomllib # type: ignore[no-redef] # fallback: already-required dep

Note: toml.load() requires a text-mode file handle, while tomllib.load() requires binary mode. If using the toml fallback you will also need to conditionally change the open(toml_path, "rb") call on line 86 to text mode, or restructure _get_release_version to detect which library is active.

Comment on lines +10 to +13
try:
import tomllib
except ModuleNotFoundError:
import tomli as tomllib # Python < 3.11
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.

P2 New undeclared optional dependency

The tomli package is not listed in setup.py, pyproject.toml, or config/extension.toml. Project guidelines explicitly say to avoid new optional dependencies when existing ones suffice. Since toml is already required everywhere in this package (it is used in setup.py itself to read extension.toml), it can replace tomli here without adding any new dependency.

Copy link
Copy Markdown
Collaborator

@AntoineRichard AntoineRichard left a comment

Choose a reason for hiding this comment

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

I feel like this could be better structured.

Comment on lines +10 to +13
try:
import tomllib
except ModuleNotFoundError:
import tomli as tomllib # Python < 3.11
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.

We should be using python3.11 at minimum. So none of this is needed.

Comment on lines +10 to +13
try:
import tomllib
except ModuleNotFoundError:
import tomli as tomllib # Python < 3.11
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.

Remove and import tomllib only.

Comment on lines +61 to +92
def _get_release_version(self) -> None:
"""Read the top-level VERSION file and each sub-package's extension.toml version.

The VERSION file at the repository root contains the Isaac Lab release version
(e.g. ``3.0.0``). Each sub-package under ``source/`` has its own version in
``config/extension.toml``.
"""
# Read root VERSION file
version_file = os.path.join(_REPO_ROOT, "VERSION")
if os.path.isfile(version_file):
try:
with open(version_file) as f:
version = f.read().strip()
if version:
self._record("isaaclab_release", version)
except Exception:
pass

# Read extension.toml versions from each source sub-package
source_dir = os.path.join(_REPO_ROOT, "source")
if os.path.isdir(source_dir):
for entry in sorted(os.listdir(source_dir)):
toml_path = os.path.join(source_dir, entry, "config", "extension.toml")
if os.path.isfile(toml_path):
try:
with open(toml_path, "rb") as f:
data = tomllib.load(f)
version = data.get("package", {}).get("version")
if version:
self._record(f"{entry}_ext", version)
except Exception:
pass
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.

Why not use get_version directly?

I.e.

if os.path.isdir(source_dir):
            for entry in sorted(os.listdir(source_dir)):
                version = get_version(entry)
                if version:
                    self._record(f"{entry}_ext", version

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.

Also could this not be done in _get_version_info directly?

…ion_info

- Remove tomli fallback since Python 3.11+ is minimum
- Replace _get_release_version with _get_ext_version helper
- Integrate release version and extension.toml reads into _get_version_info
- Remove fragile level-count from _REPO_ROOT comment
- Wrap os.listdir loop in try-except for consistency with the
  rest of the file's defensive error handling
Document the addition of release version and extension.toml
versions to VersionInfoRecorder benchmark output.
@AntoineRichard
Copy link
Copy Markdown
Collaborator

@greptile

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented Apr 8, 2026

Tip:

Greploops — Automatically fix all review issues by running /greploops in Claude Code. It iterates: fix, push, re-review, repeat until 5/5 confidence.

Use the Greptile plugin for Claude Code to query reviews, search comments, and manage custom context directly from your terminal.

@kellyguo11 kellyguo11 changed the title Add release version and extension.toml versions to benchmark output Add release version to benchmark output Apr 8, 2026
Comment thread source/isaaclab/docs/CHANGELOG.rst Outdated
Signed-off-by: Kelly Guo <kellyg@nvidia.com>
Copy link
Copy Markdown

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

Choose a reason for hiding this comment

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

LGTM — clean, additive change.

What it does: Reads the top-level VERSION file (e.g. 3.0.0) at benchmark time and records it as isaaclab_release. This fills the gap where pip metadata versions reflect the installed environment but not the actual release tag of the source tree.

Review notes:

  • _REPO_ROOT path traversal (6 levels up) is correct for source/isaaclab/isaaclab/test/benchmark/recorders/ → repo root ✅
  • Bare except Exception: pass is consistent with the file's existing defensive style (all version reads silently degrade) ✅
  • Purely additive — no existing benchmark output fields are modified, so backward compatibility is preserved ✅
  • Changelog and version bump look good ✅

Minor nit (non-blocking): _get_git_info() still computes its own script_dir = os.path.dirname(os.path.abspath(__file__)) on L112. Could reuse _REPO_ROOT as cwd for the subprocess.run calls there too, but that's cosmetic — the git commands work from any directory in the repo.

Note: The PR title mentions "extension.toml versions" but those were removed in the latest commit — might want to update the title to just "Add release version to benchmark output" to match the current scope.

@kellyguo11 kellyguo11 changed the title Add release version to benchmark output Adds release version to benchmark output Apr 9, 2026
@kellyguo11 kellyguo11 merged commit b59a9a2 into develop Apr 9, 2026
25 of 29 checks passed
@kellyguo11 kellyguo11 deleted the feature/benchmark-version-attribute branch April 9, 2026 00:19
mmichelis pushed a commit to mmichelis/IsaacLab that referenced this pull request Apr 10, 2026
Adds the Isaac Lab release version (from the root `VERSION` file) to the
benchmark output files.

The `VersionInfoRecorder` currently collects package versions via
`importlib.metadata` (pip) and module `__version__` attributes. These
reflect what is *installed* in the current environment, but do not
capture the **release version** of Isaac Lab itself (e.g. `3.0.0` from
the root `VERSION` file).

This makes it hard to correlate benchmark results with specific
releases, especially when the installed pip version does not match the
source tree (e.g. editable installs during development).

-
`source/isaaclab/isaaclab/test/benchmark/recorders/record_version_info.py`:
- Added reading of the top-level `VERSION` file → recorded as
`isaaclab_release`

Before (existing fields preserved):
```
isaaclab: 4.5.25
isaaclab_tasks: 1.5.15
```

After (new field added):
```
isaaclab_release: 3.0.0        # ← from VERSION file
```

Verified the recorder produces correct output with the expected field
populated. No existing fields are modified — this is purely additive.

---------

Signed-off-by: Kelly Guo <kellyg@nvidia.com>
Co-authored-by: Kelly Guo <kellyguo11@users.noreply.github.com>
Co-authored-by: Antoine RICHARD <antoiner@nvidia.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request isaac-lab Related to Isaac Lab team

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants