-
Notifications
You must be signed in to change notification settings - Fork 94
Added support for class specific custom conversion through to_temporal_json/from_temporal_json methods #819
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
base: main
Are you sure you want to change the base?
Changes from 12 commits
2745658
e9a38d9
e6315b9
896ab30
d68fee1
5deb4fb
e4a27ea
42789aa
2cfa074
82073c6
34a360b
84528fb
981878b
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 |
---|---|---|
|
@@ -489,6 +489,43 @@ class AdvancedJSONEncoder(json.JSONEncoder): | |
|
||
This encoder supports dataclasses and all iterables as lists. | ||
|
||
A class can implement to_json and from_json methods to support custom conversion logic. | ||
Custom conversion of generic classes is supported. | ||
These methods should have the following signatures: | ||
|
||
.. code-block:: python | ||
|
||
class MyClass: | ||
... | ||
|
||
@classmethod | ||
def from_json(cls, json: Any) -> MyClass: | ||
... | ||
|
||
def to_json(self) -> Any: | ||
... | ||
|
||
The to_json should return the same Python JSON types produced by JSONEncoder: | ||
|
||
+-------------------+---------------+ | ||
| Python | JSON | | ||
+===================+===============+ | ||
| dict | object | | ||
+-------------------+---------------+ | ||
| list, tuple | array | | ||
+-------------------+---------------+ | ||
| str | string | | ||
+-------------------+---------------+ | ||
| int, float | number | | ||
+-------------------+---------------+ | ||
| True | true | | ||
+-------------------+---------------+ | ||
| False | false | | ||
+-------------------+---------------+ | ||
| None | null | | ||
+-------------------+---------------+ | ||
|
||
|
||
It also uses Pydantic v1's "dict" methods if available on the object, | ||
but this is deprecated. Pydantic users should upgrade to v2 and use | ||
temporalio.contrib.pydantic.pydantic_data_converter. | ||
|
@@ -499,6 +536,15 @@ def default(self, o: Any) -> Any: | |
|
||
See :py:meth:`json.JSONEncoder.default`. | ||
""" | ||
# Custom encoding and decoding through to_json and from_json | ||
# to_json should be an instance method with only self argument | ||
to_json = "to_json" | ||
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 too could not find this method used anywhere (Pydantic V1 and V2 have similar but different names). However, I am concerned our users may have it defined on a model and this code may start to break on upgrade. I wonder if we need some kind of global opt-out or something just to be safe. One option could just be module level const 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'm also concerned that the It's pretty likely that anyone who already has a method with that name has the expected semantics. 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. Also, in some cases pydantic's 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 think we need to keep it Temporal specific, I think @mfateev just did some searching and saw no library was expecting 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. IMO the name should not be
The Python objects wouldn't even conform to JSON, e.g. they could have non-string keys. As they say on the pydantic-ai GitHub page, Pydantic is very dominant in the Python AI-oriented ecosystem
so, in addition to it making sense from a typing POV, it also makes sense for our conventions to be consistent with those of Pydantic. Calling the method Although other libraries have in the past used "to_json" to mean "return jsonable python", it's clear that Pydantic has taken a stance against that in favor of using names that are more correct from a type POV. 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'd rather not be consistent with Pydantic or be consistent with any library. I think these methods should be Temporal-SDK-only methods that have no other purpose or expectations to match any other use but Temporal's. I think 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. @mfateev - after discussion with @dandavison, can we rename these methods to |
||
if hasattr(o, to_json): | ||
attr = getattr(o, to_json) | ||
if not callable(attr): | ||
raise TypeError(f"Type {o.__class__}: to_json must be a method") | ||
return attr() | ||
|
||
# Dataclass support | ||
if dataclasses.is_dataclass(o): | ||
return dataclasses.asdict(o) | ||
|
@@ -1429,6 +1475,15 @@ def value_to_type( | |
raise TypeError(f"Value {value} not in literal values {type_args}") | ||
return value | ||
|
||
# Has from_json class method (must have to_json as well) | ||
from_json = "from_json" | ||
if hasattr(hint, from_json): | ||
attr = getattr(hint, from_json) | ||
attr_cls = getattr(attr, "__self__") | ||
if not callable(attr) or not attr_cls == origin: | ||
raise TypeError(f"Type {hint}: temporal_from_json must be a class method") | ||
return attr(value) | ||
|
||
is_union = origin is Union | ||
if sys.version_info >= (3, 10): | ||
is_union = is_union or isinstance(origin, UnionType) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2243,12 +2243,44 @@ def assert_expected(self) -> None: | |
assert self.field1 == "some value" | ||
|
||
|
||
T = typing.TypeVar("T") | ||
|
||
|
||
class MyGenericClass(typing.Generic[T]): | ||
""" | ||
Demonstrates custom conversion and that it works even with generic classes. | ||
""" | ||
|
||
def __init__(self, field1: str): | ||
self.field1 = field1 | ||
self.field2 = "foo" | ||
|
||
@classmethod | ||
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. Can we add a test for |
||
def from_json(cls, json_obj: Any) -> MyGenericClass: | ||
return MyGenericClass(str(json_obj) + "_from_json") | ||
|
||
def to_json(self) -> Any: | ||
return self.field1 + "_to_json" | ||
|
||
def assert_expected(self, value: str) -> None: | ||
# Part of the assertion is that this is the right type, which is | ||
# confirmed just by calling the method. We also check the field. | ||
assert str(self.field1) == value | ||
|
||
|
||
@activity.defn | ||
async def data_class_typed_activity(param: MyDataClass) -> MyDataClass: | ||
param.assert_expected() | ||
return param | ||
|
||
|
||
@activity.defn | ||
async def generic_class_typed_activity( | ||
param: MyGenericClass[str], | ||
) -> MyGenericClass[str]: | ||
return param | ||
|
||
|
||
@runtime_checkable | ||
@workflow.defn(name="DataClassTypedWorkflow") | ||
class DataClassTypedWorkflowProto(Protocol): | ||
|
@@ -2306,6 +2338,24 @@ async def run(self, param: MyDataClass) -> MyDataClass: | |
start_to_close_timeout=timedelta(seconds=30), | ||
) | ||
param.assert_expected() | ||
generic_param = MyGenericClass[str]("some_value2") | ||
generic_param = await workflow.execute_activity( | ||
generic_class_typed_activity, | ||
generic_param, | ||
start_to_close_timeout=timedelta(seconds=30), | ||
) | ||
generic_param.assert_expected( | ||
"some_value2_to_json_from_json_to_json_from_json" | ||
) | ||
generic_param = MyGenericClass[str]("some_value2") | ||
generic_param = await workflow.execute_local_activity( | ||
generic_class_typed_activity, | ||
generic_param, | ||
start_to_close_timeout=timedelta(seconds=30), | ||
) | ||
generic_param.assert_expected( | ||
"some_value2_to_json_from_json_to_json_from_json" | ||
) | ||
child_handle = await workflow.start_child_workflow( | ||
DataClassTypedWorkflow.run, | ||
param, | ||
|
@@ -2348,7 +2398,9 @@ async def test_workflow_dataclass_typed(client: Client, env: WorkflowEnvironment | |
"Java test server: https://github.com/temporalio/sdk-core/issues/390" | ||
) | ||
async with new_worker( | ||
client, DataClassTypedWorkflow, activities=[data_class_typed_activity] | ||
client, | ||
DataClassTypedWorkflow, | ||
activities=[data_class_typed_activity, generic_class_typed_activity], | ||
) as worker: | ||
val = MyDataClass(field1="some value") | ||
handle = await client.start_workflow( | ||
|
@@ -2373,7 +2425,9 @@ async def test_workflow_separate_protocol(client: Client): | |
# This test is to confirm that protocols can be used as "interfaces" for | ||
# when the workflow impl is absent | ||
async with new_worker( | ||
client, DataClassTypedWorkflow, activities=[data_class_typed_activity] | ||
client, | ||
DataClassTypedWorkflow, | ||
activities=[data_class_typed_activity, generic_class_typed_activity], | ||
) as worker: | ||
assert isinstance(DataClassTypedWorkflow(), DataClassTypedWorkflowProto) | ||
val = MyDataClass(field1="some value") | ||
|
@@ -2395,7 +2449,9 @@ async def test_workflow_separate_abstract(client: Client): | |
# This test is to confirm that abstract classes can be used as "interfaces" | ||
# for when the workflow impl is absent | ||
async with new_worker( | ||
client, DataClassTypedWorkflow, activities=[data_class_typed_activity] | ||
client, | ||
DataClassTypedWorkflow, | ||
activities=[data_class_typed_activity, generic_class_typed_activity], | ||
) as worker: | ||
assert issubclass(DataClassTypedWorkflow, DataClassTypedWorkflowAbstract) | ||
val = MyDataClass(field1="some value") | ||
|
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 would move this to
JSONPlainPayloadConverter
instead of here since this class is only forto_json
. May also be worth a tiny statement aboutto_json
/from_json
support in the "Data Conversion" section of the README.