Skip to content
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

Fix issues with local image as st.chat_message avatar #7130

Merged
merged 11 commits into from Aug 8, 2023

Conversation

LukasMasuch
Copy link
Collaborator

@LukasMasuch LukasMasuch commented Aug 7, 2023

Describe your changes

This PR fixes two issues with using local images as avatar for st.chat_message:

  1. Use the Streamlit endpoints implementation to construct an absolute URL for the image.
  2. Use the delta path string as media ID if the image is loaded into the media file manager (same as with st.image).

Testing Plan

Update the relevant tests. Added e2e test with local image and migrated test from cypres to playwright.


Contribution License Agreement

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

@LukasMasuch LukasMasuch changed the title Fix issues with local image as message avatar Fix issues with local image as st.chat_message avatar Aug 7, 2023
@LukasMasuch LukasMasuch marked this pull request as ready for review August 7, 2023 18:11
@@ -194,7 +197,9 @@ def chat_message(
):
# For selected labels, we are mapping the label to an avatar
avatar = name.lower()
avatar_type, converted_avatar = _process_avatar_input(avatar)
avatar_type, converted_avatar = _process_avatar_input(
avatar, self.dg._get_delta_path_str()
Copy link
Collaborator

Choose a reason for hiding this comment

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

@LukasMasuch one question I have, since I am not super familiar with the chat_message design.
Is self.dg._get_delta_path_str()return unique "coordinates" for different chat_messages?

I looked into the media_file_manager implementation and looks like this should not cause "multiple uploads of the same file" / "unwanted premature file removals for the file" issues.

Just to confirm that everything works correctly, could we add another playwright check where we check that the same image used multiple times as an avatar for different chat_message doesn't cause any errors?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

oki 👍 I added one more e2e check that reuses the same local image. Does that cover it?

Copy link
Collaborator

Choose a reason for hiding this comment

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

yes, that covers exactly that case :)

@LukasMasuch LukasMasuch merged commit caaa9d6 into develop Aug 8, 2023
49 checks passed
eric-skydio pushed a commit to eric-skydio/streamlit that referenced this pull request Dec 20, 2023
* Fix issues with local image as message avatar

* Add chat message e2e playwright test

* Fix unit test issue

* Update chat message snapshots

* Wait until images have loaded

* Add second message reusing the same image

* Increment counter

* Add additional snapshot

* Change timeout

* Change how we wait for image to load

* Add missing snapshot
zyxue pushed a commit to zyxue/streamlit that referenced this pull request Mar 22, 2024
* Fix issues with local image as message avatar

* Add chat message e2e playwright test

* Fix unit test issue

* Update chat message snapshots

* Wait until images have loaded

* Add second message reusing the same image

* Increment counter

* Add additional snapshot

* Change timeout

* Change how we wait for image to load

* Add missing snapshot
zyxue pushed a commit to zyxue/streamlit that referenced this pull request Apr 16, 2024
* Fix issues with local image as message avatar

* Add chat message e2e playwright test

* Fix unit test issue

* Update chat message snapshots

* Wait until images have loaded

* Add second message reusing the same image

* Increment counter

* Add additional snapshot

* Change timeout

* Change how we wait for image to load

* Add missing snapshot
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants