fastapi
Update `create_cloned_field` to use a global cache
Closes https://github.com/tiangolo/fastapi/issues/4644 by instantiating a global dictionary to store cloned fields. This prevents repeated copying of models which can have significant performance affects. Please see the issue for more details.
This uses a WeakKeyDictionary so we avoid holding a reference to the model. I doubt this will have much practical affect since response model types are often globally defined, but it seems like good practice. This should not increase memory overhead since all the models generated by create_cloned_field will be in scope for the scope of the FastAPI application anyway.
It assigns the dictionary as a default value instead of moving the parameter to an explicitly global value. This allows a new cache to be passed if desired. However, mutable default values can be confusing and it would be fair to move it out of the function signature.
#4644 includes profiling indicating the performance increases here.
As mentioned in the issue, https://github.com/tiangolo/fastapi/issues/894#issuecomment-576484427 refers to an alternative approach that would require work in Pydantic that would make this function irrelevant. That seems better in the long, but this is an easier patch for now.
@orfisko Yeah it is! I missed that implementation since it wasn't linked to an issue. The choices here are a little different:
- The
cloned_typesparameter is retained. This prevents a breaking change to the function signature and allows a caller to opt-out of the cache or provide a different one. - A
WeakKeyDictionaryis used. This is mostly a matter of best-practice preventing a cache from creating strong references to types. This could prevent increased memory usage if types are defined dynamically. I doubt it'll have much practical effect though.
Does not close #4644 - see comment in the linked issue.
For others who read this, this comment refers specifically to an issue with using dynamically generated models with ForwardRef fields with this patch. Even then, you can work around the problem by updating forward refs. If someone provides an MRE, I can look into resolving the issue. This does fix the memory consumption and initialization times, see the update from the same user in https://github.com/tiangolo/fastapi/issues/4644#issuecomment-1091825267.
📝 Docs preview for commit 4d7d18e90d4d79913b79c35053eb0479e4a7b42d at: https://636aa2502be5da007c6dc54e--fastapi.netlify.app
![github-actions[bot] avatar](https://avatars.githubusercontent.com/in/15368?v=4 "Published by a pub.dev verified publisher"){kind=small}
📝 Docs preview for commit d7414905dd42f80c3d676deafbc04522537015cd at: https://636aa752e169f6006f8afab1--fastapi.netlify.app
This is a great addition and sorely needed for the application I'm working on. Please let me know if there's anything I can do to help push this over the finish line!
Note that coverage is failing because this utility is not explicitly tested and consequently there's not coverage for actually using a custom cloned_types value.
@tiangolo The CI failures at https://github.com/tiangolo/fastapi/actions/runs/3535147825/jobs/5932836062 appear to be related to an change in GitHub actions that breaks Python when using actions/cache@v3. We encountered this at Prefect as well, resetting the cache resolved the issue. See https://github.com/tiangolo/fastapi/pull/5680.
📝 Docs preview for commit 6ab34f200e915d73dbfea3f428c347e79b268332 at: https://637e77005289633eb24dbfd5--fastapi.netlify.app
📝 Docs preview for commit b64052ffb20b0b1c1eeac704517b5c92faa76627 at: https://638366a8319a3972152f2ec9--fastapi.netlify.app
📝 Docs preview for commit f378dfadfef09114bc11c3a56651c8f440927fa2 at: https://63837048cb2392708ef8aca8--fastapi.netlify.app
📝 Docs preview for commit 076f15a0a4e49edbb86d614206cc2e26e4e63a58 at: https://639ce7f5ecf1fd09839b403e--fastapi.netlify.app
Is this ready to ship? Anything I can do to help push this across the finish line?
This is ready to ship ~although it does not currently meet coverage requirements for the project~. The maintainer has marked this as an issue to investigate when they have time.
@tiangolo any chance we could get this merged, if it's ready to go? this is a huge win, making both app startup and test startup significantly faster.
📝 Docs preview for commit bf5c3385f236b346c1347bfd675f00f86c199697 at: https://63fcf8ba6775e90057a15a43--fastapi.netlify.app
📝 Docs preview for commit d8bea1e45546aae6ddd8ca7e7514a87e6e75433a at: https://643b6e9ed45d4c5bfe2bf01b--fastapi.netlify.app
📝 Docs preview for commit ac827259919b06c6239c72b939a8c3a89c8c1f3d at: https://645507701034401069366570--fastapi.netlify.app
📝 Docs preview for commit 718425927facec27993d8b3800ef527ec80baa1e at: https://645ea712446d774207e234db--fastapi.netlify.app
Thanks for keeping this up to date @madkinsz. Personally I think this is an improvement over the proposal in #4105 and would be my preference. @tiangolo, sorry to bug but can you tell shed light on what's holding this up?
📝 Docs preview for commit 730717c915089adad667814d66b4a37c621801e5 at: https://646f7e9ccc99ff0466e10339--fastapi.netlify.app
Awesome, thanks a lot @madkinsz! 🙇
Thanks everyone for the comments, sorry for the long delay. 😅 🙈
I've been working on the migration to Pydantic v2, which will make this clone_field logic unnecessary altogether, as everything will be done on the Pydantic-core (Rust) side. Nevertheless, I'll first make a release with this improvement.
This will be available in FastAPI 0.96.0, released in the next hours. 🚀
@huonw I'm making you an honorary co-author of this PR for your work on https://github.com/tiangolo/fastapi/pull/4105 that was before this. I'm taking this one just for the simplification in the changes/diff, the weakref, and keeping the signature. 🤓
