From ab0caf4be64d0e874fa22dc31f3dfa6812472122 Mon Sep 17 00:00:00 2001 From: Philippe Martin Date: Tue, 30 Apr 2024 12:39:07 +0200 Subject: [PATCH] Remove system prompt from playground creation page (#1020) * frontend: remove system prompt from creation form Signed-off-by: Philippe Martin * backend: remove systemPrompt parameter for playground creation Signed-off-by: Philippe Martin --------- Signed-off-by: Philippe Martin --- .../src/managers/playgroundV2Manager.spec.ts | 68 +++---------------- .../src/managers/playgroundV2Manager.ts | 17 +---- packages/backend/src/studio-api-impl.ts | 4 +- .../src/pages/PlaygroundCreate.svelte | 13 +--- packages/shared/src/StudioAPI.ts | 2 +- 5 files changed, 17 insertions(+), 87 deletions(-) diff --git a/packages/backend/src/managers/playgroundV2Manager.spec.ts b/packages/backend/src/managers/playgroundV2Manager.spec.ts index b56f76aeb..e2a845d2f 100644 --- a/packages/backend/src/managers/playgroundV2Manager.spec.ts +++ b/packages/backend/src/managers/playgroundV2Manager.spec.ts @@ -81,7 +81,7 @@ test('submit should throw an error if the server is stopped', async () => { } as unknown as InferenceServer, ]); const manager = new PlaygroundV2Manager(webviewMock, inferenceManagerMock, taskRegistryMock, telemetryMock); - await manager.createPlayground('playground 1', { id: 'model1' } as ModelInfo, '', 'tracking-1'); + await manager.createPlayground('playground 1', { id: 'model1' } as ModelInfo, 'tracking-1'); vi.mocked(inferenceManagerMock.getServers).mockReturnValue([ { @@ -114,7 +114,7 @@ test('submit should throw an error if the server is unhealthy', async () => { } as unknown as InferenceServer, ]); const manager = new PlaygroundV2Manager(webviewMock, inferenceManagerMock, taskRegistryMock, telemetryMock); - await manager.createPlayground('p1', { id: 'model1' } as ModelInfo, '', 'tracking-1'); + await manager.createPlayground('p1', { id: 'model1' } as ModelInfo, 'tracking-1'); const playgroundId = manager.getConversations()[0].id; await expect(manager.submit(playgroundId, 'dummyUserInput')).rejects.toThrowError( 'Inference server is not healthy, currently status: unhealthy.', @@ -140,7 +140,7 @@ test('create playground should create conversation.', async () => { ]); const manager = new PlaygroundV2Manager(webviewMock, inferenceManagerMock, taskRegistryMock, telemetryMock); expect(manager.getConversations().length).toBe(0); - await manager.createPlayground('playground 1', { id: 'model-1' } as ModelInfo, '', 'tracking-1'); + await manager.createPlayground('playground 1', { id: 'model-1' } as ModelInfo, 'tracking-1'); const conversations = manager.getConversations(); expect(conversations.length).toBe(1); @@ -176,7 +176,7 @@ test('valid submit should create IPlaygroundMessage and notify the webview', asy } as unknown as OpenAI); const manager = new PlaygroundV2Manager(webviewMock, inferenceManagerMock, taskRegistryMock, telemetryMock); - await manager.createPlayground('playground 1', { id: 'dummyModelId' } as ModelInfo, undefined, 'tracking-1'); + await manager.createPlayground('playground 1', { id: 'dummyModelId' } as ModelInfo, 'tracking-1'); const date = new Date(2000, 1, 1, 13); vi.setSystemTime(date); @@ -245,7 +245,7 @@ test('submit should send options', async () => { } as unknown as OpenAI); const manager = new PlaygroundV2Manager(webviewMock, inferenceManagerMock, taskRegistryMock, telemetryMock); - await manager.createPlayground('playground 1', { id: 'dummyModelId' } as ModelInfo, undefined, 'tracking-1'); + await manager.createPlayground('playground 1', { id: 'dummyModelId' } as ModelInfo, 'tracking-1'); const playgrounds = manager.getConversations(); await manager.submit(playgrounds[0].id, 'dummyUserInput', { temperature: 0.123, max_tokens: 45, top_p: 0.345 }); @@ -282,7 +282,6 @@ test('creating a new playground should send new playground to frontend', async ( id: 'model-1', name: 'Model 1', } as unknown as ModelInfo, - '', 'tracking-1', ); expect(webviewMock.postMessage).toHaveBeenCalledWith({ @@ -307,7 +306,6 @@ test('creating a new playground with no name should send new playground to front id: 'model-1', name: 'Model 1', } as unknown as ModelInfo, - '', 'tracking-1', ); expect(webviewMock.postMessage).toHaveBeenCalledWith({ @@ -333,7 +331,6 @@ test('creating a new playground with no model served should start an inference s id: 'model-1', name: 'Model 1', } as unknown as ModelInfo, - '', 'tracking-1', ); expect(createInferenceServerMock).toHaveBeenCalledWith( @@ -370,7 +367,6 @@ test('creating a new playground with the model already served should not start a id: 'model-1', name: 'Model 1', } as unknown as ModelInfo, - '', 'tracking-1', ); expect(createInferenceServerMock).not.toHaveBeenCalled(); @@ -399,7 +395,6 @@ test('creating a new playground with the model server stopped should start the i id: 'model-1', name: 'Model 1', } as unknown as ModelInfo, - '', 'tracking-1', ); expect(createInferenceServerMock).not.toHaveBeenCalled(); @@ -417,7 +412,6 @@ test('delete conversation should delete the conversation', async () => { id: 'model-1', name: 'Model 1', } as unknown as ModelInfo, - '', 'tracking-1', ); @@ -437,7 +431,6 @@ test('creating a new playground with an existing name shoud fail', async () => { id: 'model-1', name: 'Model 1', } as unknown as ModelInfo, - '', 'tracking-1', ); await expect( @@ -447,7 +440,6 @@ test('creating a new playground with an existing name shoud fail', async () => { id: 'model-2', name: 'Model 2', } as unknown as ModelInfo, - '', 'tracking-2', ), ).rejects.toThrowError('a playground with the name a name already exists'); @@ -465,9 +457,9 @@ test('requestCreatePlayground should call createPlayground and createTask, then }); const createPlaygroundSpy = vi.spyOn(manager, 'createPlayground').mockResolvedValue('playground-1'); - const id = await manager.requestCreatePlayground('a name', { id: 'model-1' } as ModelInfo, ''); + const id = await manager.requestCreatePlayground('a name', { id: 'model-1' } as ModelInfo); - expect(createPlaygroundSpy).toHaveBeenCalledWith('a name', { id: 'model-1' } as ModelInfo, '', expect.any(String)); + expect(createPlaygroundSpy).toHaveBeenCalledWith('a name', { id: 'model-1' } as ModelInfo, expect.any(String)); expect(createTaskMock).toHaveBeenCalledWith('Creating Playground environment', 'loading', { trackingId: id, }); @@ -494,9 +486,9 @@ test('requestCreatePlayground should call createPlayground and createTask, then }); const createPlaygroundSpy = vi.spyOn(manager, 'createPlayground').mockRejectedValue(new Error('an error')); - const id = await manager.requestCreatePlayground('a name', { id: 'model-1' } as ModelInfo, ''); + const id = await manager.requestCreatePlayground('a name', { id: 'model-1' } as ModelInfo); - expect(createPlaygroundSpy).toHaveBeenCalledWith('a name', { id: 'model-1' } as ModelInfo, '', expect.any(String)); + expect(createPlaygroundSpy).toHaveBeenCalledWith('a name', { id: 'model-1' } as ModelInfo, expect.any(String)); expect(createTaskMock).toHaveBeenCalledWith('Creating Playground environment', 'loading', { trackingId: id, }); @@ -520,27 +512,6 @@ test('requestCreatePlayground should call createPlayground and createTask, then }); describe('system prompt', () => { - test('create playground with system prompt should init the conversation with one message', async () => { - vi.mocked(inferenceManagerMock.getServers).mockReturnValue([ - { - status: 'running', - models: [ - { - id: 'model1', - }, - ], - } as unknown as InferenceServer, - ]); - const manager = new PlaygroundV2Manager(webviewMock, inferenceManagerMock, taskRegistryMock, telemetryMock); - await manager.createPlayground('playground 1', { id: 'model1' } as ModelInfo, 'dummySystemPrompt', 'tracking-1'); - - const conversations = manager.getConversations(); - expect(conversations.length).toBe(1); - expect(conversations[0].messages.length).toBe(1); - expect(conversations[0].messages[0].role).toBe('system'); - expect(conversations[0].messages[0].content).toBe('dummySystemPrompt'); - }); - test('set system prompt on non existing conversation should throw an error', async () => { vi.mocked(inferenceManagerMock.getServers).mockReturnValue([ { @@ -559,25 +530,6 @@ describe('system prompt', () => { }).toThrowError('conversation with id invalid does not exist.'); }); - test('set system prompt should overwrite existing system prompt', async () => { - vi.mocked(inferenceManagerMock.getServers).mockReturnValue([ - { - status: 'running', - models: [ - { - id: 'model1', - }, - ], - } as unknown as InferenceServer, - ]); - const manager = new PlaygroundV2Manager(webviewMock, inferenceManagerMock, taskRegistryMock, telemetryMock); - await manager.createPlayground('playground 1', { id: 'model1' } as ModelInfo, 'dummySystemPrompt', 'tracking-1'); - - const conversations = manager.getConversations(); - manager.setSystemPrompt(conversations[0].id, 'newSystemPrompt'); - expect(manager.getConversations()[0].messages[0].content).toBe('newSystemPrompt'); - }); - test('set system prompt should throw an error if user already submit message', async () => { vi.mocked(inferenceManagerMock.getServers).mockReturnValue([ { @@ -608,7 +560,7 @@ describe('system prompt', () => { } as unknown as OpenAI); const manager = new PlaygroundV2Manager(webviewMock, inferenceManagerMock, taskRegistryMock, telemetryMock); - await manager.createPlayground('playground 1', { id: 'dummyModelId' } as ModelInfo, undefined, 'tracking-1'); + await manager.createPlayground('playground 1', { id: 'dummyModelId' } as ModelInfo, 'tracking-1'); const date = new Date(2000, 1, 1, 13); vi.setSystemTime(date); diff --git a/packages/backend/src/managers/playgroundV2Manager.ts b/packages/backend/src/managers/playgroundV2Manager.ts index 926b5bd08..116799169 100644 --- a/packages/backend/src/managers/playgroundV2Manager.ts +++ b/packages/backend/src/managers/playgroundV2Manager.ts @@ -50,7 +50,7 @@ export class PlaygroundV2Manager implements Disposable { this.#conversationRegistry.deleteConversation(conversationId); } - async requestCreatePlayground(name: string, model: ModelInfo, systemPrompt?: string): Promise { + async requestCreatePlayground(name: string, model: ModelInfo): Promise { const trackingId: string = getRandomString(); const task = this.taskRegistry.createTask('Creating Playground environment', 'loading', { trackingId: trackingId, @@ -58,10 +58,9 @@ export class PlaygroundV2Manager implements Disposable { const telemetry: Record = { hasName: !!name, - hasSystemPrompt: !!systemPrompt, modelId: model.id, }; - this.createPlayground(name, model, systemPrompt, trackingId) + this.createPlayground(name, model, trackingId) .then((playgroundId: string) => { this.taskRegistry.updateTask({ ...task, @@ -100,12 +99,7 @@ export class PlaygroundV2Manager implements Disposable { return trackingId; } - async createPlayground( - name: string, - model: ModelInfo, - systemPrompt: string | undefined, - trackingId: string, - ): Promise { + async createPlayground(name: string, model: ModelInfo, trackingId: string): Promise { if (!name) { name = this.getFreeName(); } @@ -116,11 +110,6 @@ export class PlaygroundV2Manager implements Disposable { // Create conversation const conversationId = this.#conversationRegistry.createConversation(name, model.id); - // If system prompt let's add it to the conversation - if (systemPrompt !== undefined && systemPrompt.length > 0) { - this.submitSystemPrompt(conversationId, systemPrompt); - } - // create/start inference server if necessary const servers = this.inferenceManager.getServers(); const server = servers.find(s => s.models.map(mi => mi.id).includes(model.id)); diff --git a/packages/backend/src/studio-api-impl.ts b/packages/backend/src/studio-api-impl.ts index 3b3dcff40..e00e85fbc 100644 --- a/packages/backend/src/studio-api-impl.ts +++ b/packages/backend/src/studio-api-impl.ts @@ -78,9 +78,9 @@ export class StudioApiImpl implements StudioAPI { }); } - async requestCreatePlayground(name: string, model: ModelInfo, systemPrompt?: string): Promise { + async requestCreatePlayground(name: string, model: ModelInfo): Promise { try { - return this.playgroundV2.requestCreatePlayground(name, model, systemPrompt); + return this.playgroundV2.requestCreatePlayground(name, model); } catch (err: unknown) { console.error('Something went wrong while trying to create playground environment', err); throw err; diff --git a/packages/frontend/src/pages/PlaygroundCreate.svelte b/packages/frontend/src/pages/PlaygroundCreate.svelte index daed7c71a..da6a10ea4 100644 --- a/packages/frontend/src/pages/PlaygroundCreate.svelte +++ b/packages/frontend/src/pages/PlaygroundCreate.svelte @@ -19,7 +19,6 @@ let localModels: ModelInfo[]; $: localModels = $modelsInfo.filter(model => model.file); $: availModels = $modelsInfo.filter(model => !model.file); let modelId: string | undefined = undefined; -let systemPrompt: string | undefined = undefined; let submitted: boolean = false; let playgroundName: string; let errorMsg: string | undefined = undefined; @@ -58,7 +57,7 @@ async function submit() { submitted = true; try { // Using || and not && as we want to have the empty string systemPrompt passed as undefined - trackingId = await studioClient.requestCreatePlayground(playgroundName, model, systemPrompt || undefined); + trackingId = await studioClient.requestCreatePlayground(playgroundName, model); } catch (err: unknown) { trackingId = undefined; console.error('Something wrong while trying to create the playground.', err); @@ -173,16 +172,6 @@ onDestroy(() => { {/if} - - - {#if errorMsg !== undefined} diff --git a/packages/shared/src/StudioAPI.ts b/packages/shared/src/StudioAPI.ts index a3248818f..3ac553e8c 100644 --- a/packages/shared/src/StudioAPI.ts +++ b/packages/shared/src/StudioAPI.ts @@ -151,7 +151,7 @@ export abstract class StudioAPI { */ abstract createSnippet(options: RequestOptions, language: string, variant: string): Promise; - abstract requestCreatePlayground(name: string, model: ModelInfo, systemPrompt?: string): Promise; + abstract requestCreatePlayground(name: string, model: ModelInfo): Promise; /** * Delete a conversation