-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
Support Message API for chatbot and chatinterface #8422
Conversation
🪼 branch checks and previews
Install Gradio from this PR pip install https://gradio-builds.s3.amazonaws.com/96d6e61c927fcf15374934cfde976c0a25000db3/gradio-4.37.2-py3-none-any.whl Install Gradio Python Client from this PR pip install "gradio-client @ git+https://github.com/gradio-app/gradio@96d6e61c927fcf15374934cfde976c0a25000db3#subdirectory=client/python" Install Gradio JS Client from this PR npm install https://gradio-builds.s3.amazonaws.com/96d6e61c927fcf15374934cfde976c0a25000db3/gradio-client-1.2.1.tgz |
🦄 change detectedThis Pull Request includes changes to the following packages.
With the following changelog entry.
|
4aedebd
to
88942ea
Compare
@freddyaboulton this looks great! Just one quibble: I'd suggest not using "openai" as the name of the format. It might be that they modify their message format in a way that we don't want to track. Also Anthropic and others use the same format as well. What about So thinking how we could add support for components into this: we could modify the class Message(GradioModel):
role: str
metadata: Metadata = Field(default_factory=Metadata)
content: str | FileData | Component |
Yes makes sense regarding renaming. What about "messages" (the name used by tgi/transformers and from the looks of it is actually the industry standard name transformers docs anthropic docs ) |
"messages" sounds good, compatibility with transformers/tgi makes more sense 👍 |
This format looks good.
Regrading this, it would have to be another dict
|
imo it would be much nicer DX if the |
Yes that is what I had in mind, we can handle the conversion from component instance to internal payload format in pre/postprocess |
3c28903
to
a6519b8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This demo isn't working for me. Getting a couple of errors saying "Data incompatible with openai format"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed - issue was a typo in the msg_format
parameter.
demo/chatbot_with_tools/run.py
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very cool! Just some comments on the UI:
- Its not clear that you can click on the error message or tool use message and expand to get more details. I would suggest replacing with an accordion-like element where the toggle icon is clear
- As seen in the screenshot, the color of the tool use message is the "primary" color even though the rest of the bot message is not that color. In fact, its the user message that is the primary color, which is confusing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Btw it'd be awesome if we can include a real example of tool use with transformers agents or one of the other llm providers in our docs!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will make the font color more consistent! I like the minimal toolbox though and it matches the hugging face styling. Does anyone else have thoughts?
Agreed about the demo but to make it interesting we need to add api tokens etc so don't want to include it in tests or the repo. Will prepare something for the launch though!
) | ||
return ChatbotData(root=processed_messages) | ||
return ChatbotDataTuples(root=[]) | ||
if self.msg_format == "tuples": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: for all of our other components that can accept different types (e.g. Image
with multiple types such as PIL images, numpy arrays, string filepaths), we don't enforce the type
in postprocess()
-- we just automatically figure it out based on value
. Potentially, we could do the same thing here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We use the msg_format to set the data model and api info. So I want to make sure it matches in postprocess. Otherwise, it would be possible for the API info to tell you to expect dictionaries but you get tuples.
Nits:
import gradio as gr
with gr.Blocks() as demo:
gr.Chatbot([
gr.ChatMessage(role="user", content="Hello!"),
gr.ChatMessage(role="user", content="Hello!")
], msg_format="messages")
demo.launch()
import gradio as gr
with gr.Blocks() as demo:
gr.Chatbot([
gr.ChatMessage(role="abc", content="Hello!"),
], msg_format="messages")
demo.launch()
import gradio as gr
demo = gr.ChatInterface(lambda x,y:x, msg_format="messages")
demo.launch() @freddyaboulton very nice PR! Made a first pass and left some comments above. Down to do another deeper review again once these comments are addressed |
Thanks for the review @abidlabs !! I think I got all of the comments (and added some more unit tests because of them :) ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM @freddyaboulton this looks great! Tested gr.Chatbot
and gr.ChatInterface
with the new format and everything is working as expected.
I just have three small points of feedback, which I'll leave below
(1) The first concerns this:
Although I agree that this makes demos easier to write, this introduces a different behavior for iterators in the very special case where you are iterating from a ChatInterface with msg_format="messages". This is likely to confuse users who are used to sending the complete message with
and then use it via the client, e.g. from gradio_client import Client
client = Client("http://127.0.0.1:7864/")
result = client.predict(
message="Hello!!",
api_name="/chat"
)
print(result) You only get "!" (the final token). But if you run the regular version of this demo (with msg_format="tuples"), you get the entire final string: "You typed: Hello!!". This introduces a discrepancy between what a user would observe if they used the Gradio UI and what you get when you make a prediction with the client. |
(2) Just to reiterate the earlier point about the design of tools, I think we can improve the UI quite a bit
On the second point, I think an accordion would be the best UI. This is one area I like Streamlit's UI. If its an accordion, we should keep the accordion open if the accordion is the final message, but then collapse it if there are subsequent messages. cc @pngwn @hannahblair on this front. This isn't a blocker but I think having a nice UI for tools will facilitate some nice viral comms down the road |
(3) Let's add some docs for this, perhaps in the chatbot/chatinterface guides. Excited to do some nice comms here! |
Discussed with @freddyaboulton and we don't need this:
if we are bringing the bubbles back. @pngwn perhaps you could review the design of the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a few notes/ questions!
|
||
export let elem_id = ""; | ||
export let elem_classes: string[] = []; | ||
export let visible = true; | ||
export let value: messages = []; | ||
export let value: TupleFormat | Message[] = []; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd rather we didn't have these different types everywhere, can we transform the tuple format to the standard Message format in the backend before we send them down?
Although I guess this would be breaking for the clients, maybe we can only do this in 5.0.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes it would be breaking for clients so lets do in 5.0!
$: _value = | ||
msg_format === "tuples" | ||
? normalise_tuples(value as TupleFormat, root) | ||
: normalise_messages(value as Message[], root); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DO we need to handle these separately? Can we not have a single function that deals with them all or is there ambiguity?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is slightly different processing steps for each case so thought that keeping them separate would be best
js/chatbot/shared/ChatBot.svelte
Outdated
message: NormalisedMessage, | ||
selected: string | null | ||
): void { | ||
dispatch("like", { | ||
index: [i, j], | ||
value: message, | ||
index: i, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this correct? If the user is using tuples don't we still need both i
and j
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes you're right! Will fix
function group_messages( | ||
messages: NormalisedMessage[] | ||
): NormalisedMessage[][] { | ||
const groupedMessages: NormalisedMessage[][] = []; | ||
let currentGroup: NormalisedMessage[] = []; | ||
let currentRole: MessageRole | null = null; | ||
|
||
for (const message of messages) { | ||
if (!(message.role === "assistant" || message.role === "user")) { | ||
continue; | ||
} | ||
if (message.role === currentRole) { | ||
currentGroup.push(message); | ||
} else { | ||
if (currentGroup.length > 0) { | ||
groupedMessages.push(currentGroup); | ||
} | ||
currentGroup = [message]; | ||
currentRole = message.role; | ||
} | ||
} | ||
|
||
if (currentGroup.length > 0) { | ||
groupedMessages.push(currentGroup); | ||
} | ||
|
||
return groupedMessages; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the purpose of grouping messages like this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So that they would be displayed in the same bubble but we got rid of the bubbles lol
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are bringing the bubbles back but should they be in the same bubble if they are different messages? Maybe it looks cleaner though. Happy to leave it like this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think for sure the bot messages look cleaner in the same bubble. Happy to iterate with you when we bring them back.
js/chatbot/shared/MessageBox.svelte
Outdated
<!-- svelte-ignore a11y-click-events-have-key-events --> | ||
<!-- svelte-ignore a11y-no-static-element-interactions --> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should make this a button and remove these ignore comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm. Thanks for working on this @freddyaboulton
Thanks everyone for the reviews! I addressed all comments and updated the chatbot docs and release notes. Will prepare a guide soon. |
Description
Main Changes:
msg_format
parameter toChatbot
andChatInterface
so that messages can be returned as either the current list of tuples or a dictionary that is a superset of the messages/"OAI" format, e.g. {"role": "user", "content": ""}.Ifmsg_format
is "messages", then inChatInterface
developers can just yield the next token. They don't have to yield the entire message up to and including that token. I think this makes demos easier to write. And lets developers simply yield from their iterator._testcase.py
in a demo directory corresponding to an e2e test, that demo will also be loaded to the e2e app. And you can usego_to_testcase
to navigate to that testcase.Messages format overview
The message format is a dict with two required keys
role
andcontent
. There is an additionalmetadata
key that is not required but it can be used for tools and additional info about the message. Most messages returned from an "openai compatible" client will be compatible with gradio.The implementation of the message is below:
Examples
Realistic Inference API Streaming
ChatInterface Streaming
ChatInterface Multimodal
Chatbot
Multimodal Chatbot
Closes: #7118