Add warning and direct to Pydantic converter sample if Pydantic models are detected

[aezomz]

What was changed

Add warning and direct to Pydantic converter sample if Pydantic models are detected

Why?

In order to support those, custom converter is needed. It will be better if we warn and redirect the users.

Change is to support Datetime, as we can control how we serde datetime in the pydantic model.
This is to support the model.json() callable kind as well.

Checklist

1. Closes
   should close Pydantic models with nested non-JSON-friendly field types cannot be serialized #143 in term of warning user to use custom converter

2. How was this tested:
   Please refer to pytest

3. Any docs updates needed?
   Lets add an example if the idea is accepted.

Please let me know if any changes are needed, feel free to edit as well.

[CLAassistant]

All committers have signed the CLA.

[cretz]

My primary concern is we guarantee all of our serializable things work with lists, dictionaries, etc. What if I had a list of Pydantic models? The better way is to have full Pydantic JSON support through a custom converter which I need to write at temporalio/samples-python#25. That would use pydantic.json.pydantic_encoder as mentioned at https://docs.pydantic.dev/usage/dataclasses/#json-dumping which would solve all issues. This is only solving some.

I wrote this class knowing it might be given a different encoder when instantiated. I will try to get to that sample soon. We may consider dropping/deprecating the limited Pydantic support we have in favor of a custom converter once I have that sample written.

[aezomz]

I thought its quite odd to send a List[PydanticModels] since Temporal recommend sending a single struct as params.. But its arguably correct too since its still a single struct.

I think having limited support is fine, if ppl want to use List[PydanticModels] then they can go for the custom converter? Otherwise the current support is enough to convert most of the use cases?

Writing it like this wont have any problem though, like in the test.
class MyPydanticDTClassList(pydantic.BaseModel): foo_list: List[MyPydanticDTClass]

Since am sending my payload from GO, if i use a custom converter, i need to create one for GO too to send the correct encoding type right? I think thats alot of effort, if we want to make cross language calling easier.

Anyway do you want me to drop this PR, since custom pydantic converter is probably the correct and best solution?

[cretz]

I think having limited support is fine, if ppl want to use List[PydanticModels] then they can go for the custom converter? Otherwise the current support is enough to convert most of the use cases?

Why not always go for custom converter if it's a short bit of code? Then you get guaranteed Pydantic support in all cases and you don't have to think about it. I will write it soon and if it seems easy enough to incorporate for users, will suggest that and deprecate/warn against built-in Pydantic use which is known to be hacky.

Otherwise, I'd just suggest putting this json() dump into the recursive value dumper and parse it right back out to a dictionary, but that has perf concerns.

[aezomz]

Sure I will do the latter first, if performance is a concern. I will try the custom converter out to see if i can get it right.

[cretz]

Suggested change

	parse_obj_attr = inspect.getattr_static(value, "json", None)
	json_method = getattr(value, "json", None)

And just use that method if present. We need to make sure we work with proxied pydantic models which can happen in a sandbox so we need to not bypass __getattribute__.

[aezomz]

we for loop for pydantic obj here.

[aezomz]

I created this as a custom converter and uses GO custom converter to send encoding json/pydantic
Do you think it make sense to add it to the source code, or you prefer me to drop this and PR the python samples instead?

class JSONPydanticPayloadConverter(EncodingPayloadConverter):
    """Converter for 'json/pydantic' payloads supporting pydantic model.

    For encoding, uses PydanticModel.json(), which you can use pydantic model
    config to deserialize non standard types.

    For decoding, this uses PydanticModel definition, which you can use validator
    to rebuild the pydantic model.
    """

If we add it to the source code, we can have a flag to use pydantic json converter instead ?
Otherwise user will have to do this like in ur example.

class PydanticPayloadConverter(CompositePayloadConverter):
    def __init__(self) -> None:
        # Just add ours as first before the defaults
        super().__init__(
            JSONPydanticPayloadConverter(),
            *DefaultPayloadConverter().converters.values(),
        )

[cretz]

Do you think it make sense to add it to the source code, or you prefer me to drop this and PR the python samples instead?

I think we want it in the sample. I don't want to add a dependency on Pydantic.

As for your implementation, I'd do it a bit differently (I was gonna write this sample maybe as soon as today/tomorrow). Specifically:

• The JSONPydanticPayloadConverter should extend JSONPlainPayloadConverter and just pass pydantic.json.pydantic_encoder as the encoder kwarg to the base class __init__. This also lets it leverage the existing decode logic which does work
• Leave the encoding alone, we still want json/plain for this, you are just making Python model-specific encoding here it shouldn't affect the encoding (e.g. Go JSON should still work with it just fine)
• This converter should replace the existing JSON plain converter as last converter in the list. This means it is the fall through catch all which is important.
• Don't forget to put with workflow.unsafe.imports_passed_through(): above your pydantic import
• Feel free to take some tests from tests/test_converter.py
• Unfortunately this still isn't going to be able to solve [Bug] SandboxWorkflowRunner doesn't use correct Pydantic field types in some cases #207 so some good docs need to be made in the README of the sample
• Once the sample is merged, come back here and add a https://docs.python.org/3/library/warnings.html (maybe just lazily on first time a type is seen) for anyone using the default payload converter with Pydantic models in either direction telling them to use the sample approach

If you'd like, you can wait just a bit until I write it

[aezomz]

Sure, i can help to add the warnings. I will wait for ur sample converter and learn from it. :)
I think there is no rush for me now, since I know how to use the custom converter and understand the code better.
Thanks again!

[cretz]

I have opened temporalio/samples-python#44. The converter code is quite small.

[aezomz]

Updated to show warning for only the function we are overwriting in the sample custom converter.

[cretz]

Sorry it took so long to get back to this