From 2959a439a80431d9a2492c856154f7140c307903 Mon Sep 17 00:00:00 2001 From: Marika Marszalkowski Date: Tue, 3 Sep 2024 15:42:45 +0200 Subject: [PATCH 1/7] fix: caching --- .../src/utils/deployment-cache.test.ts | 91 ++++++++++++------- .../gen-ai-hub/src/utils/deployment-cache.ts | 20 +++- .../src/utils/deployment-resolver.test.ts | 12 +-- 3 files changed, 79 insertions(+), 44 deletions(-) diff --git a/packages/gen-ai-hub/src/utils/deployment-cache.test.ts b/packages/gen-ai-hub/src/utils/deployment-cache.test.ts index 81b23ee8..881c084f 100644 --- a/packages/gen-ai-hub/src/utils/deployment-cache.test.ts +++ b/packages/gen-ai-hub/src/utils/deployment-cache.test.ts @@ -28,42 +28,65 @@ describe('deployment cache', () => { }); }); - it('should cache all deployments independent of potentially given models', () => { - const opts = { - scenarioId: 'foundation-models', - model: { name: 'gpt-4o', version: 'latest' } - }; + describe('should cache all deployments independent of potentially given models', () => { + beforeEach(() => { + deploymentCache.setAll( + { + scenarioId: 'foundation-models', + model: { name: 'gpt-4o', version: 'latest' } + }, + [ + mockAiDeployment('deployment-id1', { + name: 'gpt-35-turbo', + version: 'latest' + }), + mockAiDeployment('deployment-id2', { + name: 'gpt-35-turbo', + version: '123' + }) + ] + ); + }); - deploymentCache.setAll(opts, [ - mockAiDeployment('deployment-id1', { - name: 'gpt-35-turbo', - version: 'latest' - }), - mockAiDeployment('deployment-id2', { - name: 'gpt-35-turbo', - version: '123' - }) - ]); + it('cache nothing for unlisted model names', () => { + expect( + deploymentCache.get({ + scenarioId: 'foundation-models', + model: { name: 'gpt-4o', version: 'latest' } + }) + ).toBeUndefined(); + }); - expect(deploymentCache.get(opts)).toBeUndefined(); - expect( - deploymentCache.get({ - ...opts, - model: { - name: 'gpt-35-turbo', - version: 'latest' - } - })?.id - ).toEqual('deployment-id1'); - expect( - deploymentCache.get({ - ...opts, - model: { - name: 'gpt-35-turbo', - version: '123' - } - })?.id - ).toEqual('deployment-id2'); + it('retrieve the deployment matching the model name and version', () => { + expect( + deploymentCache.get({ + scenarioId: 'foundation-models', + model: { + name: 'gpt-35-turbo', + version: '123' + } + })?.id + ).toEqual('deployment-id2'); + }); + + it('retrieve the first deployment matching the model name when version is missing', () => { + expect( + deploymentCache.get({ + scenarioId: 'foundation-models', + model: { + name: 'gpt-35-turbo' + } + })?.id + ).toEqual('deployment-id1'); + }); + + it('retrieve the deployment when model is missing', () => { + expect( + deploymentCache.get({ + scenarioId: 'foundation-models' + })?.id + ).toEqual('deployment-id1'); + }); }); it('should cache only the first deployments for equal models and versions', () => { diff --git a/packages/gen-ai-hub/src/utils/deployment-cache.ts b/packages/gen-ai-hub/src/utils/deployment-cache.ts index 010a0604..b1f85bef 100644 --- a/packages/gen-ai-hub/src/utils/deployment-cache.ts +++ b/packages/gen-ai-hub/src/utils/deployment-cache.ts @@ -9,7 +9,7 @@ function getCacheKey({ model, resourceGroup = 'default' }: DeploymentResolutionOptions) { - return `${scenarioId}-${executableId}-${model?.name ?? ''}-${model?.version ?? ''}-${resourceGroup}`; + return `${scenarioId}:${executableId}:${model?.name ?? ''}:${model?.version ?? ''}:${resourceGroup}`; } interface Deployment { @@ -51,14 +51,26 @@ function createDeploymentCache(cache: Cache) { * @param deployments - Deployments to retrieve the IDs and models from. */ setAll: ( - opts: Omit, + opts: DeploymentResolutionOptions, deployments: AiDeployment[] ): void => { // go backwards to cache the first deployment ID for each model [...deployments].reverse().forEach(deployment => { - cache.set(getCacheKey({ ...opts, model: extractModel(deployment) }), { - entry: transformDeploymentForCache(deployment) + const model = extractModel(deployment); + const entry = transformDeploymentForCache(deployment); + cache.set(getCacheKey({ ...opts, model }), { + entry }); + + if (model) { + cache.set(getCacheKey({ ...opts, model: undefined }), { entry }); + if (model.version) { + cache.set( + getCacheKey({ ...opts, model: { ...model, version: undefined } }), + { entry } + ); + } + } }); }, clear: () => cache.clear() diff --git a/packages/gen-ai-hub/src/utils/deployment-resolver.test.ts b/packages/gen-ai-hub/src/utils/deployment-resolver.test.ts index c6c08da1..0730dd0c 100644 --- a/packages/gen-ai-hub/src/utils/deployment-resolver.test.ts +++ b/packages/gen-ai-hub/src/utils/deployment-resolver.test.ts @@ -29,7 +29,7 @@ describe('deployment resolver', () => { const id = await resolveDeploymentId({ scenarioId: 'foundation-models' }); - expect(id).toBe('1'); + expect(id).toEqual('1'); }); it('should return the first deployment with the correct model name', async () => { @@ -37,7 +37,7 @@ describe('deployment resolver', () => { scenarioId: 'foundation-models', model: { name: 'gpt-4o' } }); - expect(id).toBe('1'); + expect(id).toEqual('1'); }); it('should return the deployment with the correct model name and version', async () => { @@ -45,7 +45,7 @@ describe('deployment resolver', () => { scenarioId: 'foundation-models', model: { name: 'gpt-4o', version: '0613' } }); - expect(id).toBe('2'); + expect(id).toEqual('2'); }); it('should retrieve deployment from cache if available', async () => { @@ -55,8 +55,8 @@ describe('deployment resolver', () => { }; deploymentCache.set(opts, { id: '1' } as AiDeployment); const id = await resolveDeploymentId(opts); - expect(id).toBe('1'); - expect(nock.isDone()).toBe(false); + expect(id).toEqual('1'); + expect(nock.isDone()).toEqual(false); }); it('should throw in case no deployment with the given model name is found', async () => { @@ -131,7 +131,7 @@ describe('deployment resolver', () => { resourceGroup: 'otherId' }); - expect(id).toBe('5'); + expect(id).toEqual('5'); }); }); From 37eaadd614448bcbaaee909dda48a1032c89f0d4 Mon Sep 17 00:00:00 2001 From: Marika Marszalkowski Date: Tue, 3 Sep 2024 18:15:18 +0200 Subject: [PATCH 2/7] implement caching without full model explicitly --- .../gen-ai-hub/src/utils/deployment-cache.ts | 71 +++++++++++++++---- 1 file changed, 56 insertions(+), 15 deletions(-) diff --git a/packages/gen-ai-hub/src/utils/deployment-cache.ts b/packages/gen-ai-hub/src/utils/deployment-cache.ts index b1f85bef..e928a056 100644 --- a/packages/gen-ai-hub/src/utils/deployment-cache.ts +++ b/packages/gen-ai-hub/src/utils/deployment-cache.ts @@ -1,5 +1,6 @@ import { Cache } from '@sap-cloud-sdk/connectivity/internal.js'; import { type AiDeployment } from '@sap-ai-sdk/ai-core'; +import { unique } from '@sap-cloud-sdk/util'; import { type DeploymentResolutionOptions } from './deployment-resolver.js'; import { extractModel, type FoundationModel } from './model.js'; @@ -54,29 +55,69 @@ function createDeploymentCache(cache: Cache) { opts: DeploymentResolutionOptions, deployments: AiDeployment[] ): void => { - // go backwards to cache the first deployment ID for each model - [...deployments].reverse().forEach(deployment => { - const model = extractModel(deployment); - const entry = transformDeploymentForCache(deployment); - cache.set(getCacheKey({ ...opts, model }), { + const cacheEntries = getUniqueCacheEntries( + [...deployments].map(deployment => + transformDeploymentForCache(deployment) + ) + ); + + [ + ...cacheEntries, + ...getCacheEntriesForModelsWithoutVersion(cacheEntries), + ...getCacheEntriesForNoModel(cacheEntries) + ].forEach(entry => { + cache.set(getCacheKey({ ...opts, model: entry.model }), { entry }); - - if (model) { - cache.set(getCacheKey({ ...opts, model: undefined }), { entry }); - if (model.version) { - cache.set( - getCacheKey({ ...opts, model: { ...model, version: undefined } }), - { entry } - ); - } - } }); }, clear: () => cache.clear() }; } +function getUniqueCacheEntries(cacheEntries: Deployment[]): Deployment[] { + const modelMap: Record = {}; + const modelNames = unique(cacheEntries.map(({ model }) => model?.name ?? '')); + modelNames.forEach(modelName => { + modelMap[modelName] = unique( + cacheEntries.map(({ model }) => model?.version ?? '') + ); + }); + + return modelNames.flatMap(modelName => + modelMap[modelName].map( + modelVersion => + cacheEntries.find( + entry => + entry.model?.name === modelName && + entry.model?.version === modelVersion + )! + ) + ); +} + +function getCacheEntriesForModelsWithoutVersion( + cacheEntries: Deployment[] +): Deployment[] { + const modelNames = unique( + cacheEntries + .filter(({ model }) => model?.name && model.version) + .map(({ model }) => model?.name as string) + ); + + return modelNames.map(modelName => ({ + // cannot be undefined + id: cacheEntries.find(({ model }) => model?.name === modelName)!.id, + model: { name: modelName, version: undefined } + })); +} + +function getCacheEntriesForNoModel(cacheEntries: Deployment[]): Deployment[] { + return cacheEntries[0]?.model + ? [{ ...cacheEntries[0], model: undefined }] + : []; +} + function transformDeploymentForCache(deployment: AiDeployment): Deployment { return { id: deployment.id, From bb6d739143acad0253538939f0c49523252645bd Mon Sep 17 00:00:00 2001 From: Marika Marszalkowski Date: Tue, 3 Sep 2024 19:12:26 +0200 Subject: [PATCH 3/7] Revert "implement caching without full model explicitly" This reverts commit 37eaadd614448bcbaaee909dda48a1032c89f0d4. --- .../gen-ai-hub/src/utils/deployment-cache.ts | 71 ++++--------------- 1 file changed, 15 insertions(+), 56 deletions(-) diff --git a/packages/gen-ai-hub/src/utils/deployment-cache.ts b/packages/gen-ai-hub/src/utils/deployment-cache.ts index e928a056..b1f85bef 100644 --- a/packages/gen-ai-hub/src/utils/deployment-cache.ts +++ b/packages/gen-ai-hub/src/utils/deployment-cache.ts @@ -1,6 +1,5 @@ import { Cache } from '@sap-cloud-sdk/connectivity/internal.js'; import { type AiDeployment } from '@sap-ai-sdk/ai-core'; -import { unique } from '@sap-cloud-sdk/util'; import { type DeploymentResolutionOptions } from './deployment-resolver.js'; import { extractModel, type FoundationModel } from './model.js'; @@ -55,69 +54,29 @@ function createDeploymentCache(cache: Cache) { opts: DeploymentResolutionOptions, deployments: AiDeployment[] ): void => { - const cacheEntries = getUniqueCacheEntries( - [...deployments].map(deployment => - transformDeploymentForCache(deployment) - ) - ); - - [ - ...cacheEntries, - ...getCacheEntriesForModelsWithoutVersion(cacheEntries), - ...getCacheEntriesForNoModel(cacheEntries) - ].forEach(entry => { - cache.set(getCacheKey({ ...opts, model: entry.model }), { + // go backwards to cache the first deployment ID for each model + [...deployments].reverse().forEach(deployment => { + const model = extractModel(deployment); + const entry = transformDeploymentForCache(deployment); + cache.set(getCacheKey({ ...opts, model }), { entry }); + + if (model) { + cache.set(getCacheKey({ ...opts, model: undefined }), { entry }); + if (model.version) { + cache.set( + getCacheKey({ ...opts, model: { ...model, version: undefined } }), + { entry } + ); + } + } }); }, clear: () => cache.clear() }; } -function getUniqueCacheEntries(cacheEntries: Deployment[]): Deployment[] { - const modelMap: Record = {}; - const modelNames = unique(cacheEntries.map(({ model }) => model?.name ?? '')); - modelNames.forEach(modelName => { - modelMap[modelName] = unique( - cacheEntries.map(({ model }) => model?.version ?? '') - ); - }); - - return modelNames.flatMap(modelName => - modelMap[modelName].map( - modelVersion => - cacheEntries.find( - entry => - entry.model?.name === modelName && - entry.model?.version === modelVersion - )! - ) - ); -} - -function getCacheEntriesForModelsWithoutVersion( - cacheEntries: Deployment[] -): Deployment[] { - const modelNames = unique( - cacheEntries - .filter(({ model }) => model?.name && model.version) - .map(({ model }) => model?.name as string) - ); - - return modelNames.map(modelName => ({ - // cannot be undefined - id: cacheEntries.find(({ model }) => model?.name === modelName)!.id, - model: { name: modelName, version: undefined } - })); -} - -function getCacheEntriesForNoModel(cacheEntries: Deployment[]): Deployment[] { - return cacheEntries[0]?.model - ? [{ ...cacheEntries[0], model: undefined }] - : []; -} - function transformDeploymentForCache(deployment: AiDeployment): Deployment { return { id: deployment.id, From f3ff3567c2f47c3313bc119670b51fffcf8fb32d Mon Sep 17 00:00:00 2001 From: Marika Marszalkowski Date: Tue, 3 Sep 2024 19:30:58 +0200 Subject: [PATCH 4/7] refactor --- .../gen-ai-hub/src/utils/deployment-cache.ts | 32 +++++++++++-------- 1 file changed, 18 insertions(+), 14 deletions(-) diff --git a/packages/gen-ai-hub/src/utils/deployment-cache.ts b/packages/gen-ai-hub/src/utils/deployment-cache.ts index b1f85bef..d25e6671 100644 --- a/packages/gen-ai-hub/src/utils/deployment-cache.ts +++ b/packages/gen-ai-hub/src/utils/deployment-cache.ts @@ -55,22 +55,26 @@ function createDeploymentCache(cache: Cache) { deployments: AiDeployment[] ): void => { // go backwards to cache the first deployment ID for each model - [...deployments].reverse().forEach(deployment => { - const model = extractModel(deployment); - const entry = transformDeploymentForCache(deployment); - cache.set(getCacheKey({ ...opts, model }), { - entry + const cacheEntries = [...deployments] + .reverse() + .map(deployment => transformDeploymentForCache(deployment)) + .flatMap(entry => { + const entries = [entry]; + // if the entry has a model, also cache it without model and version + if (entry.model) { + entries.push({ id: entry.id }); + } + // if the entry has a model version, also cache it for without model version + if (entry.model?.version) { + entries.push({ id: entry.id, model: { name: entry.model.name } }); + } + return entries; }); - if (model) { - cache.set(getCacheKey({ ...opts, model: undefined }), { entry }); - if (model.version) { - cache.set( - getCacheKey({ ...opts, model: { ...model, version: undefined } }), - { entry } - ); - } - } + cacheEntries.forEach(entry => { + cache.set(getCacheKey({ ...opts, model: entry.model }), { + entry + }); }); }, clear: () => cache.clear() From 1efc5f35c87106685526efe10d52de615004511a Mon Sep 17 00:00:00 2001 From: Matthias Kuhr Date: Wed, 4 Sep 2024 10:03:30 +0200 Subject: [PATCH 5/7] Shorten --- packages/gen-ai-hub/src/utils/deployment-cache.ts | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/packages/gen-ai-hub/src/utils/deployment-cache.ts b/packages/gen-ai-hub/src/utils/deployment-cache.ts index d25e6671..833d1428 100644 --- a/packages/gen-ai-hub/src/utils/deployment-cache.ts +++ b/packages/gen-ai-hub/src/utils/deployment-cache.ts @@ -55,7 +55,7 @@ function createDeploymentCache(cache: Cache) { deployments: AiDeployment[] ): void => { // go backwards to cache the first deployment ID for each model - const cacheEntries = [...deployments] + [...deployments] .reverse() .map(deployment => transformDeploymentForCache(deployment)) .flatMap(entry => { @@ -69,13 +69,12 @@ function createDeploymentCache(cache: Cache) { entries.push({ id: entry.id, model: { name: entry.model.name } }); } return entries; + }) + .forEach(entry => { + cache.set(getCacheKey({ ...opts, model: entry.model }), { + entry + }); }); - - cacheEntries.forEach(entry => { - cache.set(getCacheKey({ ...opts, model: entry.model }), { - entry - }); - }); }, clear: () => cache.clear() }; From 2b0ccd4c4daeff05e3d1f0754d6baaabb989b843 Mon Sep 17 00:00:00 2001 From: Marika Marszalkowski Date: Wed, 4 Sep 2024 10:08:26 +0200 Subject: [PATCH 6/7] schlimmprovement? --- .../gen-ai-hub/src/utils/deployment-cache.ts | 19 +++++++------------ 1 file changed, 7 insertions(+), 12 deletions(-) diff --git a/packages/gen-ai-hub/src/utils/deployment-cache.ts b/packages/gen-ai-hub/src/utils/deployment-cache.ts index d25e6671..d4d37f46 100644 --- a/packages/gen-ai-hub/src/utils/deployment-cache.ts +++ b/packages/gen-ai-hub/src/utils/deployment-cache.ts @@ -58,18 +58,13 @@ function createDeploymentCache(cache: Cache) { const cacheEntries = [...deployments] .reverse() .map(deployment => transformDeploymentForCache(deployment)) - .flatMap(entry => { - const entries = [entry]; - // if the entry has a model, also cache it without model and version - if (entry.model) { - entries.push({ id: entry.id }); - } - // if the entry has a model version, also cache it for without model version - if (entry.model?.version) { - entries.push({ id: entry.id, model: { name: entry.model.name } }); - } - return entries; - }); + .flatMap(entry => [ + entry, + { id: entry.id }, + ...(entry.model + ? [{ id: entry.id, model: { name: entry.model.name } }] + : []) + ]); cacheEntries.forEach(entry => { cache.set(getCacheKey({ ...opts, model: entry.model }), { From 9f0e7699fdca7af8e589d338ab34d1a053173c92 Mon Sep 17 00:00:00 2001 From: cloud-sdk-js Date: Wed, 4 Sep 2024 08:10:53 +0000 Subject: [PATCH 7/7] fix: Changes from lint --- packages/gen-ai-hub/src/utils/deployment-cache.ts | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/packages/gen-ai-hub/src/utils/deployment-cache.ts b/packages/gen-ai-hub/src/utils/deployment-cache.ts index 58bd0566..324ca97a 100644 --- a/packages/gen-ai-hub/src/utils/deployment-cache.ts +++ b/packages/gen-ai-hub/src/utils/deployment-cache.ts @@ -64,11 +64,12 @@ function createDeploymentCache(cache: Cache) { ...(entry.model ? [{ id: entry.id, model: { name: entry.model.name } }] : []) - ]).forEach(entry => { - cache.set(getCacheKey({ ...opts, model: entry.model }), { - entry + ]) + .forEach(entry => { + cache.set(getCacheKey({ ...opts, model: entry.model }), { + entry + }); }); - }); }, clear: () => cache.clear() };