Fixes attribute reordering in configclass with partial type annotations - #6157
Conversation
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 |
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>
ca4e204 to
523d69a
Compare
|
Thanks a lot @0xadvait ! Merging this :) |
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