temporalio / samples-python

Samples for working with the Temporal Python SDK
MIT License
131 stars 56 forks source link

[Bug] Update Pydantic converter example to reflect `with_child_unrestricted` changes #126

Open gregbrowndev opened 4 months ago

gregbrowndev commented 4 months ago

What are you really trying to do?

I want to use Pydantic and datetime objects as input/outputs/signals/queries to workflows and activities.

Describe the bug

The recommended reference example for the Pydantic converter creates a new sandbox to work around a bug in Pydantic.

There is a TODO in the code below:

https://github.com/temporalio/samples-python/blob/main/pydantic_converter/worker.py

that says:

TODO(cretz): Use with_child_unrestricted when https://github.com/temporalio/sdk-python/issues/254 is fixed and released

The issue in question has now been merged but the example hasn't been updated.

Minimal Reproduction

https://github.com/temporalio/samples-python/blob/main/pydantic_converter/worker.py

Environment/Versions

n/a

Additional context

n/a

Thanks!

cretz commented 4 months ago

Thanks! Agreed we can now perform that TODO.

baegmon commented 4 months ago

Any update on this one? Currently I'm using Pydantic v2 models with:

from pydantic_core import to_jsonable_python

and

workflow_runner=SandboxedWorkflowRunner(
    restrictions=SandboxRestrictions.default.with_passthrough_modules(
        "pydantic",
    )
)

Would love to see an example without seeing warnings from https://github.com/temporalio/sdk-python/blob/main/temporalio/converter.py#L561

cretz commented 4 months ago

That warning is unrelated to this issue. This issue is simply changing how the sandbox is created. That warning is because presumably you are not using a custom converter like https://github.com/temporalio/samples-python/blob/main/pydantic_converter/converter.py and configuring it on the client.

baegmon commented 4 months ago

That warning is unrelated to this issue. This issue is simply changing how the sandbox is created. That warning is because presumably you are not using a custom converter like https://github.com/temporalio/samples-python/blob/main/pydantic_converter/converter.py and configuring it on the client.

I've tried that example actually, with just one difference which is using from pydantic_core import to_jsonable_python over pydantic_encoder for v2 on the converter: https://github.com/temporalio/samples-python/blob/main/pydantic_converter/converter.py#L32

I am still getting the issue on temporalio==1.6.0, do you not get this error? This is just on a default python 3.12 devcontainer.

cretz commented 4 months ago

We have not tested or created a sample for v2. There is a separate issue at https://github.com/temporalio/samples-python/issues/97 for that. But I think the issue to resolve this unrelated TODO for v1 is unrelated to your issue.

(also transferring this issue to the samples repo since it doesn't belong in primary repo)