Pydantic contrib module #757

dandavison · 2025-01-31T13:24:17Z

Fixes #621, #496

Provide official pydantic support in a new module temporalio.contrib.pydantic.
This support is based on v2, but we do retain the existing code that should allow v1 to still be used.
Due to our wrapping of the datetime.datetime class, and datetime.datetime instances, with proxy objects, we have to unwrap proxied objects to allow pydantic to work correctly.

See also #627, #207

TODO:

manually test v1 usage via sample

Accompanying samples PR: temporalio/samples-python#163

dandavison · 2025-02-04T14:26:50Z

temporalio/worker/workflow_sandbox/_restrictions.py

+        # constructor. Return the field value.
+        if len(args) == 1:
+            return args[0]
+        return super().__new__(cls)


TODO: pydantic is calling the _RestrictedProxy constructor with a single (non-wrapped) datetime.datetime argument. Returning args[0] doesn't seem correct, but does work. I need to better understand exactly why pydantic is making this call. The stack looks like

-> o1, _ = make_pydantic_objects() /Users/dan/src/temporalio/sdk-python/tests/contrib/test_pydantic.py(41)make_pydantic_objects() -> MyPydanticModel( /Users/dan/src/temporalio/sdk-python/.venv/lib/python3.9/site-packages/pydantic/main.py(214)__init__() -> validated_self = self.__pydantic_validator__.validate_python(data, self_instance=self) > /Users/dan/src/temporalio/sdk-python/temporalio/worker/workflow_sandbox/_restrictions.py(958)__new__()

Pydantic v2 "validation" (i.e. model instance construction) is implemented in Rust, and since intervening stack frames are missing, I think that pydantic/main.py(214) is calling into pydantic's Rust layer. But I would like to understand why pydantic is calling the constructor with the datetime value as the single argument.

need to better understand exactly why pydantic is making this call

Agreed, I am also curious why Pydantic is instantiating the class with a single argument. ChatGPT mentions that with "type adaption", the absence of __get_pydantic_core_schema__ and/or the __init__ looking a certain way may make Pydantic think it it's a wrapper type.

When pydantic instantiates a model containing a field whose type annotation is proxied

With other create_model code in this PR, pydantic should never be instantiating a model with proxied annotations right?

This was happening because the PR was implementing __get_pydantic_core_schema__ incorrectly. Fixed at efcd456

dandavison · 2025-02-04T14:54:39Z

temporalio/worker/workflow_sandbox/_restrictions.py

@@ -943,26 +952,37 @@ def r_op(obj: Any, other: Any) -> Any:

 def _is_restrictable(v: Any) -> bool:
    return v is not None and not isinstance(
-        v, (bool, int, float, complex, str, bytes, bytearray)
+        v, (bool, int, float, complex, str, bytes, bytearray, datetime)


TODO: As things stand this means that users will be able to do my_datetime.now(). But datetime.datetime.now() will still be protected against, which is the usual way to call it. One option is to make this change only when conditional on HAVE_PYDANTIC.

If we get rid of this change and allow datetime instances to be restricted, then Pydantic validation fails. This failure is in Pydantic's Rust layer: https://github.com/pydantic/pydantic-core/blob/main/src/errors/types.rs#L706-L707

datetime_field Input should be a valid datetime [type=datetime_type, input_value=datetime.datetime(2000, 1, 2, 3, 4, 5), input_type=_RestrictedProxy] For further information visit https://errors.pydantic.dev/2.10/v/datetime_type

I think we never meant for the instances to be restricted just the static methods, so this is fine IMO. What about datetime.date, datetime.time, and datetime.timedelta objects, do we need explicit support for them too (at least the former)?

dandavison · 2025-02-04T15:02:12Z

tests/worker/workflow_sandbox/test_runner.py

-        # building their validators causing it to use date instead
-        # TODO(cretz): https://github.com/temporalio/sdk-python/issues/207
-        if pydantic.compiled:
-            assert isinstance(PydanticMessage(content=workflow.now()).content, date)


pydantic.compiled is not present in v2

dandavison · 2025-02-04T15:02:29Z

tests/worker/workflow_sandbox/test_runner.py

-        if pydantic.compiled:
-            assert isinstance(PydanticMessage(content=workflow.now()).content, date)
-        else:
-            assert isinstance(PydanticMessage(content=workflow.now()).content, datetime)


This is tested in test_pydantic.py

dandavison · 2025-02-04T15:12:03Z

temporalio/contrib/pydantic.py

+        json_payload_converter = JSONPlainPayloadConverter(
+            encoder=PydanticJSONEncoder,
+            custom_type_converters=[PydanticModelTypeConverter()],
+        )


Our pydantic sample was implementing to_payload. This is slightly incorrect because it means that our AdvancedJSONEncoder was not being used. In practice, pydantic provides a function (called to_jsonable_python in v2) with similar functionality to our AdvancedJSONEncoder, but nevertheless, we should ensure that we fall back to AdvancedJSONEncoder in case we add to it in the future. That's what we do here, by using JSONPlainPayloadConverter directly.

cretz · 2025-02-04T15:34:02Z

README.md

@@ -325,6 +319,14 @@ This notably doesn't include any `date`, `time`, or `datetime` objects as they m
 Users are strongly encouraged to use a single `dataclass` for parameter and return types so fields with defaults can be
 easily added without breaking compatibility.

+To use pydantic model instances (or python objects containing pydantic model instances), use


I think we should make this a whole subsection, e.g. ##### Pydantic

cretz · 2025-02-04T15:36:35Z

pyproject.toml

@@ -35,6 +35,7 @@ grpcio = {version = "^1.48.2", optional = true}
 opentelemetry-api = { version = "^1.11.1", optional = true }
 opentelemetry-sdk = { version = "^1.11.1", optional = true }
 protobuf = ">=3.20"
+pydantic = { version = "^2.10.6", optional = true }


Can you confirm that pydantic v1 users aren't bound by our version constraint here if they never add the extra? Maybe the easiest way to confirm is somehow have a sample or test that confirms existing pydantic support?

I've changed the bound to

pydantic = { version = ">=1.10.0,<3.0.0", optional = true }

and tested using the v1 sample: https://github.com/temporalio/samples-python/pull/163/files#diff-80f59f3959404e07adb4ae966fa2c502346ee73a3e7e22b2a958a70db13f8526

cretz · 2025-02-04T15:37:07Z

temporalio/contrib/pydantic.py

+    # pydantic v1
+    from pydantic.json import pydantic_encoder as to_jsonable_python  # type: ignore


Are we saying this contrib library works even with pydantic v1? If so, we should change our version constraint to be >= whatever the absolute lowest we support is. If not, we should probably error if they try to import this.

I've dropped any attempt to support v1 in the new contrib module, and also reverted any attempt to support v2 via the pre-contrib-module v1 hacks we have. So the PR is now more or less a strict addition of a v2-only contrib module, retaining the existing v1 hacks.

cretz · 2025-02-04T15:42:55Z

temporalio/contrib/pydantic.py

+        if temporalio.workflow.unsafe.in_sandbox():
+            # Unwrap proxied model field types so that Pydantic can call their constructors
+            model = pydantic.create_model(
+                model.__name__,
+                **{  # type: ignore
+                    name: (RestrictionContext.unwrap_if_proxied(f.annotation), f)
+                    for name, f in model.model_fields.items()
+                },
+            )


A couple of questions here just due to my lack of knowledge...

Arguably models should not be defined/loaded in the sandbox right? That means they are reloaded for every workflow run. They should be imported via passthrough. I wonder if we should have a warning if we detect any model fields are proxied? Or maybe a warning if we see Pydantic models being loaded in the sandbox instead of passed through? EDIT: Seeing test I can see situations where people define their models in the sandbox, even if we discourage it.

Is create_model cheap? Meaning is this what Pydantic often uses anyways to instantiate a model instance, or do they somehow cache this definition and manually doing create_model isn't meant for each time you need the instance? If it is not cheap, we should both consider caching and consider not calling it if none of the types are proxied.

Happily we don't need to worry about this. The problem I was solving here was fixed by the fixed implementation of __get_pydantic_core_schema__.

cretz · 2025-02-04T15:44:56Z

temporalio/contrib/pydantic.py

+                },
+            )
+        if hasattr(model, "model_validate"):
+            return model.model_validate(value)


I assume this raises a Pydantic-specific exception that a user could add to workflow failure exception types if they wanted? (if not, we should consider a usability improvement to have an option to wrap this in application error)

Yes, it raises pydantic.ValidationError

cretz · 2025-02-04T15:57:04Z

temporalio/worker/workflow_sandbox/_restrictions.py

@@ -943,26 +952,37 @@ def r_op(obj: Any, other: Any) -> Any:

 def _is_restrictable(v: Any) -> bool:
    return v is not None and not isinstance(
-        v, (bool, int, float, complex, str, bytes, bytearray)
+        v, (bool, int, float, complex, str, bytes, bytearray, datetime)


I think we never meant for the instances to be restricted just the static methods, so this is fine IMO. What about datetime.date, datetime.time, and datetime.timedelta objects, do we need explicit support for them too (at least the former)?

cretz · 2025-02-04T16:01:19Z

temporalio/worker/workflow_sandbox/_restrictions.py

+        if HAVE_PYDANTIC and __name == "__get_pydantic_core_schema__":
+            return object.__getattribute__(self, "__get_pydantic_core_schema__")


Hrmm, trying to think if we even want this generic to void pydantic code inside our sandbox. I guess all ways to do it would require that the contrib library is at least imported by the user somewhere in the process, but maybe that is ok? If that is ok, I wonder if a side-effect of importing the contrib is ideal over altering the sandbox even if pydantic is even available? Though I admit I kinda like the idea of this patch if pydantic is even available regardless of contrib lib usage, so maybe this should remain as is.

I did wonder about this a bit. I don't see much downside with the way it is currently.

cretz · 2025-02-04T16:03:10Z

tests/contrib/test_pydantic.py

+
+class MyPydanticModel(BaseModel):
+    ip_field: IPv4Address
+    datetime_field: datetime


Can we add datetime.date, datetime.time, and datetime.timedelta in here? And maybe uuid.UUID while we're at it since we have restrictions in that module too.

Done, the test suite has been significantly expanded.

cretz · 2025-02-04T16:05:36Z

tests/contrib/test_pydantic.py

+ShortSequence = Annotated[SequenceType, Len(max_length=2)]
+
+
+class MyPydanticModel(BaseModel):


I will say for many users they will be importing this model from a different module on passthrough. This test confirms the model works when defined in the sandbox, but I wonder if we also need at least a little coverage for models defined out of the sandbox (and passed through).

I've structured the test suite so that models.py (containing several model definitions) is imported in-sandbox, but a few of the models are defined in a different file and imported under the unsafe.imports_passed_through context manager.

cretz · 2025-02-04T16:08:01Z

temporalio/worker/workflow_sandbox/_restrictions.py

+        # constructor. Return the field value.
+        if len(args) == 1:
+            return args[0]
+        return super().__new__(cls)


need to better understand exactly why pydantic is making this call

Agreed, I am also curious why Pydantic is instantiating the class with a single argument. ChatGPT mentions that with "type adaption", the absence of __get_pydantic_core_schema__ and/or the __init__ looking a certain way may make Pydantic think it it's a wrapper type.

When pydantic instantiates a model containing a field whose type annotation is proxied

With other create_model code in this PR, pydantic should never be instantiating a model with proxied annotations right?

cretz · 2025-02-06T21:15:03Z

tests/contrib/test_pydantic.py

+        assert self.str_short_sequence == ["my-string-1", "my-string-2"]
+
+
+class MyPydanticDatetimeModel(BaseModel):


Just to make us feel better, can we toss timedelta in here or somewhere and make sure it works (I'm sure it does since we don't proxy it)

Yes, was just doing that in fact! ac3583c

dandavison · 2025-02-10T17:53:49Z

temporalio/worker/workflow_sandbox/_restrictions.py

    # assertion rewriter
    "typing",
+    "typing_extensions",


Without this TypedDict fields are broken.

TypedDict fields are broken for Pydantic only or in general in the SDK? For the former, I can see that if they are using typing_extensions, but for the latter that'd be a bit strange unless you're using typing_extensions as a user. Also, can you make a comment above this line just saying why it is here?

Just for Pydantic. It caused a problem with pytest's assertion rewriter, as you noted for typing on the line above. There's no drawback to passing it through, right?
Added a comment.

Right, no real drawback here (it's effectively part of stdlib IMO). We technically separated the restriction sets as "minimum", "with temporal", and "maximum" just in case people wanted to be more restrictive, but the default is "maximum" which is basically what everyone uses. You could move this from minimum to one of the others, but it doesn't really matter.

dandavison · 2025-02-10T17:58:08Z

temporalio/worker/workflow_sandbox/_restrictions.py

@@ -1032,7 +1065,7 @@ def __getitem__(self, key: Any) -> Any:
    )
    __str__ = _RestrictedProxyLookup(str)  # type: ignore
    __bytes__ = _RestrictedProxyLookup(bytes)
-    __format__ = _RestrictedProxyLookup()  # type: ignore
+    __format__ = _RestrictedProxyLookup(format)  # type: ignore


This fixes a small bug in the existing sandboxing. Without this, test_restricted_proxy_dunder_methods fails for pathlib.Path fields.

cretz

Looks great, only minor things

README.md

cretz · 2025-02-11T13:42:53Z

README.md

+Pydantic v1 is not supported by this data converter. If you are not yet able to upgrade from Pydantic v1, see
+https://github.com/temporalio/samples-python/tree/main/pydantic_converter/v1 for limited v1 support.
+
+Do not use pydantic's [strict mode](https://docs.pydantic.dev/latest/concepts/strict_mode/).


Can you add a quick phrase/sentence to the end on why? Maybe "Strict mode does not work with the Python SDK sandbox because of X"?

This is a good call. I think this comment should really have been marked TODO. At the very least it needs to be changed to say that it's only proxied types that can't be used as fields with strict=True, but I need to look into this again to see what the correct course of action is.

cretz · 2025-02-11T13:45:39Z

pyproject.toml

@@ -35,6 +35,7 @@ grpcio = {version = "^1.48.2", optional = true}
 opentelemetry-api = { version = "^1.11.1", optional = true }
 opentelemetry-sdk = { version = "^1.11.1", optional = true }
 protobuf = ">=3.20"
+pydantic = { version = ">=1.10.0,<3.0.0", optional = true }


Hrmm, so I suggested the >=1.x because I thought we were going to support v1 with our contrib converter. But now that it sounds like we're not, might as well do a 2.x version constraint correct? Or does this version constraint somehow apply even when not using the extra?

Ah I thought a ^2.x bound would cause pip's dependency resolver to reject attempts to install 1.x along with temporalio. Changed to

pydantic = { version = "^2.0.0", optional = true }

I thought we were going to support v1 with our contrib converter. But now that it sounds like we're not

Yes, I backed out of that because (a) I think v2 is widely used already, (b) we want people to use v2 because v1 doesn't work with our sandbox, (c) it's conceptually clearer to have our new thing v2 only and the old hacks v1 only, and (d) not a huge challenge but we'd have to write a backport of to_jsonable_python for v1, or similar shim.

Works for me

cretz · 2025-02-11T13:48:08Z

temporalio/worker/workflow_sandbox/_restrictions.py

    # assertion rewriter
    "typing",
+    "typing_extensions",


TypedDict fields are broken for Pydantic only or in general in the SDK? For the former, I can see that if they are using typing_extensions, but for the latter that'd be a bit strange unless you're using typing_extensions as a user. Also, can you make a comment above this line just saying why it is here?

cretz

Nothing blocking, though curious why datetime.date is no longer a restricted proxy instance

cretz · 2025-02-11T15:49:45Z

tests/worker/workflow_sandbox/test_runner.py

-        assert (
-            type(date(2010, 1, 20))
-            == temporalio.worker.workflow_sandbox._restrictions._RestrictedProxy
-        )


Can you help me understand why this assertion no longer holds?

It's because we no longer proxy instances of date, just classes and methods. You agreed with that course of action:

I think we never meant for the instances to be restricted just the static methods

Shall we double-check this decision? Why were we proxing them originally -- just to prevent things like my_datetime.now()? (which would be unusual Python that I can't think of a use case for, but is technically a sandbox hole that we're opening up.)

I remember now, thanks! So type(type(date)) I guess is a _RestrictedProxy heh

dandavison force-pushed the pydantic branch 10 times, most recently from 34d1ae2 to 9f5b771 Compare February 4, 2025 04:55

dandavison commented Feb 4, 2025

View reviewed changes

dandavison force-pushed the pydantic branch 2 times, most recently from fa2c8b5 to a1cdf1b Compare February 4, 2025 14:39

dandavison commented Feb 4, 2025

View reviewed changes

dandavison force-pushed the pydantic branch from a1cdf1b to b3a636d Compare February 4, 2025 15:01

dandavison commented Feb 4, 2025

View reviewed changes

dandavison marked this pull request as ready for review February 4, 2025 15:24

dandavison requested a review from a team as a code owner February 4, 2025 15:24

cretz reviewed Feb 4, 2025

View reviewed changes

cretz reviewed Feb 6, 2025

View reviewed changes

dandavison force-pushed the pydantic branch 2 times, most recently from d83249b to 8b1ae77 Compare February 7, 2025 12:41

dandavison marked this pull request as draft February 10, 2025 01:46

dandavison added 5 commits February 9, 2025 20:46

Copy of pydantic v1 converter from samples-python

106f584

Cleanup

4d9943b

update pydantic to v2

72c9cd1

Drive-by: use zip_longest

e9e0a84

Get rid of pydantic sandbox hack

819b7ee

dandavison added 2 commits February 10, 2025 10:29

Fix lint

1770a75

Reorganize

40914be

dandavison force-pushed the pydantic branch from 7a6e22d to 40914be Compare February 10, 2025 16:27

dandavison added 3 commits February 10, 2025 11:47

Define some models inside the sandbox and some outside

caef6de

lint

775e4ee

Warn pydantic users who are not using contrib.pydantic

bc949b8

dandavison force-pushed the pydantic branch from 0d3fd0c to e42d9e3 Compare February 10, 2025 17:45

dandavison commented Feb 10, 2025

View reviewed changes

dandavison mentioned this pull request Feb 10, 2025

Update pydantic samples to use contrib module temporalio/samples-python#163

Open

dandavison added 2 commits February 10, 2025 18:07

Cleanup

8d6c827

Support pydantic v1 and v2

535b07e

dandavison force-pushed the pydantic branch from f2843e3 to b31d8eb Compare February 10, 2025 23:07

dandavison added 2 commits February 10, 2025 19:20

Drop v1 support in data converter

7b6d124

Drop v2 support outside contrib

f5fcfa8

dandavison force-pushed the pydantic branch 2 times, most recently from 5c2c452 to 0ce4836 Compare February 11, 2025 01:17

dandavison marked this pull request as ready for review February 11, 2025 01:59

dandavison added 3 commits February 10, 2025 21:01

README

5e510ff

Include pydantic extra

00bae93

date instances are no longer proxied

7f25816

dandavison force-pushed the pydantic branch from 424d766 to 7f25816 Compare February 11, 2025 02:01

dandavison added 2 commits February 10, 2025 21:47

Fix tests on Windows

0c17f89

doctoc

e2492b7

dandavison requested a review from cretz February 11, 2025 02:48

cretz reviewed Feb 11, 2025

View reviewed changes

dandavison added 2 commits February 11, 2025 10:04

Restrict pydantic range to v2 in pyproject.toml

26fbef1

Fix warning message

22ef2c6

dandavison force-pushed the pydantic branch from 88d358e to 22ef2c6 Compare February 11, 2025 15:29

cretz approved these changes Feb 11, 2025

View reviewed changes

		# pydantic v1
		from pydantic.json import pydantic_encoder as to_jsonable_python # type: ignore

		if HAVE_PYDANTIC and __name == "__get_pydantic_core_schema__":
		return object.__getattribute__(self, "__get_pydantic_core_schema__")

		ShortSequence = Annotated[SequenceType, Len(max_length=2)]


		class MyPydanticModel(BaseModel):

		assert self.str_short_sequence == ["my-string-1", "my-string-2"]


		class MyPydanticDatetimeModel(BaseModel):

Pydantic contrib module #757

Are you sure you want to change the base?

Pydantic contrib module #757

Conversation

dandavison commented Jan 31, 2025 • edited Loading

dandavison Feb 4, 2025 • edited Loading

Choose a reason for hiding this comment

cretz Feb 4, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dandavison Feb 4, 2025 • edited Loading

Choose a reason for hiding this comment

cretz Feb 4, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dandavison Feb 4, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dandavison Feb 11, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cretz Feb 4, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cretz Feb 4, 2025 • edited Loading

Choose a reason for hiding this comment

cretz Feb 4, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cretz Feb 4, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dandavison Feb 6, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dandavison Feb 10, 2025 • edited Loading

Choose a reason for hiding this comment

cretz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dandavison Feb 11, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cretz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dandavison commented Jan 31, 2025 •

edited

Loading

dandavison Feb 4, 2025 •

edited

Loading

cretz Feb 4, 2025 •

edited

Loading

dandavison Feb 4, 2025 •

edited

Loading

cretz Feb 4, 2025 •

edited

Loading

dandavison Feb 4, 2025 •

edited

Loading

dandavison Feb 11, 2025 •

edited

Loading

cretz Feb 4, 2025 •

edited

Loading

cretz Feb 4, 2025 •

edited

Loading

cretz Feb 4, 2025 •

edited

Loading

cretz Feb 4, 2025 •

edited

Loading

dandavison Feb 6, 2025 •

edited

Loading

dandavison Feb 10, 2025 •

edited

Loading

dandavison Feb 11, 2025 •

edited

Loading