-
Notifications
You must be signed in to change notification settings - Fork 105
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 system prompt natively for Gemini 1.5 #22
Conversation
3ddbf1a
to
9a8a69f
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.
Food for thought: I intended ModelInfo to be user-facing indicators of what's supported. So a model that has a synthetic system prompt would still support a system prompt as far as ModelInfo is concerned. I'm not sure how to rationalize that with the need to change model factory behavior based on underlying capability tho...
js/plugins/googleai/src/gemini.ts
Outdated
if (messages[0].role === 'system') { | ||
systemInstruction = toGeminiSystemInstruction(messages[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.
I'd recommend just detecting and splicing off the first system message. Something like:
const systemMessage = messages.find(m => m.role === 'system');
if (systemMessage) messages.splice(messages.indexOf(systemMessage), 1);
const systemInstruction = toGeminiSystemInstruction(systemMessage);
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 had this :) but it seemed to negatively impact the trace, as the system message was no longer part of the "input".
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.
How about above you do const messages = [...request.messages]
to copy the array, then it won't be modified in place and should still show up in the trace.
Yes, this is a great point. I'll noodle on this some more. |
d820a7f
to
46e9642
Compare
Updated |
9282992
to
f012c69
Compare
f012c69
to
a9e6d20
Compare
a9e6d20
to
b07f7b9
Compare
b07f7b9
to
3e67554
Compare
Gemini 1.5 supports system instructions natively, but we do not utilize it currently. This proposal would fix that.
The main gotcha here, is that Gemini implemented
systemInstructions
as a new input parameter, rather than leveraging messages with therole=system
. Since ourGenerateRequest
abstraction is currently predicated on this paradigm (role=system
) I had to do a bit of legwork. This proposal will:role=system
role=user
and pass to the model assystemInstructions
role=system
appears anywhere else in the requestAlternatively, we could change GenerateRequest to accept
systemInstructions
directly.