-
-
Notifications
You must be signed in to change notification settings - Fork 2.3k
Building support for decoupling Dash from Flask and supporting Quart / FastAPI servers #3430
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
base: dev
Are you sure you want to change the base?
Conversation
work to modularize the dash eco-system and decouple from Flask
∙ - added types to BaseFactory to remove linting errors on create app in Flask and Quart Factory
… `request` context
…e flow. Made endpoints for downloading the reqs
* ∙ - remove contextvar from flask and quart only FastApi now relies on that ∙ - backend __init__ now holds the global request adapter and backend which get set on app initialisation ∙ request adapter and server can now be call from everywhere after the app initialised ∙ - added normal top level imports because the modules get matching loaded - but bad Import Error message when quart or equivilent are not installed ∙ - added _ as prefix to backends to avoid importing errors with their underlying ∙ - Can now move to remove unnecessary passing of the server object ∙ * Moved get_server_type to backends * ∙ moved async validation to validation ∙ replaced request.get_path with request.path ∙ * Moved custom backend check to _validation.py * Removed server injection of server methods - they use self.server now * removed use_async from dispatch server methods and use dash_app._use_async removed remaining set request process from flask * adding custom error handling per backend, tests and adjustments to the flow. Made endpoints for downloading the reqs * adjusments for formatting * adjustment to retest backend * Added Dash app as type to servers * adding missing reqs association * Addedd basic typing to servers * fixing minor linting issues * Fixed weird AI shit * Cleanup before heavy pull * Merged latest changes * f rebase * f rebase * Added Dash app as type to servers * Addedd basic typing to servers --------- Co-authored-by: Christian Giessel <cgiessel@tesla.com> Co-authored-by: BSd3v <82055130+BSd3v@users.noreply.github.com>
* ∙ Added has_request_context to base_server ∙ removed flask specific import in _validate and use backends.backend.has_request_context now * ∙ added context to request adapter ∙ callback context uses request adapter context now ∙ * added get_root_path to dash _utils removed flask.helpers.get_root_path usage * moved compress to server implementations flask fully decoupled from dash * fixed compress in quart and fastapi * Fixed server.config in fastapi to use config file * Update dash/backends/_fastapi.py Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * removed unused flask import in pages * Update dash/backends/_quart.py Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * removed flask specific return type to remove global dependency --------- Co-authored-by: Christian Giessel <cgiessel@tesla.com> Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
T4rk1n
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.
This is looking good, a few fixes are needed and I left a few suggestion.
- The serve_health route in dash.py still using flask.Response directly and need to be added in the backends.
- There is a few Flask references left in comments on the Dash class that need to be changed.
- The fastapi app is not running in debug mode if the app filename name is not "app.py".
| """ | ||
|
|
||
| def wrap(func: _t.Callable[[], _f.Response]): | ||
| def wrap(func: _t.Callable[[], _t.Any]): |
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.
This can be a future pain point to update existing hooks, maybe we can try adding a flask.Response converter in the adapter?
| _backend_imports = { | ||
| "flask": ("dash.backends._flask", "FlaskDashServer"), | ||
| "fastapi": ("dash.backends._fastapi", "FastAPIDashServer"), | ||
| "quart": ("dash.backends._quart", "QuartDashServer"), | ||
| } |
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.
I think we should have a way for this to be customizable, maybe add a hook for that. Then the backend library can have the hook inside it's init.
| <textarea readonly>{formatted_tb}</textarea> | ||
| </div> | ||
| <div class=\"explanation\"> | ||
| The debugger caught an exception in your ASGI application. You can now |
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.
| The debugger caught an exception in your ASGI application. You can now | |
| The debugger caught an exception in your Dash application. You can now |
| @property # pragma: no cover - interface | ||
| @abstractmethod | ||
| def cookies(self): | ||
| raise NotImplementedError() | ||
|
|
||
| @property # pragma: no cover - interface | ||
| @abstractmethod | ||
| def headers(self): | ||
| raise NotImplementedError() |
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.
Maybe we can type those as dict, then make sure the backends returns a dict copied from all the cookies/headers.
In the same way, maybe we can add a single get_cookie(...) and get_header(...) for single usage.
| @abstractmethod | ||
| def make_response( | ||
| self, data, mimetype=None, content_type=None | ||
| ) -> Any: # pragma: no cover - interface | ||
| pass |
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.
The usual usecase for the response in the dash context is to set cookies/headers, maybe we should a global set_cookie(...) and set_header akin to set_props that can be used across the different backends with ease.
| if kwargs.get("reload"): | ||
| # Dynamically determine the module name from the file path | ||
| file_path = frame.filename | ||
| spec = spec_from_file_location("app", file_path) |
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.
We need the real file here, when I try with an app not named app with debug=True I get:
INFO: Uvicorn running on http://127.0.0.1:8050 (Press CTRL+C to quit)
INFO: Started reloader process [80425] using StatReload
ERROR: Error loading ASGI app. Could not import module "app".
WARNING: StatReload detected changes in 'quart_app.py'. Reloading...
ERROR: Error loading ASGI app. Could not import module "app".
| include_in_schema=False, | ||
| ) | ||
|
|
||
| def dispatch(self, dash_app: Dash): |
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.
Might be a good time to rename this to a better name like serve_callback.
| if Response is None: | ||
| raise RuntimeError("Quart not installed; cannot generate Response") |
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.
This should not be happening, we should raise back the import error with a custom message to install dash[quart].
| return Response( | ||
| pkgutil.get_data("dash", "favicon.ico"), content_type="image/x-icon" | ||
| ) |
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.
This is the same for all the backends, maybe it be in the base and just call the proper make_response to handle that?
| def serve_component_suites( | ||
| self, dash_app: Dash, package_name: str, fingerprinted_path: str | ||
| ): # noqa: ARG002 unused req preserved for interface parity | ||
| path_in_pkg, has_fingerprint = check_fingerprint(fingerprinted_path) | ||
| _validate.validate_js_path(dash_app.registered_paths, package_name, path_in_pkg) | ||
| extension = "." + path_in_pkg.split(".")[-1] | ||
| mimetype = mimetypes.types_map.get(extension, "application/octet-stream") | ||
| package = sys.modules[package_name] | ||
| dash_app.logger.debug( | ||
| "serving -- package: %s[%s] resource: %s => location: %s", | ||
| package_name, | ||
| getattr(package, "__version__", "unknown"), | ||
| path_in_pkg, | ||
| package.__path__, | ||
| ) | ||
| data = pkgutil.get_data(package_name, path_in_pkg) | ||
| headers = {} | ||
| if has_fingerprint: | ||
| headers["Cache-Control"] = "public, max-age=31536000" | ||
|
|
||
| if Response is None: | ||
| raise RuntimeError("Quart not installed; cannot generate Response") | ||
| return Response(data, content_type=mimetype, headers=headers) |
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.
This function will mostly be the same across backends with a difference in the Response type, in this case we are missing the etag stuff that might need to be adapted.
I think maybe we could have a Response type that is dash specific and then we have a ResponseAdapter like we do for the RequestAdapter.
This is an open PR draft, to contribute please target my forked branch.
The goal of this PR is to modularize the Dash setup to be independent of Flask (will fallback to Flask) and allow devs to configure their own backend.
fixes #1571