-
Notifications
You must be signed in to change notification settings - Fork 299
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
[wip] JSON IDL V2 #2741
[wip] JSON IDL V2 #2741
Conversation
Signed-off-by: Future-Outlier <[email protected]>
def to_json(self, ctx: FlyteContext, python_val: T, python_type: Type[T], expected: LiteralType) -> Literal: | ||
if isinstance(python_val, dict): | ||
json_str = json.dumps(python_val) | ||
json_bytes = json_str.encode("UTF-8") | ||
return Literal(scalar=Scalar(json=Json(value=json_bytes, serialization_format="UTF-8"))) | ||
|
||
if not dataclasses.is_dataclass(python_val): |
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.
I don't want to reuse the code in to_literal
and to_python_val
because I think this will be far more readable and more easier to customize behavior in the future when we want to have more flexible change to the JSON IDL object.
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.
I haven't come up with the scenario, but this is just my instinct.
flytekit/core/type_engine.py
Outdated
serialization_format = json_idl_object.serialization_format | ||
json_str = None | ||
if serialization_format == "UTF-8": | ||
json_str = value.decode("UTF-8") | ||
if json_str is None: | ||
json_str = value.decode("UTF-8") # default to UTF-8 |
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.
serialization_format = json_idl_object.serialization_format | |
json_str = None | |
if serialization_format == "UTF-8": | |
json_str = value.decode("UTF-8") | |
if json_str is None: | |
json_str = value.decode("UTF-8") # default to UTF-8 | |
serialization_format = json_idl_object.serialization_format or "UTF-8" | |
json_str = None | |
if serialization_format == "UTF-8": | |
json_str = value.decode("UTF-8") | |
else: | |
raise ValueError(...) |
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.
will do this later, thank you.
Signed-off-by: Future-Outlier <[email protected]> Co-authored-by: pingsutw <[email protected]>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2741 +/- ##
===========================================
- Coverage 78.61% 45.20% -33.42%
===========================================
Files 193 194 +1
Lines 19691 19850 +159
Branches 4101 2887 -1214
===========================================
- Hits 15481 8973 -6508
- Misses 3492 10424 +6932
+ Partials 718 453 -265 ☔ View full report in Codecov by Sentry. |
Signed-off-by: Future-Outlier <[email protected]>
Signed-off-by: Future-Outlier <[email protected]>
Signed-off-by: Future-Outlier <[email protected]>
Signed-off-by: Future-Outlier <[email protected]>
flytekit/core/type_engine.py
Outdated
else: | ||
raise ValueError( | ||
f"Bytes can't be converted to JSON String.\n" | ||
f"Unsupported serialization format: {serialization_format}" | ||
) | ||
return json.loads(json_str) |
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.
Questoin: Should we assert expected_python_type == type(json.loads(json_str))
?
Signed-off-by: Future-Outlier <[email protected]>
flytekit/core/type_engine.py
Outdated
python_val = json.loads(json_str) | ||
expected_python_val = expected_python_type(python_val) |
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.
Signed-off-by: Future-Outlier <[email protected]>
Signed-off-by: Future-Outlier <[email protected]>
Signed-off-by: Future-Outlier <[email protected]>
Signed-off-by: Future-Outlier <[email protected]>
Signed-off-by: Future-Outlier <[email protected]>
flytekit/core/type_engine.py
Outdated
def from_json(self, ctx: FlyteContext, json_idl_object: Json, expected_python_type: Type[T]) -> typing.List[T]: | ||
""" | ||
Process JSON IDL object and convert it to the corresponding Python value. | ||
Handles both simple types and recursive structures like List[List[int]] or List[List[float]]. | ||
""" | ||
|
||
def recursive_from_json(ctx: FlyteContext, json_value: typing.Any, expected_python_type: Type[T]) -> typing.Any: | ||
""" | ||
Recursively process JSON objects, converting them to their corresponding Python values based on | ||
the expected Python type (e.g., handling List[List[int]] or List[List[float]]). | ||
""" | ||
# Check if the type is a List | ||
if typing.get_origin(expected_python_type) is list: | ||
# Get the subtype, which should be the type of the list's elements | ||
sub_type = self.get_sub_type(expected_python_type) | ||
# Recursively process each element in the list | ||
return [recursive_from_json(ctx, item, sub_type) for item in json_value] | ||
|
||
# Check if the type is a Dict | ||
elif typing.get_origin(expected_python_type) is dict: | ||
# For Dicts, get key and value types | ||
key_type, val_type = typing.get_args(expected_python_type) | ||
# Recursively process each key and value in the dict | ||
return {recursive_from_json(ctx, k, key_type): recursive_from_json(ctx, v, val_type) for k, v in | ||
json_value.items()} | ||
|
||
# Base case: if it's not a list or dict, we assume it's a simple type and return it | ||
try: | ||
return expected_python_type(json_value) # Cast to the expected type | ||
except Exception as e: | ||
raise ValueError(f"Could not cast {json_value} to {expected_python_type}: {e}") | ||
|
||
# Handle the serialization format | ||
value = json_idl_object.value | ||
serialization_format = json_idl_object.serialization_format | ||
if serialization_format == "UTF-8": | ||
# Decode JSON string | ||
json_value = json.loads(value.decode("utf-8")) | ||
else: | ||
raise ValueError(f"Unknown serialization format {serialization_format}") | ||
|
||
# Call the recursive function to handle nested structures | ||
return recursive_from_json(ctx, json_value, expected_python_type) | ||
|
||
def to_python_value(self, ctx: FlyteContext, lv: Literal, expected_python_type: Type[T]) -> typing.List[typing.Any]: # type: ignore | ||
scalar = lv.scalar | ||
if scalar and scalar.json: | ||
return self.from_json(ctx, scalar.json, expected_python_type) |
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.
Hi, @wild-endeavor
Does this look good to you?
propeller will return json idl object
to the List Transformer, and we will handle it recursively.
propeller get the list json str: https://github.com/flyteorg/flyte/pull/5735/files#diff-ee7f936e440a7e043b3bc7acb4ea255ba991dea8f3144d24ab276c3a292de018R103-R113
Signed-off-by: Future-Outlier <[email protected]>
Signed-off-by: Future-Outlier <[email protected]>
…propeller and get literal type error in dataclass (needs to use type hints) Signed-off-by: Future-Outlier <[email protected]>
hints = get_type_hints(t) | ||
# Get the type of each field from dataclass | ||
for field in t.__dataclass_fields__.values(): # type: ignore | ||
try: | ||
literal_type[field.name] = TypeEngine.to_literal_type(field.type) | ||
name = field.name | ||
python_type = hints.get(field.name, field.type) | ||
literal_type[name] = TypeEngine.to_literal_type(python_type) |
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.
This is for issue here: flyteorg/flyte#5427
If we use t.__dataclass_fields__.values()
only, we will set sub_fruit
as str
, but not JSON
.
…need that Signed-off-by: Future-Outlier <[email protected]>
Flyte Backend
flyteorg/flyte#5735
Tracking issue
flyteorg/flyte#5318
Why are the changes needed?
What changes were proposed in this pull request?
The attribute access roadmap:
get_literal_type
error: [BUG] Accessing attributes fails on complex types flyte#5427dataclass with flyte types
and accessdataclass with flyte types
dataclass with flyte types
and accessflyte types
How was this patch tested?
List and nested List
code
Dict Transformer
code
Dataclass Transformer
code
Setup process
Screenshots
List Transformer
Dict Transformer
Dataclass Transformer
Check all the applicable boxes
Related PRs
Docs link