[Core] Resolve manager term defaults and validate randomization distributions by StafaH · Pull Request #8127 · isaac-sim/IsaacLab

StafaH · 2026-09-29T05:28:00Z

Description

Manager term constructors currently repeat defaults already declared in their callable signatures. This can make construction disagree with execution: omitting an optional operation raises KeyError in several randomization terms, and Gaussian mass scaling (0.5, 0.1) is rejected as a reversed (low, high) range.

Resolve omitted keyword defaults into the term's existing params mapping before construction and scene resolution. The stable and experimental managers share the implementation; mutable defaults are copied per term and explicit values, including None, are preserved. Direct class construction receives the same defaults after super().__init__. Backend and task constructors now consume those resolved parameters. Observation-manager out buffers remain manager-owned, and task guards for required values now check None where appropriate.

Replace scale-only validation with distribution-aware construction checks for mass, inertia, actuator gains, joint parameters, and fixed tendons. Gaussian parameters are (mean, std), uniform ranges remain ordered, log-uniform bounds must be positive, and non-finite parameters fail early. These checks cover add, abs, and scale; the existing scale-sign policy remains separate from distribution validity. Exception documentation uses one ValueError entry per constructor.

No validation or signature inspection is added to simulation/reset loops. Runtime parameter changes continue through the same mapping; this change does not add revalidation of arbitrary mutations after construction. A positive Gaussian mean still does not guarantee positive samples.

Fixes #3514. This is a broader alternative to #8123, with shared parameter-default resolution and repository-wide adoption.

Validation

183 passed across 15 test files, each run in its own process: stable manager suites; event requirements; camera terms and camera/manager integration; experimental frontend and reward parity; task curricula; PhysX, Newton and OVPhysX event integration.
Confirmed the default-resolution regression fails when default population is disabled, and the Gaussian regression fails against the original mass constructor with the reported bound-order error.
uv run isaaclab -f and the changelog gate passed against the upstream PR base; GitHub pre-commit and changelog checks also passed.
Clean uv run isaaclab --docs build passed with warnings treated as errors; the final incremental build also passed.
Existing dependency deprecation warnings remain in test output.

Type of change

Bug fix

Release backport

Backport this pull request to the active release branch after it merges into develop

Checklist

I have read and understood the contribution guidelines
I have run the pre-commit checks with uv run isaaclab --format
I have made corresponding changes to the documentation
My changes generate no new warnings
I have validated the changed behavior with focused coverage and applied the test-audit criteria
I have added a changelog fragment for every touched package
My name already exists in CONTRIBUTORS.md

greptile-apps · 2026-09-29T05:37:54Z

Confidence Score: 4/5

[Medium risk] Refactors manager term parameter defaults and adds randomization validation.

The PR should not merge until direct OVPhysX material-term construction works with the existing omitted make_consistent value.

Findings

Omitted flag breaks construction ▶

Summary

The PR populates manager-term parameters from callable defaults before construction and scene resolution, and adds distribution-aware validation for physics randomization.

Shared default resolution covers stable and experimental managers and replaces duplicated constructor fallbacks.
The OVPhysX material constructor no longer supports an existing direct-construction test path that omits make_consistent.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart LR
  A[Term configuration] --> B[Manager resolves callable defaults]
  B --> C[Scene resolution and class construction]
  C --> D[Term invocation]
  A --> E[Direct construction]
  E --> F{ManagerTermBaseCfg?}
  F -- Yes --> G[Populate defaults]
  F -- No --> H[Defaults not populated]
  H --> I[OVPhysX make_consistent KeyError]

_{Reviews (1) · Last reviewed commit: "Resolve manager term defaults and valida..."}

greptile-apps · 2026-09-29T05:37:57Z

        ranges = torch.tensor([static_friction_range, dynamic_friction_range, restitution_range], device="cpu")
        self.material_buckets = math_utils.sample_uniform(ranges[:, 0], ranges[:, 1], (num_buckets, 3), device="cpu")
-        if cfg.params.get("make_consistent", False):
+        if cfg.params["make_consistent"]:


Omitted flag breaks construction

The existing OVPhysX material tests construct this term with a SimpleNamespace config that omits make_consistent. Defaults are populated only for ManagerTermBaseCfg, so this lookup now raises KeyError instead of using the previous False fallback. Direct construction fails before material randomization can run.

Suggested change

if cfg.params["make_consistent"]:

if cfg.params.get("make_consistent", False):

isaaclab-review-bot

Isaac Lab Review Bot

Reviewed the centralized callable-default resolution and distribution-aware randomization validation. The changed construction paths consistently materialize omitted defaults before class initialization and scene resolution, while preserving explicit values and isolating mutable defaults.

Design and architecture: The shared _populate_term_defaults helper is integrated into stable and experimental manager construction paths, including deferred nested-term processing. Observation terms retain manager ownership of the out parameter by rejecting configured values and removing the injected default before invocation.
API: Checked converted cfg.params[...] consumers against their callable signatures and traced wrapper/backend parameter flow for material and gravity randomization. The ManagerTermBaseCfg.params behavior is documented, mutable defaults are copied, explicit None is preserved, and affected packages include changelog fragments.
Implementation: The validation helper correctly distinguishes Gaussian (mean, std) from uniform and log-uniform bounds, rejects non-finite values and invalid log-uniform bounds, and preserves per-field scale-sign policies across mass, inertia, actuator, joint, and tendon terms. Runtime mutations remain intentionally unvalidated, so callers changing parameters after construction retain responsibility for validity.

No blocking issues. No inline issue met the actionable-evidence threshold; the assessment above records the review feedback.

Automated review; human maintainers own approval decisions.

ooctipus · 2026-09-30T20:52:48Z

Rebased onto develop at c1a8bfa and force-pushed with a lease. Kept the upstream root-reset change from #8105: call-time ranges remain authoritative, and the removed constructor cache stays removed. Also migrated the existing changelog fragments to the current format.

Validation: 119 tests passed across eight focused files covering manager defaults, event validation, mutable root-reset ranges, observations, rewards, terminations, and camera terms. Formatting and changelog checks passed.

ooctipus · 2026-09-30T20:52:49Z

run-ci

ooctipus · 2026-09-30T22:18:59Z

Fixed in b679225. Two material-randomization tests still used SimpleNamespace configs, bypassing the default initialization added by this PR and causing KeyError: make_consistent. They now use EventTermCfg and deliberately omit make_consistent to exercise its default. No production fallback or new tests.

Both affected tests pass on CPU and CUDA (GPU 0); all 8 manager-default tests and formatting checks also pass. Disabling default initialization makes the corrected test fail, confirming it covers the contract.

ooctipus · 2026-09-30T22:19:10Z

run-ci

ooctipus · 2026-09-30T22:37:55Z

The OV suite now passes. I checked the two remaining failures:

Core 2/3 never started pytest. The image action found a dependency-cache hit and created the registry tag, but run-package-tests does not enable pull-on-deps-hit, leaving no local image for docker run. I retriggered that job now that the exact registry tag exists.
Contrib fails in the Franka Pour spawner, before manager initialization. I reproduced it with only the spawner and an in-memory USD stage. The authored asset has NewtonMimicAPI on finger joint 1 but no PhysxMimicJointAPI:linear on joint 2; the task expects both, then removes the Newton constraint. Dropping the check would therefore remove the remaining mimic constraint. This task file is byte-identical on current develop and this PR. It needs an asset/task correction, not a manager-default fallback.

…ibutions (#8127) # Description Manager term constructors currently repeat defaults already declared in their callable signatures. This can make construction disagree with execution: omitting an optional `operation` raises `KeyError` in several randomization terms, and Gaussian mass scaling `(0.5, 0.1)` is rejected as a reversed `(low, high)` range. Resolve omitted keyword defaults into the term's existing `params` mapping before construction and scene resolution. The stable and experimental managers share the implementation; mutable defaults are copied per term and explicit values, including `None`, are preserved. Direct class construction receives the same defaults after `super().__init__`. Backend and task constructors now consume those resolved parameters. Observation-manager `out` buffers remain manager-owned, and task guards for required values now check `None` where appropriate. Replace scale-only validation with distribution-aware construction checks for mass, inertia, actuator gains, joint parameters, and fixed tendons. Gaussian parameters are `(mean, std)`, uniform ranges remain ordered, log-uniform bounds must be positive, and non-finite parameters fail early. These checks cover `add`, `abs`, and `scale`; the existing scale-sign policy remains separate from distribution validity. Exception documentation uses one `ValueError` entry per constructor. No validation or signature inspection is added to simulation/reset loops. Runtime parameter changes continue through the same mapping; this change does not add revalidation of arbitrary mutations after construction. A positive Gaussian mean still does not guarantee positive samples. Fixes #3514. This is a broader alternative to #8123, with shared parameter-default resolution and repository-wide adoption. ## Validation - **183 passed across 15 test files**, each run in its own process: stable manager suites; event requirements; camera terms and camera/manager integration; experimental frontend and reward parity; task curricula; PhysX, Newton and OVPhysX event integration. - Confirmed the default-resolution regression fails when default population is disabled, and the Gaussian regression fails against the original mass constructor with the reported bound-order error. - `uv run isaaclab -f` and the changelog gate passed against the upstream PR base; GitHub pre-commit and changelog checks also passed. - Clean `uv run isaaclab --docs` build passed with warnings treated as errors; the final incremental build also passed. - Existing dependency deprecation warnings remain in test output. ## Type of change - Bug fix ## Release backport - [x]  Backport this pull request to the active release branch after it merges into `develop` ## Checklist - [x] I have read and understood the contribution guidelines - [x] I have run the pre-commit checks with `uv run isaaclab --format` - [x] I have made corresponding changes to the documentation - [x] My changes generate no new warnings - [x] I have validated the changed behavior with focused coverage and applied the test-audit criteria - [x] I have added a changelog fragment for every touched package - [x] My name already exists in `CONTRIBUTORS.md` --------- Co-authored-by: Octi Zhang <zhengyuz@nvidia.com> (cherry picked from commit e73d1f1)

isaaclab-bot · 2026-09-30T23:22:31Z

Backported to release/3.0.0 as 4f3f2dc.

# Description Restores the `CameraImageBase` fallback to a subclass's `default_data_type` when observation term parameters do not include `data_type`. `image_normals` intentionally reads the fixed `"normals"` output and does not expose a public `data_type` argument. The strict parameter lookup introduced in #8127 therefore raises `KeyError: 'data_type'` when the observation manager constructs the term. Falling back in the base class keeps the existing `image_normals` API unchanged while preserving explicit `data_type` configuration for the other image terms. Fixes NVBUG 6886551. No new dependencies. ## Type of change - Bug fix (non-breaking change which fixes an issue) ## Release backport Not applicable: this pull request targets `release/3.0.0` directly. ## Screenshots Not applicable; this fixes an observation-term construction failure. ## Validation - Confirmed the regression test fails before the fix with `KeyError: 'data_type'`. - Camera-image unit tests: 25 passed. - `uv run isaaclab -f`: all code and style hooks passed. - The changelog validator passed against the `upstream/release/3.0.0` tip. ## Checklist - [x] I have read and understood the [contribution guidelines](https://isaac-sim.github.io/IsaacLab/main/source/refs/contributing.html) - [x] I have run the [`pre-commit` checks](https://pre-commit.com/) with `uv run isaaclab --format` - [x] I have made corresponding changes to the documentation - [x] My changes generate no new warnings - [x] I have validated the changed behavior with focused coverage and applied the [test-audit criteria](https://github.com/isaac-sim/IsaacLab/blob/develop/skills/developer/test-audit/SKILL.md) to the test change - [x] I have added a changelog fragment under `source/<pkg>/changelog.d/` for every touched package - [x] I have added my name to the `CONTRIBUTORS.md` or my name already exists there

Since isaac-sim#8127 the base class reads data_type from the term parameters, which the manager fills from each subclass's __call__ signature. The default_data_type attribute was no longer read and suggested a fallback that does not exist, which is how image_normals ended up without a data_type argument.

# Description Adds the missing `data_type: str = "normals"` parameter to `image_normals` and uses it to select the camera output. The new parameter is keyword-only so the existing positional order for `normalize`, `channel_first`, and `frame_stack` remains compatible. Manager terms resolve callable defaults into `cfg.params` before construction. Unlike the other per-modality image terms, `image_normals` did not declare its data type in the callable signature, so the strict `CameraImageBase` constructor raised `KeyError: 'data_type'`. This change fixes the signature rather than adding a fallback to the base class, keeping default resolution consistent with #8127. Fixes NVBUG 6886551 No new dependencies. ## Type of change - Bug fix (non-breaking change which fixes an issue) ## Release backport - [ ]  Backport this pull request to the active release branch after it merges into `develop` The release-branch compatibility fix is already open as #8373, so an automated duplicate backport is not requested. ## Screenshots Not applicable; this fixes observation-term construction. ## Validation - Confirmed the default-resolution regression test fails on the unmodified `develop` branch with `KeyError: 'data_type'`. - Confirmed the positional-compatibility regression test fails before the follow-up fix with `KeyError: False` and passes once `data_type` is keyword-only. - Camera-image unit tests: 26 passed. - `uv run isaaclab -f`: passed, including the changelog gate against `upstream/develop`. - `uv run --isolated --extra dev --extra ov -- make -C docs current-docs`: passed without warnings. ## Checklist - [x] I have read and understood the [contribution guidelines](https://isaac-sim.github.io/IsaacLab/main/source/refs/contributing.html) - [x] I have run the [`pre-commit` checks](https://pre-commit.com/) with `uv run isaaclab --format` - [x] I have made corresponding changes to the documentation - [x] My changes generate no new warnings - [x] I have validated the changed behavior with focused coverage and applied the [test-audit criteria](https://github.com/isaac-sim/IsaacLab/blob/develop/skills/developer/test-audit/SKILL.md) to the test change - [x] I have added a changelog fragment under `source/<pkg>/changelog.d/` for every touched package - [x] I have added my name to the `CONTRIBUTORS.md` or my name already exists there --------- Co-authored-by: Antoine RICHARD <antoiner@nvidia.com>

github-actions Bot added bug Something isn't working isaac-lab Related to Isaac Lab team labels Sep 29, 2026

StafaH marked this pull request as ready for review September 29, 2026 05:33

StafaH requested a review from a team September 29, 2026 05:33

StafaH requested review from AntoineRichard, fatimaanes, huidongc, hujc7, kellyguo11, marcodiiga, ooctipus, pbarejko and rilei-nvidia as code owners September 29, 2026 05:33

greptile-apps Bot reviewed Sep 29, 2026

View reviewed changes

isaaclab-review-bot Bot reviewed Sep 29, 2026

View reviewed changes

ooctipus approved these changes Sep 30, 2026

View reviewed changes

Resolve manager term defaults and validate randomization distributions

2220fb3

StafaH changed the title ~~Resolve manager term defaults and validate randomization distributions~~ [Core] Resolve manager term defaults and validate randomization distributions Sep 30, 2026

Update changelog fragments to the current format

9d409eb

ooctipus force-pushed the fix/term-defaults-distribution-validation branch from 52b6a0e to 9d409eb Compare September 30, 2026 20:52

isaaclab-bot Bot added ci:run-docker Trigger the on-demand Docker and GPU CI workflow and removed ci:run-docker Trigger the on-demand Docker and GPU CI workflow labels Sep 30, 2026

Use event configs in OVPhysX material tests

b679225

isaaclab-bot Bot added ci:run-docker Trigger the on-demand Docker and GPU CI workflow and removed ci:run-docker Trigger the on-demand Docker and GPU CI workflow labels Sep 30, 2026

ooctipus merged commit e73d1f1 into isaac-sim:develop Sep 30, 2026
85 of 88 checks passed

StafaH mentioned this pull request Oct 1, 2026

Validate gaussian scale randomization parameters as mean and std #8123

Closed

7 of 8 tasks

This was referenced Oct 7, 2026

[Release 3.0.0] Restore default camera image data type #8373

Merged

[Core] Fix image normals data type resolution #8375

Merged

StafaH deleted the fix/term-defaults-distribution-validation branch October 10, 2026 19:44

Navigation Menu

Repository navigation

[Core] Resolve manager term defaults and validate randomization distributions - #8127

[Core] Resolve manager term defaults and validate randomization distributions#8127
ooctipus merged 3 commits into
isaac-sim:developfrom
StafaH:fix/term-defaults-distribution-validation

StafaH commented Sep 29, 2026 •

edited by ooctipus

Loading

Uh oh!

greptile-apps Bot commented Sep 29, 2026 •

edited

Loading

Uh oh!

greptile-apps Bot Sep 29, 2026

Uh oh!

isaaclab-review-bot Bot left a comment

Uh oh!

ooctipus commented Sep 30, 2026

Uh oh!

ooctipus commented Sep 30, 2026

Uh oh!

ooctipus commented Sep 30, 2026

Uh oh!

ooctipus commented Sep 30, 2026

Uh oh!

ooctipus commented Sep 30, 2026

Uh oh!

Uh oh!

isaaclab-bot Bot commented Sep 30, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

	if cfg.params["make_consistent"]:
	if cfg.params.get("make_consistent", False):

Conversation

StafaH commented Sep 29, 2026 • edited by ooctipus Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Validation

Type of change

Release backport

Checklist

Uh oh!

greptile-apps Bot commented Sep 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Confidence Score: 4/5

Findings

Summary

Diagram

Uh oh!

greptile-apps Bot Sep 29, 2026

Choose a reason for hiding this comment

Uh oh!

isaaclab-review-bot Bot left a comment

Choose a reason for hiding this comment

Isaac Lab Review Bot

Uh oh!

ooctipus commented Sep 30, 2026

Uh oh!

ooctipus commented Sep 30, 2026

Uh oh!

ooctipus commented Sep 30, 2026

Uh oh!

ooctipus commented Sep 30, 2026

Uh oh!

ooctipus commented Sep 30, 2026

Uh oh!

Uh oh!

isaaclab-bot Bot commented Sep 30, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

StafaH commented Sep 29, 2026 •

edited by ooctipus

Loading

greptile-apps Bot commented Sep 29, 2026 •

edited

Loading