-
Notifications
You must be signed in to change notification settings - Fork 15.9k
Replace DagBag from global app state to with FastAPI dependency #50372
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
Conversation
bugraoz93
left a comment
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.
Love it. Handling this with annotated depends is a great idea and follows the best practices. Not loading the request each time we need DagBag is already great for security measures and prevents leakage, as mentioned. Only CI and tests need some care
This change replaces the usage of `app.state.dag_bag` with a proper FastAPI dependency for better dependency injection and testability. The DagBag is now provided through a dependency that can be easily mocked in tests and follows FastAPI best practices as mentioned [here](https://fastapi.tiangolo.com/reference/fastapi/#fastapi.FastAPI.state:~:text=You%20normally%20wouldn%27t%20use%20this%20in%20FastAPI%2C%20for%20most%20of%20the%20cases%20you%20would%20instead%20use%20FastAPI%20dependencies.). - Explicit Dependencies: Using `DagBagDep` makes it clear in the function signature what each endpoint needs, rather than hiding the dependency in app.state. - Better Testing: Dependencies can be easily mocked (via FastAPI's `dependency_overrides`) in tests without modifying app state, making unit testing simpler and more reliable. - Request Isolation: Each request gets its own `DagBag` instance through the dependency, preventing any potential state leakage between requests. - Type Safety: The dependency system provides better type checking and IDE support compared to accessing `app.state`.
919c857 to
1a33d1e
Compare
CI is green now 🎉 |
jason810496
left a comment
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.
Thanks! Having DagBag provided via FastAPI Depends is indeed much easier to trace than using the global app state.
|
Follow up: For the Auth Manager, should we also consider replacing the use of the global app state with FastAPI |
amoghrajesh
left a comment
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.
Much cleaner, thank you!
Yeah, go for it |
ashb
left a comment
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.
cc @kaxil
Previously, DagBag was injected via onlyFastAPI dependency as part of the refactor in apache#50372, but a new instance was created for each request. This change updates the dependency to resolve DagBag from `app.state`, ensuring it is only instantiated once at startup while keeping benefits of FastAPI dependency.
Previously, DagBag was injected via onlyFastAPI dependency as part of the refactor in apache#50372, but a new instance was created for each request. This change updates the dependency to resolve DagBag from `app.state`, ensuring it is only instantiated once at startup while keeping benefits of FastAPI dependency.
Previously, DagBag was injected via onlyFastAPI dependency as part of the refactor in apache#50372, but a new instance was created for each request. This change updates the dependency to resolve DagBag from `app.state`, ensuring it is only instantiated once at startup while keeping benefits of FastAPI dependency.
Previously, DagBag was injected via onlyFastAPI dependency as part of the refactor in apache#50372 , but a new instance was created for each request. This change updates the dependency to resolve DagBag from `app.state`, ensuring it is only instantiated once at startup while keeping benefits of FastAPI dependency.
Previously, DagBag was injected via onlyFastAPI dependency as part of the refactor in apache#50372 , but a new instance was created for each request. This change updates the dependency to resolve DagBag from `app.state`, ensuring it is only instantiated once at startup while keeping benefits of FastAPI dependency.
Previously, DagBag was injected via onlyFastAPI dependency as part of the refactor in apache#50372 , but a new instance was created for each request. This change updates the dependency to resolve DagBag from `app.state`, ensuring it is only instantiated once at startup while keeping benefits of FastAPI dependency.
Previously, DagBag was injected via onlyFastAPI dependency as part of the refactor in #50372 , but a new instance was created for each request. This change updates the dependency to resolve DagBag from `app.state`, ensuring it is only instantiated once at startup while keeping benefits of FastAPI dependency.
(cherry picked from commit 3275cb2)
Previously, DagBag was injected via onlyFastAPI dependency as part of the refactor in #50372 , but a new instance was created for each request. This change updates the dependency to resolve DagBag from `app.state`, ensuring it is only instantiated once at startup while keeping benefits of FastAPI dependency. (cherry picked from commit 3b49f9d)
(cherry picked from commit 3275cb2)
Previously, DagBag was injected via onlyFastAPI dependency as part of the refactor in #50372 , but a new instance was created for each request. This change updates the dependency to resolve DagBag from `app.state`, ensuring it is only instantiated once at startup while keeping benefits of FastAPI dependency. (cherry picked from commit 3b49f9d)
Previously, DagBag was injected via onlyFastAPI dependency as part of the refactor in apache#50372 , but a new instance was created for each request. This change updates the dependency to resolve DagBag from `app.state`, ensuring it is only instantiated once at startup while keeping benefits of FastAPI dependency.
Previously, DagBag was injected via onlyFastAPI dependency as part of the refactor in apache/airflow#50372 , but a new instance was created for each request. This change updates the dependency to resolve DagBag from `app.state`, ensuring it is only instantiated once at startup while keeping benefits of FastAPI dependency. (cherry picked from commit 3b49f9d99e8967cf3f21f6ad58789380beaac83e) GitOrigin-RevId: e9fb9ad2362f6c5300f2b1451615be875323a8b8
Previously, DagBag was injected via onlyFastAPI dependency as part of the refactor in apache/airflow#50372 , but a new instance was created for each request. This change updates the dependency to resolve DagBag from `app.state`, ensuring it is only instantiated once at startup while keeping benefits of FastAPI dependency. GitOrigin-RevId: 3b49f9d99e8967cf3f21f6ad58789380beaac83e
This change replaces the usage of
app.state.dag_bagwith a proper FastAPI dependency for better dependency injection and testability. The DagBag is now provided through a dependency that can be easily mocked in tests and follows FastAPI best practices as mentioned here.DagBagDepmakes it clear in the function signature what each endpoint needs, rather than hiding the dependency in app.state.dependency_overrides) in tests without modifying app state, making unit testing simpler and more reliable.DagBaginstance through the dependency, preventing any potential state leakage between requests.app.state.This will help me have better dependency injection in #50300
^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named
{pr_number}.significant.rstor{issue_number}.significant.rst, in airflow-core/newsfragments.