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
Conversation
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. |
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.
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?
@crazyscientist, would be amazing if you could review this and make sure it's working with your use case? |
@adriangb it seems 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 🤔 |
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? |
Oh wait you fixed the issue. Never mind, I think this should work! |
We should definitely report this to mem ray. I'll review this later. |
@samuelcolvin thank you for the heads up. When I run our test suite with this fix, I unfortunately keep getting the |
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 Also, I'm yet again impressed with memray, I guess we should use it more. |
Haha no problem! 😊
Yeah I can open similar PR against |
@samuelcolvin I already opened the V2 PR so I wont forget it: #5056 |
awesome, thank you. |
[![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, [#​5052](https://togithub.com/pydantic/pydantic/issues/5052) by [@​MarkusSintonen](https://togithub.com/MarkusSintonen) - Invalidate mypy cache if plugin config changes, [#​5007](https://togithub.com/pydantic/pydantic/issues/5007) by [@​cdce8p](https://togithub.com/cdce8p) - Fix `RecursionError` when deep-copying dataclass types wrapped by pydantic, [#​4949](https://togithub.com/pydantic/pydantic/issues/4949) by [@​mbillingr](https://togithub.com/mbillingr) - Fix `X | Y` union syntax breaking `GenericModel`, [#​4146](https://togithub.com/pydantic/pydantic/issues/4146) by [@​thenx](https://togithub.com/thenx) - Switch coverage badge to show coverage for this branch/release, [#​5060](https://togithub.com/pydantic/pydantic/issues/5060) by [@​samuelcolvin](https://togithub.com/samuelcolvin) #### New Contributors - [@​thenx](https://togithub.com/thenx) made their first contribution in [pydantic/pydantic#4977 - [@​mbillingr](https://togithub.com/mbillingr) made their first contribution in [pydantic/pydantic#4963 - [@​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>
[![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, [#​5052](https://togithub.com/pydantic/pydantic/issues/5052) by [@​MarkusSintonen](https://togithub.com/MarkusSintonen) - Invalidate mypy cache if plugin config changes, [#​5007](https://togithub.com/pydantic/pydantic/issues/5007) by [@​cdce8p](https://togithub.com/cdce8p) - Fix `RecursionError` when deep-copying dataclass types wrapped by pydantic, [#​4949](https://togithub.com/pydantic/pydantic/issues/4949) by [@​mbillingr](https://togithub.com/mbillingr) - Fix `X | Y` union syntax breaking `GenericModel`, [#​4146](https://togithub.com/pydantic/pydantic/issues/4146) by [@​thenx](https://togithub.com/thenx) - Switch coverage badge to show coverage for this branch/release, [#​5060](https://togithub.com/pydantic/pydantic/issues/5060) by [@​samuelcolvin](https://togithub.com/samuelcolvin) #### New Contributors - [@​thenx](https://togithub.com/thenx) made their first contribution in [pydantic/pydantic#4977 - [@​mbillingr](https://togithub.com/mbillingr) made their first contribution in [pydantic/pydantic#4963 - [@​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>
We are on it bloomberg/pytest-memray#68 . We are in the process of shipping some big stuff to |
This PR broke the usage of parametrized bases with
GenericModel
s. 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 inpydantic/generics.py
. When the limit is hit it starts to break usage of parametrized bases like here #4131 (comment) and here #4483Original 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.