You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "ianbuss (via GitHub)" <gi...@apache.org> on 2024/11/08 11:22:28 UTC
[PR] Ensure lifespans of mounted FastAPI sub-apps are called [airflow]
ianbuss opened a new pull request, #43817:
URL: https://github.com/apache/airflow/pull/43817
FastAPI does not run Starlette lifespan events in mounted sub-applications by default (see [docs](https://fastapi.tiangolo.com/advanced/events/#sub-applications)), so we use the suggestion in [this issue](https://github.com/fastapi/fastapi/issues/811#issuecomment-1870030103) to run all mounted sub-application lifespans using an async context manager stack.
To ensure that lifespan events are run in the TestApp we use a context manager for the test_app as outlined [here](https://www.starlette.io/lifespan/#running-lifespan-in-tests). To allow assertions about the lifespans we add a simple boolean variable to application state to indicate the lifespan has been run.
<!-- Please keep an empty line above the dashes. -->
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst#pull-request-guidelines)** for more information.
In case of fundamental code changes, an Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvement+Proposals)) is needed.
In case of a new dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
In case of backwards incompatible changes please leave a note in a newsfragment file, named `{pr_number}.significant.rst` or `{issue_number}.significant.rst`, in [newsfragments](https://github.com/apache/airflow/tree/main/newsfragments).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Ensure lifespans of mounted FastAPI sub-apps are called [airflow]
Posted by "ashb (via GitHub)" <gi...@apache.org>.
ashb merged PR #43817:
URL: https://github.com/apache/airflow/pull/43817
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Ensure lifespans of mounted FastAPI sub-apps are called [airflow]
Posted by "ianbuss (via GitHub)" <gi...@apache.org>.
ianbuss commented on code in PR #43817:
URL: https://github.com/apache/airflow/pull/43817#discussion_r1834221476
##########
tests/api_fastapi/test_app.py:
##########
@@ -19,6 +19,15 @@
from unittest import mock
+def test_main_app_lifespan(client):
+ with client() as test_client:
+ test_app = test_client.app
+
+ # assert the app was created and lifespan was called
+ assert test_app
+ assert test_app.state.lifespan_called, "Lifespan not called on Execution API app."
Review Comment:
No indeed :) That's why the test is called `test_main_app_lifespan` and there's one for the execution app below. And yes, each app has its own starlette state object.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Ensure lifespans of mounted FastAPI sub-apps are called [airflow]
Posted by "ashb (via GitHub)" <gi...@apache.org>.
ashb commented on code in PR #43817:
URL: https://github.com/apache/airflow/pull/43817#discussion_r1834202618
##########
airflow/api_fastapi/app.py:
##########
@@ -28,6 +30,18 @@
app: FastAPI | None = None
+@asynccontextmanager
+async def lifespan(app: FastAPI):
+ async with AsyncExitStack() as stack:
+ for route in app.routes:
+ if isinstance(route, Mount) and isinstance(route.app, FastAPI):
+ await stack.enter_async_context(
+ route.app.router.lifespan_context(route.app),
Review Comment:
Core isn't a separate app I don't think -- it's some routers
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Ensure lifespans of mounted FastAPI sub-apps are called [airflow]
Posted by "ianbuss (via GitHub)" <gi...@apache.org>.
ianbuss commented on code in PR #43817:
URL: https://github.com/apache/airflow/pull/43817#discussion_r1834216466
##########
airflow/api_fastapi/app.py:
##########
@@ -28,6 +30,18 @@
app: FastAPI | None = None
+@asynccontextmanager
+async def lifespan(app: FastAPI):
+ async with AsyncExitStack() as stack:
+ for route in app.routes:
+ if isinstance(route, Mount) and isinstance(route.app, FastAPI):
+ await stack.enter_async_context(
+ route.app.router.lifespan_context(route.app),
Review Comment:
It will always have a lifespan https://github.com/encode/starlette/blob/c2e3a39b09a613553ee03586589ed9cd0fbf07f3/starlette/routing.py#L612 but I can add a check in if you want to ensure there's no regression.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Ensure lifespans of mounted FastAPI sub-apps are called [airflow]
Posted by "ashb (via GitHub)" <gi...@apache.org>.
ashb commented on code in PR #43817:
URL: https://github.com/apache/airflow/pull/43817#discussion_r1834179201
##########
airflow/api_fastapi/app.py:
##########
@@ -28,6 +30,18 @@
app: FastAPI | None = None
+@asynccontextmanager
+async def lifespan(app: FastAPI):
+ async with AsyncExitStack() as stack:
+ for route in app.routes:
+ if isinstance(route, Mount) and isinstance(route.app, FastAPI):
+ await stack.enter_async_context(
+ route.app.router.lifespan_context(route.app),
Review Comment:
Does this need an `if route.app.lifespan`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Ensure lifespans of mounted FastAPI sub-apps are called [airflow]
Posted by "ianbuss (via GitHub)" <gi...@apache.org>.
ianbuss commented on code in PR #43817:
URL: https://github.com/apache/airflow/pull/43817#discussion_r1834221476
##########
tests/api_fastapi/test_app.py:
##########
@@ -19,6 +19,15 @@
from unittest import mock
+def test_main_app_lifespan(client):
+ with client() as test_client:
+ test_app = test_client.app
+
+ # assert the app was created and lifespan was called
+ assert test_app
+ assert test_app.state.lifespan_called, "Lifespan not called on Execution API app."
Review Comment:
No indeed :) That's why the test is called `test_main_app_lifespan` and there's one for the execution app below.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Ensure lifespans of mounted FastAPI sub-apps are called [airflow]
Posted by "ashb (via GitHub)" <gi...@apache.org>.
ashb commented on code in PR #43817:
URL: https://github.com/apache/airflow/pull/43817#discussion_r1834213700
##########
tests/api_fastapi/test_app.py:
##########
@@ -19,6 +19,15 @@
from unittest import mock
+def test_main_app_lifespan(client):
+ with client() as test_client:
+ test_app = test_client.app
+
+ # assert the app was created and lifespan was called
+ assert test_app
+ assert test_app.state.lifespan_called, "Lifespan not called on Execution API app."
Review Comment:
Does each app have it's own state object? If so I don't think we are asserting the subapp's lifespan is called here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Ensure lifespans of mounted FastAPI sub-apps are called [airflow]
Posted by "ianbuss (via GitHub)" <gi...@apache.org>.
ianbuss commented on code in PR #43817:
URL: https://github.com/apache/airflow/pull/43817#discussion_r1834205757
##########
airflow/api_fastapi/app.py:
##########
@@ -28,6 +30,18 @@
app: FastAPI | None = None
+@asynccontextmanager
+async def lifespan(app: FastAPI):
+ async with AsyncExitStack() as stack:
+ for route in app.routes:
+ if isinstance(route, Mount) and isinstance(route.app, FastAPI):
+ await stack.enter_async_context(
+ route.app.router.lifespan_context(route.app),
Review Comment:
You're right. Doing a quick sanity check
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Ensure lifespans of mounted FastAPI sub-apps are called [airflow]
Posted by "ianbuss (via GitHub)" <gi...@apache.org>.
ianbuss commented on code in PR #43817:
URL: https://github.com/apache/airflow/pull/43817#discussion_r1834185890
##########
airflow/api_fastapi/app.py:
##########
@@ -28,6 +30,18 @@
app: FastAPI | None = None
+@asynccontextmanager
+async def lifespan(app: FastAPI):
+ async with AsyncExitStack() as stack:
+ for route in app.routes:
+ if isinstance(route, Mount) and isinstance(route.app, FastAPI):
+ await stack.enter_async_context(
+ route.app.router.lifespan_context(route.app),
Review Comment:
Possibly. I don't think so though as I didn't add one to core and the unit test didn't barf. But it might be a good addition anyway.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Ensure lifespans of mounted FastAPI sub-apps are called [airflow]
Posted by "ianbuss (via GitHub)" <gi...@apache.org>.
ianbuss commented on code in PR #43817:
URL: https://github.com/apache/airflow/pull/43817#discussion_r1834244827
##########
airflow/api_fastapi/app.py:
##########
@@ -28,6 +30,18 @@
app: FastAPI | None = None
+@asynccontextmanager
+async def lifespan(app: FastAPI):
+ async with AsyncExitStack() as stack:
+ for route in app.routes:
+ if isinstance(route, Mount) and isinstance(route.app, FastAPI):
+ await stack.enter_async_context(
+ route.app.router.lifespan_context(route.app),
Review Comment:
I think it should be safe to merge as is.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org