Session state yield points #7373
Conversation
Leaving in the tests temporarily, until I decide what to do with them
Hopefully this works, since I can't reliably run these locally
AnOctopus
changed the title
There was a problem hiding this comment.
So I'm understanding this correctly.
Currently, we are disconnecting the session state from the script runner when a stop request occurs. This is to avoid session state being populated with new information after the stop request comes in.
Instead, we are doing the following:
- Allow any changes to Session State to have the opportunity to consider changes in execution (stop or rerun). (I have a question on Read operations, but I imagine it's just to shutdown faster).
- There's a change on the script finish to not have session state handle a script finish event if the script finish had an intervention (error or Streamlit Stop/Rerun event). I'm having trouble understanding how this connects to the PR description. Can you elaborate more?
| ) -> None: | ||
| """Called when our script finishes executing, even if it finished | ||
| early with an exception. We perform post-run cleanup here. | ||
| """ | ||
| # Tell session_state to update itself in response | ||
| self._session_state.on_script_finished(ctx.widget_ids_this_run) | ||
| if not premature_stop: |
There was a problem hiding this comment.
If we are handling this for StopException/Rerun Exception to indicate the use of st.stop/rerun, don't we want to update the session state and handle it?
I may be perhaps misunderstanding the value of ignoring the change.
There was a problem hiding this comment.
Currently, if a stop/rerun comes in and fast reruns is enabled (which is ~always the case, very very few apps turn it off), on_script_finished will be a no-op for the disconnected session state. This prevents widgets that were going to be rendered, but didn't because it was interrupted, from losing their state. To retain this behavior, since it no longer happens via the disconnect mechanism, we have to conditionally avoid the session state cleanup step via the script runner instead.
There was a problem hiding this comment.
Let me walk through an example and make sure I understand.
Let's look at the following script:
import streamlit as st
st.button("misdirection")
if st.button("belly"):
st.write("Here's an issue")
st.stop()
st.write("This may not get written if the button is pressed")When we run it, no major issue. When we click the button, we do the following:
- The button's value should be set to true (trigger)
- The script executes. The button flow runs and
st.stop()is called. - We request to stop
- (This part may be a change from my original understanding) That eventually raises an Exception, specifically a StopException, right?
- The script finishes and calls on_script_finished.
- It should call session state on script finished because we want to reset the button trigger? Or to remove the stale widgets (the example doesn't provide this scenario)?
There was a problem hiding this comment.
1-4 is still correct.
5. For the code on develop, if fast reruns is enabled, on_script_finished runs but does nothing (immediate return if disconnected). For the PR code, on_script_finished is instead not called at all.
6. The button trigger gets reset at the beginning of the next script run, due to a change I made recently for a bug fix. Possibly we should remove the trigger reset call from on_script_finished, to simplify things.
There was a problem hiding this comment.
Okay. This makes more sense now. Would love if we can try to simplify the flow for understanding.
| del self._state[key] | ||
|
|
||
| def __contains__(self, key: str) -> bool: | ||
| self._yield_callback() |
There was a problem hiding this comment.
Do we really need it in the read operations? Or are we just using this as an opportunity to end faster?
There was a problem hiding this comment.
Just using it to end faster
| ) -> None: | ||
| """Called when our script finishes executing, even if it finished | ||
| early with an exception. We perform post-run cleanup here. | ||
| """ | ||
| # Tell session_state to update itself in response | ||
| self._session_state.on_script_finished(ctx.widget_ids_this_run) | ||
| if not premature_stop: |
There was a problem hiding this comment.
Okay. This makes more sense now. Would love if we can try to simplify the flow for understanding.
Replace SafeSessionState's disconnect mechanism with additional yield points, to potentially end script runs faster and avoid spurious errors in logs, and slightly simplify the implementation.
Replace SafeSessionState's disconnect mechanism with additional yield points, to potentially end script runs faster and avoid spurious errors in logs, and slightly simplify the implementation.
Replace SafeSessionState's disconnect mechanism with additional yield points, to potentially end script runs faster and avoid spurious errors in logs, and slightly simplify the implementation.
Describe your changes
Replace
SafeSessionStatedisconnect with script runner yield points, to achieve the same safety with slightly less complexity, shorter runtimes, and fewer spurious errors logged.Testing Plan
e2e test added, unit tests updated
Contribution License Agreement
By submitting this pull request you agree that all contributions to this project are made under the Apache 2.0 license.