Skip to content
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

Pydantic 1.9.1 deepcopy breaks Redis caching, possibly other things #333

Closed
ubernostrum opened this issue Aug 3, 2022 · 9 comments · Fixed by #336
Closed

Pydantic 1.9.1 deepcopy breaks Redis caching, possibly other things #333

ubernostrum opened this issue Aug 3, 2022 · 9 comments · Fixed by #336

Comments

@ubernostrum
Copy link

I'm open to the possibility that this should be treated as an issue in Pydantic because it's a breaking change there, and pydantic/pydantic#4184 appears to be a variation of this.

I have a Starlite application which configures its caching as follows (excerpt from my actual app.py):

import redis
import starlite

from .config import settings

cache_config = (
    starlite.CacheConfig(backend=redis.from_url(settings.CACHE_URL))
    if settings.CACHE_URL is not None
    else starlite.app.DEFAULT_CACHE_CONFIG
)

config.settings in this case is a pydantic.BaseSettings instance which reads the Redis URL from an environment variable, if set. The fallback logic is for local development where a Redis instance may not be running.

I recently did an upgrade of my dependency tree which moved from Pydantic 1.9.0 to Pydantic 1.9.1, and my web worker processes now fail to boot. Reverting to Pydantic 1.9.0 resolves this.

The failure occurs at instantiating the Starlite application object:

File /opt/venv/lib/python3.10/site-packages/pydantic/decorator.py:40, in pydantic.decorator.validate_arguments.validate.wrapper_function()

File /opt/venv/lib/python3.10/site-packages/pydantic/decorator.py:133, in pydantic.decorator.ValidatedFunction.call()

File /opt/venv/lib/python3.10/site-packages/pydantic/decorator.py:130, in pydantic.decorator.ValidatedFunction.init_model_instance()

File /opt/venv/lib/python3.10/site-packages/pydantic/main.py:341, in pydantic.main.BaseModel.__init__()

ValidationError: 1 validation error for Init
cache_config
  cannot pickle '_thread.lock' object (type=type_error)

The root of the issue appears to be that in Pydantic 1.9.1, Pydantic defaults to performing a copy.deepcopy() of model members during validation, which in turn fails on encountering any non-pickle-able object (as in the above traceback where it attempts to deepcopy() a Redis client instance). Pydantic 1.9.0 performed only a shallow copy.

This behavior can be disabled by setting the copy_on_model_validation option to a false-y value in the config for a Pydantic model class, or by passing it in the config dictionary of the validate_arguments decorator, and that may be the simplest workaround to apply for now while Pydantic decides what to do about this. It may also be the correct long-term choice since it's likely that non-pickle-able objects, such as caching clients, will be passed in arguments to the Starlite constructor from time to time.

This issue appears to be present in any version of Starlite which decorates Starlite.__init__() with validate_arguments, when using Pydantic 1.9.1 (I've tested Starlite 1.7.3 and 1.3.3), and disappears on reverting to Pydantic 1.9.0.

@Goldziher
Copy link
Contributor

Ok, thanks for the analysis. Would you like to submit a PR?

@ubernostrum
Copy link
Author

If you'd be OK with passing "copy_on_model_validation": False as the workaround, I can put together a PR for it in the next couple days. The tricky part will be the testing -- do you have a preference for a non-pickle-able object to pass to Starlite to verify the fix?

@Goldziher
Copy link
Contributor

Absolutely

@peterschutt
Copy link
Contributor

Here's a repro for the pydantic side:

import tempfile
from typing import Any

import pydantic


class Container(pydantic.BaseModel):
    fh: Any


@pydantic.validate_arguments(config={"copy_on_model_validation": False})
def receives_container(container: Container) -> None:
    ...


with tempfile.TemporaryFile() as fh:
    receives_container(Container(fh=fh))

Results in:

Traceback (most recent call last):
  File "/home/peter/PycharmProjects/copy-on-model-validation/main.py", line 17, in <module>
    receives_container(Container(fh=fh))
  File "pydantic/decorator.py", line 40, in pydantic.decorator.validate_arguments.validate.wrapper_function
  File "pydantic/decorator.py", line 133, in pydantic.decorator.ValidatedFunction.call
  File "pydantic/decorator.py", line 130, in pydantic.decorator.ValidatedFunction.init_model_instance
  File "pydantic/main.py", line 341, in pydantic.main.BaseModel.__init__
pydantic.error_wrappers.ValidationError: 1 validation error for ReceivesContainer
container
  cannot pickle '_io.BufferedRandom' object (type=type_error)

I created that as I found this issue: pydantic/pydantic#3284 and wanted a quick test.. and as that issue suggests @pydantic.validate_arguments(config={"copy_on_model_validation": False}) has no effect.

Setting it on the Config object on Container does fix it.

@ubernostrum
Copy link
Author

ubernostrum commented Aug 3, 2022

Well, if passing it to validate_arguments doesn't actually work, then I guess I won't try that.

@Goldziher
Copy link
Contributor

So whats the solution? we strip validate arguments or we pin to 1.9? Or is there an alternative?

@ubernostrum
Copy link
Author

As a user, I'm pinning to Pydantic 1.9.0 until they decide what they're going to do about it. I'm not sure, and I don't presume to tell you, what the best choice is for Starlite -- both pinning and remove validate_arguments seem like they have downsides.

@peterschutt
Copy link
Contributor

We should probably set copy_on_model_validation = False on CacheConfig.Config to address this specific case, and see if any other issues arise. It's not as if 1.9.1 is a new release, so there's unlikely to be many more similar issues in my opinion.

@ubernostrum
Copy link
Author

I just did a rebuild of my app using Pydantic 1.9.1 and Starlite 1.8.1, and does indeed seem to work. Thanks for the quick fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants