Skip to content

Update pydantic-duality to fix the infinite recursion bug#1902

Merged
jvstme merged 1 commit intodstackai:masterfrom
zmievsa:update-pydantic-duality
Oct 29, 2024
Merged

Update pydantic-duality to fix the infinite recursion bug#1902
jvstme merged 1 commit intodstackai:masterfrom
zmievsa:update-pydantic-duality

Conversation

@zmievsa
Copy link
Copy Markdown
Contributor

@zmievsa zmievsa commented Oct 27, 2024

It was a long one. Here's the entire process for the fix:

Resolves zmievsa/pydantic-duality#21

  1. Here's the minimal reproducible example:
import abc

from pydantic import BaseModel
from pydantic_duality import DualBaseModel


class AzureStoredConfig(DualBaseModel):
    resource_group: str


# from cryptography.hazmat.primitives.hashes import HashContext

# Which actually contains the code below:

# ```


class HashContext(metaclass=abc.ABCMeta):
    pass


from cryptography.hazmat.bindings._rust import openssl as rust_openssl

HashContext.register(rust_openssl.hashes.Hash)


# ```


class AzureConfig(AzureStoredConfig, BaseModel):
    pass


class GCPVolumeDiskBackendData(DualBaseModel):
    disk_type: str
  1. Here's the fix in pydantic-duality
  2. Here's an alternative fix within dstack

I am still not sure what exactly is causing the bug but here's what happens:

  1. Because pydantic-duality now uses DualBaseModel.__response__ in subclass checks, the response model gets generated (exactly once though) during the very first subclass check
  2. In one of those checks, we have issubclass(annotation, pydantic.BaseModel). The specific class that caused the issue here is str so issubclass(str, pydantic.BaseModel)
  3. For some unknown and weird reason, it calls DualBaseModel.__subclasshook__ instead of using BaseModel's or type's default subclass check. Not sure how it is possible but I verified that everything from the code block above is absolutely necessary for this situation to happen

@zmievsa
Copy link
Copy Markdown
Contributor Author

zmievsa commented Oct 27, 2024

NOTICE that I dropped support for python 3.8 as it reached its EOL. Is that fine with you?

@peterschmidt85
Copy link
Copy Markdown
Contributor

cc @jvstme on Python 3.8

@r4victor
Copy link
Copy Markdown
Collaborator

r4victor commented Oct 28, 2024

@zmievsa, thanks for the writeup and the fix! We're going to support 3.13 and drop 3.8 also, so that's fine.

Regarding the issue, I have a hypothesis what can be the root cause:

For some unknown and weird reason, it calls DualBaseModel.__subclasshook__ instead of using BaseModel's or type's default subclass check.

This is due to this ABCMeta.__subclasscheck__ logic that calls __subclasscheck__ of subclassed metaclasses:

https://github.com/python/cpython/blob/19e93e2e269889ecb3c4c039091abff489f247c2/Lib/_py_abc.py#L140-L144

The problem only appears if there is any other MyMetaclass.register() call because it invalidates the negative subclass cache:

https://github.com/python/cpython/blob/19e93e2e269889ecb3c4c039091abff489f247c2/Lib/_py_abc.py#L28-L33

So the behaviour does not depend on any cryptography logic. This example will do:

import abc

from pydantic import BaseModel
from pydantic_duality import DualBaseModel


class AzureStoredConfig(DualBaseModel):
    resource_group: str


class MyMeta(metaclass=abc.ABCMeta):
    pass


MyMeta.register(int)


class AzureConfig(AzureStoredConfig, BaseModel):
    pass


class GCPVolumeDiskBackendData(DualBaseModel):
    disk_type: str

@zmievsa
Copy link
Copy Markdown
Contributor Author

zmievsa commented Oct 28, 2024

@r4victor nice catch! You are likely right.

@jvstme
Copy link
Copy Markdown
Collaborator

jvstme commented Oct 28, 2024

I will take care of merging this PR just a bit later, once we drop Python 3.8. Probably today. Thanks, @zmievsa!

upd: will rebase and merge after #1910

@jvstme jvstme force-pushed the update-pydantic-duality branch from bd2a91b to d21b0ab Compare October 29, 2024 08:03
@jvstme jvstme merged commit 4330c3f into dstackai:master Oct 29, 2024
@zmievsa zmievsa deleted the update-pydantic-duality branch October 29, 2024 17:54
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 this pull request may close these issues.

After update to 1.2.3: RecursionError: maximum recursion depth exceeded

5 participants