Fix update_class_from_dict rejecting union-annotated overrides - #7873
harshal-96 wants to merge 2 commits into
Conversation
update_class_from_dict checked an override value's type against the stored value's runtime type. A field annotated as a union and currently holding one member of it (e.g. seed: int | None defaulting to None) therefore rejected every override of the other member type with '[Config]: Incorrect type under namespace'. Consult the field's type annotation before rejecting a value whose type differs from the stored value's. Objects without a resolvable annotation for the key keep the stored-value check, so plain dicts and unannotated attributes behave as before. Fixes isaac-sim#3236 Signed-off-by: harshal-96 <harshal.dhandrut@gmail.com>
github-actions
Bot
added
bug
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if origin is not None: | ||
| return isinstance(value, origin) |
There was a problem hiding this comment.
A valid override for an existing Literal[...] | None field, such as drive_type: Literal["force", "acceleration"] | None = None, reaches this code with origin set to typing.Literal. Because typing.Literal cannot be passed to isinstance, the update raises TypeError instead of accepting the valid literal value. Invalid values can also leak TypeError instead of the documented ValueError.
| try: | ||
| hints = get_type_hints(type(obj)) | ||
| except Exception: | ||
| # unresolvable forward references or exotic annotations; fall back to | ||
| # the stored-value type check | ||
| return False |
There was a problem hiding this comment.
Resolving every annotation on the class means one unrelated unresolved forward reference disables annotation-based validation for all fields. VisualizerCfg, for example, references BaseVisualizer, which is imported only under TYPE_CHECKING; resolving that class's hints therefore fails and returns False. Valid updates to its other union-annotated fields then fall back to the stored NoneType check and are rejected, so the union fix remains incomplete for these config classes.
![isaaclab-review-bot[bot]](https://avatars.githubusercontent.com/u/157846462?s=60&v=4){kind=small}
There was a problem hiding this comment.
Isaac Lab Review Bot
The targeted relaxation fixes union-annotated scalar overrides while preserving prior successful paths, but the annotation walker can raise TypeError for union members whose typing origin is not a runtime class.
- Design and architecture: The private helpers keep annotation handling localized to the previously fatal scalar mismatch branch. However, the generic-origin abstraction assumes all origins are valid
isinstancetargets, which is not true for typing special forms such asLiteral. - API: The public signature remains unchanged and ordinary union overrides are widened as intended. For annotations such as
Literal["a", "b"] | None, however, an override can now leakTypeErrorrather than completing the update or producing the documented mismatchValueError. - Implementation: Union recursion,
NoneType, and plain runtime classes are handled correctly. The unconditionalisinstance(value, origin)must guard against non-class typing origins or otherwise handle them explicitly so annotation inspection cannot crash.
Minor fixes needed. Posted 1 actionable finding inline.
Automated review; human maintainers own approval decisions.
| if origin is Union or origin is types.UnionType: | ||
| return any(_annotation_admits_value(arg, value) for arg in get_args(annotation)) | ||
| if origin is not None: | ||
| return isinstance(value, origin) |
There was a problem hiding this comment.
🟡 Warning · Implementation — Non-class annotation origins crash isinstance check
get_origin() returns non-class special forms for annotations such as Literal[...] or ClassVar[...]. For a field annotated Literal["a", "b"] | None = None holding None, an override reaches the union recursion and then isinstance(value, typing.Literal), raising TypeError: typing.Literal cannot be used with isinstance() instead of accepting the value or raising the ValueError documented by update_class_from_dict. Guard with isinstance(origin, type) and return False otherwise.
…ion checks Address the two review findings on the annotation-based type check: - A Literal member of a union (e.g. mode: Literal['force', 'acceleration'] | None) reached isinstance(value, typing.Literal), which raises TypeError. Literal members are now checked by value, with type equality so bools do not match int literals, and other non-class typing origins are not admitted instead of raising. - typing.get_type_hints resolves every annotation on the class at once, so one name imported only under TYPE_CHECKING (e.g. BaseVisualizer in VisualizerCfg) disabled the union fix for all fields of that class. The lookup now walks the MRO and evaluates only the requested field's annotation, and when that annotation is itself a union with unresolvable members it evaluates the members individually, so class_type: type[BaseVisualizer] | str | None accepts a string override through the str member. Unresolvable members are skipped, which can only widen acceptance, never reject values the stored-value check would have accepted. Signed-off-by: harshal-96 <harshal.dhandrut@gmail.com>
|
Both findings addressed in 922a624. Literal members of a union are now checked by value, with type equality so bools do not match int literals, and non-class typing origins are not admitted instead of raising. The annotation lookup no longer uses get_type_hints: it walks the MRO and evaluates only the requested field's annotation, and for a string union with unresolvable members it evaluates the members individually, so class_type on VisualizerCfg accepts a string override through the str member. Added three regression tests covering the Literal case, the unresolvable sibling field, and the partially resolvable union; all fail on the previous head and pass now. Full suites: 62 passed under both a plain Python 3.12 env and the Isaac Sim 6.0 bundled python. |
This branch has not been deployed
1 participant
Description
update_class_from_dictchecks an override value's type against the stored value's runtime type. A field annotated as a union and currently holding one member of it therefore rejects every override of the other member type. The reported case:seed: int | Nonedefaults toNone, sofrom_dict({"seed": 123})on an environment config raises[Config]: Incorrect type under namespace: /seed. Expected: <class 'NoneType'>, Received: <class 'int'>.This PR makes the type check consult the field's type annotation before rejecting a value whose type differs from the stored value's:
_annotation_admits_valuechecks a value against an annotation, member-wise for unions (bothX | Yandtyping.Union[X, Y]) and against the origin container for parameterized generics, mirroring the runtime check it backs up._field_annotation_admits_valueresolves the annotation viatyping.get_type_hintson the object's class. When no annotation can be resolved (plain dicts, unannotated attributes, unresolvable forward references), it returns False and the stored-value check stays authoritative, so existing behavior is preserved everywhere outside the union case.The acceptance criterion from the issue (the MRE runs without raising) now holds:
Fixes #3236
Type of change
Testing
Three new regression tests in
source/isaaclab/test/utils/test_configclass.py:test_config_update_dict_union_annotated_none: the reported failure; fails on develop, passes with the fix.test_config_update_dict_union_rejects_non_member_type: a value outside the union is still rejected.test_config_update_dict_type_mismatch_without_annotation: unannotated attributes keep the stored-value type check.Results (Windows, Isaac Sim 6.0.0-rc.59 install, its bundled Python 3.12.13 / torch 2.10.0+cu128):
pytest source/isaaclab/test/utils/test_dict.py source/isaaclab/test/utils/test_configclass.py: 59 passed (56 baseline + 3 new), run with the Isaac Simpython.bat.update_class_from_dictdoes not handle union types properly #3236 (withAppLauncher(headless=True, kit_args="--no-window")booting the real simulator): raises the reportedValueErroron develop, printsparam = 42with this PR.ruff check --fixandruff format(pinned v0.14.10 from.pre-commit-config.yaml) produce no further changes;tools/changelog/cli.py check developpasses.Checklist
source/<pkg>/changelog.d/for every touched packageCONTRIBUTORS.mdor my name already exists there