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
Do not override model_post_init
in subclass with private attrs
#7302
Changes from 3 commits
9c3c398
39e2c60
031eda6
d6f90ea
a8fc2b0
ecef4e0
6181f80
d5095a9
a0591c0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -104,9 +104,9 @@ def __new__( | |||||||||||||||||||
namespace, config_wrapper.ignored_types, class_vars, base_field_names | ||||||||||||||||||||
) | ||||||||||||||||||||
if private_attributes: | ||||||||||||||||||||
if 'model_post_init' in namespace: | ||||||||||||||||||||
original_model_post_init = get_model_post_init(namespace, bases) | ||||||||||||||||||||
if original_model_post_init is not None: | ||||||||||||||||||||
# if there are private_attributes and a model_post_init function, we handle both | ||||||||||||||||||||
original_model_post_init = namespace['model_post_init'] | ||||||||||||||||||||
|
||||||||||||||||||||
def wrapped_model_post_init(self: BaseModel, __context: Any) -> None: | ||||||||||||||||||||
"""We need to both initialize private attributes and call the user-defined model_post_init | ||||||||||||||||||||
|
@@ -266,6 +266,17 @@ def init_private_attributes(self: BaseModel, __context: Any) -> None: | |||||||||||||||||||
object_setattr(self, '__pydantic_private__', pydantic_private) | ||||||||||||||||||||
|
||||||||||||||||||||
|
||||||||||||||||||||
def get_model_post_init(namespace: dict[str, Any], bases: tuple[type[Any], ...]) -> Callable[..., Any] | None: | ||||||||||||||||||||
"""Get the `model_post_init` method from the namespace or the class bases, or `None` if not defined.""" | ||||||||||||||||||||
if 'model_post_init' in namespace: | ||||||||||||||||||||
return namespace['model_post_init'] | ||||||||||||||||||||
|
||||||||||||||||||||
for base in bases: | ||||||||||||||||||||
model_post_init = getattr(base, 'model_post_init', None) | ||||||||||||||||||||
if model_post_init is not None: | ||||||||||||||||||||
return model_post_init | ||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
I think this is probably the "most correct" version of this, since we are already using the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The above is also closely related to what we do when setting There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unfortunately even using for base in mro_for_bases(bases):
model_post_init = base.__dict__.get('model_post_init')
if model_post_init is not None and model_post_init is not BaseModel.model_post_init:
return model_post_init Edit: that did the trick (see There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @Viicos this is a good catch — but actually I think this is a bug in the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure, do you want to do it in a separate PR? Might be easier if it breaks some tests. Then I'll rebase on There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Doesn't break any tests — I think my preference would be to just put it in this PR since the test you've added is actually a good test of that change. To reduce the back-and-forth I'll just push the commit in hopes you are okay with it; I can revert that if you prefer though, and move it elsewhere. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Totally ok with it, thanks for the last push! |
||||||||||||||||||||
|
||||||||||||||||||||
|
||||||||||||||||||||
def inspect_namespace( # noqa C901 | ||||||||||||||||||||
namespace: dict[str, Any], | ||||||||||||||||||||
ignored_types: tuple[type[Any], ...], | ||||||||||||||||||||
|
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 will actually always be defined for any base, as any subclass of
BaseModel
will inherit from the original empty method:pydantic/pydantic/main.py
Lines 417 to 421 in c987bc6
So if we have the following:
get_model_post_init
will use the emptymodel_post_init
available onA
, and skip the custom one fromB
. Any ideas on how to prevent this? Maybe a custom flag could help, similar to:pydantic/pydantic/main.py
Lines 167 to 168 in c987bc6
?
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.
If your concern is to make sure attribute lookup only considers things defined on the exact class, the way we achieve that elsewhere is to look in
base.__dict__
. So, if you doif 'model_post_init' in base.__dict__
, that tells you if it was defined specifically inbase
vs. in one of its subclasses. The best way to do it might be to loop over the__mro__
for the class looking for items in the classes'__dict__
s. But it's not clear to me if that is relevant here.Thinking this through, I think if we want to auto-generate a method but not have that auto-generated version factor into inheritance, using a flag like you've described is probably the best way. But, since in this case the method is defined on BaseModel, I think the correct behavior would actually involve calling the BaseModel version in your code above, since that is consistent with standard python behavior for method resolution:
Not sure if I'm overlooking something important here; in general I'm open to other points of view but I think it will be important that whatever we do doesn't introduce breaking changes here. More generally, I won't be surprised if the best solution ends up being for users to just be more proactive about overriding
model_post_init
in subclasses when inheriting from multiple base models and not being able to put the one with the desiredmodel_post_init
first in the inheritance order.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.
Also, I think if all you are wanting to do is specifically ignore
BaseModel.model_post_init
, it might be best to add something like:It's not clear to me if that's how it should work, or if any such generated method should be ignored (this might be the case if you are adding more private attributes for example, but never defined the
model_post_init
explicitly).Either way, if we change this, I think we should probably add a section to the docs about how this is expected to work because it's starting to get away from typical python behavior. (But I think is likely still enough of an improvement in behavior to be worth making the change.)