Skip to content

Commit

Permalink
Fix trigger value regression with st.rerun (#7643)
Browse files Browse the repository at this point in the history
  • Loading branch information
AnOctopus authored and vdonato committed Nov 2, 2023
1 parent 57c522b commit 833ef6a
Show file tree
Hide file tree
Showing 5 changed files with 33 additions and 11 deletions.
6 changes: 5 additions & 1 deletion lib/streamlit/runtime/scriptrunner/script_runner.py
Original file line number Diff line number Diff line change
Expand Up @@ -536,7 +536,11 @@ def _run_script(self, rerun_data: RerunData) -> None:
self._session_state[SCRIPT_RUN_WITHOUT_ERRORS_KEY] = True
except RerunException as e:
rerun_exception_data = e.rerun_data
premature_stop = True
# Interruption due to a rerun is usually from `st.rerun()`, which
# we want to count as a script completion so triggers reset.
# It is also possible for this to happen if fast reruns is off,
# but this is very rare.
premature_stop = False

except StopException:
# This is thrown when the script executes `st.stop()`.
Expand Down
1 change: 1 addition & 0 deletions lib/streamlit/runtime/state/session_state.py
Original file line number Diff line number Diff line change
Expand Up @@ -534,6 +534,7 @@ def on_script_finished(self, widget_ids_this_run: set[str]) -> None:
run. Any widget state whose ID does *not* appear in this set
is considered "stale" and will be removed.
"""
self._reset_triggers()
self._remove_stale_widgets(widget_ids_this_run)

def _reset_triggers(self) -> None:
Expand Down
14 changes: 6 additions & 8 deletions lib/streamlit/testing/v1/app_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,6 @@
WidgetList,
)
from streamlit.testing.v1.local_script_runner import LocalScriptRunner
from streamlit.testing.v1.util import patch_config_options
from streamlit.util import HASHLIB_KWARGS
from streamlit.web.bootstrap import _fix_matplotlib_crash

Expand Down Expand Up @@ -302,13 +301,12 @@ def _run(
new_secrets._secrets = self.secrets
st.secrets = new_secrets

with patch_config_options({"runner.postScriptGC": False}):
script_runner = LocalScriptRunner(self._script_path, self.session_state)
self._tree = script_runner.run(widget_state, self.query_params, timeout)
self._tree._runner = self
# Last event is SHUTDOWN, so the corresponding data includes query string
query_string = script_runner.event_data[-1]["client_state"].query_string
self.query_params = parse.parse_qs(query_string)
script_runner = LocalScriptRunner(self._script_path, self.session_state)
self._tree = script_runner.run(widget_state, self.query_params, timeout)
self._tree._runner = self
# Last event is SHUTDOWN, so the corresponding data includes query string
query_string = script_runner.event_data[-1]["client_state"].query_string
self.query_params = parse.parse_qs(query_string)

# teardown
with source_util._pages_cache_lock:
Expand Down
18 changes: 18 additions & 0 deletions lib/streamlit/testing/v1/local_script_runner.py
Original file line number Diff line number Diff line change
Expand Up @@ -18,12 +18,14 @@
from typing import Any
from urllib import parse

from streamlit import runtime
from streamlit.proto.ForwardMsg_pb2 import ForwardMsg
from streamlit.proto.WidgetStates_pb2 import WidgetStates
from streamlit.runtime.forward_msg_queue import ForwardMsgQueue
from streamlit.runtime.memory_uploaded_file_manager import MemoryUploadedFileManager
from streamlit.runtime.scriptrunner import RerunData, ScriptRunner, ScriptRunnerEvent
from streamlit.runtime.scriptrunner.script_cache import ScriptCache
from streamlit.runtime.scriptrunner.script_run_context import ScriptRunContext
from streamlit.runtime.state.safe_session_state import SafeSessionState
from streamlit.testing.v1.element_tree import ElementTree, parse_tree_from_messages

Expand Down Expand Up @@ -121,6 +123,22 @@ def script_stopped(self) -> bool:
return True
return False

def _on_script_finished(
self, ctx: ScriptRunContext, event: ScriptRunnerEvent, premature_stop: bool
) -> None:
# Only call `_remove_stale_widgets`, so that the state of triggers is still
# visible in the element tree.
if not premature_stop:
self._session_state._state._remove_stale_widgets(ctx.widget_ids_this_run)

# Signal that the script has finished. (We use SCRIPT_STOPPED_WITH_SUCCESS
# even if we were stopped with an exception.)
self.on_event.send(self, event=event)

# Remove orphaned files now that the script has run and files in use
# are marked as active.
runtime.get_instance().media_file_mgr.remove_orphaned_files()


def require_widgets_deltas(runner: LocalScriptRunner, timeout: float = 3) -> None:
"""Wait for the given ScriptRunner to emit a completion event. If the timeout
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -588,13 +588,14 @@ def test_widgets(self):
scriptrunner, ["True", "matey!", "2", "True", "loop_forever"]
)

# Rerun with previous values. Everything should be the same.
# Rerun with previous values. The button should be reset;
# everything else should be the same.
scriptrunner.clear_forward_msgs()
scriptrunner.request_rerun(RerunData())

require_widgets_deltas([scriptrunner])
self._assert_text_deltas(
scriptrunner, ["True", "matey!", "2", "True", "loop_forever"]
scriptrunner, ["True", "matey!", "2", "False", "loop_forever"]
)

finally:
Expand Down

1 comment on commit 833ef6a

@amazingjoe
Copy link

Choose a reason for hiding this comment

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

Thank you for patching this. I had to downgrade back to 1.27.x from 1.28.0 and this sounds like its fixing the issue I was running into. Looking forward to updating back to the latest version.

Please sign in to comment.