Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Comments

⚡ Updatecreate_cloned_field to use a global cache and improve startup performance#4645

Merged
tiangolo merged 13 commits intofastapi:masterfrom
zanieb:create-cloned-field
Jun 3, 2023
Merged

⚡ Updatecreate_cloned_field to use a global cache and improve startup performance#4645
tiangolo merged 13 commits intofastapi:masterfrom
zanieb:create-cloned-field

Conversation

@zanieb
Copy link
Contributor

@zaniebzanieb commentedMar 4, 2022•
edited
Loading

Resolves issue reported in#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 aWeakKeyDictionary 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 bycreate_cloned_field will be in scope for the scope of the FastAPI application anyway — users have noted this can significantly reduce memory consumption as it does not create as many model copies.

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. edit: this change was made to meet test coverage.

#4644 includes profiling indicating the performance increases here.

As mentioned in the issue,#894 (comment) 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.

See also discussion at#8609
Related to#4644

jlowin, metemaddar, greyfox-dev, emann, acookin, rob-ryszewski, DJRHails, ulan-yisaev, Lawouach, WP-LKL, and 2 more reacted with thumbs up emoji
@ddanier
Copy link

Does not close#4644 - see comment in the linked issue.

@orfisko
Copy link

Isn't this a similar solution as what has been proposed here?#4105

@zanieb
Copy link
ContributorAuthor

@orfisko Yeah it is! I missed that implementation since it wasn't linked to an issue. The choices here are a little different:

  • Thecloned_types parameter 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.
  • AWeakKeyDictionary is 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.
rob-ryszewski and followben reacted with thumbs up emoji

@zanieb
Copy link
ContributorAuthor

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 withForwardRef 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. Thisdoes fix the memory consumption and initialization times, see the update from the same user in#4644 (comment).

@github-actions
Copy link
Contributor

📝 Docs preview for commit4d7d18e at:https://636aa2502be5da007c6dc54e--fastapi.netlify.app

@github-actions
Copy link
Contributor

📝 Docs preview for commitd741490 at:https://636aa752e169f6006f8afab1--fastapi.netlify.app

@michaelgmiller1
Copy link

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!

@exherb
Copy link

when is this going to merge and release?

@zanieb
Copy link
ContributorAuthor

zanieb commentedNov 23, 2022•
edited
Loading

Note that coverage is failing because this utility is not explicitly tested and consequently there's not coverage for actually using a customcloned_types value.

@tiangolo The CI failures athttps://github.com/tiangolo/fastapi/actions/runs/3535147825/jobs/5932836062 appear to be related to an change in GitHub actions that breaks Python when usingactions/cache@v3. We encountered this at Prefect as well, resetting the cache resolved the issue. See#5680.

@github-actions
Copy link
Contributor

📝 Docs preview for commit6ab34f2 at:https://637e77005289633eb24dbfd5--fastapi.netlify.app

@github-actions
Copy link
Contributor

📝 Docs preview for commitb64052f at:https://638366a8319a3972152f2ec9--fastapi.netlify.app

@github-actions
Copy link
Contributor

📝 Docs preview for commitf378dfa at:https://63837048cb2392708ef8aca8--fastapi.netlify.app

@github-actions
Copy link
Contributor

📝 Docs preview for commit076f15a at:https://639ce7f5ecf1fd09839b403e--fastapi.netlify.app

@michaelgmiller1
Copy link

Is this ready to ship? Anything I can do to help push this across the finish line?

@zanieb
Copy link
ContributorAuthor

zanieb commentedJan 4, 2023•
edited
Loading

This is ready to shipalthough it does not currently meet coverage requirements for the project. The maintainer has marked this as an issue to investigate when they have time.

@michaelgmiller1
Copy link

@tiangolo any chance we could get this merged, if it's ready to go? this is ahuge win, making both app startup and test startup significantly faster.

greyfox-dev, varneyo, timabilov, spro, ddanier, emann, Lawouach, followben, lcwill, and rob-ryszewski reacted with thumbs up emoji

@github-actions
Copy link
Contributor

📝 Docs preview for commitbf5c338 at:https://63fcf8ba6775e90057a15a43--fastapi.netlify.app

@Lawouach
Copy link

Indeed@tiangolo. This would be a nice win all around :)

@github-actions
Copy link
Contributor

📝 Docs preview for commitd8bea1e at:https://643b6e9ed45d4c5bfe2bf01b--fastapi.netlify.app

@github-actions
Copy link
Contributor

📝 Docs preview for commitac82725 at:https://645507701034401069366570--fastapi.netlify.app

@acookin
Copy link

@tiangolo What's required to get this merged?

ulan-yisaev reacted with thumbs up emoji

@github-actions
Copy link
Contributor

📝 Docs preview for commit7184259 at:https://645ea712446d774207e234db--fastapi.netlify.app

@followben
Copy link

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?

@github-actions
Copy link
Contributor

📝 Docs preview for commit730717c at:https://646f7e9ccc99ff0466e10339--fastapi.netlify.app

@ulan-yisaev
Copy link

Hey team, looking forward for this merge :)

@tiangolotiangolo changed the titleUpdatecreate_cloned_field to use a global cache⚡ Updatecreate_cloned_field to use a global cache and improve startup performanceJun 3, 2023
@tiangolo
Copy link
Member

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 thisclone_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#4105 that was before this. I'm taking this one just for the simplification in the changes/diff, the weakref, and keeping the signature. 🤓

rob-ryszewski, anton-shauchenia, ddanier, heitorlessa, zanieb, huonw, Vegemash, Fogapod, michaelgmiller1, billpalombi, and 3 more reacted with hooray emoji

@tiangolotiangoloenabled auto-merge (squash)June 3, 2023 13:37
@tiangolotiangolo merged commit27618aa intofastapi:masterJun 3, 2023
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

1 more reviewer

@rob-ryszewskirob-ryszewskirob-ryszewski left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

11 participants

@zanieb@ddanier@orfisko@michaelgmiller1@exherb@Lawouach@acookin@followben@ulan-yisaev@tiangolo@rob-ryszewski
[8]ページ先頭
©2009-2026 Movatter.jp