Skip to content

Fix backend detection in testing runners - #3944

Open
cyphercodes wants to merge 3 commits into
plotly:devfrom
cyphercodes:fix-3942-runner-backend-detection
Open

Fix backend detection in testing runners#3944
cyphercodes wants to merge 3 commits into
plotly:devfrom
cyphercodes:fix-3942-runner-backend-detection

Conversation

@cyphercodes

Copy link
Copy Markdown

Fixes #3942.

ThreadedRunner and MultiProcessRunner determined whether to pass Flask's threaded=True option by inspecting the concrete server class module. That misclassified wrapped or subclassed FastAPI/Quart servers whose classes are defined elsewhere.

Use Dash's already-resolved app.backend.server_type instead, while preserving the Flask fallback. The regression coverage includes a wrapped FastAPI-shaped server and the FastAPI, Quart, and Flask option paths.

Contributor Checklist

  • I have broken down my PR scope into the following TODO tasks
    • Detect runner behavior from the resolved Dash backend
    • Add regression coverage for wrapped ASGI servers and Flask fallback
  • I have run the tests locally and they passed. (refer to testing section in contributing)
  • I have added tests, or extended existing tests, to cover any new features or bugs fixed in this PR

optionals

  • I have added entry in the CHANGELOG.md
  • If this PR needs a follow-up in dash docs, community thread, I have mentioned the relevant URLS as follows
    • this GitHub #PR number updates the dash docs
    • here is the show and tell thread in Plotly Dash community

Local verification

  • python -m pytest tests/unit/test_app_runners.py -k 'wrapped_fastapi or uses_backend_type' -q — 4 passed
  • python -m pytest tests/unit/test_testing.py -q — 1 passed
  • Black, Flake8, Pylint, Pyright (production file), and git diff --check

@T4rk1n T4rk1n left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good, just some minor comments and need to add a changelog entry.

Comment thread dash/testing/application_runners.py Outdated
Comment on lines +32 to +36
server_type = getattr(getattr(app, "backend", None), "server_type", "flask")
if server_type in ("fastapi", "quart"):
app.run(**options)
else:
app.run(threaded=True, **options)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We lost the comments indicating that we need the threaded=True for flask testing.

Comment thread dash/testing/application_runners.py Outdated


def _run_app(app, options):
server_type = getattr(getattr(app, "backend", None), "server_type", "flask")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is duplicated code at line 221-222, could potentially extract that into a _server_type function to prevent drift.

@cyphercodes

Copy link
Copy Markdown
Author

Addressed the review feedback:

Verification:

  • python -m pytest tests/unit/test_app_runners.py -q -k 'wrapped_fastapi or run_app_uses_backend_type'
  • git diff --check

Note: the full tests/unit/test_app_runners.py file still has pre-existing local environment/setup failures in this fresh venv (dash.dcc._js_dist missing for smoke setup and process server timeout), so I used the focused backend detection tests above.

@sonarqubecloud

Copy link
Copy Markdown

@Mukller Mukller left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Reviewed the refactor — this is the right direction. Module-path sniffing (__class__.__module__.startswith("fastapi")) breaks for any wrapped/instrumented server (APM agents, test wrappers), while reading app.backend.server_type is semantic and survives decoration. The deduplication into _run_app/_server_type also removes the two copies of the same ladder that could drift apart. Unit test with the fake instrumentation.wrapper module covers exactly the regression scenario. 👍

Two observations, neither blocking:

  1. Unregistered ASGI apps are still misdetected — a raw FastAPI/Quart app without Dash's .backend attribute falls through to the Flask branch and gets threaded=True, which uvicorn.run(**options) will choke on (TypeError: unexpected keyword). Out of scope for this PR since Dash-managed backends always set the attribute, but worth either a docs note in dash.testing or a friendlier error when threaded reaches an unknown app.run.

  2. Follow-up sweep: are there other startswith("fastapi") / startswith("quart") ladders left in the codebase (e.g., in dash/testing/plugin.py or composite runners)? If yes, a follow-up PR reusing _server_type there would finish the job.

Changelog entry format matches the neighboring entries. Nice, tight PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

3 participants