-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
Refactor logic to support Pydantic's Field() function in dataclasses
#12051
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
Conversation
Deploying pydantic-docs with
|
| Latest commit: |
b1e3452
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://035322ad.pydantic-docs.pages.dev |
| Branch Preview URL: | https://dataclass-field-fix.pydantic-docs.pages.dev |
a3f3148 to
c0431fd
Compare
CodSpeed Performance ReportMerging #12051 will not alter performanceComparing Summary
|
Coverage reportClick to see where and how coverage changed
This report was generated by python-coverage-comment-action |
||||||||||||||||||||||||||||||||||||
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.
Wow, powerful!
If there's no alternative, moving ahead with this seems the most reasonable option to preserve correct semantics.
It would be nice if we could localise the patching just to the under-construction type somehow. I posted an idea but no idea if it actually works.
| !!! note | ||
| This approach is far from ideal, and can probably be the source of unwanted side effects/race conditions. | ||
| The previous implemented approach was mutating the `__annotations__` dict of `cls`, which is no longer a | ||
| safe operation in Python 3.14+, and resulted in unexpected behavior with field ordering anyway. |
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.
Agreed, scary. If this became a problem, I think we could do stuff like set the field to be a wrapper object which checks what thread it's on and only exhibits the patched behaviour on this thread, but even that is observable so why bother 😢
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.
Yeah I think if we ever get issues about this, we could wrap the mutation in a threading lock (similar to what I've initially tried to do in #11851). For this to be an issue, a user would have to dynamically create Pydantic dataclasses in a multi-threaded environment, and have them subclassing a stdlib dataclass with at least one field using the Pydantic's Field() function which is quite unlikely to happen.
| new_dc_field.kw_only = True | ||
| if default.repr is not True: | ||
| new_dc_field.repr = default.repr | ||
| dc_fields[field_name] = new_dc_field |
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.
I wonder, rather than mutating the fields of the base objects, is there a world where we instead patch the class attributes of the type under construction? That might be significantly less far-reaching in potential conflict?
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.
The issue is that the dataclasses module is doing roughly:
fields: dict[str, Field] = {}
for b in cls.__mro__[-1:0:-1]:
base_fields = getattr(b, '__dataclass_fields__', None)
if base_fields is not None:
for f_name, f in base_fields.items():
fields[f_name] = f
# Then fetch the annotations of the class under construction and build fields for itI thought about patching the whole __dataclass_fields__ attribute directly, without mutating the original dict, but I don't know which one is best. I think we can revisit if we ever get issues.
1588780 to
c19f555
Compare
c19f555 to
bceff23
Compare
|
FastAPI failures are unrelated, we merged a PR that changed the JSON Schema for decimals. |
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.
Discussion makes sense, lgtm 👍
Change Summary
Fixes #12045.
Part of #11613.
Related issue number
Checklist