fix: Handle list[Model], dict[str, Model] and Optional[Model] in nested type detection#630
fix: Handle list[Model], dict[str, Model] and Optional[Model] in nested type detection#630andreahlert wants to merge 2 commits intoflyteorg:mainfrom
Conversation
cc2218c to
2ebaea8
Compare
|
@wild-endeavor @kumare3 Hey! Picked up item 1 from #6887 - the list/map gap in PR #426.
Bonus bug: optional fields with defaults were silently skipped because |
|
cc @AdilFayyaz can you PTAL? |
2ebaea8 to
9a3f813
Compare
…ed type detection The nested schema type detection from PR flyteorg#426 did not resolve $ref for list items, dict additionalProperties, or anyOf (Optional) fields. This caused KeyError or incorrect type inference when using Pydantic or dataclass models with fields like list[NestedModel], dict[str, NestedModel], or Optional[NestedModel]. Changes: - Add $ref resolution to _get_element_type for centralized handling - Extract _resolve_ref, _resolve_property_type, _resolve_single_type, and _resolve_typed_property helpers to eliminate duplicated $ref logic - Properly validate Optional via anyOf (check len==2 and null second element) - Add default=None for optional fields not in required list - Include optional properties in property_order so fields with defaults are not skipped during schema traversal - Extend __init__ wrapper to convert list[dict] and dict[str, dict] to their respective nested types Ref: flyteorg/flyte#6887 Signed-off-by: André Ahlert <andre@aex.partners>
9a3f813 to
76597a5
Compare
|
Thank you for working on this. I do have some concerns, I believe Can you please confirm if these cases pass, as I don't see any tests on recursive/nested components:
|
You're right. _get_element_tpe doesn't handle nested arrays or objects recursively, so List[List[int]] and List[Dict[str, int]] would break. List[Optional[int]] should work since we already handle anyOf`. Will add the recursive cases and tests for all three. |
|
Good catch. Pushed a fix - _get_element_type now recurses into array items and object additionalProperties, so List[List[int]] and List[Dict[str, int]] work correctly. Added roundtrip tests for all three cases (List[List[int]], List[Dict[str, int]], List[Optional[int]]), all passing. |
Address review feedback: _get_element_type now handles nested containers (List[List[int]], List[Dict[str, int]]) by recursing into array items and object additionalProperties. Also adds List[Optional[int]] confirmation via tests.
2ea6160 to
3fd84ca
Compare
|
Thanks for the contribution. We were able to handle this issue ourselves in the meantime. Here's the relevant PR: #640 |
Motivation
PR #426 added support for nested Pydantic/dataclass schema detection via
$ref, but as noted in flyteorg/flyte#6887 (item 1), it does not handlelistandmapcontainers with nested models.When a schema contains
list[NestedModel],dict[str, NestedModel], orOptional[NestedModel], the$refinsideitems,additionalProperties, oranyOfwas not resolved, causingKeyErroror incorrect type inference (falling back tostr).Changes
Centralized
$refresolutionInstead of inlining
$refresolution at every call site, this PR follows the same approach as flytekit PR #3375:_get_element_typenow accepts an optionalschemaparameter and resolves$refcentrally. This means any container type (list,dict,Optional, or nested combinations) automatically gets$refsupport.New helper functions
_resolve_ref: Centralized$refpath resolution to schema definition_resolve_property_type: Resolves a JSON schema property to a(name, type)tuple, handling$ref,anyOf, and all type variants_resolve_single_type: Resolves the inner type ofOptional[T]from the firstanyOfelement, properly handling$ref,array,object, and primitive types_resolve_typed_property: Resolves a property with a known type string (array, object, primitives)_is_optional_anyof: Validates that ananyOftruly representsOptional[T](exactly 2 elements, second isnull) instead of blindly assuming anyanyOfwith$refis OptionalBug fixes
Optional[T]default values: Fields not in the schema'srequiredlist now getdefault=Nonein the generated dataclass, so constructing with omitted optional fields works correctlyOptional[List[Model]]/Optional[Dict[str, Model]]: Previously would crash withKeyErrorbecause the code accessedproperty_val["items"]instead ofanyOf[0]["items"]- now handled correctly via_resolve_single_typeUnion[TypeA, TypeB]: Previously anyanyOfwith$refwas blindly wrapped inOptional- now properly validatedrequiredare included inproperty_orderso fields with defaults (likeOptional[X] = None) are not skipped during schema traversal__init__wrapperExtended dict-to-object conversion to handle
list[dict]->list[Model]anddict[str, dict]->dict[str, Model].Tests
Added
test_nested_lists_maps.pywith 10 test cases covering:list[NestedModel]roundtrip (Pydantic + dataclass)dict[str, NestedModel]roundtrip (Pydantic + dataclass)Optional[NestedModel]with value and withNoneOptional[NestedModel]omitted (default toNone)[],{})All existing type engine tests pass with zero regressions.
Ref: flyteorg/flyte#6887