-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
adding support for async callbacks and page layouts #3089
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
…ayouts reference this in order to determine whether or not they should load as async functions.
…ing at the function to determine
…r testing and not relying on a specific string to be present as a comment
…e as `async`. Added more defined error messages when libraries are missing or async was used without async turned on.
…ing the clearing input values to make sure that we were only triggering callbacks when we desired.
…eck for async and wrapping a view.
…being cleared allows the failed integrations to pass
…t name to note if the image is `async` vs default
# Conflicts: # .circleci/config.yml # dash/_callback.py # dash/background_callback/managers/celery_manager.py # dash/dash.py
…for missing `callback_ctx`
@BSd3v is there a feature ticket for this change anywhere? |
Not that I can find. I've seen it come up a couple times on the forums. |
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.
Looks good, there is quite a bit of code duplication but I guess there is not really a choice with this implementation, most of the code of the has been extracted well into functions 👍
Just need to merge with dev and resolve the conflict and should be good.
test-312-async: &test | ||
working_directory: ~/dash | ||
docker: | ||
- image: cimg/python:3.12.1-browsers | ||
auth: | ||
username: dashautomation | ||
password: $DASH_PAT_DOCKERHUB | ||
environment: | ||
PERCY_ENABLE: 1 | ||
PERCY_PARALLEL_TOTAL: -1 | ||
PUPPETEER_SKIP_CHROMIUM_DOWNLOAD: True | ||
PYVERSION: python312 | ||
REDIS_URL: redis://localhost:6379 | ||
- image: cimg/redis:6.2.6 | ||
auth: | ||
username: dashautomation | ||
password: $DASH_PAT_DOCKERHUB | ||
parallelism: 3 | ||
steps: | ||
- checkout: | ||
path: ~/dash | ||
- run: | ||
name: Add chrome keys & update. | ||
command: | | ||
wget -q -O - https://dl.google.com/linux/linux_signing_key.pub | sudo apt-key add - | ||
sudo apt-get update | ||
- run: echo $PYVERSION > ver.txt | ||
- run: cat requirements/*.txt > requirements-all.txt | ||
- restore_cache: | ||
key: dep-{{ checksum ".circleci/config.yml" }}-{{ checksum "ver.txt" }}-{{ checksum "requirements-all.txt" }} | ||
- browser-tools/install-browser-tools: | ||
chrome-version: 120.0.6099.71 | ||
install-firefox: false | ||
install-geckodriver: false | ||
- attach_workspace: | ||
at: ~/dash | ||
- run: | ||
name: ️️🏗️ Install package | ||
command: | | ||
. venv/bin/activate | ||
npm ci | ||
pip install dash-package/dash-package.tar.gz[async,ci,dev,testing,celery,diskcache] --progress-bar off | ||
pip list | grep dash | ||
- run: | ||
name: 🧪 Run Integration Tests | ||
command: | | ||
. venv/bin/activate | ||
npm run citest.integration | ||
- store_artifacts: | ||
path: test-reports | ||
- store_test_results: | ||
path: test-reports | ||
- store_artifacts: | ||
path: /tmp/dash_artifacts | ||
|
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.
Can we move the new tests to a github action, maybe bundled with the background tests?
if asyncio.iscoroutinefunction(fn): | ||
func = partial(ctx.run, async_run) | ||
asyncio.run(func()) | ||
else: | ||
ctx.run(run) |
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.
Are we guaranteed to be out of the asyncio loop? In which case, the run might fail. Otherwise it might be better to only have the async_run definition so there is less code repetition.
if asyncio.iscoroutinefunction(fn): | ||
func = partial(ctx.run, async_run) | ||
asyncio.run(func()) | ||
else: | ||
ctx.run(run) |
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.
Same comment as per celery_manager, but for this one I am pretty sure there is no loop running.
try: | ||
import asgiref # pylint: disable=unused-import, import-outside-toplevel # noqa: F401, C0415 | ||
|
||
name += "_async" | ||
except ImportError: | ||
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.
We don't really need the percy integration if we run the tests separately in a new github action.=
@@ -31,7 +31,7 @@ | |||
"private::test.unit-dash": "pytest tests/unit", | |||
"private::test.unit-renderer": "cd dash/dash-renderer && npm run test", | |||
"private::test.unit-generation": "cd @plotly/dash-generator-test-component-typescript && npm ci && npm test", | |||
"private::test.integration-dash": "TESTFILES=$(circleci tests glob \"tests/integration/**/test_*.py\" | circleci tests split --split-by=timings) && pytest --headless --nopercyfinalize --junitxml=test-reports/junit_intg.xml ${TESTFILES}", | |||
"private::test.integration-dash": "TESTFILES=$(circleci tests glob \"tests/integration/**/test_*.py\" | circleci tests split --split-by=timings) && pytest --headless --nopercyfinalize --junitxml=test-reports/junit_intg.xml ${TESTFILES} && python rerun_failed_tests.py", |
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 have a package pytest-rerunfailures
, we used to use when the tests were very flaky. We could re-enable it for the time being.
func_kwargs, | ||
app, | ||
original_packages, | ||
False, |
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.
If this false
is hardcoded it should be removed from where it is used.
I feel like there is a bit too much in this tuple, it may be worth it to define a NamedTuple and add -> DispatchContext
to the function signature.
) | ||
|
||
|
||
def _get_callback_manager(kwargs, background): |
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 can add the typing here:
def _get_callback_manager(kwargs, background): | |
def _get_callback_manager(kwargs: dict, background: dict) -> Union[BaseBackgroundCallbackManager, None]: |
output_value = [] | ||
flat_output_values = [] | ||
_prepare_response( | ||
output_value, | ||
output_spec, | ||
multi, | ||
response, | ||
callback_ctx, | ||
app, | ||
original_packages, | ||
background, | ||
has_update, | ||
has_output, | ||
output, | ||
callback_id, | ||
allow_dynamic_callbacks, |
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.
Too much arguments, maybe with the NamedTuple we can pass these arguments as one?
This PR is attempting to allow for layouts and callbacks to be performed in a way that will allow for async functions natively.