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

Exclude pydantic 2.10.2 #1562

Closed
wants to merge 1 commit into from
Closed

Conversation

dokterbob
Copy link
Collaborator

@dokterbob
Copy link
Collaborator Author

dokterbob commented Dec 3, 2024

The CI installs from poetry lock file (for reproducibility and to avoid resolving deps all the time), which uses pydantic 1. We might consider testing both 1 and 2, although we can then start doing that with a frightening about of deps!

Trying install without lock file now, but it's taking forever to lock dependencies...

Ergo; feedback from community on this one welcome.

@Viicos
Copy link
Contributor

Viicos commented Dec 3, 2024

Hi @dokterbob, Pydantic maintainer here.

A couple things to note:

  • Pydantic V2 (released a year and a half ago) is changing a lot of things from V1. Depending on your usage of Pydantic, it might be tricky to support both. But looking at your code base, you are mainly defining dataclasses/models and did not make use of validators, so seems like it should be alright.
  • In 2.10, we did a complete refactor of the forward annotations evaluation logic. I'm currently investigating your use case, and I'll get back to you once this done. However, do note that "Foo is not fully defined; you should define T" error when using new generic syntax with dataclass pydantic/pydantic#11031 is completely unrelated from the issues you are seeing.

@Viicos
Copy link
Contributor

Viicos commented Dec 3, 2024

Ok so turns out the change is unrelated to my second bullet point.

In the chainlit.config module, you are importing symbols in an if TYPE_CHECKING: block to avoid circular imports:

if TYPE_CHECKING:
from fastapi import Request, Response
from chainlit.action import Action
from chainlit.message import Message
from chainlit.types import ChatProfile, InputAudioChunk, Starter, ThreadDict
from chainlit.user import User

Later in the same module, you are making use of these type annotations in Pydantic dataclasses, e.g.:

@dataclass()
class CodeSettings:
# Developer defined callbacks for each action. Key is the action name, value is the callback function.
action_callbacks: Dict[str, Callable[["Action"], Any]]

At runtime, TYPE_CHECKING evaluates to False so the imported symbols are not available; but Pydantic needs to evaluate the forward references to understand which type is used to apply validation/serialization. Now it just so happens that these imported symbols are only used inside Callable[[...], ...] annotations.

As of today, Pydantic does not inspect the parameters of the Callable special form, and just checks whether the provided value is a callable (but the number and/or types of the callable parameters/return type are not checked):

def some_func() -> None:
    return None

class Model(BaseModel):
    my_func: Callable[[int, str, "SomeTypeThatDoesntExist"], bool]

Model(my_func=some_func)  # Validation succeeds

In 2.10.2, as a consequence of a small performance improvement, we now always try to evaluate the forward annotation, even for Callable types. But using Callable[["Action"], Any] with "Action" being undefined was very fragile because:

  • If the string annotation was enclosing the whole type (i.e. "Callable[[Action], Any]), it would have failed already in previous Pydantic verisons due to how forward annotations are evaluated by Python.
  • In the future, we might implement full support for Callable types, meaning Pydantic would have to understand what "Action" refers to.

What I would suggest is:

  • Try to fix the circular imports. In general, this isn't an easy task, and looking at your code architecture, it doesn't seem trivial.
  • Use the following trick in the chainlit.config module:
    if TYPE_CHECKING:
        from fastapi import Request, Response
    
        from chainlit.action import Action
        from chainlit.message import Message
        from chainlit.types import ChatProfile, InputAudioChunk, Starter, ThreadDict
        from chainlit.user import User
    else:
        # Pydantic needs to resolve forward annotations. Because all of these are used
        # within `typing.Callable`, alias to `Any` as Pydantic does not perform validation
        # of callable argument/return types anyway.
        Request = Response = Action = Message = ChatProfile = InputAudioChunk = Starter = ThreadDict = User = Any
    Note that this is still a bit dangerous, as if you start to make use of these types outside typing.Callable, Pydantic will assume they are Any. I opened Fix forward annotations evaluation issue in chainlit.config #1564 showing this.

@dokterbob
Copy link
Collaborator Author

Thank you so much @Viicos, for this excellent contribution.

I only afforded a quick look and it seemed that the similar error message together with the recent release suggested a bug on your end. Appreciate your clarification and super appreciate your fix!

  • Try to fix the circular imports. In general, this isn't an easy task, and looking at your code architecture, it doesn't seem trivial.

:')

Every pass I make I refactor a bit -- eventually we should not have to avoid type checking anywhere. But there's a long way to go. ;)

@dokterbob dokterbob closed this Dec 3, 2024
@dokterbob dokterbob deleted the dokterbob/exclude_broken_pydantic branch December 3, 2024 18:26
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.

2 participants