Skip to content

Fixes attribute reordering in configclass with partial type annotations#6157

Open
0xadvait wants to merge 1 commit into
isaac-sim:developfrom
0xadvait:0xadvait/fix-configclass-ordering
Open

Fixes attribute reordering in configclass with partial type annotations#6157
0xadvait wants to merge 1 commit into
isaac-sim:developfrom
0xadvait:0xadvait/fix-configclass-ordering

Conversation

@0xadvait

Copy link
Copy Markdown

Description

When type annotations are provided on only some attributes of a configclass, the resulting field order does not follow the declaration order: all annotated attributes jump ahead of the non-annotated ones. This breaks the documented guarantee that attribute order is preserved, which matters in particular for InteractiveSceneCfg, where the attribute order determines the scene entity creation order.

@configclass
class SceneCfg(InteractiveSceneCfg):
    plane = AssetBaseCfg(...)
    robot = ArticulationCfg(...)
    peg: ArticulationCfg = ArticulationCfg(...)
    hole: ArticulationCfg = ArticulationCfg(...)
    camera = TiledCameraCfg(...)
    light = AssetBaseCfg(...)

# before this fix:
# ['peg', 'hole', 'plane', 'robot', 'camera', 'light']
# with this fix:
# ['plane', 'robot', 'peg', 'hole', 'camera', 'light']

Root cause: _add_annotation_types bulk-adds each base class's __annotations__ into the type-hints dictionary before iterating over the class members (hints.update(ann) prior to the base.__dict__ walk), so annotated fields are always inserted first.

Fix: build the hints dictionary in a single pass over base.__dict__ (which preserves declaration order), picking up the explicit annotation when one exists and deducing the type from the default value otherwise. A trailing hints.update(ann) keeps the previous behavior for corner cases (annotation-only declarations, re-annotated inherited members, members skipped by _skippable_class_member) — for keys already present it only refreshes the type and keeps the position.

The semantics are otherwise unchanged: cross-base ordering (parent fields first, overrides keep the parent position), MISSING handling, ClassVar handling, and nested-class handling all behave as before.

Verification

  • New regression test test_configclass_mixed_type_annotations_ordering (mirrors the issue repro, plus an inheritance case). It fails before the fix and passes after.
  • Full test_configclass.py suite passes (44 tests).
  • The complete source/isaaclab/test/utils/ suite was run on CPU before and after the change with byte-identical results (the only failing tests are CUDA-/Nucleus-dependent and fail identically on both sides).
  • An AST scan over source/ found 81 configclasses whose field order changes with this fix (i.e., classes currently affected by the silent reordering), of which ~15 are InteractiveSceneCfg subclasses. For these, entity creation order now matches the declaration order, which is what the documentation promises (e.g., terrain/ground declared first now actually spawns first). Managers and scene entities are referenced by name, so no name-based lookups are affected.

Fixes #1949
Related to #1743 (the same hoisting also affected None-typed members before it was special-cased)

Type of change

  • Bug fix (non-breaking change which fixes an issue)

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

When type annotations were provided on only some attributes of a
configclass, all annotated attributes were collected into the type
hints before the non-annotated ones, causing them to jump ahead in
the resulting field order. This broke the documented guarantee that
attribute order follows the declaration order, which matters for
scene configs where entity order determines spawn order.

Build the hints dictionary while iterating over the class members in
declaration order, picking up the explicit annotation when present
and deducing the type from the default value otherwise. Annotations
without a corresponding class member are appended afterwards.

Fixes isaac-sim#1949

Signed-off-by: Advait Jayant <advait@vannalabs.ai>
@0xadvait 0xadvait requested a review from Mayankm96 as a code owner June 11, 2026 23:35
@github-actions github-actions Bot added bug Something isn't working isaac-lab Related to Isaac Lab team labels Jun 11, 2026
@greptile-apps

greptile-apps Bot commented Jun 11, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR fixes a long-standing attribute reordering bug in _add_annotation_types inside configclass.py, where annotated fields (with explicit type hints) were bulk-inserted into the hints dictionary before the member-iteration loop, causing them to always precede non-annotated fields regardless of declaration order.

  • The fix builds the hints dict in a single pass over base.__dict__ (which preserves Python insertion/declaration order), applying explicit annotations in-loop and deferring a trailing hints.update(ann) to pick up annotation-only declarations and refresh types on re-annotated inherited members.
  • A new regression test (test_configclass_mixed_type_annotations_ordering) covers both the base-class and inheritance cases and would have caught the original bug; all 44 existing tests continue to pass.

Confidence Score: 4/5

Safe to merge. The change is surgical and isolated to _add_annotation_types; existing semantics for all common configclass patterns are preserved and verified by the full test suite.

The fix is well-reasoned and the new test directly reproduces the reported bug. The one subtlety is that pure annotation-only fields (no default, not even MISSING) will now be appended after value-bearing members rather than before non-annotated ones — an unusual pattern in practice but a real behavioral difference not covered by the new tests.

A second look at source/isaaclab/isaaclab/utils/configclass.py lines 320-323 is worthwhile to confirm the comment is accurate for the annotation-only edge case and to decide whether an additional test is warranted.

Important Files Changed

Filename Overview
source/isaaclab/isaaclab/utils/configclass.py Core fix: moves hints.update(ann) from before the member-iteration loop to after it, and adds an in-loop if key in ann branch; correctly preserves declaration order for mixed-annotation classes while a trailing update handles annotation-only and re-annotated inherited members.
source/isaaclab/test/utils/test_configclass.py Adds two new test fixtures and one test function that directly reproduces the issue and the inheritance variant; assertions cover both __dict__ key order and to_dict() key order.
source/isaaclab/changelog.d/0xadvait-fix-configclass-ordering.rst New changelog fragment; accurately describes the bug and the affected surface. No issues.
CONTRIBUTORS.md Adds the contributor's name in alphabetical order. No issues.

Reviews (1): Last reviewed commit: "Fix configclass field reordering with mi..." | Re-trigger Greptile

Comment on lines +320 to +323
# add remaining annotations that do not have a corresponding class member (e.g. annotation-only
# declarations) or whose member was skipped above. For keys already present in the hints,
# this only refreshes the type and keeps their original position.
hints.update(ann)

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 Ordering of pure annotation-only fields shifts to end

For fields declared as field: SomeType with no default value (not even MISSING), the entry is absent from base.__dict__ and therefore never encountered in the loop. The trailing hints.update(ann) appends it to hints after all value-bearing members, so such fields now land at the end of the hints dict for that class rather than at their declaration position. In the old code hints.update(ann) ran first, so annotation-only fields appeared before non-annotated ones.

This is a corner case: configclasses almost universally assign MISSING to required fields (which are present in __dict__), so real-world impact should be negligible. That said, the comment's phrase "keeps the previous behavior" is not fully accurate for this sub-case — it might be worth adjusting the wording (and possibly adding a short test) to avoid future confusion.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Good eye -- this was considered. A field declared with a type annotation but no value at all (not even MISSING) cannot occur in a valid configclass: _process_mutable_types raises ValueError for it right after annotation processing, because the field appears in the annotations but has no corresponding class member (the error message explicitly directs users to assign dataclasses.MISSING when no default is wanted). With = MISSING assigned, the field is present in base.__dict__ and is interleaved at its declaration position like any other member.

The one reachable case the trailing hints.update(ann) serves is a subclass re-annotating an inherited field without assigning a new value -- there the key already exists in hints from the parent pass, so the update only refreshes the type and the position is preserved (dict updates keep insertion order). That behavior is unchanged from before this PR.

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.

1 participant