Skip to content

Address SQA bug: camera support from Experiment config.#953

Open
alexmillane wants to merge 1 commit into
mainfrom
alex/fix/unify_enable_cameras
Open

Address SQA bug: camera support from Experiment config.#953
alexmillane wants to merge 1 commit into
mainfrom
alex/fix/unify_enable_cameras

Conversation

@alexmillane

Copy link
Copy Markdown
Collaborator

Summary

Address issue that shipped Experiment configs failed unless --enable_cameras was also passed on the CLI.

Detailed description

  • Every shipped Experiment config sets environment.enable_cameras: true, but AppLauncher needs camera support before SimulationApp starts and typed YAML was only composed afterwards, so the runner asserted and told the user to repeat the flag on the CLI.
  • typed_experiment_requires_cameras now reads the YAML envelope and Hydra overrides (via Hydra's own OverridesParser) without importing environment modules, so experiment_runner derives camera support from the config for typed and legacy Experiments alike. The former user-facing assert is now an internal invariant check.
  • enable_cameras moved onto ArenaEnvironmentCfg instead of being redeclared by 15 environment configs; lift_object and press_button now honor it and dexsuite_lift rejects it, since KukaAllegroEmbodiment has no camera configuration.
  • Docs no longer pass --enable_cameras alongside --experiment_config, and Experiment YAML envelope errors now surface before Isaac Sim starts rather than after.
  • Addresses 6491232

Read enable_cameras from the Experiment before SimulationApp starts so
experiment_runner no longer requires --enable_cameras on the CLI, and move
enable_cameras onto ArenaEnvironmentCfg so every environment declares it once.

Signed-off-by: alex <amillane@nvidia.com>
environment_values = run_values.get("environment")
if not isinstance(environment_values, dict):
return False
enable_cameras = environment_values.get("enable_cameras", False)

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.

🔵 Reader assumes the base default stays False

This defaults an absent enable_cameras to False, which only matches composition because ArenaEnvironmentCfg.enable_cameras is False and no environment overrides it. If some environment ever defaults cameras on, this pre-startup read would disagree with the composed config and trip _assert_camera_support_enabled for a perfectly valid Experiment. Worth a short NOTE: here tying this default to the cfg default so a future maintainer keeps the two in sync?

@greptile-apps

greptile-apps Bot commented Jul 25, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

The PR derives Isaac Sim camera startup support from typed Experiment YAML and Hydra overrides.

  • Moves enable_cameras into the shared environment configuration base.
  • Propagates camera selection into lift-object and press-button embodiments and rejects cameras for dexsuite lift.
  • Updates Experiment-runner invariants, tests, and documentation so the CLI camera flag is no longer required.

Confidence Score: 3/5

The unsupported embodiment-constructor path should be fixed before merging because selecting kuka_allegro for either changed factory now aborts environment construction.

The main pre-startup camera detection is internally consistent, but the new unconditional constructor keyword breaks a currently registered embodiment on two configurable environment-building paths.

Files Needing Attention: isaaclab_arena_environments/lift_object_environment.py, isaaclab_arena_environments/press_button_environment.py

Important Files Changed

Filename Overview
isaaclab_arena/hydra/typed_experiment_loader.py Adds pre-startup parsing of YAML and Hydra camera settings without importing simulation-dependent environment modules.
isaaclab_arena/evaluation/experiment_runner.py Enables AppLauncher camera support from the Experiment configuration and retains a post-composition invariant check.
isaaclab_arena/environments/arena_environment_factory.py Centralizes the default enable_cameras field on the shared typed environment configuration.
isaaclab_arena_environments/lift_object_environment.py Propagates the camera flag to embodiments, but registered embodiments without that constructor keyword can now fail during environment build.
isaaclab_arena_environments/press_button_environment.py Propagates the camera flag unconditionally and shares the unsupported-embodiment constructor failure.
isaaclab_arena_environments/dexsuite_lift_environment.py Explicitly rejects camera-enabled configurations because its Kuka Allegro embodiment has no camera configuration.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart LR
  A[Experiment YAML and Hydra overrides] --> B[Pre-startup camera requirement read]
  B --> C[SimulationAppContext]
  C --> D[Compose typed Experiment]
  D --> E[Build selected environment]
  E --> F[Instantiate embodiment with enable_cameras]
Loading

Reviews (1): Last reviewed commit: "Address SQA bug: derive camera support f..." | Re-trigger Greptile

Comment on lines +52 to +54
embodiment = self.asset_registry.get_asset_by_name(cfg.embodiment)(
enable_cameras=cfg.enable_cameras, concatenate_observation_terms=True
)

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 Unsupported embodiment constructor keyword

When either lift_object or press_button selects the registered kuka_allegro embodiment, the factory unconditionally passes enable_cameras, but KukaAllegroEmbodiment.__init__ does not accept that keyword, causing environment construction to fail with TypeError even when cameras are disabled.

Knowledge Base Used: Environment builder flow

@arena-review-bot

Copy link
Copy Markdown
Contributor

🤖 Isaac Lab-Arena Review Bot

Summary

This fixes the SQA bug where shipped Experiment configs failed unless --enable_cameras was also passed on the CLI. The camera requirement is now read from the config before Isaac Sim starts (parsing the YAML envelope and Hydra overrides without importing environment modules), and enable_cameras is lifted onto the base ArenaEnvironmentCfg instead of being redeclared by 15 environment configs. Clean, well-scoped change with strong test coverage.

Findings

🔵 Improvement — typed_experiment_loader.py:177 — The pre-startup reader defaults an absent enable_cameras to False, which stays correct only as long as the base cfg default is False and no environment overrides it. If an environment ever defaults cameras on, the pre-startup read would disagree with composition and trip _assert_camera_support_enabled. A short NOTE: linking the two would help future maintainers. (Non-blocking — the invariant already fails loud and is tested.)

Test Coverage

Strong. The new tests are pure-Python config-loading tests (they monkeypatch SimulationAppContext, no sim start, so the inner/outer pattern is correctly not needed; they land in Phase 1). Coverage includes: the real shipped camera-sensitivity YAML enabling cameras end-to-end, the parametrized YAML-vs-override matrix asserting the pre-startup read agrees with the composed Experiment (the key invariant), non-boolean rejection for both declarations and overrides, and the reworded invariant assertion. This is exactly the safety net this two-path design needs.

Verdict

Ship it

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant