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
ask the system prompt from the Playground creation form #643
ask the system prompt from the Playground creation form #643
Conversation
Signed-off-by: Philippe Martin <phmartin@redhat.com>
packages/shared/src/StudioAPI.ts
Outdated
@@ -143,7 +143,7 @@ export abstract class StudioAPI { | |||
*/ | |||
abstract createSnippet(options: RequestOptions, language: string, variant: string): Promise<string>; | |||
|
|||
abstract requestCreatePlayground(name: string, model: ModelInfo): Promise<string>; | |||
abstract requestCreatePlayground(name: string, model: ModelInfo, systemPrompt: string): Promise<string>; |
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.
abstract requestCreatePlayground(name: string, model: ModelInfo, systemPrompt: string): Promise<string>; | |
abstract requestCreatePlayground(name: string, model: ModelInfo, systemPrompt?: string): Promise<string>; |
@@ -55,7 +56,7 @@ async function submit() { | |||
// disable submit button | |||
submitted = true; | |||
try { | |||
trackingId = await studioClient.requestCreatePlayground(playgroundName, model); | |||
trackingId = await studioClient.requestCreatePlayground(playgroundName, model, systemPrompt ?? ''); |
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.
trackingId = await studioClient.requestCreatePlayground(playgroundName, model, systemPrompt ?? ''); | |
trackingId = await studioClient.requestCreatePlayground(playgroundName, model, systemPrompt); |
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.
That was my first attempt, but the problem I can see is that systemPrompt can be either undefined or an empty string, depending on if the user lets the textarea untouched, or edits its content and removes it. I wanted to have only one value (the empty string) to indicate the systemPrompt is empty. But as the two values are handled correctly on the other side, we can do like this if you prefer
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 that the empty value should not be allowed, and be replaced with undefined
, as it should be fully optional from the frontend POV
@@ -74,9 +74,9 @@ export class StudioApiImpl implements StudioAPI { | |||
}); | |||
} | |||
|
|||
async requestCreatePlayground(name: string, model: ModelInfo): Promise<string> { | |||
async requestCreatePlayground(name: string, model: ModelInfo, systemPrompt: string): Promise<string> { |
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.
async requestCreatePlayground(name: string, model: ModelInfo, systemPrompt: string): Promise<string> { | |
async requestCreatePlayground(name: string, model: ModelInfo, systemPrompt?: string): Promise<string> { |
@@ -54,13 +54,13 @@ export class PlaygroundV2Manager extends Publisher<PlaygroundV2[]> implements Di | |||
this.notify(); | |||
} | |||
|
|||
async requestCreatePlayground(name: string, model: ModelInfo): Promise<string> { | |||
async requestCreatePlayground(name: string, model: ModelInfo, systemPrompt: string): Promise<string> { |
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.
async requestCreatePlayground(name: string, model: ModelInfo, systemPrompt: string): Promise<string> { | |
async requestCreatePlayground(name: string, model: ModelInfo, systemPrompt?: string): Promise<string> { |
@@ -94,7 +94,7 @@ export class PlaygroundV2Manager extends Publisher<PlaygroundV2[]> implements Di | |||
return trackingId; | |||
} | |||
|
|||
async createPlayground(name: string, model: ModelInfo, trackingId: string): Promise<string> { | |||
async createPlayground(name: string, model: ModelInfo, systemPrompt: string, trackingId: string): Promise<string> { |
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.
async createPlayground(name: string, model: ModelInfo, systemPrompt: string, trackingId: string): Promise<string> { | |
async createPlayground(name: string, model: ModelInfo, systemPrompt: string | undefined, trackingId: string): Promise<string> { |
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.
Great job ! We might want later to use something like
createPlayground(options?: { system?: string, name?: string ....})
to easily add optional parameters
Signed-off-by: Philippe Martin <phmartin@redhat.com>
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
Co-authored-by: Jeff MAURY <jmaury@redhat.com> Signed-off-by: Philippe Martin <feloy1@gmail.com>
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, it looks really good
I see this has been merged but the UX does not work for me |
We need to agreed that the experience is primarily designed for quick iterations for a developer. First time user User interacting with the model User iterating customizing the use of the model
|
The system prompt is visible in the chat as the first message of the discussion, with the "System Prompt" title:
|
Ok, I did not see the system prompt getting displayed at the top. Now tested, that's better. |
I agree with @slemeur making the system prompt un-editable is not the right experience. You should be able to edit on the fly, like how it was in an earlier iteration. If you are concerned about coherence of the conversation, since the system prompt is always the first entry, you likely just want to reset to conversation history upon setting a new system prompt. |
What does this PR do?
This PR moves the system prompt textarea to the creation form of the Playground.
Screenshot / video of UI
system-prompt-in-creation-form.mp4
What issues does this PR fix or reference?
Fixes #617
How to test this PR?
Create a playground with/without system prompt and check that it is displayed in the playground conversation if set.