Added support for class specific custom conversion through to_temporal_json/from_temporal_json methods

[mfateev]

What was changed

Added ability to customize a class conversion by adding to_temporal_json and from_temporal_json methods.

Why?

Some classes, for example, a reference to a large value stored in an external database, require custom conversion logic.

[cretz]

LGTM, only a couple of comments. @dandavison - any opinions here?

[cretz]

I would move this to JSONPlainPayloadConverter instead of here since this class is only for to_json. May also be worth a tiny statement about to_json/from_json support in the "Data Conversion" section of the README.

[cretz]

Can we add a test for @staticmethod too just in case?

[cretz]

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 TO_JSON_METHOD_NAME = "to_json" (and from equivalent) so they could change it . Thoughts?

[dandavison]

I'm also concerned that the to_json name might be in use by users with different semantics (after all, to_json should really return bytes). How about calling it "to_jsonable_python"? This is the name that pydantic have chosen for this functionality: https://docs.pydantic.dev/latest/api/pydantic_core/#pydantic_core.to_jsonable_python

It's pretty likely that anyone who already has a method with that name has the expected semantics.

[dandavison]

Also, in some cases pydantic's to_jsonable_python will be a suitable implementation of this method, in which case the name will be helpfully suggestive.

[cretz]

I think we need to keep it Temporal specific, I think @mfateev just did some searching and saw no library was expecting to_json/from_json on models (I can't find any either) so he used it. If we are concerned about clashing, which I admittedly am, I would prefer temporal_to_json and temporal_from_json so it's not accidentally used for any other purpose.

[dandavison]

IMO the name should not be to_json for at least two reasons:

1. A function named to_json should return valid JSON bytes, not Python objects.

2. Pydantic uses the name to_json to return valid JSON bytes, not Python objects.

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

the validation layer of the OpenAI SDK, the Anthropic SDK, LangChain, LlamaIndex, AutoGPT, Transformers, CrewAI, Instructor and many more

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 to_jsonable_python would be consistent with Pydantic and would almost never clash with a method that wasn't suitable for purpose. But we could prefix with temporal_, or introduce a TO_JSONABLE_PYTHON_METHOD_NAME escape hatch.

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.

[cretz]

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 to_temporal_json and from_temporal_json (or switch verb and temporal around there) is ideal to make sure there is no confusion about what they are for or their very limited use.

[cretz]

@mfateev - after discussion with @dandavison, can we rename these methods to temporal_to_jsonable_python and temporal_from_jsonable_python to both clarify it's Temporal specific and not to make users think it returns/accepts JSON strings/bytes?

[cretz]

This LGTM, this ok with you @dandavison? @mfateev - saw this was converted back to draft, some stuff still being worked on?

EDIT: See thread above, sorry, but can we do one more change to method names to change them to temporal_to_jsonable_python and temporal_from_jsonable_python?

[mfateev]

I'm evaluating an alternative approach that doesn't require this. So let's punt on this until I decide if this is really needed in the near future.