-
Notifications
You must be signed in to change notification settings - Fork 4k
Fix stale element issue caused by spinners #6859
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
Conversation
tconkling
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.
Ship it!
| import streamlit as st | ||
| @st.experimental_memo(experimental_allow_widgets=True) | ||
| @st.cache_data(experimental_allow_widgets=True, show_spinner=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.
Why are these tests changed to use show_spinner=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.
The testing utilities are not able to handle containers, which seems to be a known problem:
| # TODO does not work for `st.container` which has no block proto |
That's why I had to deactivate the show_spinner in those examples, otherwise there would be a container in the element tree -> causing an exception with this testing utilities.
Describe your changes
This PR attempts to fix the behavior of
st.spinnerso that it does not cause stale elements. Instead of hiding the spinner in the frontend viadg.empty(), this PR usesdg.container(). An empty container gets ignored on the frontend; in comparison, andg.empty()placeholder is treated as an empty element in the front end, resulting in unwanted stale elements in some situations. The stale issue is caused by a scenario where in the first run, we are using a spinner, but in the second run, we are just showing the data, for example:The first run uses two elements, while the second one uses only one. This will cause a duplicated greyed-out version of the dataframe until the script has fully finished.
Alternatively, we could introduce some kind of
dg.clear()ordg.empty(clear=True)command that removes the current element from the frontend.Testing Plan
Updated unit tests related to
st.spinner. I also had to deactivate the spinners in some caching tests since the testing utility does not work yet withst.containeras mentioned here:streamlit/lib/streamlit/testing/element_tree.py
Line 991 in 3407ec7
Contribution License Agreement
By submitting this pull request you agree that all contributions to this project are made under the Apache 2.0 license.