diff --git a/.changeset/great-pears-share.md b/.changeset/great-pears-share.md new file mode 100644 index 0000000000..f5d8432983 --- /dev/null +++ b/.changeset/great-pears-share.md @@ -0,0 +1,5 @@ +--- +"gradio": patch +--- + +fix:Fix two chat-history bugs in `gr.ChatInterface` and `gr.load_chat` diff --git a/gradio/chat_interface.py b/gradio/chat_interface.py index 33c91f2857..5f598721e1 100644 --- a/gradio/chat_interface.py +++ b/gradio/chat_interface.py @@ -925,7 +925,10 @@ async def _submit_fn( history: list[MessageDict], *args, ) -> tuple: - inputs = [message, history] + list(args) + # `list(history)` so that appending to it inside the chat function does not + # change the conversation. Shallow, matching `_append_message_to_history`, so + # editing one of the messages in place still shows through. See #10823. + inputs = [message, list(history)] + list(args) if self.is_async: response = await self.fn(*inputs) else: @@ -949,7 +952,9 @@ async def _stream_fn( tuple, None, ]: - inputs = [message, history] + list(args) + # `list(history)` for the same reason as in `_submit_fn`: appending to it in the + # generator's body must not be able to change the conversation. See #10823. + inputs = [message, list(history)] + list(args) if self.is_async: generator = self.fn(*inputs) else: diff --git a/gradio/external.py b/gradio/external.py index cfdf237ed4..221326921d 100644 --- a/gradio/external.py +++ b/gradio/external.py @@ -748,6 +748,25 @@ def fn(*data): IMAGE_FILE_EXTENSIONS = (".png", ".jpg", ".jpeg", ".gif", ".webp") +_TEXT_FILE_EXTENSIONS_LOWERCASE = tuple(ext.lower() for ext in TEXT_FILE_EXTENSIONS) + + +def _is_text_encoded_file(path: str) -> bool: + # Matched case-insensitively on both sides, the way `MultimodalTextbox` matches + # `file_types`, so that the `.R` and `.Rmd` entries above cover `plot.r` too. + return path.lower().endswith(_TEXT_FILE_EXTENSIONS_LOWERCASE) + + +def _text_encoded_file_as_prompt(path: str) -> str: + if is_http_url_like(path): + response = httpx.get(path) + response.raise_for_status() + name, contents = Path(httpx.URL(path).path).name, response.text + else: + name, contents = Path(path).name, Path(path).read_text() + return f"\n## {name}\n{contents}" + + def format_conversation( history: list[NormalizedMessageDict], new_message: str | MultimodalValue ) -> list[dict]: @@ -760,18 +779,28 @@ def format_conversation( f"Invalid message format: {message['content']}. Each element must have a type key." ) elif content["type"] == "file": - new_content.append( - { - "type": "image_url", - "image_url": { - "url": encode_url_or_file_to_base64(content["file"]["path"]) # type: ignore - }, - } - ) + path = content["file"]["path"] # type: ignore + if _is_text_encoded_file(path): + # Appended to the prompt as text, the same as when the message was + # first sent. Base64-encoding a text file as `image_url` (which is + # what used to happen once the turn was in the history) makes a + # non-multimodal model reject every subsequent request. See #11331. + new_content.append( + {"type": "text", "text": _text_encoded_file_as_prompt(path)} + ) + else: + new_content.append( + { + "type": "image_url", + "image_url": {"url": encode_url_or_file_to_base64(path)}, + } + ) else: new_content.append(content) - message["content"] = new_content - conversation.append(message) + # A new dict rather than `message["content"] = new_content`: these are the dicts + # in `chatbot_state`, and rewriting them there replaces the attachment shown in + # the transcript (and saved by `save_history`) with what was sent to the model. + conversation.append({**message, "content": new_content}) if isinstance(new_message, str): text = new_message files = [] @@ -780,7 +809,7 @@ def format_conversation( files = new_message.get("files", []) image_files, text_encoded = [], [] for file in files: - if file.lower().endswith(TEXT_FILE_EXTENSIONS): + if _is_text_encoded_file(file): text_encoded.append(file) else: image_files.append(file) @@ -799,12 +828,7 @@ def format_conversation( ) if text or text_encoded: text = text or "" - text += "\n".join( - [ - f"\n## {Path(file).name}\n{Path(file).read_text()}" - for file in text_encoded - ] - ) + text += "\n".join([_text_encoded_file_as_prompt(file) for file in text_encoded]) conversation.append( {"role": "user", "content": [{"type": "text", "text": text}]} ) diff --git a/test/test_chat_interface.py b/test/test_chat_interface.py index 6ad26f049f..9fa701e1f8 100644 --- a/test/test_chat_interface.py +++ b/test/test_chat_interface.py @@ -278,6 +278,37 @@ def test_example_caching_with_additional_inputs_already_rendered( Message(role="assistant", content=[TextMessage(text="ro")]), ] + @pytest.mark.asyncio + async def test_history_mutation_in_fn_is_ignored_consistently(self): + # Mutating `history` inside the chat function used to change the conversation + # for a non-streaming function but not for a streaming one, because the two + # paths copied the history at different points relative to running the + # function. See https://github.com/gradio-app/gradio/issues/10823. + def mutating(message, history): + history.append({"role": "assistant", "content": "INJECTED BY FN"}) + return "reply" + + def mutating_stream(message, history): + history.append({"role": "assistant", "content": "INJECTED BY FN"}) + yield "reply" + + expected = [ + {"role": "user", "content": "hi"}, + {"role": "assistant", "content": "reply"}, + ] + + original_history: list[Any] = [] + _, non_streaming_history = await gr.ChatInterface(mutating)._submit_fn( + "hi", original_history + ) + assert non_streaming_history == expected + assert original_history == [] + + stream = gr.ChatInterface(mutating_stream)._stream_fn("hi", original_history) + streamed = [chunk async for chunk in stream] + assert streamed[-1][1] == expected + assert original_history == [] + def test_custom_chatbot_with_events(self): with gr.Blocks() as demo: chatbot = gr.Chatbot() diff --git a/test/test_external.py b/test/test_external.py index 946e437dfb..4c18198cae 100644 --- a/test/test_external.py +++ b/test/test_external.py @@ -1,3 +1,4 @@ +import copy import os import tempfile import textwrap @@ -5,6 +6,7 @@ from typing import cast from unittest.mock import MagicMock, patch +import httpx import huggingface_hub import pytest @@ -413,6 +415,98 @@ def test_load_chat_with_streaming(mock_openai): assert responses == ["Hello", "Hello World", "Hello World!"] +def test_format_conversation_replays_text_files_as_text(tmp_path): + # A pasted long prompt arrives as a text file, which is inlined into the prompt on + # the turn it is sent. Once that turn was in the history it used to be re-sent as + # `image_url`, so every later message made a non-multimodal model reject the + # request. See https://github.com/gradio-app/gradio/issues/11331. + from gradio.external import format_conversation + + text_file = tmp_path / "pasted_text.txt" + text_file.write_text("a very long pasted prompt") + image_file = tmp_path / "photo.png" + image_file.write_bytes(b"\x89PNG\r\n\x1a\n") + # `.R` is in `TEXT_FILE_EXTENSIONS` and `MultimodalTextbox` matches extensions + # case-insensitively, so both spellings have to be inlined as text here. + r_file = tmp_path / "plot.r" + r_file.write_text("plot(1:10)") + + history = [ + { + "role": "user", + "content": [{"type": "file", "file": {"path": str(text_file)}}], + }, + {"role": "assistant", "content": [{"type": "text", "text": "ok"}]}, + { + "role": "user", + "content": [{"type": "file", "file": {"path": str(image_file)}}], + }, + {"role": "assistant", "content": [{"type": "text", "text": "ok"}]}, + { + "role": "user", + "content": [{"type": "file", "file": {"path": str(r_file)}}], + }, + {"role": "assistant", "content": [{"type": "text", "text": "ok"}]}, + ] + history_before = copy.deepcopy(history) + + conversation = format_conversation(history, "and now a short one") # type: ignore + + # the text file is inlined as text, the same as when it was first sent... + assert conversation[0]["content"] == [ + {"type": "text", "text": "\n## pasted_text.txt\na very long pasted prompt"} + ] + # ...while an image is still sent as an image + image_content = conversation[2]["content"][0] + assert image_content["type"] == "image_url" + assert image_content["image_url"]["url"].startswith("data:image/png;base64,") + + assert conversation[4]["content"] == [ + {"type": "text", "text": "\n## plot.r\nplot(1:10)"} + ] + + assert conversation[-1] == { + "role": "user", + "content": [{"type": "text", "text": "and now a short one"}], + } + + # The messages passed in are the ones in `chatbot_state`, so rewriting them in place + # would replace the attachments in the transcript with what was sent to the model. + assert history == history_before + + +def test_format_conversation_replays_remote_text_files_as_text(monkeypatch): + # A text file that is a URL rather than a local upload has to be inlined as text + # too, rather than base64-encoded into `image_url` like an image would be. + from gradio.external import format_conversation + + def fake_get(url, *args, **kwargs): + assert url == "https://example.com/files/notes.txt" + return httpx.Response( + 200, text="remote notes", request=httpx.Request("GET", url) + ) + + monkeypatch.setattr(httpx, "get", fake_get) + + history = [ + { + "role": "user", + "content": [ + { + "type": "file", + "file": {"path": "https://example.com/files/notes.txt"}, + } + ], + } + ] + + conversation = format_conversation(history, "and now a short one") # type: ignore + + assert conversation[0]["content"] == [ + {"type": "text", "text": "\n## notes.txt\nremote notes"} + ] + + def test_load_chat_textbox_override(): from gradio import ChatInterface