-
Notifications
You must be signed in to change notification settings - Fork 10
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
Change to app factory pattern #224
Conversation
…icorn, gunicorn, and FastAPI. It allows parameterised creation of the App object among other benefits.
05a71fd
to
deb6a19
Compare
There should only be one and I don't think they should be as open as they are @lalewis1 could you please look into this next week - it's lower priority & I've added it here: #227
Nick added this one and I believe it was for OGC Records compliance, we'll need to revisit it. |
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.
Tests passing & have run locally without issue. Have not got my head around the pattern yet - will need to read up on it.
With a user being able to pass their own settings object, would this imply they're using Prez as a python import/library?
If my understanding is correct, in the current form Prez is more of an application template, where the intention is the user would clone or fork it, modify the source files to add their own routers, new APIs, set their own However I'm thinking of a situation where you could take take a built prez module (like in the docker container), import it into a main.py file, build your own I have a branch in my local codebase that does exactly that to build out the Prez-as-a-function implementation for use in the Azure Function Apps environment. |
Its not very different to the old pattern. Running the app from the commandline in uvicorn is basically the same as before except instead of giving uvicorn the module path of the app instance, you give it the module path of a function, (and add the In non-parameterised use cases, the result is exactly the same as before. But for someone importing Prez as a module, they can call the app factory with different parameters to get out different versions of the app, and modify the app themselves, before passing it to uvicorn. It means you can do things like this: # main.py
import uvicorn
from prez.app import assemble_app
from prez.config import Settings
my_settings = Settings()
prez_app = assemble_app(title="My Custom Prez", version="1.0", settings=my_settings)
# add app customisations
my_port = 8001
if __name__ == "__main__":
uvicorn.run(prez_app, port=my_port, reload=True) or # main.py
from fastapi import FastAPI
import uvicorn
from prez.app import assemble_app
from prez.config import Settings
def my_custom_factory() -> FastAPI:
my_settings = Settings()
my_app = assemble_app(title="My Custom Prez", version="1.1", settings=my_settings)
# customize my_app here
return my_app
my_port = 8001
if __name__ == "__main__":
uvicorn.run(my_custom_factory, factory=True, port=my_port, reload=True) or for Azure Functions: # function_app.py
import azure.functions as func
from prez.app import assemble_app
from prez.config import Settings
my_settings = Settings()
prez_app = assemble_app(title="Azure Prez function", version="1.0", settings=my_settings)
# add extra app routes required
app = func.AsgiFunctionApp(app=prez_app, http_auth_level=func.AuthLevel.FUNCTION) |
Thanks for the explanation Ashley - merging this in now |
See this thread for discussion of the benefits of the factory pattern in Uvicorn. Support for this feature was merged to Uvicorn in Oct 2020, and is commonly used. It is also supported in other ASGI runners.
This change set was motivated initially for my need to pass a base prefix (aka,
root_path
) to theFastAPI()
constructor inprez.app
(for deploying in a container on Azure). After modifying the app creation code to allow parameterising the construction, it was immediately obvious I could allow other variables to parameterize the creation of theFastAPI
instance. This led to even being able to optionally use a whole differentSettings
instance if the user wants to manually create their own and pass it in rather than using the one auto-generated in by Pydantic inprez.config
.We use this pattern in a couple of FastAPI applications we've written recently at CSIRO, and it seems robust.
I have two questions that I came across while editing the
app.py
file:starlette CORSMiddleware
and the localadd_cors_headers
middleware. Both do approximately the same thing. I've left both in use in this change, but I feel like only one is needed._get_sparql_service_description()
is inapp.py
, it looks important but it doesn't seem to be used anywhere. I've left it intact in this change, but perhaps it needs to be removed, or maybe it is supposed to be used somewhere else?