De-experimentalize st.dialog #9020

raethlein · 2024-07-03T08:35:27Z

Describe your changes

Merge after #9019

With #9019 landing, we are also de-experimentializing the @st.experimental_dialog decorator and introduce @st.dialog. As a consequence of the aforementioned PR,
dialogs can then

call other fragments or be opened from fragments, although dialogs are not allowed to open other dialogs
dialogs can be opened by callbacks (closes Getting "Could not find fragment with id 1c78f2182f4bcfbd2cda9b9971306dc7" error while calling a dialog (fragment) function from a callback. #8591)

Update all used instances of @st.experimental_dialog to @st.dialog.

GitHub Issue Link (if applicable)

Testing Plan

Unit Tests (JS and/or Python)
- Added a unit test to ensure that the experimental-decorator still continues to work

Closes #9063

Contribution License Agreement

By submitting this pull request you agree that all contributions to this project are made under the Apache 2.0 license.

lib/tests/streamlit/elements/layouts_test.py

lib/streamlit/elements/dialog_decorator.py

The reason we were seeing the bug in #8591 is due to the incorrect assumption that we can always totally clear out a session's fragment storage before a full script run as fragments will immediately be repopulated anyway. This isn't quite true as a fragment can be added by a callback, and in this case clearing out `FragmentStorage` before the full script run will lose it. To fix this, we keep track of the fragments added throughout a script run, and on a full script run we don't clear out fragments that were just added.

I realized while working on the `st.rerun(scope="fragment")` feature that `ctx. fragment_ids_this_run` should always be `rerun_data.fragment_id_queue` in set form. We still want to keep the distinction around since the frontend isn't aware of the order fragments will be run, but we should simplify things to not needlessly copy around the same information in a slightly different representation.

This PR adds the new `scope` kwarg to the `st.rerun()` function. With `scope="app"` (the default), `st.rerun()` work as before. If you set `scope="fragment"` within a fragment, however, it's now possible to specify that only the currently running fragment reruns.

…orage methods (#9011) A source of many (but potentially not all) of the reports in #8494 involve a silly mistake that I made where we're catching all `KeyError`s that may be thrown from a fragment and interpreting them as coming from when we attempted to fetch the fragment from our `FragmentStorage`. Of course, they could also be coming from the fragment function itself, and in these cases we're currently raising an error with a misleading / incorrect / opaque message. This PR fixes this issue by throwing and expecting a more specific subclass of `KeyError` in `FragmentStorage` operations

Fix two issues: - the `current_fragment_delta_path` could have been taken from the wrong snapshot - always add the `current_fragment_id` to the set of new fragment ids. This prevents an issue where a fragment rerun is triggered during full app execution. - Explanation of why no additional tests are needed - Unit Tests (JS and/or Python) - E2E Tests - Add e2e test to ensure that the `current_fragment_id` error does not happen - Any manual testing needed? --- **Contribution License Agreement** By submitting this pull request you agree that all contributions to this project are made under the Apache 2.0 license.

Right now, dialogs within an app always generate the same fragment id. This is problematic and can lead to mixing contents of different dialogs. Add a new parameter that is used by the dialog decorator to pass the actually wrapped function name to the fragment in order to generate the hash. This behavior started to surface in the `feature/st.fragment` branch; prior to that we overwrote fragments with the same id in the storage and since we only allow a single dialog at any given time, this issue stayed unrecognized. - Unit Tests (JS and/or Python) - Unit tests seem difficult to add for this because if we use the `DeltaGeneratorTestCase`, the dialogs have different delta paths when opened simultaneously versus the same delta path in a real app after a rerun happened. Hence, I figured the e2e test resembles the chain of sequence best. - E2E Tests - Add an e2e test that triggers the sequence of action that leads to the bug --- **Contribution License Agreement** By submitting this pull request you agree that all contributions to this project are made under the Apache 2.0 license.

In the current state, an exception in a fragment will cause queued fragments to stop from running. But instead we want queued fragments to still run even if a previous fragment raised an exception (even if the error happens in a full app run; the error will be displayed but the app will continue to run). To do so, we add except blocks into the loop and continue running the other fragments in the queue. Also, we want to stop app execution when a fragment raises an error during a full app run. This is why the `exec_func_with_error_handling` call is removed in `fragment.py`. - Explanation of why no additional tests are needed - Unit Tests (JS and/or Python) - E2E Tests - Any manual testing needed? --- **Contribution License Agreement** By submitting this pull request you agree that all contributions to this project are made under the Apache 2.0 license.

## Describe your changes Merge **after** streamlit#9019 With streamlit#9019 landing, we are also de-experimentializing the `@st.experimental_dialog` decorator and introduce `@st.dialog`. As a consequence of the aforementioned PR, dialogs can then - call other fragments or be opened from fragments, although dialogs are not allowed to open other dialogs - dialogs can be opened by callbacks (closes streamlit#8591) Update all used instances of `@st.experimental_dialog` to `@st.dialog`. ## GitHub Issue Link (if applicable) ## Testing Plan - Unit Tests (JS and/or Python) - Added a unit test to ensure that the experimental-decorator still continues to work Closes streamlit#9063 --- **Contribution License Agreement** By submitting this pull request you agree that all contributions to this project are made under the Apache 2.0 license. --------- Co-authored-by: Vincent Donato <vincent@streamlit.io>

raethlein requested a review from vdonato July 3, 2024 08:35

raethlein added do-not-merge security-assessment-completed change:feature impact:users labels Jul 3, 2024

raethlein force-pushed the feature/st.dialog branch from 388ad2f to 4a48804 Compare July 3, 2024 13:21

vdonato added the QA-Needed label Jul 3, 2024

vdonato approved these changes Jul 3, 2024

View reviewed changes

lib/tests/streamlit/elements/layouts_test.py Outdated Show resolved Hide resolved

lib/streamlit/elements/dialog_decorator.py Outdated Show resolved Hide resolved

raethlein force-pushed the feature/st.dialog branch from 4a48804 to b021346 Compare July 4, 2024 11:20

vdonato mentioned this pull request Jul 10, 2024

close a slow dialog may have chance to break the whole ui #9063

Closed

4 tasks

raethlein force-pushed the feature/st.dialog branch from b021346 to 32af0de Compare July 11, 2024 15:11

vdonato force-pushed the feature/st.dialog branch from 32af0de to 1b25f9d Compare July 12, 2024 21:32

vdonato added QA-Done and removed do-not-merge QA-Needed labels Jul 15, 2024

vdonato and others added 11 commits July 15, 2024 21:30

Remove mistakenly added whitespace

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.

GPG key ID: B5690EEEBB952194

Verified
Learn about vigilant mode

7dda7d8

De-experimentalize fragment

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.

GPG key ID: B5690EEEBB952194

Verified
Learn about vigilant mode

443f25d

Update e2e test

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.

GPG key ID: B5690EEEBB952194

Verified
Learn about vigilant mode

dff94e4

Address feedback

Loading
Loading status checks…

dbaeb8a

raethlein force-pushed the feature/st.dialog branch from 1b25f9d to dbaeb8a Compare July 15, 2024 19:48

raethlein merged commit 340f3f7 into develop Jul 15, 2024
33 checks passed

raethlein deleted the feature/st.dialog branch August 8, 2024 14:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

De-experimentalize st.dialog #9020

De-experimentalize st.dialog #9020

raethlein commented Jul 3, 2024 •

edited by vdonato

Loading

De-experimentalize st.dialog #9020

De-experimentalize st.dialog #9020

Conversation

raethlein commented Jul 3, 2024 • edited by vdonato Loading

Describe your changes

GitHub Issue Link (if applicable)

Testing Plan

raethlein commented Jul 3, 2024 •

edited by vdonato

Loading