From 6fdc45686a065ba3f580356b9f7c9d9d4d37c590 Mon Sep 17 00:00:00 2001 From: Carson Date: Tue, 30 Jul 2024 10:35:22 -0500 Subject: [PATCH 1/9] Close #1592: delay sending of chat UI messages until reactive graph is flushed --- shiny/ui/_chat.py | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/shiny/ui/_chat.py b/shiny/ui/_chat.py index bf0293b2f..560140dd8 100644 --- a/shiny/ui/_chat.py +++ b/shiny/ui/_chat.py @@ -973,14 +973,18 @@ async def _remove_loading_message(self): await self._send_custom_message("shiny-chat-remove-loading-message", None) async def _send_custom_message(self, handler: str, obj: ClientMessage | None): - await self._session.send_custom_message( - "shinyChatMessage", - { - "id": self.id, - "handler": handler, - "obj": obj, - }, - ) + + async def _do_send(): + await self._session.send_custom_message( + "shinyChatMessage", + { + "id": self.id, + "handler": handler, + "obj": obj, + }, + ) + + self._session.on_flushed(_do_send, once=True) @add_example(ex_dir="../api-examples/chat") From 60b2667f554559a117dcfcc1ffef7a5eb174d06a Mon Sep 17 00:00:00 2001 From: Carson Date: Tue, 30 Jul 2024 10:37:34 -0500 Subject: [PATCH 2/9] Update changelog --- CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 18b6137f2..b3e5ad57d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,6 +17,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 * `ui.Chat()` now works as expected inside Shiny modules. (#1582) +* `ui.Chat()` now works as expected when the UI is dynamically rendered and starting `messages` are supplied. (#1593) + * Fixed bug where calling `.update_filter(None)` on a data frame renderer did not visually reset non-numeric column filters. (It did reset the column's filtering, just not the label). Now it resets filter's label. (#1557) * Require shinyswatch >= 0.7.0 and updated examples accordingly. (#1558) From 939e108dd0f0d4474072a5a07f345898a3065700 Mon Sep 17 00:00:00 2001 From: Carson Date: Tue, 30 Jul 2024 10:45:24 -0500 Subject: [PATCH 3/9] Add playwright test --- .../shiny/components/chat/dynamic_ui/app.py | 8 ++++++++ .../chat/dynamic_ui/test_chat_dynamic_ui.py | 15 +++++++++++++++ 2 files changed, 23 insertions(+) create mode 100644 tests/playwright/shiny/components/chat/dynamic_ui/app.py create mode 100644 tests/playwright/shiny/components/chat/dynamic_ui/test_chat_dynamic_ui.py diff --git a/tests/playwright/shiny/components/chat/dynamic_ui/app.py b/tests/playwright/shiny/components/chat/dynamic_ui/app.py new file mode 100644 index 000000000..18090c11c --- /dev/null +++ b/tests/playwright/shiny/components/chat/dynamic_ui/app.py @@ -0,0 +1,8 @@ +from shiny.express import render, ui + +chat = ui.Chat(id="chat", messages=["A starting message"]) + + +@render.ui +def chat_output(): + return chat.ui() diff --git a/tests/playwright/shiny/components/chat/dynamic_ui/test_chat_dynamic_ui.py b/tests/playwright/shiny/components/chat/dynamic_ui/test_chat_dynamic_ui.py new file mode 100644 index 000000000..a761b9777 --- /dev/null +++ b/tests/playwright/shiny/components/chat/dynamic_ui/test_chat_dynamic_ui.py @@ -0,0 +1,15 @@ +from playwright.sync_api import Page, expect +from utils.deploy_utils import skip_on_webkit + +from shiny.playwright import controller +from shiny.run import ShinyAppProc + + +@skip_on_webkit +def test_validate_chat_basic(page: Page, local_app: ShinyAppProc) -> None: + page.goto(local_app.url) + + chat = controller.Chat(page, "chat") + + expect(chat.loc).to_be_visible(timeout=30 * 1000) + chat.expect_latest_message("A starting message", timeout=30 * 1000) From d6c0c9406c2898cd5350d34da29daf0cc7da17b4 Mon Sep 17 00:00:00 2001 From: Carson Date: Wed, 31 Jul 2024 12:22:35 -0500 Subject: [PATCH 4/9] Send stream chunks immediately (after waiting for flush to start the stream) --- shiny/ui/_chat.py | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/shiny/ui/_chat.py b/shiny/ui/_chat.py index 560140dd8..347ea1c44 100644 --- a/shiny/ui/_chat.py +++ b/shiny/ui/_chat.py @@ -563,7 +563,7 @@ async def append_message_stream(self, message: Iterable[Any] | AsyncIterable[Any async def _stream_task(): await self._append_message_stream(message) - _stream_task() + self._session.on_flushed(_stream_task, once=True) # Since the task runs in the background (outside/beyond the current context, # if any), we need to manually raise any exceptions that occur @@ -641,7 +641,9 @@ async def _send_append_message( # print(msg) - await self._send_custom_message(msg_type, msg) + # When streaming (i.e., chunk is thruthy), we can send messages immediately + # since we already waited for the flush in order to start the stream + await self._send_custom_message(msg_type, msg, on_flushed=chunk is False) # TODO: Joe said it's a good idea to yield here, but I'm not sure why? # await asyncio.sleep(0) @@ -972,8 +974,9 @@ def destroy(self): async def _remove_loading_message(self): await self._send_custom_message("shiny-chat-remove-loading-message", None) - async def _send_custom_message(self, handler: str, obj: ClientMessage | None): - + async def _send_custom_message( + self, handler: str, obj: ClientMessage | None, on_flushed: bool = True + ): async def _do_send(): await self._session.send_custom_message( "shinyChatMessage", @@ -984,7 +987,10 @@ async def _do_send(): }, ) - self._session.on_flushed(_do_send, once=True) + if on_flushed: + self._session.on_flushed(_do_send, once=True) + else: + await _do_send() @add_example(ex_dir="../api-examples/chat") From ecca2e341d69659d1792359a4da71ac04754c0ba Mon Sep 17 00:00:00 2001 From: Carson Sievert Date: Wed, 28 Aug 2024 15:23:12 -0500 Subject: [PATCH 5/9] Update shiny/ui/_chat.py --- shiny/ui/_chat.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/shiny/ui/_chat.py b/shiny/ui/_chat.py index 17d3f784c..a2b003a97 100644 --- a/shiny/ui/_chat.py +++ b/shiny/ui/_chat.py @@ -642,7 +642,7 @@ async def _send_append_message( # print(msg) - # When streaming (i.e., chunk is thruthy), we can send messages immediately + # When streaming (i.e., chunk is truthy), we can send messages immediately # since we already waited for the flush in order to start the stream await self._send_custom_message(msg_type, msg, on_flushed=chunk is False) # TODO: Joe said it's a good idea to yield here, but I'm not sure why? From a05fc2cec3f528acf0389abcf32e47b0bf5282d6 Mon Sep 17 00:00:00 2001 From: Carson Date: Wed, 28 Aug 2024 15:57:47 -0500 Subject: [PATCH 6/9] Invoke task right away, but wait until flush to actually start the stream --- shiny/ui/_chat.py | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/shiny/ui/_chat.py b/shiny/ui/_chat.py index a2b003a97..d343fd2b3 100644 --- a/shiny/ui/_chat.py +++ b/shiny/ui/_chat.py @@ -562,9 +562,12 @@ async def append_message_stream(self, message: Iterable[Any] | AsyncIterable[Any # Run the stream in the background to get non-blocking behavior @reactive.extended_task async def _stream_task(): - await self._append_message_stream(message) + async def _do_stream(): + await self._append_message_stream(message) - self._session.on_flushed(_stream_task, once=True) + self._session.on_flushed(_do_stream, once=True) + + _stream_task() # Since the task runs in the background (outside/beyond the current context, # if any), we need to manually raise any exceptions that occur From baab1fb51f312dc5d6beba10c94f0ac93c1c89ea Mon Sep 17 00:00:00 2001 From: Carson Date: Wed, 28 Aug 2024 16:26:14 -0500 Subject: [PATCH 7/9] Revert "Invoke task right away, but wait until flush to actually start the stream" This reverts commit a05fc2cec3f528acf0389abcf32e47b0bf5282d6. --- shiny/ui/_chat.py | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/shiny/ui/_chat.py b/shiny/ui/_chat.py index d343fd2b3..a2b003a97 100644 --- a/shiny/ui/_chat.py +++ b/shiny/ui/_chat.py @@ -562,12 +562,9 @@ async def append_message_stream(self, message: Iterable[Any] | AsyncIterable[Any # Run the stream in the background to get non-blocking behavior @reactive.extended_task async def _stream_task(): - async def _do_stream(): - await self._append_message_stream(message) + await self._append_message_stream(message) - self._session.on_flushed(_do_stream, once=True) - - _stream_task() + self._session.on_flushed(_stream_task, once=True) # Since the task runs in the background (outside/beyond the current context, # if any), we need to manually raise any exceptions that occur From 1f71ac7bd79372d8b216c24ae906b6b1621cf168 Mon Sep 17 00:00:00 2001 From: Carson Date: Wed, 28 Aug 2024 16:27:58 -0500 Subject: [PATCH 8/9] Update test expectations to reflect concurrent append_message()/append_message_stream() ordering --- .../shiny/components/chat/stream/test_chat_stream.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/playwright/shiny/components/chat/stream/test_chat_stream.py b/tests/playwright/shiny/components/chat/stream/test_chat_stream.py index 4c69e3e4d..1fa09a0c2 100644 --- a/tests/playwright/shiny/components/chat/stream/test_chat_stream.py +++ b/tests/playwright/shiny/components/chat/stream/test_chat_stream.py @@ -21,10 +21,10 @@ def test_validate_chat(page: Page, local_app: ShinyAppProc) -> None: expect(chat.loc_input_button).to_be_disabled() messages = [ - "FIRST FIRST FIRST", "SECOND SECOND SECOND", - "THIRD THIRD THIRD", "FOURTH FOURTH FOURTH", + "FIRST FIRST FIRST", + "THIRD THIRD THIRD", "FIFTH FIFTH FIFTH", ] # Allow for any whitespace between messages From a249fa35cbd285ef1c546a94f2f0749175a4168a Mon Sep 17 00:00:00 2001 From: Carson Date: Wed, 28 Aug 2024 16:34:43 -0500 Subject: [PATCH 9/9] Update changelog --- CHANGELOG.md | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a819d0d61..52b0ed73b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -22,12 +22,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 * A handful of fixes for `ui.Chat()`, including: * A fix for use inside Shiny modules. (#1582) * `.messages(format="google")` now returns the correct role. (#1622) + * `ui.Chat(messages)` are no longer dropped when dynamically rendered. (#1593) * `transform_assistant_response` can now return `None` and correctly handles change of content on the last chunk. (#1641) * An empty `ui.input_date()` value no longer crashes Shiny. (#1528) -* `ui.Chat()` now works as expected when the UI is dynamically rendered and starting `messages` are supplied. (#1593) - * Fixed bug where calling `.update_filter(None)` on a data frame renderer did not visually reset non-numeric column filters. (It did reset the column's filtering, just not the label). Now it resets filter's label. (#1557) * Require shinyswatch >= 0.7.0 and updated examples accordingly. (#1558)