ask the system prompt from the Playground creation form #643

feloy · 2024-03-22T15:04:29Z

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.

Signed-off-by: Philippe Martin <phmartin@redhat.com>

axel7083 · 2024-03-22T15:09:55Z

packages/shared/src/StudioAPI.ts

@@ -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>;


Suggested change

abstract requestCreatePlayground(name: string, model: ModelInfo, systemPrompt: string): Promise<string>;

abstract requestCreatePlayground(name: string, model: ModelInfo, systemPrompt?: string): Promise<string>;

axel7083 · 2024-03-22T15:10:15Z

packages/frontend/src/pages/PlaygroundCreate.svelte

@@ -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 ?? '');


Suggested change

trackingId = await studioClient.requestCreatePlayground(playgroundName, model, systemPrompt ?? '');

trackingId = await studioClient.requestCreatePlayground(playgroundName, model, systemPrompt);

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

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

axel7083 · 2024-03-22T15:10:47Z

packages/backend/src/studio-api-impl.ts

@@ -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> {


Suggested change

async requestCreatePlayground(name: string, model: ModelInfo, systemPrompt: string): Promise<string> {

async requestCreatePlayground(name: string, model: ModelInfo, systemPrompt?: string): Promise<string> {

axel7083 · 2024-03-22T15:11:00Z

packages/backend/src/managers/playgroundV2Manager.ts

@@ -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> {


Suggested change

async requestCreatePlayground(name: string, model: ModelInfo, systemPrompt: string): Promise<string> {

async requestCreatePlayground(name: string, model: ModelInfo, systemPrompt?: string): Promise<string> {

axel7083 · 2024-03-22T15:11:17Z

packages/backend/src/managers/playgroundV2Manager.ts

@@ -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> {


Suggested change

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> {

axel7083

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>

jeffmaury

LGTM

packages/backend/src/managers/playgroundV2Manager.ts

Co-authored-by: Jeff MAURY <jmaury@redhat.com> Signed-off-by: Philippe Martin <feloy1@gmail.com>

vrothberg

LGTM, it looks really good

slemeur · 2024-03-25T08:54:32Z

I see this has been merged but the UX does not work for me
and I have a big -1 on this change

slemeur · 2024-03-25T09:06:19Z

We need to agreed that the experience is primarily designed for quick iterations for a developer.

First time user
We are asking the user to define something that is not yet known. The change it will do to the way the model interact will not be understood AND it will not be editable "on the go"

User interacting with the model
As a user I've defined a system prompt, but now that I'm in the playground environment, I don't see it anymore. So if I start a playground one day and get back to it another day, I'll not know what I had defined as system prompt.
Exact same bad UX will happen when I'm creating multiple playground environments

User iterating customizing the use of the model
The UX proposed and involved by this change is breaking the whole purpose of the screen.
As a user, I want to be able to adjust and iterate into the definition of how my model must interact. It's a multi-stage purposes.

User will start without defining a system prompt
User will adjust the settings and add a system prompt, replay the discussion to observe how it change the behavior of the model
User will continue to adjust the system prompt and would like to be able to compare the behavior
It's going to be an iterative process, but here, because of this UX, the user is forced to each time get back to the creation of the playground environment.

feloy · 2024-03-25T09:10:00Z

We need to agreed that the experience is primarily designed for quick iterations for a developer.

First time user We are asking the user to define something that is not yet known. The change it will do to the way the model interact will not be understood AND it will not be editable "on the go"

User interacting with the model As a user I've defined a system prompt, but now that I'm in the playground environment, I don't see it anymore. So if I start a playground one day and get back to it another day, I'll not know what I had defined as system prompt. Exact same bad UX will happen when I'm creating multiple playground environments

The system prompt is visible in the chat as the first message of the discussion, with the "System Prompt" title:

User iterating customizing the use of the model The UX proposed and involved by this change is breaking the whole purpose of the screen. As a user, I want to be able to adjust and iterate into the definition of how my model must interact. It's a multi-stage purposes.

User will start without defining a system prompt

User will adjust the settings and add a system prompt, replay the discussion to observe how it change the behavior of the model

User will continue to adjust the system prompt and would like to be able to compare the behavior
It's going to be an iterative process, but here, because of this UX, the user is forced to each time get back to the creation of the playground environment.

slemeur · 2024-03-25T10:13:33Z

Ok, I did not see the system prompt getting displayed at the top. Now tested, that's better.
We'll move forward with this solution for 0.3 - but will hopefully be able to refine the experience for 1.0

MichaelClifford · 2024-03-25T14:14:33Z

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.

ask the system prompt from the Playground creation form

500ef50

Signed-off-by: Philippe Martin <phmartin@redhat.com>

feloy requested a review from a team as a code owner March 22, 2024 15:04

feloy requested review from benoitf, axel7083 and lstocchi March 22, 2024 15:04

feloy mentioned this pull request Mar 22, 2024

feat: set system prompt at the beginning #625

Closed

axel7083 reviewed Mar 22, 2024

View reviewed changes

axel7083 approved these changes Mar 22, 2024

View reviewed changes

feloy requested a review from slemeur March 22, 2024 15:27

feloy mentioned this pull request Mar 22, 2024

Prevent modification of the system prompt after having started the playground's discussion #617

Closed

pass empty string systemPrompt as undefined

b3f375a

Signed-off-by: Philippe Martin <phmartin@redhat.com>

jeffmaury approved these changes Mar 22, 2024

View reviewed changes

packages/backend/src/managers/playgroundV2Manager.ts Outdated Show resolved Hide resolved

Update packages/backend/src/managers/playgroundV2Manager.ts

6d66d77

Co-authored-by: Jeff MAURY <jmaury@redhat.com> Signed-off-by: Philippe Martin <feloy1@gmail.com>

vrothberg approved these changes Mar 25, 2024

View reviewed changes

feloy merged commit 1613e13 into containers:main Mar 25, 2024
4 checks passed

axel7083 mentioned this pull request Mar 25, 2024

fix(playground): disable the system prompt textarea when loading state #654

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ask the system prompt from the Playground creation form #643

ask the system prompt from the Playground creation form #643

feloy commented Mar 22, 2024

axel7083 Mar 22, 2024

axel7083 Mar 22, 2024 •

edited

feloy Mar 22, 2024

axel7083 Mar 22, 2024 •

edited

axel7083 Mar 22, 2024

axel7083 Mar 22, 2024

axel7083 Mar 22, 2024

axel7083 left a comment

jeffmaury left a comment

vrothberg left a comment

slemeur commented Mar 25, 2024

slemeur commented Mar 25, 2024

feloy commented Mar 25, 2024

slemeur commented Mar 25, 2024

MichaelClifford commented Mar 25, 2024

	abstract requestCreatePlayground(name: string, model: ModelInfo, systemPrompt: string): Promise<string>;
	abstract requestCreatePlayground(name: string, model: ModelInfo, systemPrompt?: string): Promise<string>;

	trackingId = await studioClient.requestCreatePlayground(playgroundName, model, systemPrompt ?? '');
	trackingId = await studioClient.requestCreatePlayground(playgroundName, model, systemPrompt);

	async requestCreatePlayground(name: string, model: ModelInfo, systemPrompt: string): Promise<string> {
	async requestCreatePlayground(name: string, model: ModelInfo, systemPrompt?: string): Promise<string> {

	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> {

ask the system prompt from the Playground creation form #643

ask the system prompt from the Playground creation form #643

Conversation

feloy commented Mar 22, 2024

What does this PR do?

Screenshot / video of UI

What issues does this PR fix or reference?

How to test this PR?

axel7083 Mar 22, 2024

Choose a reason for hiding this comment

axel7083 Mar 22, 2024 • edited

Choose a reason for hiding this comment

feloy Mar 22, 2024

Choose a reason for hiding this comment

axel7083 Mar 22, 2024 • edited

Choose a reason for hiding this comment

axel7083 Mar 22, 2024

Choose a reason for hiding this comment

axel7083 Mar 22, 2024

Choose a reason for hiding this comment

axel7083 Mar 22, 2024

Choose a reason for hiding this comment

axel7083 left a comment

Choose a reason for hiding this comment

jeffmaury left a comment

Choose a reason for hiding this comment

vrothberg left a comment

Choose a reason for hiding this comment

slemeur commented Mar 25, 2024

slemeur commented Mar 25, 2024

feloy commented Mar 25, 2024

slemeur commented Mar 25, 2024

MichaelClifford commented Mar 25, 2024

axel7083 Mar 22, 2024 •

edited

axel7083 Mar 22, 2024 •

edited