Fixes attribute reordering in configclass with partial type annotations#6157
Fixes attribute reordering in configclass with partial type annotations#61570xadvait wants to merge 1 commit into
Conversation
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>
Greptile SummaryThis PR fixes a long-standing attribute reordering bug in
Confidence Score: 4/5Safe 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
Reviews (1): Last reviewed commit: "Fix configclass field reordering with mi..." | Re-trigger Greptile |
| # 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) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
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 forInteractiveSceneCfg, where the attribute order determines the scene entity creation order.Root cause:
_add_annotation_typesbulk-adds each base class's__annotations__into the type-hints dictionary before iterating over the class members (hints.update(ann)prior to thebase.__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 trailinghints.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),
MISSINGhandling,ClassVarhandling, and nested-class handling all behave as before.Verification
test_configclass_mixed_type_annotations_ordering(mirrors the issue repro, plus an inheritance case). It fails before the fix and passes after.test_configclass.pysuite passes (44 tests).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).source/found 81 configclasses whose field order changes with this fix (i.e., classes currently affected by the silent reordering), of which ~15 areInteractiveSceneCfgsubclasses. 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
Checklist
pre-commitchecks with./isaaclab.sh --formatconfig/extension.tomlfileCONTRIBUTORS.mdor my name already exists there