Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork2.2k
RefactorFieldInfo creation implementation#11898
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Required to fix a type checking issue
The existing implementation did not warn when a callablewas used first, then a dict. Also use a plain `UserWarning`as `PydanticJsonSchemaWarning` is meant to be used*during* JSON Schema generation.
Both were actually inconsistent, in particular when using the`warnings.deprecated` class as metadata, we would only setthe message string `FieldInfo.deprecated`.
We don't do any mutations of the assigned `Field()` anymore.
| ) | ||
| default = assigned_value.default.__get__(None, cls) | ||
| assigned_value.default = default | ||
| assigned_value._attributes_set['default'] = default |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Next step would be to get rid of the_attributes_set logic when merging instances, which is error prone as you need to update it whenever you do a manual assignment on aFieldInfo instance.
| @@ -213,7 +213,7 @@ def __init__(self, **kwargs: Unpack[_FieldInfoInputs]) -> None: | |||
| See the signature of `pydantic.fields.Field` for more details about the expected arguments. | |||
| """ | |||
| self._attributes_set = {k: v for k, v in kwargs.items() if v is not _Unset} | |||
| self._attributes_set = {k: v for k, v in kwargs.items() if v is not _Unset and k not in self.metadata_lookup} | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This is kind of important, although it also works without the change:
When we merge field infos, we don't want to reinclude kwargs that are transformed to metadata elements (gt ->annotated_types.Gt, etc). This will result in unnecessary metadata classes to be created (at the end of theFieldInfo.__init__() logic).
codspeed-hqbot commentedMay 21, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
CodSpeed Performance ReportMerging#11898 willdegrade performances by 13.42%Comparing Summary
Benchmarks breakdown
|
github-actionsbot commentedMay 21, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Coverage reportClick to see where and how coverage changed
This report was generated bypython-coverage-comment-action | ||||||||||||||||||||||||||||||||||||
cloudflare-workers-and-pagesbot commentedMay 22, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Deploying pydantic-docs with |
| Latest commit: | 3ce5218 |
| Status: | ✅ Deploy successful! |
| Preview URL: | https://96843f8e.pydantic-docs.pages.dev |
| Branch Preview URL: | https://fieldinfo-cleanup.pydantic-docs.pages.dev |
| def _copy(self) -> Self: | ||
| """Return a copy of the `FieldInfo` instance.""" | ||
| # Note: we can't define a custom `__copy__()`, as `FieldInfo` is being subclassed | ||
| # by some third-party libraries with extra attributes defined (and as `FieldInfo` | ||
| # is slotted, we can't make a copy of the `__dict__`). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Alternatively, we can drop slots onFieldInfo, and do the following:
def__copy__(self)->Self:copied=type(self)()copied.__dict__=self.__dict__forattr_namein ('metadata','_attributes_set','_qualifiers'):# Apply "deep-copy" behavior on collections attributes:value=getattr(self,attr_name).copy()setattr(copied,attr_name,value)returncopied
| @@ -505,28 +505,6 @@ class AnnotatedFieldModel(BaseModel): | |||
| } | |||
| ] | |||
| # Ensure that the inner annotation does not override the outer, even for metadata: | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Due to one of the bugs being fixed in this PR (related to the usage of forward references), the constraint inAnnotated was dropped. Now the test is failing because the unexpected order (described in#10507) applies.
| # HACK 2: FastAPI is subclassing `FieldInfo` and historically expected the actual | ||
| # instance's type to be preserved when constructing new models with its subclasses as assignments. | ||
| # This code is never reached by Pydantic itself, and in an ideal world this shouldn't be necessary. | ||
| if not metadata and isinstance(default, FieldInfo) and type(default) is not FieldInfo: | ||
| field_info = default._copy() | ||
| field_info._attributes_set.update(attr_overrides) | ||
| for k, v in attr_overrides.items(): | ||
| setattr(field_info, k, v) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Unfortunate stuff introduced since#6862..
cc@zmievsa, I've tried looking into the Cadwyn issues but got too scared by the code base. |
zmievsa commentedMay 23, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Apologies for not fixing it yesterday. I'll fix it in a second Update: oh, I see that it's unrelated to my recent problems with Cadwyn CI. Well, I'll fix it now anyways :) |
No rush, the third-party CI is not blocking and this is only going to be included in 2.12 anyway. |
zmievsa commentedMay 24, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@Viicos WDYT about this approach for extracting metadata? Is this the correct interface to use? (it works according to my tests but I am worried that I'm using the wrong abstraction from pydantic for extracting it |
…or type checkers)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@Viicos Great work Victorien, the separate commits and your comments were very helpful! Just 2 questions aboutprepend_metadata.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
565cea9 intomainUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Change Summary
(best reviewed commit per commit).
Fixes#11870,fixes#11876,fixes#11978.
Fixes#11122 (the three bugs described in the issue).
Issue#10507isn't fixed with this, but at least now properly documented as a workaround in the implementation.
Most of the context of this PR is described in#11122.
The main thing being done here is the added
_construct()classmethod, that centralizes most of the merging logic and avoids repetition (which actually wasn't mirrored in every code path, that's why many inconsistencies existed depending on whether you assigned aField()to an attribute). We avoid copyingFieldInfoinstances everywhere, and doing buggy metadata handling.As a result, the
merge_field_infos()method is unused (you can compare both this method and_construct()to see what changed — the latter is much simpler). We need to deprecate it as third-party projects rely on it (unfortunately..). I propose marking it as deprecatedonly for type checkers for now, and have a runtime deprecation emitted in V3?openapi-python-client third party failure is unrelated, their CI is currently failing.