Do not override model_post_init in subclass with private attrs

[Viicos]

Change Summary

Related issue number

Fixes #7293

Checklist

• The pull request title is a good summary of the changes - it will be used in the changelog
• Unit tests for the changes exist
• Tests pass on CI
• Documentation reflects the changes where applicable
• My PR is ready to review, please add a comment including the phrase "please review" to assign reviewers

Selected Reviewer: @hramezani

[Viicos]

Please review

[Viicos]

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

	def model_post_init(self, __context: Any) -> None:
	"""Override this method to perform additional initialization after `__init__` and `model_construct`.
	This is useful if you want to do some validation that requires the entire model to be initialized.
	"""
	pass

So if we have the following:

from pydantic import BaseModel

class A(BaseModel):
    a: int = 1

class B(BaseModel):
    b: int = 1

    def model_post_init(self, __context):
        """Do custom things"""

class C(A, B):
    _private: bool = True

get_model_post_init will use the empty model_post_init available on A, and skip the custom one from B. 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

	# The following line sets a flag that we use to determine when `__init__` gets overridden by the user
	__init__.__pydantic_base_init__ = True

?

[dmontagu]

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 do if 'model_post_init' in base.__dict__, that tells you if it was defined specifically in base 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:

class A:
    def my_method(self):
        print('A')

class B(A):
    pass

class C:
    def my_method(self):
        print('C')

class D(B, C):
    pass

D().my_method()
#> A

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 desired model_post_init first in the inheritance order.

[dmontagu]

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:

if model_post_init is BaseModel.model_post_init:
    continue

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.)

[dmontagu]

Suggested change

	for base in bases:
	model_post_init = getattr(base, 'model_post_init', None)
	if model_post_init is not None:
	return model_post_init
	from ..main import BaseModel
	model_post_init = get_attribute_from_bases(bases, 'model_post_init')
	if model_post_init is not BaseModel.model_post_init:
	return model_post_init

I think this is probably the "most correct" version of this, since we are already using the get_attribute_from_bases function which handles mro the same as python, etc.

[dmontagu]

The above is also closely related to what we do when setting __pydantic_post_init__ (i.e., ignoring BaseModel.model_post_init and follows the same logic as we do when setting __hash__ (i.e., using the mro).

[Viicos]

Unfortunately even using get_attribute_from_bases, the test is still failing (in fact get_attribute_from_bases is doing something quite similar, i.e. using hasattr and getattr, so we face the same issue). Maybe doing something like the following:

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 test_model_post_init_correct_mro)

[dmontagu]

@Viicos this is a good catch — but actually I think this is a bug in the get_attribute_from_bases function — it shouldn't be using hasattr and getattr for exactly this reason. I think looking in the class __dict__ should fix this and allow us to use that function, if it does, I would propose we make that change. Any objections? If not, I can push a commit and merge this.

[Viicos]

Sure, do you want to do it in a separate PR? Might be easier if it breaks some tests. Then I'll rebase on main once merged

[dmontagu]

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.

[Viicos]

Totally ok with it, thanks for the last push!