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 v2 model inspect.signature lost information #7978

Open
1 task done
JoshYuJump opened this issue Oct 31, 2023 · 18 comments
Open
1 task done

Pydantic v2 model inspect.signature lost information #7978

JoshYuJump opened this issue Oct 31, 2023 · 18 comments
Assignees
Labels
bug V2 Bug related to Pydantic V2
Milestone

Comments

@JoshYuJump
Copy link

JoshYuJump commented Oct 31, 2023

Initial Checks

  • I confirm that I'm using Pydantic V2

Description

Packages versions:
pydantic 2.4.2
pydantic_core 2.10.1
pydantic-settings 2.0.3

class QueryParams(BaseModel):
    q: Annotated[list[str], Query()]

signature = inspect.signature(call)

In Pydantic v1, signature value is

<Signature (*, q: typing.Annotated[list[str], Query(PydanticUndefined)]) -> None>

While in Pydantic v2, signature value is

<Signature (*, q: list[str]) -> None>

The lost information is Query()

Example Code

No response

Python, Pydantic & OS Version

pydantic version: 2.4.2
pydantic-core version: 2.10.1
pydantic-core build: profile=release pgo=false
install path: /Users/byu4/Library/Caches/pypoetry/virtualenvs/next-Kg1YMAHS-py3.10/lib/python3.10/site-packages/pydantic
python version: 3.10.11 (main, Apr  7 2023, 07:33:46) [Clang 14.0.0 (clang-1400.0.29.202)]
platform: macOS-13.6-x86_64-i386-64bit
related packages: typing_extensions-4.8.0 fastapi-0.104.0 pydantic-settings-2.0.3 email-validator-2.1.0.post1
@JoshYuJump JoshYuJump added bug V2 Bug related to Pydantic V2 pending Awaiting a response / confirmation labels Oct 31, 2023
@howsunjow
Copy link
Contributor

howsunjow commented Nov 1, 2023

import inspect

from fastapi import Query
from typing import Annotated, List
from pydantic import BaseModel
from pydantic.dataclasses import dataclass

class QueryParams(BaseModel):
    q: Annotated[list[str], Query()]

@dataclass
class QueryParamsDataclass():
    q: Annotated[list[str], Query()]

inspect.signature(QueryParams)
inspect.signature(QueryParamsDataclass)

Replicates the issue. QueryParamsDataclass has the expected signature

<Signature (q: typing.Annotated[list[str], Query(PydanticUndefined)]) -> None>

so I suppose you could use that as a workaround if it's acceptable for FastApi.

@sydney-runkle
Copy link
Member

Hi @JoshYuJump,

This does look like a bug. @howsunjow, thanks for the example with the comparison to the dataclass signature.

The good news is, I think this might be fixed in the latest version of pydantic. Trying with latest, I get:

import inspect

from typing import List
from typing_extensions import Annotated
from pydantic import BaseModel
from pydantic.dataclasses import dataclass


class SomeMetadata(BaseModel):
    pass


class QueryParams(BaseModel):
    q: Annotated[List[str], SomeMetadata()]


@dataclass
class QueryParamsDataclass():
    q: Annotated[List[str], SomeMetadata()]


print(inspect.signature(QueryParams))
#> (*, q: typing_extensions.Annotated[List[str], SomeMetadata()]) -> None
print(inspect.signature(QueryParamsDataclass))
#> (q: typing_extensions.Annotated[List[str], SomeMetadata()]) -> None

We should be releasing v2.5 quite soon (hopefully today or tomorrow). I'll leave this open until we verify it's working for you with the new version @JoshYuJump.

@sydney-runkle sydney-runkle self-assigned this Nov 2, 2023
@sydney-runkle sydney-runkle removed the pending Awaiting a response / confirmation label Nov 2, 2023
@howsunjow
Copy link
Contributor

There's still a problem with the latest. The problem seems to be when you use FieldInfo (or a descendent class) in the annotation.

import inspect

from typing import Annotated, List
from pydantic import BaseModel
from pydantic.dataclasses import dataclass
from pydantic.fields import FieldInfo

class QueryParams(BaseModel):
    q: Annotated[list[str], FieldInfo(default=['abc'], annotation="List of strings")]

@dataclass
class QueryParamsDataclass():
    q: Annotated[list[str], FieldInfo(default=['abc'], annotation="List of strings")]

inspect.signature(QueryParams)
#> <Signature (*, q: list[str] = ['abc']) -> None>
inspect.signature(QueryParamsDataclass)
#> <Signature (q: typing.Annotated[list[str], FieldInfo(annotation=str, required=False, default=['abc'])]) -> None>

@sydney-runkle
Copy link
Member

@howsunjow,

Thanks for the feedback. I'll look into this 👍

@howsunjow
Copy link
Contributor

I did a bit more investigation. In pydantic 1.10.13

class QueryParams(BaseModel):
    q: Annotated[list[str], FieldInfo(default=['123'], annotation="List of strings")]

fails with

Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "pydantic/main.py", line 197, in pydantic.main.ModelMetaclass.__new__
  File "pydantic/fields.py", line 497, in pydantic.fields.ModelField.infer
  File "pydantic/fields.py", line 469, in pydantic.fields.ModelField._get_field_info
ValueError: `Field` default cannot be set in `Annotated` for 'q'
class QueryParams(BaseModel):
    q: Annotated[list[str], FieldInfo(annotation="List of strings")]

works and gives the signature

<Signature (*, q: typing.Annotated[list[str], FieldInfo(default=PydanticUndefined, extra={'annotation': 'List of strings'})]) -> None>

Can we assume that this should be the correct behaviour for 2.0?

@JoshYuJump
Copy link
Author

@sydney-runkle Any news on it?

@sydney-runkle
Copy link
Member

@JoshYuJump,

I haven't looked into this further yet. Would you like to take a shot at a fix?

@JoshYuJump
Copy link
Author

@sydney-runkle

I found v2 try to split Annotated field to type and metadata.

for example:

q: Annotated[list[str], Query()]

It will be split to 2 parts in FieldInfo instance:
type is list[str]
meta is metadata

The behavior is different in v1, v1 doesn't split it.

So, your team prefer to split it, but FastAPI not support it?

@JoshYuJump
Copy link
Author

@sydney-runkle The is another issue that, metadata Query() will lost in classmethod
FieldInfo.from_annotation

>>> annotation
typing.Annotated[list[str], Query(None)]
>>> result = FieldInfo.from_annotation(annotation)

result.metadata is empty list. It should be [Query(None),] as your design.

@howsunjow
Copy link
Contributor

I've submitted a PR (PR #8318) to try and fix the issue. Instead of going into the weeds of what is "correct" I aimed for consistency with pydantic version 1 when it comes to signatures.

@sydney-runkle sydney-runkle added this to the v2.6.0 milestone Dec 7, 2023
@sydney-runkle
Copy link
Member

@howsunjow @JoshYuJump,

Looking into this more now. I'm curious what your use case is for making this change.

As far as I can tell, it's not essential that we include all of the information specified in an Annotation block within the signature. I understand that there's a difference in the results of inspect.signature compared to V1, but I don't think that this alone is enough to constitute a change.

@JoshYuJump
Copy link
Author

@sydney-runkle After more research, I found the v2's annotation is different design of v1's, some information is stored in metadata attribute in v2.

My case is work with FastAPI, A lot of people have the same problem as me.
If the design is so, maybe FastAPI is not work well with Pydantic V2.

tiangolo/fastapi#10331
tiangolo/fastapi#10556

@sydney-runkle
Copy link
Member

An update on this - the fix I have proposed above breaks FastAPI schema tests, so we're working on fixing both.

Going to pivot my attention to getting 2.6 out, but will return to this as a priority for 2.7!

@dmontagu
Copy link
Contributor

@JoshYuJump it would be helpful if you could explain what you are trying to achieve with FastAPI at a higher level. While we can change what the signature contains, presumably that won't help you if FastAPI doesn't handle the updated signatures correctly (which I think is what Sydney noticed — changing the way we generate the signature causes defaults to get mishandled in some cases).

We can definitely work to make this better in both Pydantic and FastAPI together, but understanding what the end use case is will probably make it easier for us to make simultaneous modifications to Pydantic and FastAPI to get everything working.

In particular, if you could provide a code snippet involving FastAPI and Pydantic together and explain what you are trying to achieve / how it used to work vs. how it works now, it will make it a lot easier for us to address that use case, rather than making independent changes to signature generation that may or may not actually help.

@dmontagu
Copy link
Contributor

@JoshYuJump Actually, I reviewed one of the issues you linked, and think I understand the use case:

from fastapi import FastAPI, Query, Depends
from pydantic import BaseModel
from starlette.testclient import TestClient


class Request(BaseModel):
    scalar_parameter: int = Query()  # perfectly fine, added to parameters.
    list_parameter: list[int] = Query()  # this is moved to body instead of query.


app = FastAPI()


@app.get(path="/endpoint-with-query-params")
def get_resource(
    scalar_parameter: int = Query(),
    list_parameter: list[int] = Query()
) -> str:
    return f"scalar_parameter: {scalar_parameter}, list_parameter: {list_parameter}"


@app.get(path="/endpoint-with-model-depends")
def get_resource(req: Request = Depends()) -> str:
    return f"scalar_parameter: {req.scalar_parameter}, list_parameter: {req.list_parameter}"


response = TestClient(app).get("/endpoint-with-query-params?scalar_parameter=1&list_parameter=1&list_parameter=2")
print(f'{response.status_code}: {response.content.decode()}')
#> 200: "scalar_parameter: 1, list_parameter: [1, 2]"

response = TestClient(app).get("/endpoint-with-model-depends?scalar_parameter=1&list_parameter=1&list_parameter=2")
print(f'{response.status_code}: {response.content.decode()}')
#> 422: {"detail":[{"type":"missing","loc":["body"],"msg":"Field required","input":null,"url":"https://errors.pydantic.dev/2.6/v/missing"}]}

# You can see the problem here — because list[int] is inferred as being a body parameter, 
# without the default assignment to Query() it is mishandled:
print(Request.__signature__)
#> (*, scalar_parameter: int, list_parameter: list[int]) -> None

It seems to me you would like both of the endpoints above to return a 200 (with the same response). If it did that, would that address your issue?

@JoshYuJump
Copy link
Author

@JoshYuJump Actually, I reviewed one of the issues you linked, and think I understand the use case:

from fastapi import FastAPI, Query, Depends
from pydantic import BaseModel
from starlette.testclient import TestClient


class Request(BaseModel):
    scalar_parameter: int = Query()  # perfectly fine, added to parameters.
    list_parameter: list[int] = Query()  # this is moved to body instead of query.


app = FastAPI()


@app.get(path="/endpoint-with-query-params")
def get_resource(
    scalar_parameter: int = Query(),
    list_parameter: list[int] = Query()
) -> str:
    return f"scalar_parameter: {scalar_parameter}, list_parameter: {list_parameter}"


@app.get(path="/endpoint-with-model-depends")
def get_resource(req: Request = Depends()) -> str:
    return f"scalar_parameter: {req.scalar_parameter}, list_parameter: {req.list_parameter}"


response = TestClient(app).get("/endpoint-with-query-params?scalar_parameter=1&list_parameter=1&list_parameter=2")
print(f'{response.status_code}: {response.content.decode()}')
#> 200: "scalar_parameter: 1, list_parameter: [1, 2]"

response = TestClient(app).get("/endpoint-with-model-depends?scalar_parameter=1&list_parameter=1&list_parameter=2")
print(f'{response.status_code}: {response.content.decode()}')
#> 422: {"detail":[{"type":"missing","loc":["body"],"msg":"Field required","input":null,"url":"https://errors.pydantic.dev/2.6/v/missing"}]}

# You can see the problem here — because list[int] is inferred as being a body parameter, 
# without the default assignment to Query() it is mishandled:
print(Request.__signature__)
#> (*, scalar_parameter: int, list_parameter: list[int]) -> None

It seems to me you would like both of the endpoints above to return a 200 (with the same response). If it did that, would that address your issue?

Yes, you are right.
The issue I found is the list_parameter should not moved to body.

@sydney-runkle sydney-runkle modified the milestones: v2.6.0, v2.7.0 Jan 20, 2024
@sydney-runkle sydney-runkle removed this from the v2.7.0 milestone Mar 26, 2024
@JoshYuJump
Copy link
Author

@sydney-runkle well, removed from the milestone?

@sydney-runkle
Copy link
Member

I'll add to 2.8!

@sydney-runkle sydney-runkle added this to the v2.8.0 milestone Mar 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug V2 Bug related to Pydantic V2
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants