Skip to content
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

fix: Fix broken parametrized bases with GenericModels #5052

Merged
merged 18 commits into from Feb 15, 2023

Conversation

MarkusSintonen
Copy link
Contributor

@MarkusSintonen MarkusSintonen commented Feb 14, 2023

This PR broke the usage of parametrized bases with GenericModels. Problem arises especially in applications having large number of generic data models (in our case a large FastAPI based application). The issue roots to the dictionary size limiting in pydantic/generics.py. When the limit is hit it starts to break usage of parametrized bases like here #4131 (comment) and here #4483

Original PR tried to fix a memory leak by limiting the size of the internal dictionaries. But it seems its not correct way to do it as it can break generics usage. So instead this PR relies on weak dictionaries to handle any leaks with dynamic model usage.

Fix #4131.

@MarkusSintonen MarkusSintonen marked this pull request as draft February 14, 2023 07:47
@MarkusSintonen MarkusSintonen marked this pull request as ready for review February 14, 2023 09:03
@MarkusSintonen
Copy link
Contributor Author

FYI with this change we are not observing memory leak as the original issue did. We are running a similar patch in production like the change here.

Copy link
Member

@samuelcolvin samuelcolvin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Humm, in principle this all looks good, we should definitely use this in V2.

I'm a bit of a wimp about making changes like this in V1.10 - I've seen too many have unexpected consequences.

@pydantic/pydantic-maintainers what do you think?

pydantic/generics.py Show resolved Hide resolved
@samuelcolvin
Copy link
Member

@crazyscientist, would be amazing if you could review this and make sure it's working with your use case?

@MarkusSintonen
Copy link
Contributor Author

@adriangb it seems pytest-memray is bugged with parametrized tests having long names 😞 It works when not using --memray-bin-path param. But then we dont get the full result artifacts.

Its using the test name as the file name here https://github.com/bloomberg/pytest-memray/blob/1.4.0/src/pytest_memray/plugin.py#L139 Which breaks with long generated test names. Maybe it would be worth reporting to them 🤔

@adriangb
Copy link
Member

Thanks for tracking that down! 100% worth reporting. For now we can probably just put that test in it's own file and only run it on its own. What do you think?

@adriangb
Copy link
Member

Oh wait you fixed the issue. Never mind, I think this should work!

@samuelcolvin
Copy link
Member

We should definitely report this to mem ray.

I'll review this later.

@crazyscientist
Copy link

@samuelcolvin thank you for the heads up. When I run our test suite with this fix, I unfortunately keep getting the KeyError raised as described in #4131.

@samuelcolvin samuelcolvin enabled auto-merge (squash) February 15, 2023 09:32
@samuelcolvin
Copy link
Member

This is great, thanks all for your input.

In particular @MarkusSintonen thanks for your perseverance on this issue, I suspect you have the dubious honour of making the most complex use of pydantic generics.

We need this to be copied to main, probably worth waiting for #4970 to be merged. @MarkusSintonen would you be willing to contribute this?

Also, I'm yet again impressed with memray, I guess we should use it more.

@samuelcolvin samuelcolvin merged commit aea3449 into pydantic:1.10.X-fixes Feb 15, 2023
@MarkusSintonen
Copy link
Contributor Author

This is great, thanks all for your input. In particular @MarkusSintonen thanks for your perseverance on this issue, I suspect you have the dubious honour of making the most complex use of pydantic generics.

Haha no problem! 😊

We need this to be copied to main, probably worth waiting for #4970 to be merged. @MarkusSintonen would you be willing to contribute this?

Yeah I can open similar PR against main.

@MarkusSintonen
Copy link
Contributor Author

@samuelcolvin I already opened the V2 PR so I wont forget it: #5056

@samuelcolvin
Copy link
Member

awesome, thank you.

renovate bot added a commit to allenporter/flux-local that referenced this pull request Feb 18, 2023
[![Mend
Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Change | Age | Adoption | Passing | Confidence |
|---|---|---|---|---|---|
| [pydantic](https://togithub.com/pydantic/pydantic) | `==1.10.4` ->
`==1.10.5` |
[![age](https://badges.renovateapi.com/packages/pypi/pydantic/1.10.5/age-slim)](https://docs.renovatebot.com/merge-confidence/)
|
[![adoption](https://badges.renovateapi.com/packages/pypi/pydantic/1.10.5/adoption-slim)](https://docs.renovatebot.com/merge-confidence/)
|
[![passing](https://badges.renovateapi.com/packages/pypi/pydantic/1.10.5/compatibility-slim/1.10.4)](https://docs.renovatebot.com/merge-confidence/)
|
[![confidence](https://badges.renovateapi.com/packages/pypi/pydantic/1.10.5/confidence-slim/1.10.4)](https://docs.renovatebot.com/merge-confidence/)
|

---

### Release Notes

<details>
<summary>pydantic/pydantic</summary>

###
[`v1.10.5`](https://togithub.com/pydantic/pydantic/releases/tag/v1.10.5):
(2023-02-15)

[Compare
Source](https://togithub.com/pydantic/pydantic/compare/v1.10.4...v1.10.5)

- Fix broken parametrized bases handling with `GenericModel`s with
complex sets of models,
[#&#8203;5052](https://togithub.com/pydantic/pydantic/issues/5052) by
[@&#8203;MarkusSintonen](https://togithub.com/MarkusSintonen)
- Invalidate mypy cache if plugin config changes,
[#&#8203;5007](https://togithub.com/pydantic/pydantic/issues/5007) by
[@&#8203;cdce8p](https://togithub.com/cdce8p)
- Fix `RecursionError` when deep-copying dataclass types wrapped by
pydantic,
[#&#8203;4949](https://togithub.com/pydantic/pydantic/issues/4949) by
[@&#8203;mbillingr](https://togithub.com/mbillingr)
- Fix `X | Y` union syntax breaking `GenericModel`,
[#&#8203;4146](https://togithub.com/pydantic/pydantic/issues/4146) by
[@&#8203;thenx](https://togithub.com/thenx)
- Switch coverage badge to show coverage for this branch/release,
[#&#8203;5060](https://togithub.com/pydantic/pydantic/issues/5060) by
[@&#8203;samuelcolvin](https://togithub.com/samuelcolvin)

#### New Contributors

- [@&#8203;thenx](https://togithub.com/thenx) made their first
contribution in
[pydantic/pydantic#4977
- [@&#8203;mbillingr](https://togithub.com/mbillingr) made their first
contribution in
[pydantic/pydantic#4963
- [@&#8203;MarkusSintonen](https://togithub.com/MarkusSintonen) made
their first contribution in
[pydantic/pydantic#5052

**Full Changelog**:
pydantic/pydantic@v1.10.4...v1.10.5

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined),
Automerge - At any time (no schedule defined).

🚦 **Automerge**: Enabled.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the
rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update
again.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

---

This PR has been generated by [Mend
Renovate](https://www.mend.io/free-developer-tools/renovate/). View
repository job log
[here](https://app.renovatebot.com/dashboard#github/allenporter/flux-local).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNC4xNDMuMSIsInVwZGF0ZWRJblZlciI6IjM0LjE0My4xIn0=-->

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
renovate bot added a commit to allenporter/pyrainbird that referenced this pull request Feb 18, 2023
[![Mend
Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Change | Age | Adoption | Passing | Confidence |
|---|---|---|---|---|---|
| [pydantic](https://togithub.com/pydantic/pydantic) | `==1.10.4` ->
`==1.10.5` |
[![age](https://badges.renovateapi.com/packages/pypi/pydantic/1.10.5/age-slim)](https://docs.renovatebot.com/merge-confidence/)
|
[![adoption](https://badges.renovateapi.com/packages/pypi/pydantic/1.10.5/adoption-slim)](https://docs.renovatebot.com/merge-confidence/)
|
[![passing](https://badges.renovateapi.com/packages/pypi/pydantic/1.10.5/compatibility-slim/1.10.4)](https://docs.renovatebot.com/merge-confidence/)
|
[![confidence](https://badges.renovateapi.com/packages/pypi/pydantic/1.10.5/confidence-slim/1.10.4)](https://docs.renovatebot.com/merge-confidence/)
|

---

### Release Notes

<details>
<summary>pydantic/pydantic</summary>

###
[`v1.10.5`](https://togithub.com/pydantic/pydantic/releases/tag/v1.10.5):
(2023-02-15)

[Compare
Source](https://togithub.com/pydantic/pydantic/compare/v1.10.4...v1.10.5)

- Fix broken parametrized bases handling with `GenericModel`s with
complex sets of models,
[#&#8203;5052](https://togithub.com/pydantic/pydantic/issues/5052) by
[@&#8203;MarkusSintonen](https://togithub.com/MarkusSintonen)
- Invalidate mypy cache if plugin config changes,
[#&#8203;5007](https://togithub.com/pydantic/pydantic/issues/5007) by
[@&#8203;cdce8p](https://togithub.com/cdce8p)
- Fix `RecursionError` when deep-copying dataclass types wrapped by
pydantic,
[#&#8203;4949](https://togithub.com/pydantic/pydantic/issues/4949) by
[@&#8203;mbillingr](https://togithub.com/mbillingr)
- Fix `X | Y` union syntax breaking `GenericModel`,
[#&#8203;4146](https://togithub.com/pydantic/pydantic/issues/4146) by
[@&#8203;thenx](https://togithub.com/thenx)
- Switch coverage badge to show coverage for this branch/release,
[#&#8203;5060](https://togithub.com/pydantic/pydantic/issues/5060) by
[@&#8203;samuelcolvin](https://togithub.com/samuelcolvin)

#### New Contributors

- [@&#8203;thenx](https://togithub.com/thenx) made their first
contribution in
[pydantic/pydantic#4977
- [@&#8203;mbillingr](https://togithub.com/mbillingr) made their first
contribution in
[pydantic/pydantic#4963
- [@&#8203;MarkusSintonen](https://togithub.com/MarkusSintonen) made
their first contribution in
[pydantic/pydantic#5052

**Full Changelog**:
pydantic/pydantic@v1.10.4...v1.10.5

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined),
Automerge - At any time (no schedule defined).

🚦 **Automerge**: Enabled.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the
rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update
again.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

---

This PR has been generated by [Mend
Renovate](https://www.mend.io/free-developer-tools/renovate/). View
repository job log
[here](https://app.renovatebot.com/dashboard#github/allenporter/pyrainbird).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNC4xNDMuMSIsInVwZGF0ZWRJblZlciI6IjM0LjE0My4xIn0=-->

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
@pablogsal
Copy link

pablogsal commented Feb 23, 2023

@adriangb it seems pytest-memray is bugged with parametrized tests having long names 😞 It works when not using --memray-bin-path param. But then we dont get the full result artifacts.

Its using the test name as the file name here https://github.com/bloomberg/pytest-memray/blob/1.4.0/src/pytest_memray/plugin.py#L139 Which breaks with long generated test names. Maybe it would be worth reporting to them 🤔

We are on it bloomberg/pytest-memray#68 . We are in the process of shipping some big stuff to memray core so this may take some more time than normal.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants