-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
[Bug] Support multiple "system" messages in REST API #2311
Comments
Digging into the source code, this seems to be intended behavior - the "system" message needs to be at position 0 in the list. I'll dig into the code to see if I can shoehorn in the correct behavior - if I understand correctly, I just need to concatenate tokens around each system message? |
Thanks for pointing this out. I think we can certainly enhance this behavior |
Hi @bayley thanks for bringing this up. Just want to get more clarity here. Do you mind sharing some reference examples for the cases of multiple system prompts (which may not be at position 0)? Happy to learn the expected behavior here. |
Yeah, here's a typical request sent by SillyTavern:
My understanding is the multiple system prompts in the template improve personality-following performance for some smaller models, as well as some commercial models that are otherwise reluctant to stay in character. |
@bayley do you know how these multiple system prompt get interpreted into prompt specifically? Most chat template follows a system then user/assistant alternation |
So...I was looking into this the other day as well. The text-generation-webui implementation seems to simply discard the all but the last system prompt which is clearly not right: for entry in history:
if "image_url" in entry:
image_url = entry['image_url']
if "base64" in image_url:
image_url = re.sub('^data:image/.+;base64,', '', image_url)
img = Image.open(BytesIO(base64.b64decode(image_url)))
else:
try:
my_res = requests.get(image_url)
img = Image.open(BytesIO(my_res.content))
except Exception:
raise 'Image cannot be loaded from the URL!'
buffered = BytesIO()
if img.mode in ("RGBA", "P"):
img = img.convert("RGB")
img.save(buffered, format="JPEG")
img_str = base64.b64encode(buffered.getvalue()).decode('utf-8')
content = f'<img src="data:image/jpeg;base64,{img_str}">'
else:
content = entry["content"]
role = entry["role"]
if role == "user":
user_input = content
user_input_last = True
if current_message:
chat_dialogue.append([current_message, ''])
current_message = ""
current_message = content
elif role == "assistant":
current_reply = content
user_input_last = False
if current_message:
chat_dialogue.append([current_message, current_reply])
current_message = ""
current_reply = ""
else:
chat_dialogue.append(['', current_reply])
elif role == "system":
system_message = content
if not user_input_last:
user_input = ""
return user_input, system_message, {'internal': chat_dialogue, 'visible': copy.deepcopy(chat_dialogue)} More digging is necessary to figure out what the right behavior is. An easy answer is to concatenate all of the system messages, but rumor has it the behavior of OpenAI's official models changes depending on where the system messages are in the message history, which makes me think the extra system messages are added to the context in place. The question is, are they added with tokens around them, or tokens around them? |
Right now we will implement the support via concat all system messsages |
The REST API seems to return a 400 error if the request object contains multiple messages with the "system" role. The following is a minimal reproducer:
Commenting out either of the system messages allows the script to run.
Having multiple system messages is necessary to support popular frontends such as SillyTavern, so supporting it would be a good idea.
The text was updated successfully, but these errors were encountered: