8000 ⚡️ Parallelize dependency resolution by archfz · Pull Request #13756 · fastapi/fastapi · GitHub
[go: up one dir, main page]

Skip to content

⚡️ Parallelize dependency resolution #13756

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

archfz
Copy link
@archfz archfz commented Jun 2, 2025

This resolves a serious performance issue in fastapi: async dependencies resolving sequentially. #639

I have attempted to pick up this previous work #3902 but I thought it was too complicated. So I am giving it a more simpler localized approach. Two things to mention:

  1. Since this approach breaks when contextvars are used with async generators, subtrees of the dependency graph are excluded from parallelization. This would be a limitation with this solution.
  2. For cached dependencies Future is used so await can be called multiple times on it.

Testing wise I'd need suggestions. Not sure how to test this and I also expect that many tests already cover the changes. Asserting that things are done in parallel would be beneficial but I'd need help with that.

EDIT:

Added in the meantime an option, just like cache, on the dependency definition called parallelizable. This is by default opt-out style, meaning it is True. Maybe to prevent breaking changes we can ship this with opt-in style. Let community experiment with it and in time if no issues are creeping up it can be considered to switch to opt-out style with a major release. For Security however I am thinking that maybe we should have it always False.

@@ -29,7 +29,7 @@ async def get_sub_counter(

@app.get("/sub-counter-no-cache/")
async def get_sub_counter_no_cache(
subcount: int = Depends(super_dep),
subcount: int = Depends(dep_counter),
Copy link
Author

Choose a reason for hiding this comment

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

Here I had to change this to the topmost dep, because one level lower already resolves just a tiny bit later and in that case this test is not valid anymore.

@archfz archfz force-pushed the master branch 2 times, most recently from 3add1cd to 21be878 Compare June 2, 2025 13:08
@archfz

This comment was marked as outdated.

@archfz archfz force-pushed the master branch 7 times, most recently from cd23ae6 to 4400b4d Compare June 3, 2025 13:58
@archfz
Copy link
Author
archfz commented Jun 3, 2025

Tested this in a medium side production backend with fastapi setup. Found several issues, more notably around caching and exception handling. Fixed these, however these warrant for additional test cases.

ensure_cache(None)
return sub_dependant.name, None, exc.errors, None
except BaseException as resolution_exc:
ensure_cache(resolution_exc)
Copy link
Author

Choose a reason for hiding this comment

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

Tests case needed.

for name, result, sub_errors, sub_exception in results:
# Ensures order of exception based on dependency order.
if sub_exception:
raise sub_exception
Copy link
Author

Choose a reason for hiding this comment

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

Tests case needed.

@YuriiMotov YuriiMotov changed the title Parallelize dependency resolution. (#639) ⚡️ Parallelize dependency resolution Jun 4, 2025
@archfz archfz force-pushed the master branch 5 times, most recently from de0a117 to c39fd8d Compare June 30, 2025 08:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants
0