Repository: pydantic/pydantic
Pull request: Keep values of private attributes set within `model_post_init` in subclasses
## Change Summary

Check for an existing `__pydantic_private__` dict in `init_private_attributes` and account for values within, rather than always replacing with a new dict.

## Related issue number

Fixes https://github.com/pydantic/pydantic/issues/7091

## Checklist

* [x] The pull request title is a good summary of the changes - it will be used in the changelog
* [x] Unit tests for the changes exist
* [x] Tests pass on CI
* [x] Documentation reflects the changes where applicable
* [x] My PR is ready to review, **please add a comment including the phrase "please review" to assign reviewers**


Selected Reviewer: @sydney-runkle

Problem statement:
Inheritance of Models with private attributes causes unexpected behavior via model_post_init
### Initial Checks

- [X] I confirm that I'm using Pydantic V2

### Description

When a class defines a private attribute that inherits from a model containing its own private attribute. Values set in `model_post_init` are reset due to the `wrapped_model_post_init` in `ModelMetaclass`

### Example Code

```Python
from typing import Any, Optional
from pydantic import BaseModel, PrivateAttr


class BadBase(BaseModel):
    _random: Optional[str] = PrivateAttr(None)


class OkBase(BaseModel):
    random: Optional[str] = None


class DoesNotWork(BadBase):
    a: str
    _b: Optional[str] = PrivateAttr(None)

    def model_post_init(self, __context: Any) -> None:
        self._b = "cool"
        return super().model_post_init(__context)


class Works(OkBase):
    a: str
    _b: Optional[str] = PrivateAttr(None)

    def model_post_init(self, __context: Any) -> None:
        self._b = "cool"
        return super().model_post_init(__context)


if __name__ == "__main__":
    a_dict = {"a": "string"}
    bad_model = DoesNotWork.model_validate(a_dict)
    good_model = Works.model_validate(a_dict)
    assert bad_model._b is None  # should be "cool"
    print(bad_model._b)
    assert good_model._b == "cool"
    print(good_model._b)
```


### Python, Pydantic & OS Version

```Text
pydantic version: 2.1.1
        pydantic-core version: 2.4.0
          pydantic-core build: profile=release pgo=false mimalloc=true
                 install path: /Users/drewayling/Library/Caches/pypoetry/virtualenvs/libs-vsna-tools-IjJ702vs-py3.8/lib/python3.8/site-packages/pydantic
               python version: 3.8.16 (default, Apr 25 2023, 14:05:36)  [Clang 14.0.3 (clang-1403.0.22.14.1)]
                     platform: macOS-13.5-x86_64-i386-64bit
     optional deps. installed: ['typing-extensions']
```


Selected Assignee: @lig

Hints from issue discussion:
I can confirm that the issue reproduces on 

```
             pydantic version: 2.2.1
        pydantic-core version: 2.6.1
          pydantic-core build: profile=release pgo=true
               python version: 3.11.4 (main, Jun  7 2023, 00:00:00) [GCC 13.1.1 20230511 (Red Hat 13.1.1-2)]
                     platform: Linux-6.4.10-200.fc38.x86_64-x86_64-with-glibc2.37
     optional deps. installed: ['devtools', 'email-validator', 'typing-extensions']
```
Can I ask why you wouldn't do:

```python
    def model_post_init(self, __context: Any) -> None:
        super().model_post_init(__context)
        self._b = "cool"
```

I think changing it like that would eliminate the need for #7775 entirely.

In general, I don't think I have an issue with the `pydantic_private = getattr(self, '__pydantic_private__', None) or {}` change, but changing the assignment to a `setdefault` seems a little more strange. I don't _currently_ see any issue with it, but I'm not sure all the possible consequences and if the purpose is just to let you set attributes prior to the call to the `model_post_init`, it seems less compelling to me to make that change. Though I guess I could see the argument that if you set the private attribute before _or_ after the call to `model_post_init`, you probably didn't want the "default" value to replace it.
Note that I didn't personally bump into this, I'm just looking for ways to contribute.

I agree that you could probably just call the super method before setting private attributes in most reasonable situations. But I think this looks like a very confusing sharp edge. Sometimes people might call the super method at the end without realising there's no good reason to do that. That seems plausibly the case here, where there's also a `return` that doesn't seem to achieve anything. Sometimes there might be good reasons to perform other logic unrelated to private attributes before calling the super method, so the super call is already sitting at the end when the `self._b = "cool"` line gets added and the author isn't expecting it to matter where they add it. Whatever the reason, the author shouldn't have to step into the pydantic source code and look at things like `wrapped_model_post_init` to figure out the problem. And even if they do that, it seems fair that reordering the lines just might not occur to them.

It also seems really bad if you have a late call to super that's working fine and then suddenly and very mysteriously stops working because a private attribute was added to the base class. The base class might even live in a third party library. It would be difficult to see why upgrading the library caused a breakage, and the library developer certainly wouldn't be able to prevent it. They almost certainly wouldn't predict the problem and note it in the changelog.

Anyway, I found an alternative fix, which is to wrap the whole of `init_private_attributes` with `if getattr(self, '__pydantic_private__', None) is None:` and leave the rest as it was before. This seems simpler to reason about, also passes the tests, and has the added performance benefit of only performing the inner logic once rather than redundantly looping and setting the defaults once per class in the hierarchy.
Just realised I forgot to ask - does that alternative fix sound good?
Hi @alexmojaki,

Thanks so much for your contribution. Your help with this is super appreciated 😀.

> It also seems really bad if you have a late call to super that's working fine and then suddenly and very mysteriously stops working because a private attribute was added to the base class. The base class might even live in a third party library. It would be difficult to see why upgrading the library caused a breakage, and the library developer certainly wouldn't be able to prevent it. They almost certainly wouldn't predict the problem and note it in the changelog.

I think you bring up a great point here in favor of why we should make this change. This has convinced me that the expectation that a user just orders the call to `super` and any custom logic doesn't serve as a feasible solution.

> Just realised I forgot to ask - does that alternative fix sound good?

That sounds good to me at a first glance - if you can push that change, I'll take a closer look at the tests 👍. I agree with @dmontagu that if we go with the first approach, we should revert the change associated with the `setdefault`.

Commit message: Alternative fix that only avoids setting private attribute defaults more than once

Patch to review:
diff --git a/pydantic/_internal/_model_construction.py b/pydantic/_internal/_model_construction.py
index 15dbb3ee1c1..20df15d64a3 100644
--- a/pydantic/_internal/_model_construction.py
+++ b/pydantic/_internal/_model_construction.py
@@ -259,6 +259,8 @@ def init_private_attributes(self: BaseModel, __context: Any) -> None:
         self: The BaseModel instance.
         __context: The context.
     """
+    if getattr(self, '__pydantic_private__', None) is not None:
+        return
     pydantic_private = {}
     for name, private_attr in self.__private_attributes__.items():
         default = private_attr.get_default()
diff --git a/tests/test_main.py b/tests/test_main.py
index 87324bad634..bcd00fc51bf 100644
--- a/tests/test_main.py
+++ b/tests/test_main.py
@@ -2021,6 +2021,38 @@ class C(B):
     assert calls == ['C.model_post_init']
 
 
+def test_model_post_init_subclass_setting_private_attrs():
+    """https://github.com/pydantic/pydantic/issues/7091"""
+
+    class Model(BaseModel):
+        _priv1: int = PrivateAttr(91)
+        _priv2: int = PrivateAttr(92)
+
+        def model_post_init(self, __context) -> None:
+            self._priv1 = 100
+
+    class SubModel(Model):
+        _priv3: int = PrivateAttr(93)
+        _priv4: int = PrivateAttr(94)
+        _priv5: int = PrivateAttr()
+        _priv6: int = PrivateAttr()
+
+        def model_post_init(self, __context) -> None:
+            self._priv3 = 200
+            self._priv5 = 300
+            super().model_post_init(__context)
+
+    m = SubModel()
+
+    assert m._priv1 == 100
+    assert m._priv2 == 92
+    assert m._priv3 == 200
+    assert m._priv4 == 94
+    assert m._priv5 == 300
+    with pytest.raises(AttributeError):
+        assert m._priv6 == 94
+
+
 def test_model_post_init_correct_mro():
     """https://github.com/pydantic/pydantic/issues/7293"""
     calls = []


Code review comments:
diff_hunk: @@ -259,6 +259,8 @@ def init_private_attributes(self: BaseModel, __context: Any) -> None:
         self: The BaseModel instance.
         __context: The context.
     """
+    if getattr(self, '__pydantic_private__', None) is not None:
+        return
     pydantic_private = {}
     for name, private_attr in self.__private_attributes__.items():
         default = private_attr.get_default()
path: pydantic/_internal/_model_construction.py
text: @user1:
For simplicity, can we change this to
```py
if getattr(self, '__pydantic_private__', None) is None:
   pydantic_private={}
   ...
```