-
Notifications
You must be signed in to change notification settings - Fork 76
Fix to allow Literal and Union via | in csp annotations #476
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -35,7 +35,11 @@ def __new__(cls, name, bases, dct): | |
| # Lists need to be normalized too as potentially we need to add a boolean flag to use FastList | ||
| if v == FastList: | ||
| raise TypeError(f"{v} annotation is not supported without args") | ||
| if CspTypingUtils.is_generic_container(v) or CspTypingUtils.is_union_type(v): | ||
| if ( | ||
| CspTypingUtils.is_generic_container(v) | ||
| or CspTypingUtils.is_union_type(v) | ||
| or CspTypingUtils.is_literal_type(v) | ||
| ): | ||
| actual_type = ContainerTypeNormalizer.normalized_type_to_actual_python_type(v) | ||
| if CspTypingUtils.is_generic_container(actual_type): | ||
| raise TypeError(f"{v} annotation is not supported as a struct field [{actual_type}]") | ||
|
|
@@ -147,6 +151,17 @@ def serializer(val, handler): | |
|
|
||
|
|
||
| class Struct(_csptypesimpl.PyStruct, metaclass=StructMeta): | ||
| @classmethod | ||
| def type_adapter(cls): | ||
| # Late import to avoid autogen issues | ||
| from pydantic import TypeAdapter | ||
|
|
||
| internal_type_adapter = getattr(cls, "_pydantic_type_adapter", None) | ||
| if internal_type_adapter: | ||
| return internal_type_adapter | ||
| cls._pydantic_type_adapter = TypeAdapter(cls) | ||
| return cls._pydantic_type_adapter | ||
|
|
||
| @classmethod | ||
| def metadata(cls, typed=False): | ||
| if typed: | ||
|
|
@@ -191,7 +206,8 @@ def _obj_from_python(cls, json, obj_type): | |
| if CspTypingUtils.is_generic_container(obj_type): | ||
| if CspTypingUtils.get_origin(obj_type) in (typing.List, typing.Set, typing.Tuple, FastList): | ||
| return_type = ContainerTypeNormalizer.normalized_type_to_actual_python_type(obj_type) | ||
| (expected_item_type,) = obj_type.__args__ | ||
| # We only take the first item, so like for a Tuple, we would ignore arguments after | ||
| expected_item_type = obj_type.__args__[0] | ||
| return_type = list if isinstance(return_type, list) else return_type | ||
| return return_type(cls._obj_from_python(v, expected_item_type) for v in json) | ||
| elif CspTypingUtils.get_origin(obj_type) is typing.Dict: | ||
|
|
@@ -206,6 +222,13 @@ def _obj_from_python(cls, json, obj_type): | |
| return json | ||
| else: | ||
| raise NotImplementedError(f"Can not deserialize {obj_type} from json") | ||
| elif CspTypingUtils.is_union_type(obj_type): | ||
| return json ## no checks, just let it through | ||
| elif CspTypingUtils.is_literal_type(obj_type): | ||
| return_type = ContainerTypeNormalizer.normalized_type_to_actual_python_type(obj_type) | ||
| if isinstance(json, return_type): | ||
| return json | ||
| raise ValueError(f"Expected type {return_type} received {json.__class__}") | ||
| elif issubclass(obj_type, Struct): | ||
| if not isinstance(json, dict): | ||
| raise TypeError("Representation of struct as json is expected to be of dict type") | ||
|
|
@@ -223,7 +246,9 @@ def _obj_from_python(cls, json, obj_type): | |
| return obj_type(json) | ||
|
|
||
| @classmethod | ||
| def from_dict(cls, json: dict): | ||
| def from_dict(cls, json: dict, use_pydantic: bool = False): | ||
| if use_pydantic: | ||
| return cls.type_adapter().validate_python(json) | ||
| return cls._obj_from_python(json, cls) | ||
|
|
||
| def to_dict_depr(self): | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should we have an equivalent pydantic to_dict?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. sure!
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @robambalu Should we just remove to_dict_depr now? |
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -81,21 +81,21 @@ def normalized_type_to_actual_python_type(cls, typ, level=0): | |
| return [cls.normalized_type_to_actual_python_type(typ.__args__[0], level + 1), True] | ||
| if origin is typing.List and level == 0: | ||
| return [cls.normalized_type_to_actual_python_type(typ.__args__[0], level + 1)] | ||
| if origin is typing.Literal: | ||
| # Import here to prevent circular import | ||
| from csp.impl.types.instantiation_type_resolver import UpcastRegistry | ||
|
|
||
| args = typing.get_args(typ) | ||
| typ = type(args[0]) | ||
| for arg in args[1:]: | ||
| typ = UpcastRegistry.instance().resolve_type(typ, type(arg), raise_on_error=False) | ||
| if typ: | ||
| return typ | ||
| else: | ||
| return object | ||
| return cls._NORMALIZED_TYPE_MAPPING.get(CspTypingUtils.get_origin(typ), typ) | ||
| elif CspTypingUtils.is_union_type(typ): | ||
| return object | ||
| elif CspTypingUtils.is_literal_type(typ): | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I realize we just moved code here, but looks like we can probably use normalized_type_to_actual_python_type here?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. But literal takes instances of types, not types (so like Literal[7] versus Literal[int]) and normalized_type_to_actual_python_type doesnt seem to actually pull the true base class out except in this case for Literal |
||
| # Import here to prevent circular import | ||
| from csp.impl.types.instantiation_type_resolver import UpcastRegistry | ||
|
|
||
| args = typing.get_args(typ) | ||
| typ = type(args[0]) | ||
| for arg in args[1:]: | ||
| typ = UpcastRegistry.instance().resolve_type(typ, type(arg), raise_on_error=False) | ||
| if typ: | ||
| return typ | ||
| else: | ||
| return object | ||
| else: | ||
| return typ | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than an arg can we base this on whether we are using pydantic type checking ( which defaults to true now )?
My only concern is speed. If we can do some timing tests and pydantic is as fast as the current code, I would just default it to use pydantic ( and eventually remove the old impl )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The old way is a bit permissive and there will be some inconsistencies that might be annoying to fix, so I figured itd be easier to make it a flag for now. But eventually would like to switch to pydantic if its performant enough