Skip to content
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][need discussion] Pydantic Transformer guess python type #3060

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
89 changes: 87 additions & 2 deletions flytekit/extras/pydantic_transformer/transformer.py
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
import json
import os
from typing import Type
from typing import Any, List, Optional, Type

import msgpack
from google.protobuf import json_format as _json_format
from google.protobuf import struct_pb2 as _struct
from pydantic import BaseModel
from pydantic import BaseModel, create_model

from flytekit import FlyteContext
from flytekit.core.constants import CACHE_KEY_METADATA, FLYTE_USE_OLD_DC_FORMAT, MESSAGEPACK, SERIALIZATION_FORMAT
Expand Down Expand Up @@ -103,5 +103,90 @@
python_val = expected_python_type.model_validate_json(json_str, strict=False, context={"deserialize": True})
return python_val

def guess_python_type(self, literal_type: LiteralType) -> Type[BaseModel]:
"""
Reconstructs the Pydantic BaseModel subclass from the JSON schema stored in LiteralType metadata.
"""

schema = literal_type.metadata
model_name = schema.get("title", "DynamicModel")

Check warning on line 112 in flytekit/extras/pydantic_transformer/transformer.py

View check run for this annotation

Codecov / codecov/patch

flytekit/extras/pydantic_transformer/transformer.py#L111-L112

Added lines #L111 - L112 were not covered by tests

properties = schema.get("properties", {})
required_fields = schema.get("required", [])

Check warning on line 115 in flytekit/extras/pydantic_transformer/transformer.py

View check run for this annotation

Codecov / codecov/patch

flytekit/extras/pydantic_transformer/transformer.py#L114-L115

Added lines #L114 - L115 were not covered by tests

annotations = {}
field_definitions = {}

Check warning on line 118 in flytekit/extras/pydantic_transformer/transformer.py

View check run for this annotation

Codecov / codecov/patch

flytekit/extras/pydantic_transformer/transformer.py#L117-L118

Added lines #L117 - L118 were not covered by tests

for field_name, field_info in properties.items():
field_type = self._map_json_type_to_python(field_info)
annotations[field_name] = Optional[field_type] if field_name not in required_fields else field_type
field_definitions[field_name] = (field_type, ... if field_name in required_fields else None)

Check warning on line 123 in flytekit/extras/pydantic_transformer/transformer.py

View check run for this annotation

Codecov / codecov/patch

flytekit/extras/pydantic_transformer/transformer.py#L121-L123

Added lines #L121 - L123 were not covered by tests

try:
DynamicModel = create_model(model_name, **field_definitions)
return DynamicModel
except Exception as e:
raise TypeTransformerFailedError(f"Failed to create Pydantic model from schema: {e}")

Check warning on line 129 in flytekit/extras/pydantic_transformer/transformer.py

View check run for this annotation

Codecov / codecov/patch

flytekit/extras/pydantic_transformer/transformer.py#L125-L129

Added lines #L125 - L129 were not covered by tests
Comment on lines +128 to +129
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Improve exception handling patterns

Exception handling can be improved by using 'raise ... from err' pattern and avoiding f-strings in exceptions.

Code suggestion
Check the AI-generated fix before applying
Suggested change
except Exception as e:
raise TypeTransformerFailedError(f"Failed to create Pydantic model from schema: {e}")
except Exception as err:
error_msg = f"Failed to create Pydantic model from schema: {err}"
raise TypeTransformerFailedError(error_msg) from err

Code Review Run #b2a53c


Is this a valid issue, or was it incorrectly flagged by the Agent?

  • it was incorrectly flagged


def _map_json_type_to_python(self, field_info: dict) -> Any:
"""
Maps JSON schema types to Python types for Pydantic model fields.
"""
json_type = field_info.get("type")

Check warning on line 135 in flytekit/extras/pydantic_transformer/transformer.py

View check run for this annotation

Codecov / codecov/patch

flytekit/extras/pydantic_transformer/transformer.py#L135

Added line #L135 was not covered by tests
if isinstance(json_type, list):
# Handle Union types like ["string", "null"]
json_type = [t for t in json_type if t != "null"]

Check warning on line 138 in flytekit/extras/pydantic_transformer/transformer.py

View check run for this annotation

Codecov / codecov/patch

flytekit/extras/pydantic_transformer/transformer.py#L138

Added line #L138 was not covered by tests
if len(json_type) == 1:
json_type = json_type[0]

Check warning on line 140 in flytekit/extras/pydantic_transformer/transformer.py

View check run for this annotation

Codecov / codecov/patch

flytekit/extras/pydantic_transformer/transformer.py#L140

Added line #L140 was not covered by tests
else:
# More complex unions can be handled here if needed
json_type = "string" # default fallback

Check warning on line 143 in flytekit/extras/pydantic_transformer/transformer.py

View check run for this annotation

Codecov / codecov/patch

flytekit/extras/pydantic_transformer/transformer.py#L143

Added line #L143 was not covered by tests
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider robust union type handling

Consider implementing more robust handling of complex union types instead of defaulting to string. The current implementation may lead to data loss or incorrect type conversions for union types with multiple variants.

Code suggestion
Check the AI-generated fix before applying
Suggested change
json_type = "string" # default fallback
variants = [type_mapping.get(t, Any) for t in json_type]
if len(variants) > 1:
return Union[tuple(variants)] # type: ignore
return variants[0]

Code Review Run #b2a53c


Is this a valid issue, or was it incorrectly flagged by the Agent?

  • it was incorrectly flagged


type_mapping = {

Check warning on line 145 in flytekit/extras/pydantic_transformer/transformer.py

View check run for this annotation

Codecov / codecov/patch

flytekit/extras/pydantic_transformer/transformer.py#L145

Added line #L145 was not covered by tests
"string": str,
"integer": int,
"number": float,
"boolean": bool,
"object": dict,
"array": list,
}

python_type = type_mapping.get(json_type, Any)

Check warning on line 154 in flytekit/extras/pydantic_transformer/transformer.py

View check run for this annotation

Codecov / codecov/patch

flytekit/extras/pydantic_transformer/transformer.py#L154

Added line #L154 was not covered by tests

# Handle nested objects
if python_type == dict and "properties" in field_info:
# Recursively create a nested Pydantic model
nested_model = self._create_nested_model(field_info)
return nested_model

Check warning on line 160 in flytekit/extras/pydantic_transformer/transformer.py

View check run for this annotation

Codecov / codecov/patch

flytekit/extras/pydantic_transformer/transformer.py#L159-L160

Added lines #L159 - L160 were not covered by tests

# Handle arrays with specified items
if python_type == list and "items" in field_info:
item_type = self._map_json_type_to_python(field_info["items"])
return List[item_type]

Check warning on line 165 in flytekit/extras/pydantic_transformer/transformer.py

View check run for this annotation

Codecov / codecov/patch

flytekit/extras/pydantic_transformer/transformer.py#L164-L165

Added lines #L164 - L165 were not covered by tests

return python_type

Check warning on line 167 in flytekit/extras/pydantic_transformer/transformer.py

View check run for this annotation

Codecov / codecov/patch

flytekit/extras/pydantic_transformer/transformer.py#L167

Added line #L167 was not covered by tests

def _create_nested_model(self, field_info: dict) -> Type[BaseModel]:
"""
Recursively creates nested Pydantic models for objects within the schema.
"""
properties = field_info.get("properties", {})
required_fields = field_info.get("required", [])

Check warning on line 174 in flytekit/extras/pydantic_transformer/transformer.py

View check run for this annotation

Codecov / codecov/patch

flytekit/extras/pydantic_transformer/transformer.py#L173-L174

Added lines #L173 - L174 were not covered by tests

model_name = field_info.get("title", "NestedModel")
annotations = {}
field_definitions = {}

Check warning on line 178 in flytekit/extras/pydantic_transformer/transformer.py

View check run for this annotation

Codecov / codecov/patch

flytekit/extras/pydantic_transformer/transformer.py#L176-L178

Added lines #L176 - L178 were not covered by tests

for field_name, sub_field_info in properties.items():
sub_field_type = self._map_json_type_to_python(sub_field_info)
annotations[field_name] = Optional[sub_field_type] if field_name not in required_fields else sub_field_type
field_definitions[field_name] = (sub_field_type, ... if field_name in required_fields else None)

Check warning on line 183 in flytekit/extras/pydantic_transformer/transformer.py

View check run for this annotation

Codecov / codecov/patch

flytekit/extras/pydantic_transformer/transformer.py#L181-L183

Added lines #L181 - L183 were not covered by tests

try:
NestedModel = create_model(model_name, **field_definitions)
return NestedModel
except Exception as e:
raise TypeTransformerFailedError(f"Failed to create nested Pydantic model: {e}")

Check warning on line 189 in flytekit/extras/pydantic_transformer/transformer.py

View check run for this annotation

Codecov / codecov/patch

flytekit/extras/pydantic_transformer/transformer.py#L185-L189

Added lines #L185 - L189 were not covered by tests


TypeEngine.register(PydanticTransformer())
Loading