Skip to content

Adds version output for Kit and isaac Sim for benchmark scripts#6196

Open
kellyguo11 wants to merge 1 commit into
isaac-sim:developfrom
kellyguo11:benchmark-version
Open

Adds version output for Kit and isaac Sim for benchmark scripts#6196
kellyguo11 wants to merge 1 commit into
isaac-sim:developfrom
kellyguo11:benchmark-version

Conversation

@kellyguo11

Copy link
Copy Markdown
Contributor

Description

Adds logging of kit and isaac sim versions in the benchmark scripts.

Type of change

  • New feature (non-breaking change which adds functionality)
  • Documentation update

Checklist

  • I have read and understood the contribution guidelines
  • I have run the pre-commit checks with ./isaaclab.sh --format
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • I have updated the changelog and the corresponding version in the extension's config/extension.toml file
  • I have added my name to the CONTRIBUTORS.md or my name already exists there

@github-actions github-actions Bot added the isaac-lab Related to Isaac Lab team label Jun 16, 2026
@greptile-apps

greptile-apps Bot commented Jun 16, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR improves the benchmark version-recording infrastructure by replacing fragile attribute-chain strings (e.g. "app.get_app().get_build_version") with dedicated _get_isaac_sim_version and _get_kit_version methods, fixing the underlying __import__importlib.import_module bug along the way, and surfacing isaacsim_version, kit_version, and isaaclab_ov_version in the printed summary report.

  • record_version_info.py adds two new resolution strategies — reading from the running omni.kit.app instance (trying three method names in order) and falling back to Carb settings keys — plus a callable-attribute check in the generic _get_version helper.
  • backends.py reads the three new metadata keys from the version phase and prints them in the summary box.
  • Four new unit tests cover the Isaac Sim utility path, the live Kit app path, the Carb settings fallback, and the guarantee that no Kit import is triggered in kitless environments.

Confidence Score: 4/5

Safe to merge — all changes are additive, read-only version introspection with no effect on simulation logic or data pipelines.

The refactored Kit/Isaac Sim version detection is well-structured and the fallback chain is correct. The one minor gap is that str(get_isaac_sim_version()) doesn't guard against a hypothetical None return, and the getattr(carb, "settings", None) fallback in _get_kit_settings_version has no corresponding test. Neither affects production correctness today, but both are worth tidying up.

record_version_info.py — specifically the _get_isaac_sim_version wrapper and the untested carb attribute-fallback branch in _get_kit_settings_version.

Important Files Changed

Filename Overview
source/isaaclab/isaaclab/test/benchmark/recorders/record_version_info.py Core change: replaces brittle attribute-chain strings with dedicated _get_isaac_sim_version/_get_kit_version methods; fixes import → importlib.import_module and adds callable() check; logic is sound but has one minor issue where str(get_isaac_sim_version()) could surface "None" if the wrapped function ever returns None
source/isaaclab/isaaclab/test/benchmark/backends.py Adds three new version fields (isaacsim_version, kit_version, isaaclab_ov_version) to the summary print block; keys correctly match the StringMetadata names produced by get_data() via the f"{package}_version" convention
source/isaaclab/test/benchmark/test_recorders.py Adds four new tests covering the Isaac Sim utility path, Kit app path, Carb settings fallback, and the kitless no-import guarantee; the carb attribute-fallback path (getattr(carb, "settings", None)) is not exercised by any test

Sequence Diagram

%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
    participant B as SummaryMetrics (backends.py)
    participant VR as VersionInfoRecorder
    participant VU as isaaclab.utils.version
    participant KA as omni.kit.app (sys.modules)
    participant CS as carb.settings (sys.modules)

    VR->>VU: get_isaac_sim_version() [lru_cached]
    alt isaacsim available
        VU-->>VR: Version("5.x.x")
    else ImportError
        VR->>VR: _get_version("isaacsim") or _get_pkg_version("isaacsim")
    end

    VR->>KA: sys.modules.get("omni.kit.app")
    alt Kit running
        KA-->>VR: kit_app module
        VR->>KA: kit_app.get_app()
        KA-->>VR: app instance
        VR->>KA: app.get_kit_version() / get_build_version() / get_kernel_version()
        KA-->>VR: "106.x.x+release"
    else Kit not running
        VR->>CS: sys.modules.get("carb.settings")
        alt carb.settings available
            CS-->>VR: settings module
            VR->>CS: settings.get_settings().get("/app/kit/version")
            CS-->>VR: "107.x.x"
        else No carb
            VR-->>VR: None
        end
    end

    VR->>VR: _record("isaacsim", version), _record("kit", version)
    VR->>B: "get_data() → StringMetadata(name="isaacsim_version", ...)"
    B->>B: _metadata_map → version_meta["isaacsim_version"]
    B->>B: _print_box_kv("isaacsim_version", ...)
Loading
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
    participant B as SummaryMetrics (backends.py)
    participant VR as VersionInfoRecorder
    participant VU as isaaclab.utils.version
    participant KA as omni.kit.app (sys.modules)
    participant CS as carb.settings (sys.modules)

    VR->>VU: get_isaac_sim_version() [lru_cached]
    alt isaacsim available
        VU-->>VR: Version("5.x.x")
    else ImportError
        VR->>VR: _get_version("isaacsim") or _get_pkg_version("isaacsim")
    end

    VR->>KA: sys.modules.get("omni.kit.app")
    alt Kit running
        KA-->>VR: kit_app module
        VR->>KA: kit_app.get_app()
        KA-->>VR: app instance
        VR->>KA: app.get_kit_version() / get_build_version() / get_kernel_version()
        KA-->>VR: "106.x.x+release"
    else Kit not running
        VR->>CS: sys.modules.get("carb.settings")
        alt carb.settings available
            CS-->>VR: settings module
            VR->>CS: settings.get_settings().get("/app/kit/version")
            CS-->>VR: "107.x.x"
        else No carb
            VR-->>VR: None
        end
    end

    VR->>VR: _record("isaacsim", version), _record("kit", version)
    VR->>B: "get_data() → StringMetadata(name="isaacsim_version", ...)"
    B->>B: _metadata_map → version_meta["isaacsim_version"]
    B->>B: _print_box_kv("isaacsim_version", ...)
Loading

Reviews (1): Last reviewed commit: "Adds version output for Kit and isaac Si..." | Re-trigger Greptile

Comment on lines +61 to +65
try:
from isaaclab.utils.version import get_isaac_sim_version

return str(get_isaac_sim_version())
except Exception:

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 get_isaac_sim_version() returns a packaging.version.Version object (never None), so str(...) is safe today. However, if the function signature ever relaxes to allow None (e.g. in offline/kitless environments), str(None) evaluates to the literal string "None", which passes the if version: guard in _record and would appear verbatim in benchmark output. A simple None check here makes the intent explicit and guards against future regressions.

Suggested change
try:
from isaaclab.utils.version import get_isaac_sim_version
return str(get_isaac_sim_version())
except Exception:
try:
from isaaclab.utils.version import get_isaac_sim_version
version = get_isaac_sim_version()
return str(version) if version is not None else None
except Exception:

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

Comment on lines +552 to +564
def test_captures_kit_version_from_carb_settings(self, monkeypatch):
"""Test that Kit version falls back to Carb settings."""

class _Settings:
def get(self, key):
return {"/app/kit/version": "107.0.0"}.get(key)

monkeypatch.delitem(sys.modules, "omni.kit.app", raising=False)
monkeypatch.setitem(sys.modules, "carb.settings", types.SimpleNamespace(get_settings=lambda: _Settings()))

recorder = VersionInfoRecorder()

assert recorder.get_initial_data()["version_metadata"]["kit"] == "107.0.0"

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 Carb attribute-fallback path not exercised

_get_kit_settings_version has a secondary path — carb_settings = getattr(carb, "settings", None) — that is reached when carb is in sys.modules but carb.settings is not a separate module entry. The current test only covers the direct sys.modules["carb.settings"] lookup. A test that adds carb (with a .settings attribute) to sys.modules but leaves carb.settings absent would complete coverage of that branch.

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

Labels

isaac-lab Related to Isaac Lab team

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant