From a6621c988f3dd39700e2de459606f46ea7277a1b Mon Sep 17 00:00:00 2001 From: Pushpa Padti <99261071+ppadti@users.noreply.github.com> Date: Wed, 21 Aug 2024 00:55:46 +0530 Subject: [PATCH 01/14] Update dockerfile to use non numeric user id (#3088) --- Dockerfile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Dockerfile b/Dockerfile index f620e2c11c..a42412cbf0 100644 --- a/Dockerfile +++ b/Dockerfile @@ -29,7 +29,7 @@ WORKDIR /usr/src/app RUN mkdir /usr/src/app/logs && chmod 775 /usr/src/app/logs -USER default +USER 1001:0 COPY --chown=default:root --from=builder /usr/src/app/frontend/public /usr/src/app/frontend/public COPY --chown=default:root --from=builder /usr/src/app/backend/package.json /usr/src/app/backend/package.json From ac4aac62e5920b290b137e703d534f6471c23da9 Mon Sep 17 00:00:00 2001 From: jpuzz0 <96431149+jpuzz0@users.noreply.github.com> Date: Wed, 21 Aug 2024 10:59:55 -0400 Subject: [PATCH 02/14] [RHOAIENG-1104] Design custom annotation type to track storage class metadata (#3103) --- frontend/src/k8sTypes.ts | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/frontend/src/k8sTypes.ts b/frontend/src/k8sTypes.ts index 47afa85081..06f79c5d4d 100644 --- a/frontend/src/k8sTypes.ts +++ b/frontend/src/k8sTypes.ts @@ -47,12 +47,21 @@ export type DisplayNameAnnotations = Partial<{ 'openshift.io/display-name': string; // the name provided by the user }>; +export type StorageClassConfig = { + displayName: string; + isEnabled: boolean; + isDefault: boolean; + lastModified: string; + description?: string; +}; + type StorageClassAnnotations = Partial<{ // if true, enables any persistent volume claim (PVC) that does not specify a specific storage class to automatically be provisioned. // Only one, if any, StorageClass per cluster can be set as default. 'storageclass.kubernetes.io/is-default-class': 'true' | 'false'; // the description provided by the cluster admin or Container Storage Interface (CSI) provider 'kubernetes.io/description': string; + 'opendatahub.io/sc-config': StorageClassConfig; }>; export type K8sDSGResource = K8sResourceCommon & { From 626ee608c4c1eb63785a2876252f92f09b46aa79 Mon Sep 17 00:00:00 2001 From: Gage Krumbach Date: Wed, 21 Aug 2024 14:32:08 -0500 Subject: [PATCH 03/14] Refactor accelerator profile state hooks to no longer manage its own selected (#3101) * Refactor accelerator profile state hooks to no longer manage its own selected split init and selected accelerator feat: Add selected accelerator profile to assemblePodSpecOptions function * remove comments * Refactor accelerator profile state hooks to no longer manage its own selected * feat: Update acceleratorProfile in notebookServer.cy.ts The `acceleratorProfile` object in `notebookServer.cy.ts` has been modified to include a new property `unknownProfileDetected`. This change ensures that the `acceleratorProfile` state is accurately represented. --- .../src/__mocks__/mockStartNotebookData.ts | 10 +- .../mocked/applications/notebookServer.cy.ts | 2 +- .../k8s/__tests__/inferenceServices.spec.ts | 67 ++++-- .../api/k8s/__tests__/servingRuntimes.spec.ts | 27 ++- frontend/src/api/k8s/__tests__/utils.spec.ts | 195 ++++++++++++++--- frontend/src/api/k8s/inferenceServices.ts | 22 +- frontend/src/api/k8s/notebooks.ts | 9 +- frontend/src/api/k8s/servingRuntimes.ts | 30 ++- frontend/src/api/k8s/utils.ts | 29 +-- .../ServingRuntimeDetails.tsx | 6 +- .../ManageServingRuntimeModal.tsx | 30 ++- .../ServingRuntimeSizeSection.tsx | 13 +- .../ServingRuntimeTemplateSection.tsx | 8 +- .../kServeModal/ManageKServeModal.tsx | 29 ++- .../projects/useServingAcceleratorProfile.ts | 3 +- .../modelServing/screens/projects/utils.ts | 37 ++-- frontend/src/pages/modelServing/utils.ts | 31 ++- .../server/AcceleratorProfileSelectField.tsx | 96 ++++++--- .../screens/server/NotebookServerDetails.tsx | 6 +- .../screens/server/SpawnerPage.tsx | 24 ++- .../notebook/NotebookStatusToggle.tsx | 8 +- .../useNotebookAcceleratorProfile.ts | 3 +- .../screens/spawner/SpawnerFooter.tsx | 13 +- .../projects/screens/spawner/SpawnerPage.tsx | 21 +- frontend/src/pages/projects/types.ts | 4 +- frontend/src/utilities/tolerations.ts | 13 +- .../utilities/useAcceleratorProfileState.ts | 201 +++++++++--------- 27 files changed, 619 insertions(+), 318 deletions(-) diff --git a/frontend/src/__mocks__/mockStartNotebookData.ts b/frontend/src/__mocks__/mockStartNotebookData.ts index a235c14b41..c13a867eba 100644 --- a/frontend/src/__mocks__/mockStartNotebookData.ts +++ b/frontend/src/__mocks__/mockStartNotebookData.ts @@ -36,12 +36,16 @@ export const mockStartNotebookData = ({ }, }, }, - acceleratorProfile: { + initialAcceleratorProfile: { acceleratorProfile: undefined, acceleratorProfiles: [], - initialAcceleratorProfile: undefined, count: 0, - useExisting: false, + unknownProfileDetected: false, + }, + selectedAcceleratorProfile: { + profile: undefined, + count: 0, + useExistingSettings: false, }, volumes: [ { diff --git a/frontend/src/__tests__/cypress/cypress/tests/mocked/applications/notebookServer.cy.ts b/frontend/src/__tests__/cypress/cypress/tests/mocked/applications/notebookServer.cy.ts index bec4551e34..f2feb02db1 100644 --- a/frontend/src/__tests__/cypress/cypress/tests/mocked/applications/notebookServer.cy.ts +++ b/frontend/src/__tests__/cypress/cypress/tests/mocked/applications/notebookServer.cy.ts @@ -61,7 +61,7 @@ describe('NotebookServer', () => { notebookSizeName: 'XSmall', imageName: 'code-server-notebook', imageTagName: '2023.2', - acceleratorProfile: { acceleratorProfiles: [], count: 0, useExisting: false }, + acceleratorProfile: { acceleratorProfiles: [], count: 0, unknownProfileDetected: false }, envVars: { configMap: {}, secrets: {} }, state: 'started', }); diff --git a/frontend/src/api/k8s/__tests__/inferenceServices.spec.ts b/frontend/src/api/k8s/__tests__/inferenceServices.spec.ts index 34c7efc9b8..0dda0c16a9 100644 --- a/frontend/src/api/k8s/__tests__/inferenceServices.spec.ts +++ b/frontend/src/api/k8s/__tests__/inferenceServices.spec.ts @@ -28,6 +28,7 @@ import { InferenceServiceKind, ProjectKind } from '~/k8sTypes'; import { translateDisplayNameForK8s } from '~/concepts/k8s/utils'; import { ModelServingSize } from '~/pages/modelServing/screens/types'; import { AcceleratorProfileState } from '~/utilities/useAcceleratorProfileState'; +import { AcceleratorProfileSelectFieldState } from '~/pages/notebookController/screens/server/AcceleratorProfileSelectField'; jest.mock('@openshift/dynamic-plugin-sdk-utils', () => ({ k8sListResource: jest.fn(), @@ -169,10 +170,14 @@ describe('assembleInferenceService', () => { const acceleratorProfileState: AcceleratorProfileState = { acceleratorProfile: mockAcceleratorProfile({}), acceleratorProfiles: [mockAcceleratorProfile({})], - initialAcceleratorProfile: mockAcceleratorProfile({}), count: 1, - additionalOptions: {}, - useExisting: false, + unknownProfileDetected: false, + }; + + const selectedAcceleratorProfile: AcceleratorProfileSelectFieldState = { + profile: mockAcceleratorProfile({}), + count: 1, + useExistingSettings: false, }; const inferenceService = assembleInferenceService( @@ -182,6 +187,7 @@ describe('assembleInferenceService', () => { false, undefined, acceleratorProfileState, + selectedAcceleratorProfile, ); expect(inferenceService.spec.predictor.tolerations).toBeDefined(); @@ -196,10 +202,14 @@ describe('assembleInferenceService', () => { const acceleratorProfileState: AcceleratorProfileState = { acceleratorProfile: mockAcceleratorProfile({}), acceleratorProfiles: [mockAcceleratorProfile({})], - initialAcceleratorProfile: mockAcceleratorProfile({}), count: 1, - additionalOptions: {}, - useExisting: false, + unknownProfileDetected: false, + }; + + const selectedAcceleratorProfile: AcceleratorProfileSelectFieldState = { + profile: mockAcceleratorProfile({}), + count: 1, + useExistingSettings: false, }; const inferenceService = assembleInferenceService( @@ -209,6 +219,7 @@ describe('assembleInferenceService', () => { true, undefined, acceleratorProfileState, + selectedAcceleratorProfile, ); expect(inferenceService.spec.predictor.tolerations).toBeUndefined(); @@ -221,10 +232,14 @@ describe('assembleInferenceService', () => { const acceleratorProfileState: AcceleratorProfileState = { acceleratorProfile: mockAcceleratorProfile({}), acceleratorProfiles: [mockAcceleratorProfile({})], - initialAcceleratorProfile: mockAcceleratorProfile({}), count: 1, - additionalOptions: {}, - useExisting: false, + unknownProfileDetected: false, + }; + + const selectedAcceleratorProfile: AcceleratorProfileSelectFieldState = { + profile: mockAcceleratorProfile({}), + count: 1, + useExistingSettings: false, }; const inferenceService = assembleInferenceService( @@ -237,6 +252,7 @@ describe('assembleInferenceService', () => { false, undefined, acceleratorProfileState, + selectedAcceleratorProfile, ); expect(inferenceService.spec.predictor.maxReplicas).toBe(replicaCount); @@ -247,10 +263,14 @@ describe('assembleInferenceService', () => { const acceleratorProfileState: AcceleratorProfileState = { acceleratorProfile: mockAcceleratorProfile({}), acceleratorProfiles: [mockAcceleratorProfile({})], - initialAcceleratorProfile: mockAcceleratorProfile({}), count: 1, - additionalOptions: {}, - useExisting: false, + unknownProfileDetected: false, + }; + + const selectedAcceleratorProfile: AcceleratorProfileSelectFieldState = { + profile: mockAcceleratorProfile({}), + count: 1, + useExistingSettings: false, }; const inferenceService = assembleInferenceService( @@ -260,6 +280,7 @@ describe('assembleInferenceService', () => { true, undefined, acceleratorProfileState, + selectedAcceleratorProfile, ); expect(inferenceService.spec.predictor.maxReplicas).toBeUndefined(); @@ -270,10 +291,14 @@ describe('assembleInferenceService', () => { const acceleratorProfileState: AcceleratorProfileState = { acceleratorProfile: mockAcceleratorProfile({}), acceleratorProfiles: [mockAcceleratorProfile({})], - initialAcceleratorProfile: mockAcceleratorProfile({}), count: 1, - additionalOptions: {}, - useExisting: false, + unknownProfileDetected: false, + }; + + const selectedAcceleratorProfile: AcceleratorProfileSelectFieldState = { + profile: mockAcceleratorProfile({}), + count: 1, + useExistingSettings: false, }; const modelSize: ModelServingSize = { @@ -297,6 +322,7 @@ describe('assembleInferenceService', () => { false, undefined, acceleratorProfileState, + selectedAcceleratorProfile, ); expect(inferenceService.spec.predictor.model?.resources?.requests?.cpu).toBe( @@ -317,10 +343,14 @@ describe('assembleInferenceService', () => { const acceleratorProfileState: AcceleratorProfileState = { acceleratorProfile: mockAcceleratorProfile({}), acceleratorProfiles: [mockAcceleratorProfile({})], - initialAcceleratorProfile: mockAcceleratorProfile({}), count: 1, - additionalOptions: {}, - useExisting: false, + unknownProfileDetected: false, + }; + + const selectedAcceleratorProfile: AcceleratorProfileSelectFieldState = { + profile: mockAcceleratorProfile({}), + count: 1, + useExistingSettings: false, }; const modelSize: ModelServingSize = { @@ -344,6 +374,7 @@ describe('assembleInferenceService', () => { true, undefined, acceleratorProfileState, + selectedAcceleratorProfile, ); expect(inferenceService.spec.predictor.model?.resources).toBeUndefined(); diff --git a/frontend/src/api/k8s/__tests__/servingRuntimes.spec.ts b/frontend/src/api/k8s/__tests__/servingRuntimes.spec.ts index e9fb706e01..63afedbd0b 100644 --- a/frontend/src/api/k8s/__tests__/servingRuntimes.spec.ts +++ b/frontend/src/api/k8s/__tests__/servingRuntimes.spec.ts @@ -25,6 +25,7 @@ import { } from '~/api/k8s/servingRuntimes'; import { ProjectModel, ServingRuntimeModel } from '~/api/models'; import { ProjectKind, ServingRuntimeKind } from '~/k8sTypes'; +import { AcceleratorProfileSelectFieldState } from '~/pages/notebookController/screens/server/AcceleratorProfileSelectField'; import { AcceleratorProfileState } from '~/utilities/useAcceleratorProfileState'; global.structuredClone = (val: unknown) => JSON.parse(JSON.stringify(val)); @@ -115,10 +116,14 @@ describe('assembleServingRuntime', () => { const acceleratorProfileState: AcceleratorProfileState = { acceleratorProfile: mockAcceleratorProfile({}), acceleratorProfiles: [mockAcceleratorProfile({})], - initialAcceleratorProfile: mockAcceleratorProfile({}), count: 1, - additionalOptions: {}, - useExisting: false, + unknownProfileDetected: false, + }; + + const selectedAcceleratorProfile: AcceleratorProfileSelectFieldState = { + profile: mockAcceleratorProfile({}), + count: 1, + useExistingSettings: false, }; const servingRuntime = assembleServingRuntime( @@ -131,6 +136,7 @@ describe('assembleServingRuntime', () => { true, false, acceleratorProfileState, + selectedAcceleratorProfile, true, ); @@ -143,10 +149,14 @@ describe('assembleServingRuntime', () => { const acceleratorProfileState: AcceleratorProfileState = { acceleratorProfile: mockAcceleratorProfile({}), acceleratorProfiles: [mockAcceleratorProfile({})], - initialAcceleratorProfile: mockAcceleratorProfile({}), count: 1, - additionalOptions: {}, - useExisting: false, + unknownProfileDetected: false, + }; + + const selectedAcceleratorProfile: AcceleratorProfileSelectFieldState = { + profile: mockAcceleratorProfile({}), + count: 1, + useExistingSettings: false, }; const servingRuntime = assembleServingRuntime( @@ -159,6 +169,7 @@ describe('assembleServingRuntime', () => { true, false, acceleratorProfileState, + selectedAcceleratorProfile, false, ); @@ -182,6 +193,7 @@ describe('assembleServingRuntime', () => { true, false, undefined, + undefined, true, ); @@ -199,6 +211,7 @@ describe('assembleServingRuntime', () => { true, false, undefined, + undefined, false, ); @@ -426,6 +439,7 @@ describe('updateServingRuntime', () => { true, false, undefined, + undefined, false, ); it('should update serving runtimes when isCustomServingRuntimesEnabled is false', async () => { @@ -496,6 +510,7 @@ describe('createServingRuntime', () => { true, false, undefined, + undefined, false, ); it('should create serving runtimes when isCustomServingRuntimesEnabled is false', async () => { diff --git a/frontend/src/api/k8s/__tests__/utils.spec.ts b/frontend/src/api/k8s/__tests__/utils.spec.ts index 42b81ef713..f43e84cb48 100644 --- a/frontend/src/api/k8s/__tests__/utils.spec.ts +++ b/frontend/src/api/k8s/__tests__/utils.spec.ts @@ -2,6 +2,7 @@ import { mockAcceleratorProfile } from '~/__mocks__/mockAcceleratorProfile'; import { AcceleratorProfileState } from '~/utilities/useAcceleratorProfileState'; import { assemblePodSpecOptions, getshmVolume, getshmVolumeMount } from '~/api/k8s/utils'; import { ContainerResources, TolerationEffect, TolerationOperator } from '~/types'; +import { AcceleratorProfileSelectFieldState } from '~/pages/notebookController/screens/server/AcceleratorProfileSelectField'; global.structuredClone = (val: unknown) => JSON.parse(JSON.stringify(val)); @@ -12,23 +13,30 @@ describe('assemblePodSpecOptions', () => { }; const tolerationsMock = [ { - effect: 'NoSchedule', + effect: TolerationEffect.NO_SCHEDULE, key: 'nvidia.com/gpu', - operator: 'Exists', + operator: TolerationOperator.EXISTS, }, ]; - it('should assemble pod spec options correctly when resourceSetting and acceleratorProfileState is given', () => { + it('should assemble pod spec options correctly when a accelerator profile is selected', () => { const acceleratorProfileMock = mockAcceleratorProfile({}); const acceleratorProfileState: AcceleratorProfileState = { - acceleratorProfile: acceleratorProfileMock, + acceleratorProfile: undefined, acceleratorProfiles: [acceleratorProfileMock], - initialAcceleratorProfile: acceleratorProfileMock, + count: 0, + unknownProfileDetected: false, + }; + const selectedAcceleratorProfile: AcceleratorProfileSelectFieldState = { + profile: acceleratorProfileMock, count: 1, - additionalOptions: {}, - useExisting: false, + useExistingSettings: false, }; - const result = assemblePodSpecOptions(resourceSettings, acceleratorProfileState); + const result = assemblePodSpecOptions( + resourceSettings, + acceleratorProfileState, + selectedAcceleratorProfile, + ); expect(result).toStrictEqual({ affinity: {}, tolerations: tolerationsMock, @@ -47,38 +55,166 @@ describe('assemblePodSpecOptions', () => { }); }); - it('should assemble pod spec correctly with useExisting parameter set as false and affinitySettings given ', () => { + it('should assemble pod spec options correctly when there is an initial accelerator profile and it is unselected', () => { const acceleratorProfileMock = mockAcceleratorProfile({}); - const affinitySettings = { nodeAffinity: { key: 'value' } }; const acceleratorProfileState: AcceleratorProfileState = { acceleratorProfile: acceleratorProfileMock, acceleratorProfiles: [acceleratorProfileMock], - initialAcceleratorProfile: acceleratorProfileMock, count: 1, - additionalOptions: { useExisting: true }, - useExisting: false, + unknownProfileDetected: false, + }; + const selectedAcceleratorProfile: AcceleratorProfileSelectFieldState = { + profile: undefined, + count: 0, + useExistingSettings: false, + }; + const result = assemblePodSpecOptions( + resourceSettings, + acceleratorProfileState, + selectedAcceleratorProfile, + undefined, + tolerationsMock, + undefined, + { + limits: { + [acceleratorProfileMock.spec.identifier]: acceleratorProfileState.count, + cpu: '1', + memory: '1Gi', + }, + requests: { + [acceleratorProfileMock.spec.identifier]: acceleratorProfileState.count, + cpu: '0.5', + memory: '500Mi', + }, + }, + ); + expect(result).toStrictEqual({ + affinity: {}, + tolerations: [], + resources: { + limits: { + cpu: '1', + memory: '1Gi', + }, + requests: { + cpu: '0.5', + memory: '500Mi', + }, + }, + }); + }); + + it('should assemble pod spec options correctly when there is an initial accelerator profile and another is selected', () => { + const acceleratorProfileMock = mockAcceleratorProfile({}); + const acceleratorProfileMockInitial = mockAcceleratorProfile({ + identifier: 'amd.com/gpu', + }); + + const acceleratorProfileState: AcceleratorProfileState = { + acceleratorProfile: acceleratorProfileMockInitial, + acceleratorProfiles: [acceleratorProfileMock, acceleratorProfileMockInitial], + count: 1, + unknownProfileDetected: false, + }; + const selectedAcceleratorProfile: AcceleratorProfileSelectFieldState = { + profile: acceleratorProfileMock, + count: 1, + useExistingSettings: false, + }; + const result = assemblePodSpecOptions( + resourceSettings, + acceleratorProfileState, + selectedAcceleratorProfile, + undefined, + tolerationsMock, + undefined, + { + limits: { + [acceleratorProfileMockInitial.spec.identifier]: acceleratorProfileState.count, + cpu: '1', + memory: '1Gi', + }, + requests: { + [acceleratorProfileMockInitial.spec.identifier]: acceleratorProfileState.count, + cpu: '0.5', + memory: '500Mi', + }, + }, + ); + expect(result).toStrictEqual({ + affinity: {}, + tolerations: tolerationsMock, + resources: { + limits: { + [acceleratorProfileMock.spec.identifier]: selectedAcceleratorProfile.count, + cpu: '1', + memory: '1Gi', + }, + requests: { + [acceleratorProfileMock.spec.identifier]: selectedAcceleratorProfile.count, + cpu: '0.5', + memory: '500Mi', + }, + }, + }); + }); + + it('should assemble pod spec correctly with an unknown accelerator detected and another accelerator selected and affinitySettings given ', () => { + const acceleratorProfileMock = mockAcceleratorProfile({}); + const affinitySettings = { nodeAffinity: { key: 'value' } }; + const acceleratorProfileState: AcceleratorProfileState = { + acceleratorProfile: undefined, + acceleratorProfiles: [acceleratorProfileMock], + count: 1, + unknownProfileDetected: true, + }; + const selectedAcceleratorProfile: AcceleratorProfileSelectFieldState = { + profile: acceleratorProfileMock, + count: 1, + useExistingSettings: false, }; const existingResources = { - limits: { cpu: '0', memory: '0Gi' }, - requests: { cpu: '0.5', memory: '500Mi' }, + limits: { cpu: '0', memory: '0Gi', 'amd.com/gpu': 1 }, + requests: { cpu: '0.5', memory: '500Mi', 'amd.com/gpu': 1 }, }; + const existingTolerations = [ + { + effect: TolerationEffect.NO_SCHEDULE, + key: 'amd.com/gpu', + operator: TolerationOperator.EXISTS, + }, + ]; + const result = assemblePodSpecOptions( resourceSettings, acceleratorProfileState, + selectedAcceleratorProfile, undefined, - undefined, + existingTolerations, affinitySettings, existingResources, ); + expect(result).toStrictEqual({ affinity: { nodeAffinity: { key: 'value', }, }, - tolerations: tolerationsMock, + tolerations: [ + { + effect: TolerationEffect.NO_SCHEDULE, + key: 'amd.com/gpu', + operator: TolerationOperator.EXISTS, + }, + { + effect: TolerationEffect.NO_SCHEDULE, + key: 'nvidia.com/gpu', + operator: TolerationOperator.EXISTS, + }, + ], resources: { limits: { cpu: '1', memory: '1Gi', 'nvidia.com/gpu': 1 }, requests: { cpu: '0.5', memory: '500Mi', 'nvidia.com/gpu': 1 }, @@ -99,15 +235,19 @@ describe('assemblePodSpecOptions', () => { const acceleratorProfileState: AcceleratorProfileState = { acceleratorProfile: acceleratorProfileMock, acceleratorProfiles: [acceleratorProfileMock], - initialAcceleratorProfile: acceleratorProfileMock, count: 1, - additionalOptions: {}, - useExisting: true, + unknownProfileDetected: false, + }; + const selectedAcceleratorProfile: AcceleratorProfileSelectFieldState = { + profile: acceleratorProfileMock, + count: 1, + useExistingSettings: false, }; const result = assemblePodSpecOptions( resourceSettings, acceleratorProfileState, + selectedAcceleratorProfile, tolerationSettings, existingTolerations, ); @@ -135,12 +275,19 @@ describe('assemblePodSpecOptions', () => { const acceleratorProfileState: AcceleratorProfileState = { acceleratorProfile: acceleratorProfileMock, acceleratorProfiles: [acceleratorProfileMock], - initialAcceleratorProfile: acceleratorProfileMock, count: 1, - additionalOptions: { useExisting: true }, - useExisting: false, + unknownProfileDetected: false, + }; + const selectedAcceleratorProfile: AcceleratorProfileSelectFieldState = { + profile: acceleratorProfileMock, + count: 1, + useExistingSettings: false, }; - const result = assemblePodSpecOptions(resourceSettingMock, acceleratorProfileState); + const result = assemblePodSpecOptions( + resourceSettingMock, + acceleratorProfileState, + selectedAcceleratorProfile, + ); expect(result).toStrictEqual({ affinity: {}, resources: {}, diff --git a/frontend/src/api/k8s/inferenceServices.ts b/frontend/src/api/k8s/inferenceServices.ts index b4512d4fac..028688eda7 100644 --- a/frontend/src/api/k8s/inferenceServices.ts +++ b/frontend/src/api/k8s/inferenceServices.ts @@ -14,6 +14,7 @@ import { translateDisplayNameForK8s } from '~/concepts/k8s/utils'; import { applyK8sAPIOptions } from '~/api/apiMergeUtils'; import { AcceleratorProfileState } from '~/utilities/useAcceleratorProfileState'; import { ContainerResources } from '~/types'; +import { AcceleratorProfileSelectFieldState } from '~/pages/notebookController/screens/server/AcceleratorProfileSelectField'; import { getModelServingProjects } from './projects'; import { assemblePodSpecOptions } from './utils'; @@ -23,7 +24,8 @@ export const assembleInferenceService = ( editName?: string, isModelMesh?: boolean, inferenceService?: InferenceServiceKind, - acceleratorState?: AcceleratorProfileState, + initialAcceleratorProfile?: AcceleratorProfileState, + selectedAcceleratorProfile?: AcceleratorProfileSelectFieldState, ): InferenceServiceKind => { const { storage, @@ -144,7 +146,11 @@ export const assembleInferenceService = ( }, }; - const { tolerations, resources } = assemblePodSpecOptions(resourceSettings, acceleratorState); + const { tolerations, resources } = assemblePodSpecOptions( + resourceSettings, + initialAcceleratorProfile, + selectedAcceleratorProfile, + ); if (tolerations.length !== 0) { updateInferenceService.spec.predictor.tolerations = tolerations; @@ -225,7 +231,8 @@ export const createInferenceService = ( data: CreatingInferenceServiceObject, secretKey?: string, isModelMesh?: boolean, - acceleratorState?: AcceleratorProfileState, + initialAcceleratorProfile?: AcceleratorProfileState, + selectedAcceleratorProfile?: AcceleratorProfileSelectFieldState, dryRun = false, ): Promise => { const inferenceService = assembleInferenceService( @@ -234,7 +241,8 @@ export const createInferenceService = ( undefined, isModelMesh, undefined, - acceleratorState, + initialAcceleratorProfile, + selectedAcceleratorProfile, ); return k8sCreateResource( applyK8sAPIOptions( @@ -252,7 +260,8 @@ export const updateInferenceService = ( existingData: InferenceServiceKind, secretKey?: string, isModelMesh?: boolean, - acceleratorState?: AcceleratorProfileState, + initialAcceleratorProfile?: AcceleratorProfileState, + selectedAcceleratorProfile?: AcceleratorProfileSelectFieldState, dryRun = false, ): Promise => { const inferenceService = assembleInferenceService( @@ -261,7 +270,8 @@ export const updateInferenceService = ( existingData.metadata.name, isModelMesh, existingData, - acceleratorState, + initialAcceleratorProfile, + selectedAcceleratorProfile, ); return k8sUpdateResource( diff --git a/frontend/src/api/k8s/notebooks.ts b/frontend/src/api/k8s/notebooks.ts index 56e1d2cd73..f2b897538f 100644 --- a/frontend/src/api/k8s/notebooks.ts +++ b/frontend/src/api/k8s/notebooks.ts @@ -41,7 +41,8 @@ export const assembleNotebook = ( description, notebookSize, envFrom, - acceleratorProfile, + initialAcceleratorProfile, + selectedAcceleratorProfile, image, volumes: formVolumes, volumeMounts: formVolumeMounts, @@ -55,7 +56,8 @@ export const assembleNotebook = ( const { affinity, tolerations, resources } = assemblePodSpecOptions( notebookSize.resources, - acceleratorProfile, + initialAcceleratorProfile, + selectedAcceleratorProfile, tolerationSettings, existingTolerations, undefined, @@ -107,8 +109,7 @@ export const assembleNotebook = ( 'notebooks.opendatahub.io/last-image-selection': imageSelection, 'notebooks.opendatahub.io/inject-oauth': 'true', 'opendatahub.io/username': username, - 'opendatahub.io/accelerator-name': - acceleratorProfile.acceleratorProfile?.metadata.name || '', + 'opendatahub.io/accelerator-name': selectedAcceleratorProfile.profile?.metadata.name || '', }, name: notebookId, namespace: projectName, diff --git a/frontend/src/api/k8s/servingRuntimes.ts b/frontend/src/api/k8s/servingRuntimes.ts index e3b933dcb9..d00e5f2757 100644 --- a/frontend/src/api/k8s/servingRuntimes.ts +++ b/frontend/src/api/k8s/servingRuntimes.ts @@ -19,6 +19,7 @@ import { getModelServingRuntimeName } from '~/pages/modelServing/utils'; import { getDisplayNameFromK8sResource, translateDisplayNameForK8s } from '~/concepts/k8s/utils'; import { applyK8sAPIOptions } from '~/api/apiMergeUtils'; import { AcceleratorProfileState } from '~/utilities/useAcceleratorProfileState'; +import { AcceleratorProfileSelectFieldState } from '~/pages/notebookController/screens/server/AcceleratorProfileSelectField'; import { getModelServingProjects } from './projects'; import { assemblePodSpecOptions, getshmVolume, getshmVolumeMount } from './utils'; @@ -28,7 +29,8 @@ export const assembleServingRuntime = ( servingRuntime: ServingRuntimeKind, isCustomServingRuntimesEnabled: boolean, isEditing?: boolean, - acceleratorProfileState?: AcceleratorProfileState, + initialAcceleratorProfile?: AcceleratorProfileState, + selectedAcceleratorProfile?: AcceleratorProfileSelectFieldState, isModelMesh?: boolean, ): ServingRuntimeKind => { const { name: displayName, numReplicas, modelSize, externalRoute, tokenAuth } = data; @@ -71,7 +73,7 @@ export const assembleServingRuntime = ( ...(isCustomServingRuntimesEnabled && { 'opendatahub.io/template-display-name': getDisplayNameFromK8sResource(servingRuntime), 'opendatahub.io/accelerator-name': - acceleratorProfileState?.acceleratorProfile?.metadata.name || '', + selectedAcceleratorProfile?.profile?.metadata.name || '', }), }, }; @@ -80,8 +82,7 @@ export const assembleServingRuntime = ( ...updatedServingRuntime.metadata, annotations: { ...annotations, - 'opendatahub.io/accelerator-name': - acceleratorProfileState?.acceleratorProfile?.metadata.name || '', + 'opendatahub.io/accelerator-name': selectedAcceleratorProfile?.profile?.metadata.name || '', ...(isCustomServingRuntimesEnabled && { 'openshift.io/display-name': displayName.trim() }), }, }; @@ -107,7 +108,8 @@ export const assembleServingRuntime = ( const { affinity, tolerations, resources } = assemblePodSpecOptions( resourceSettings, - acceleratorProfileState, + initialAcceleratorProfile, + selectedAcceleratorProfile, undefined, servingRuntime.spec.tolerations, undefined, @@ -210,7 +212,8 @@ export const updateServingRuntime = (options: { existingData: ServingRuntimeKind; isCustomServingRuntimesEnabled: boolean; opts?: K8sAPIOptions; - acceleratorProfileState?: AcceleratorProfileState; + initialAcceleratorProfile?: AcceleratorProfileState; + selectedAcceleratorProfile?: AcceleratorProfileSelectFieldState; isModelMesh?: boolean; }): Promise => { const { @@ -218,7 +221,8 @@ export const updateServingRuntime = (options: { existingData, isCustomServingRuntimesEnabled, opts, - acceleratorProfileState, + initialAcceleratorProfile, + selectedAcceleratorProfile, isModelMesh, } = options; @@ -228,7 +232,8 @@ export const updateServingRuntime = (options: { existingData, isCustomServingRuntimesEnabled, true, - acceleratorProfileState, + initialAcceleratorProfile, + selectedAcceleratorProfile, isModelMesh, ); @@ -249,7 +254,8 @@ export const createServingRuntime = (options: { servingRuntime: ServingRuntimeKind; isCustomServingRuntimesEnabled: boolean; opts?: K8sAPIOptions; - acceleratorProfileState?: AcceleratorProfileState; + initialAcceleratorProfile?: AcceleratorProfileState; + selectedAcceleratorProfile?: AcceleratorProfileSelectFieldState; isModelMesh?: boolean; }): Promise => { const { @@ -258,7 +264,8 @@ export const createServingRuntime = (options: { servingRuntime, isCustomServingRuntimesEnabled, opts, - acceleratorProfileState, + initialAcceleratorProfile, + selectedAcceleratorProfile, isModelMesh, } = options; const assembledServingRuntime = assembleServingRuntime( @@ -267,7 +274,8 @@ export const createServingRuntime = (options: { servingRuntime, isCustomServingRuntimesEnabled, false, - acceleratorProfileState, + initialAcceleratorProfile, + selectedAcceleratorProfile, isModelMesh, ); diff --git a/frontend/src/api/k8s/utils.ts b/frontend/src/api/k8s/utils.ts index ddf008bba8..7f84cafa74 100644 --- a/frontend/src/api/k8s/utils.ts +++ b/frontend/src/api/k8s/utils.ts @@ -1,3 +1,4 @@ +import { AcceleratorProfileSelectFieldState } from '~/pages/notebookController/screens/server/AcceleratorProfileSelectField'; import { PodAffinity, ContainerResources, @@ -11,7 +12,8 @@ import { AcceleratorProfileState } from '~/utilities/useAcceleratorProfileState' export const assemblePodSpecOptions = ( resourceSettings: ContainerResources, - acceleratorProfileState?: AcceleratorProfileState, + initialAcceleratorProfile?: AcceleratorProfileState, + selectedAcceleratorProfile?: AcceleratorProfileSelectFieldState, tolerationSettings?: TolerationSettings, existingTolerations?: Toleration[], affinitySettings?: PodAffinity, @@ -27,38 +29,37 @@ export const assemblePodSpecOptions = ( requests: { ...existingResources?.requests, ...resourceSettings.requests }, }; - if ( - acceleratorProfileState?.additionalOptions?.useExisting && - !acceleratorProfileState.useExisting - ) { + // if not using existing settings, just use the new settings + if (!selectedAcceleratorProfile?.useExistingSettings) { resources = structuredClone(resourceSettings); } // Clear the last accelerator from the resources - if (acceleratorProfileState?.initialAcceleratorProfile) { + if (initialAcceleratorProfile?.acceleratorProfile) { if (resources.limits) { - delete resources.limits[acceleratorProfileState.initialAcceleratorProfile.spec.identifier]; + delete resources.limits[initialAcceleratorProfile.acceleratorProfile.spec.identifier]; } if (resources.requests) { - delete resources.requests[acceleratorProfileState.initialAcceleratorProfile.spec.identifier]; + delete resources.requests[initialAcceleratorProfile.acceleratorProfile.spec.identifier]; } } // Add back the new accelerator to the resources if count > 0 - if (acceleratorProfileState?.acceleratorProfile && acceleratorProfileState.count > 0) { + if (selectedAcceleratorProfile?.profile && selectedAcceleratorProfile.count > 0) { if (resources.limits) { - resources.limits[acceleratorProfileState.acceleratorProfile.spec.identifier] = - acceleratorProfileState.count; + resources.limits[selectedAcceleratorProfile.profile.spec.identifier] = + selectedAcceleratorProfile.count; } if (resources.requests) { - resources.requests[acceleratorProfileState.acceleratorProfile.spec.identifier] = - acceleratorProfileState.count; + resources.requests[selectedAcceleratorProfile.profile.spec.identifier] = + selectedAcceleratorProfile.count; } } const tolerations = determineTolerations( tolerationSettings, - acceleratorProfileState, + initialAcceleratorProfile, + selectedAcceleratorProfile, existingTolerations, ); return { affinity, tolerations, resources }; diff --git a/frontend/src/pages/modelServing/screens/projects/ModelMeshSection/ServingRuntimeDetails.tsx b/frontend/src/pages/modelServing/screens/projects/ModelMeshSection/ServingRuntimeDetails.tsx index fef1d92170..b56c1cff90 100644 --- a/frontend/src/pages/modelServing/screens/projects/ModelMeshSection/ServingRuntimeDetails.tsx +++ b/frontend/src/pages/modelServing/screens/projects/ModelMeshSection/ServingRuntimeDetails.tsx @@ -20,7 +20,7 @@ type ServingRuntimeDetailsProps = { const ServingRuntimeDetails: React.FC = ({ obj, isvc }) => { const { dashboardConfig } = React.useContext(AppContext); - const [acceleratorProfile] = useServingAcceleratorProfile(obj, isvc); + const acceleratorProfile = useServingAcceleratorProfile(obj, isvc); const selectedAcceleratorProfile = acceleratorProfile.acceleratorProfile; const enabledAcceleratorProfiles = acceleratorProfile.acceleratorProfiles.filter( (ac) => ac.spec.enabled, @@ -64,12 +64,12 @@ const ServingRuntimeDetails: React.FC = ({ obj, isvc }` : enabledAcceleratorProfiles.length === 0 ? 'No accelerator enabled' - : acceleratorProfile.useExisting + : acceleratorProfile.unknownProfileDetected ? 'Unknown' : 'No accelerator selected'} - {!acceleratorProfile.useExisting && acceleratorProfile.acceleratorProfile && ( + {!acceleratorProfile.unknownProfileDetected && acceleratorProfile.acceleratorProfile && ( Number of accelerators {acceleratorProfile.count} diff --git a/frontend/src/pages/modelServing/screens/projects/ServingRuntimeModal/ManageServingRuntimeModal.tsx b/frontend/src/pages/modelServing/screens/projects/ServingRuntimeModal/ManageServingRuntimeModal.tsx index ee0a74c171..b031249ef0 100644 --- a/frontend/src/pages/modelServing/screens/projects/ServingRuntimeModal/ManageServingRuntimeModal.tsx +++ b/frontend/src/pages/modelServing/screens/projects/ServingRuntimeModal/ManageServingRuntimeModal.tsx @@ -18,6 +18,8 @@ import DashboardModalFooter from '~/concepts/dashboard/DashboardModalFooter'; import { NamespaceApplicationCase } from '~/pages/projects/types'; import { ServingRuntimeEditInfo } from '~/pages/modelServing/screens/types'; import { useAccessReview } from '~/api'; +import { AcceleratorProfileSelectFieldState } from '~/pages/notebookController/screens/server/AcceleratorProfileSelectField'; +import useGenericObjectState from '~/utilities/useGenericObjectState'; import ServingRuntimeReplicaSection from './ServingRuntimeReplicaSection'; import ServingRuntimeSizeSection from './ServingRuntimeSizeSection'; import ServingRuntimeTemplateSection from './ServingRuntimeTemplateSection'; @@ -49,8 +51,13 @@ const ManageServingRuntimeModal: React.FC = ({ editInfo, }) => { const [createData, setCreateData, resetData, sizes] = useCreateServingRuntimeObject(editInfo); - const [acceleratorProfileState, setAcceleratorProfileState, resetAcceleratorProfileData] = - useServingAcceleratorProfile(editInfo?.servingRuntime); + const initialAcceleratorProfile = useServingAcceleratorProfile(editInfo?.servingRuntime); + const [selectedAcceleratorProfile, setSelectedAcceleratorProfile] = + useGenericObjectState({ + profile: undefined, + count: 0, + useExistingSettings: false, + }); const [actionInProgress, setActionInProgress] = React.useState(false); const [error, setError] = React.useState(); @@ -78,7 +85,13 @@ const ManageServingRuntimeModal: React.FC = ({ actionInProgress || tokenErrors || !inputValueValid || - !isModelServerEditInfoChanged(createData, sizes, acceleratorProfileState, editInfo); + !isModelServerEditInfoChanged( + createData, + sizes, + initialAcceleratorProfile, + selectedAcceleratorProfile, + editInfo, + ); const servingRuntimeSelected = React.useMemo( () => @@ -92,7 +105,6 @@ const ManageServingRuntimeModal: React.FC = ({ setError(undefined); setActionInProgress(false); resetData(); - resetAcceleratorProfileData(); }; const setErrorModal = (e: Error) => { @@ -115,7 +127,8 @@ const ManageServingRuntimeModal: React.FC = ({ namespace, editInfo, allowCreate, - acceleratorProfileState, + initialAcceleratorProfile, + selectedAcceleratorProfile, NamespaceApplicationCase.MODEL_MESH_PROMOTION, currentProject, undefined, @@ -162,7 +175,7 @@ const ManageServingRuntimeModal: React.FC = ({ setData={setCreateData} templates={servingRuntimeTemplates || []} isEditing={!!editInfo} - acceleratorProfileState={acceleratorProfileState} + selectedAcceleratorProfile={selectedAcceleratorProfile} /> @@ -179,8 +192,9 @@ const ManageServingRuntimeModal: React.FC = ({ setData={setCreateData} sizes={sizes} servingRuntimeSelected={servingRuntimeSelected} - acceleratorProfileState={acceleratorProfileState} - setAcceleratorProfileState={setAcceleratorProfileState} + acceleratorProfileState={initialAcceleratorProfile} + selectedAcceleratorProfile={selectedAcceleratorProfile} + setSelectedAcceleratorProfile={setSelectedAcceleratorProfile} infoContent="Select a server size that will accommodate your largest model. See the product documentation for more information." /> diff --git a/frontend/src/pages/modelServing/screens/projects/ServingRuntimeModal/ServingRuntimeSizeSection.tsx b/frontend/src/pages/modelServing/screens/projects/ServingRuntimeModal/ServingRuntimeSizeSection.tsx index d851053525..e6dc9fe2ef 100644 --- a/frontend/src/pages/modelServing/screens/projects/ServingRuntimeModal/ServingRuntimeSizeSection.tsx +++ b/frontend/src/pages/modelServing/screens/projects/ServingRuntimeModal/ServingRuntimeSizeSection.tsx @@ -9,7 +9,9 @@ import { } from '~/pages/modelServing/screens/types'; import { ServingRuntimeKind } from '~/k8sTypes'; import { isGpuDisabled } from '~/pages/modelServing/screens/projects/utils'; -import AcceleratorProfileSelectField from '~/pages/notebookController/screens/server/AcceleratorProfileSelectField'; +import AcceleratorProfileSelectField, { + AcceleratorProfileSelectFieldState, +} from '~/pages/notebookController/screens/server/AcceleratorProfileSelectField'; import { getCompatibleAcceleratorIdentifiers } from '~/pages/projects/screens/spawner/spawnerUtils'; import { AcceleratorProfileState } from '~/utilities/useAcceleratorProfileState'; import SimpleSelect from '~/components/SimpleSelect'; @@ -23,7 +25,8 @@ type ServingRuntimeSizeSectionProps = { sizes: ModelServingSize[]; servingRuntimeSelected?: ServingRuntimeKind; acceleratorProfileState: AcceleratorProfileState; - setAcceleratorProfileState: UpdateObjectAtPropAndValue; + selectedAcceleratorProfile: AcceleratorProfileSelectFieldState; + setSelectedAcceleratorProfile: UpdateObjectAtPropAndValue; infoContent?: string; }; @@ -33,7 +36,8 @@ const ServingRuntimeSizeSection: React.FC = ({ sizes, servingRuntimeSelected, acceleratorProfileState, - setAcceleratorProfileState, + selectedAcceleratorProfile, + setSelectedAcceleratorProfile, infoContent, }) => { const [supportedAcceleratorProfiles, setSupportedAcceleratorProfiles] = React.useState< @@ -111,10 +115,11 @@ const ServingRuntimeSizeSection: React.FC = ({ )} diff --git a/frontend/src/pages/modelServing/screens/projects/ServingRuntimeModal/ServingRuntimeTemplateSection.tsx b/frontend/src/pages/modelServing/screens/projects/ServingRuntimeModal/ServingRuntimeTemplateSection.tsx index 4b88b1b99a..a7ed7bed1e 100644 --- a/frontend/src/pages/modelServing/screens/projects/ServingRuntimeModal/ServingRuntimeTemplateSection.tsx +++ b/frontend/src/pages/modelServing/screens/projects/ServingRuntimeModal/ServingRuntimeTemplateSection.tsx @@ -10,14 +10,14 @@ import { } from '~/pages/modelServing/customServingRuntimes/utils'; import { isCompatibleWithAccelerator as isCompatibleWithAcceleratorProfile } from '~/pages/projects/screens/spawner/spawnerUtils'; import SimpleSelect from '~/components/SimpleSelect'; -import { AcceleratorProfileState } from '~/utilities/useAcceleratorProfileState'; +import { AcceleratorProfileSelectFieldState } from '~/pages/notebookController/screens/server/AcceleratorProfileSelectField'; type ServingRuntimeTemplateSectionProps = { data: CreatingServingRuntimeObject; setData: UpdateObjectAtPropAndValue; templates: TemplateKind[]; isEditing?: boolean; - acceleratorProfileState: AcceleratorProfileState; + selectedAcceleratorProfile: AcceleratorProfileSelectFieldState; }; const ServingRuntimeTemplateSection: React.FC = ({ @@ -25,7 +25,7 @@ const ServingRuntimeTemplateSection: React.FC { const filteredTemplates = React.useMemo( () => @@ -50,7 +50,7 @@ const ServingRuntimeTemplateSection: React.FC {isCompatibleWithAcceleratorProfile( - acceleratorProfileState.acceleratorProfile?.spec.identifier, + selectedAcceleratorProfile.profile?.spec.identifier, template.objects[0], ) && } diff --git a/frontend/src/pages/modelServing/screens/projects/kServeModal/ManageKServeModal.tsx b/frontend/src/pages/modelServing/screens/projects/kServeModal/ManageKServeModal.tsx index e35fb5f562..f1799cf66a 100644 --- a/frontend/src/pages/modelServing/screens/projects/kServeModal/ManageKServeModal.tsx +++ b/frontend/src/pages/modelServing/screens/projects/kServeModal/ManageKServeModal.tsx @@ -48,6 +48,8 @@ import { useAccessReview } from '~/api'; import { SupportedArea, useIsAreaAvailable } from '~/concepts/areas'; import { RegisteredModelDeployInfo } from '~/pages/modelRegistry/screens/RegisteredModels/useRegisteredModelDeployInfo'; import usePrefillDeployModalFromModelRegistry from '~/pages/modelRegistry/screens/RegisteredModels/usePrefillDeployModalFromModelRegistry'; +import { AcceleratorProfileSelectFieldState } from '~/pages/notebookController/screens/server/AcceleratorProfileSelectField'; +import useGenericObjectState from '~/utilities/useGenericObjectState'; import KServeAutoscalerReplicaSection from './KServeAutoscalerReplicaSection'; const accessReviewResource: AccessReviewResourceAttributes = { @@ -111,11 +113,19 @@ const ManageKServeModal: React.FC = ({ const isInferenceServiceNameWithinLimit = translateDisplayNameForK8s(createDataInferenceService.name).length <= 253; - const [acceleratorProfileState, setAcceleratorProfileState, resetAcceleratorProfileData] = - useServingAcceleratorProfile( - editInfo?.servingRuntimeEditInfo?.servingRuntime, - editInfo?.inferenceServiceEditInfo, - ); + const acceleratorProfileState = useServingAcceleratorProfile( + editInfo?.servingRuntimeEditInfo?.servingRuntime, + editInfo?.inferenceServiceEditInfo, + ); + const [ + selectedAcceleratorProfile, + setSelectedAcceleratorProfile, + resetSelectedAcceleratorProfile, + ] = useGenericObjectState({ + profile: undefined, + count: 0, + useExistingSettings: false, + }); const customServingRuntimesEnabled = useCustomServingRuntimesEnabled(); const [allowCreate] = useAccessReview({ ...accessReviewResource, @@ -188,7 +198,7 @@ const ManageKServeModal: React.FC = ({ setActionInProgress(false); resetDataServingRuntime(); resetDataInferenceService(); - resetAcceleratorProfileData(); + resetSelectedAcceleratorProfile(); setAlertVisible(true); }; @@ -218,6 +228,7 @@ const ManageKServeModal: React.FC = ({ editInfo?.servingRuntimeEditInfo, false, acceleratorProfileState, + selectedAcceleratorProfile, NamespaceApplicationCase.KSERVE_PROMOTION, projectContext?.currentProject, servingRuntimeName, @@ -233,6 +244,7 @@ const ManageKServeModal: React.FC = ({ servingRuntimeName, false, acceleratorProfileState, + selectedAcceleratorProfile, allowCreate, editInfo?.secrets, ); @@ -329,7 +341,7 @@ const ManageKServeModal: React.FC = ({ setData={setCreateDataServingRuntime} templates={servingRuntimeTemplates || []} isEditing={!!editInfo} - acceleratorProfileState={acceleratorProfileState} + selectedAcceleratorProfile={selectedAcceleratorProfile} /> @@ -355,7 +367,8 @@ const ManageKServeModal: React.FC = ({ sizes={sizes} servingRuntimeSelected={servingRuntimeSelected} acceleratorProfileState={acceleratorProfileState} - setAcceleratorProfileState={setAcceleratorProfileState} + selectedAcceleratorProfile={selectedAcceleratorProfile} + setSelectedAcceleratorProfile={setSelectedAcceleratorProfile} infoContent="Select a server size that will accommodate your largest model. See the product documentation for more information." /> diff --git a/frontend/src/pages/modelServing/screens/projects/useServingAcceleratorProfile.ts b/frontend/src/pages/modelServing/screens/projects/useServingAcceleratorProfile.ts index 53b52c97c4..c7adcd09e5 100644 --- a/frontend/src/pages/modelServing/screens/projects/useServingAcceleratorProfile.ts +++ b/frontend/src/pages/modelServing/screens/projects/useServingAcceleratorProfile.ts @@ -2,12 +2,11 @@ import { InferenceServiceKind, ServingRuntimeKind } from '~/k8sTypes'; import useAcceleratorProfileState, { AcceleratorProfileState, } from '~/utilities/useAcceleratorProfileState'; -import { GenericObjectState } from '~/utilities/useGenericObjectState'; const useServingAcceleratorProfile = ( servingRuntime?: ServingRuntimeKind | null, inferenceService?: InferenceServiceKind | null, -): GenericObjectState => { +): AcceleratorProfileState => { const acceleratorProfileName = servingRuntime?.metadata.annotations?.['opendatahub.io/accelerator-name']; const resources = diff --git a/frontend/src/pages/modelServing/screens/projects/utils.ts b/frontend/src/pages/modelServing/screens/projects/utils.ts index b69fc36f9c..b97c4059a0 100644 --- a/frontend/src/pages/modelServing/screens/projects/utils.ts +++ b/frontend/src/pages/modelServing/screens/projects/utils.ts @@ -44,6 +44,7 @@ import { import { isDataConnectionAWS } from '~/pages/projects/screens/detail/data-connections/utils'; import { removeLeadingSlash } from '~/utilities/string'; import { RegisteredModelDeployInfo } from '~/pages/modelRegistry/screens/RegisteredModels/useRegisteredModelDeployInfo'; +import { AcceleratorProfileSelectFieldState } from '~/pages/notebookController/screens/server/AcceleratorProfileSelectField'; export const getServingRuntimeSizes = (config: DashboardConfigKind): ModelServingSize[] => { let sizes = config.spec.modelServerSizes || []; @@ -314,7 +315,8 @@ const createInferenceServiceAndDataConnection = ( existingStorage: boolean, editInfo?: InferenceServiceKind, isModelMesh?: boolean, - acceleratorProfileState?: AcceleratorProfileState, + initialAcceleratorProfile?: AcceleratorProfileState, + selectedAcceleratorProfile?: AcceleratorProfileSelectFieldState, dryRun = false, ) => { if (!existingStorage) { @@ -325,14 +327,16 @@ const createInferenceServiceAndDataConnection = ( editInfo, secret.metadata.name, isModelMesh, - acceleratorProfileState, + initialAcceleratorProfile, + selectedAcceleratorProfile, dryRun, ) : createInferenceService( inferenceServiceData, secret.metadata.name, isModelMesh, - acceleratorProfileState, + initialAcceleratorProfile, + selectedAcceleratorProfile, dryRun, ), ); @@ -343,14 +347,16 @@ const createInferenceServiceAndDataConnection = ( editInfo, undefined, isModelMesh, - acceleratorProfileState, + initialAcceleratorProfile, + selectedAcceleratorProfile, dryRun, ) : createInferenceService( inferenceServiceData, undefined, isModelMesh, - acceleratorProfileState, + initialAcceleratorProfile, + selectedAcceleratorProfile, dryRun, ); }; @@ -360,7 +366,8 @@ export const getSubmitInferenceServiceResourceFn = ( editInfo?: InferenceServiceKind, servingRuntimeName?: string, isModelMesh?: boolean, - acceleratorProfileState?: AcceleratorProfileState, + initialAcceleratorProfile?: AcceleratorProfileState, + selectedAcceleratorProfile?: AcceleratorProfileSelectFieldState, allowCreate?: boolean, secrets?: SecretKind[], ): ((opts: { dryRun?: boolean }) => Promise) => { @@ -389,7 +396,8 @@ export const getSubmitInferenceServiceResourceFn = ( existingStorage, editInfo, isModelMesh, - acceleratorProfileState, + initialAcceleratorProfile, + selectedAcceleratorProfile, dryRun, ).then((inferenceService) => setUpTokenAuth( @@ -421,7 +429,8 @@ export const getSubmitServingRuntimeResourcesFn = ( namespace: string, editInfo: ServingRuntimeEditInfo | undefined, allowCreate: boolean, - acceleratorProfileState: AcceleratorProfileState, + initialAcceleratorProfile: AcceleratorProfileState, + selectedAcceleratorProfile: AcceleratorProfileSelectFieldState, servingPlatformEnablement: NamespaceApplicationCase, currentProject?: ProjectKind, name?: string, @@ -443,9 +452,9 @@ export const getSubmitServingRuntimeResourcesFn = ( const servingRuntimeName = translateDisplayNameForK8s(servingRuntimeData.name); const createTokenAuth = servingRuntimeData.tokenAuth && allowCreate; - const controlledState = isGpuDisabled(servingRuntimeSelected) - ? { count: 0, acceleratorProfiles: [], useExisting: false } - : acceleratorProfileState; + const controlledState: AcceleratorProfileSelectFieldState = isGpuDisabled(servingRuntimeSelected) + ? { count: 0, useExistingSettings: false } + : selectedAcceleratorProfile; if (!editInfo && !currentProject) { // This should be impossible to hit on resource creation, current project is undefined only on edit @@ -472,7 +481,8 @@ export const getSubmitServingRuntimeResourcesFn = ( opts: { dryRun, }, - acceleratorProfileState: controlledState, + selectedAcceleratorProfile: controlledState, + initialAcceleratorProfile, isModelMesh, }), setUpTokenAuth( @@ -496,7 +506,8 @@ export const getSubmitServingRuntimeResourcesFn = ( opts: { dryRun, }, - acceleratorProfileState: controlledState, + selectedAcceleratorProfile: controlledState, + initialAcceleratorProfile, isModelMesh, }).then((servingRuntime) => setUpTokenAuth( diff --git a/frontend/src/pages/modelServing/utils.ts b/frontend/src/pages/modelServing/utils.ts index 08414c484e..d17b91bb88 100644 --- a/frontend/src/pages/modelServing/utils.ts +++ b/frontend/src/pages/modelServing/utils.ts @@ -37,7 +37,7 @@ import { ServingRuntimeToken, } from '~/pages/modelServing/screens/types'; import { AcceleratorProfileState } from '~/utilities/useAcceleratorProfileState'; -import { getAcceleratorProfileCount } from '~/utilities/utils'; +import { AcceleratorProfileSelectFieldState } from '~/pages/notebookController/screens/server/AcceleratorProfileSelectField'; type TokenNames = { serviceAccountName: string; @@ -243,40 +243,35 @@ export const getServingRuntimeTokens = (tokens?: SecretKind[]): ServingRuntimeTo })); const isAcceleratorProfileChanged = ( - acceleratorProfileState: AcceleratorProfileState, - servingRuntime: ServingRuntimeKind, + initialAcceleratorProfile: AcceleratorProfileState, + selectedAcceleratorProfile: AcceleratorProfileSelectFieldState, ) => { - const { acceleratorProfile } = acceleratorProfileState; - const { initialAcceleratorProfile } = acceleratorProfileState; - // both are none, check if it's using existing - if (!acceleratorProfile && !initialAcceleratorProfile) { - if (acceleratorProfileState.additionalOptions?.useExisting) { - return !acceleratorProfileState.useExisting; + if (!selectedAcceleratorProfile.profile && !initialAcceleratorProfile.acceleratorProfile) { + if (selectedAcceleratorProfile.useExistingSettings) { + return !selectedAcceleratorProfile.useExistingSettings; } return false; } // one is none, another is set, changed - if (!acceleratorProfile || !initialAcceleratorProfile) { + if (!selectedAcceleratorProfile.profile || !initialAcceleratorProfile.acceleratorProfile) { return true; } // compare the name, gpu count return ( - acceleratorProfile.metadata.name !== initialAcceleratorProfile.metadata.name || - acceleratorProfileState.count !== - getAcceleratorProfileCount( - initialAcceleratorProfile, - servingRuntime.spec.containers[0].resources || {}, - ) + selectedAcceleratorProfile.profile.metadata.name !== + initialAcceleratorProfile.acceleratorProfile.metadata.name || + selectedAcceleratorProfile.count !== initialAcceleratorProfile.count ); }; export const isModelServerEditInfoChanged = ( createData: CreatingServingRuntimeObject, sizes: ModelServingSize[], - acceleratorProfileState: AcceleratorProfileState, + initialAcceleratorProfile: AcceleratorProfileState, + selectedAcceleratorProfile: AcceleratorProfileSelectFieldState, editInfo?: ServingRuntimeEditInfo, ): boolean => editInfo?.servingRuntime @@ -287,7 +282,7 @@ export const isModelServerEditInfoChanged = ( String(createData.externalRoute) || editInfo.servingRuntime.metadata.annotations['enable-auth'] !== String(createData.tokenAuth) || - isAcceleratorProfileChanged(acceleratorProfileState, editInfo.servingRuntime) || + isAcceleratorProfileChanged(initialAcceleratorProfile, selectedAcceleratorProfile) || (createData.tokenAuth && !_.isEqual( getServingRuntimeTokens(editInfo.secrets) diff --git a/frontend/src/pages/notebookController/screens/server/AcceleratorProfileSelectField.tsx b/frontend/src/pages/notebookController/screens/server/AcceleratorProfileSelectField.tsx index 3659a9c517..a9f958075c 100644 --- a/frontend/src/pages/notebookController/screens/server/AcceleratorProfileSelectField.tsx +++ b/frontend/src/pages/notebookController/screens/server/AcceleratorProfileSelectField.tsx @@ -17,41 +17,56 @@ import { OutlinedQuestionCircleIcon } from '@patternfly/react-icons'; import { isHTMLInputElement } from '~/utilities/utils'; import { AcceleratorProfileKind } from '~/k8sTypes'; import SimpleSelect, { SimpleSelectOption } from '~/components/SimpleSelect'; -import { UpdateObjectAtPropAndValue } from '~/pages/projects/types'; import { AcceleratorProfileState } from '~/utilities/useAcceleratorProfileState'; +import { UpdateObjectAtPropAndValue } from '~/pages/projects/types'; import useDetectedAccelerators from './useDetectedAccelerators'; type AcceleratorProfileSelectFieldProps = { acceleratorProfileState: AcceleratorProfileState; - setAcceleratorProfileState: UpdateObjectAtPropAndValue; supportedAcceleratorProfiles?: string[]; resourceDisplayName?: string; infoContent?: string; + selectedAcceleratorProfile: AcceleratorProfileSelectFieldState; + setSelectedAcceleratorProfile: UpdateObjectAtPropAndValue; }; +export type AcceleratorProfileSelectFieldState = + | { + profile?: AcceleratorProfileKind; + count: number; + useExistingSettings: false; + } + | { + profile: undefined; + count: 1; + useExistingSettings: true; + }; + const AcceleratorProfileSelectField: React.FC = ({ acceleratorProfileState, - setAcceleratorProfileState, supportedAcceleratorProfiles, resourceDisplayName = 'image', infoContent, + selectedAcceleratorProfile, + setSelectedAcceleratorProfile, }) => { const [detectedAccelerators] = useDetectedAccelerators(); - const { - acceleratorProfile, - count: acceleratorCount, - acceleratorProfiles, - useExisting, - additionalOptions, - } = acceleratorProfileState; + React.useEffect(() => { + setSelectedAcceleratorProfile('profile', acceleratorProfileState.acceleratorProfile); + setSelectedAcceleratorProfile('count', acceleratorProfileState.count); + setSelectedAcceleratorProfile( + 'useExistingSettings', + acceleratorProfileState.unknownProfileDetected, + ); + }, [acceleratorProfileState, setSelectedAcceleratorProfile]); const generateAcceleratorCountWarning = (newSize: number) => { - if (!acceleratorProfile) { + if (!selectedAcceleratorProfile.profile) { return ''; } - const { identifier } = acceleratorProfile.spec; + const { identifier } = selectedAcceleratorProfile.profile.spec; const detectedAcceleratorCount = Object.entries(detectedAccelerators.available).find( ([id]) => identifier === id, @@ -69,12 +84,14 @@ const AcceleratorProfileSelectField: React.FC supportedAcceleratorProfiles?.includes(cr.spec.identifier); - const enabledAcceleratorProfiles = acceleratorProfiles.filter((ac) => ac.spec.enabled); + const enabledAcceleratorProfiles = acceleratorProfileState.acceleratorProfiles.filter( + (ac) => ac.spec.enabled, + ); const formatOption = (cr: AcceleratorProfileKind): SimpleSelectOption => { const displayName = `${cr.spec.displayName}${!cr.spec.enabled ? ' (disabled)' : ''}`; @@ -112,13 +129,13 @@ const AcceleratorProfileSelectField: React.FC formatOption(ac)); let acceleratorAlertMessage: { title: string; variant: AlertVariant } | null = null; - if (acceleratorProfile && supportedAcceleratorProfiles !== undefined) { + if (selectedAcceleratorProfile.profile && supportedAcceleratorProfiles !== undefined) { if (supportedAcceleratorProfiles.length === 0) { acceleratorAlertMessage = { title: `The ${resourceDisplayName} you have selected doesn't support the selected accelerator. It is recommended to use a compatible ${resourceDisplayName} for optimal performance.`, variant: AlertVariant.info, }; - } else if (!isAcceleratorProfileSupported(acceleratorProfile)) { + } else if (!isAcceleratorProfileSupported(selectedAcceleratorProfile.profile)) { acceleratorAlertMessage = { title: `The ${resourceDisplayName} you have selected is not compatible with the selected accelerator`, variant: AlertVariant.warning, @@ -133,18 +150,21 @@ const AcceleratorProfileSelectField: React.FC { - setAcceleratorProfileState('count', Math.max(acceleratorCount + step, 1)); + setSelectedAcceleratorProfile('count', Math.max(selectedAcceleratorProfile.count + step, 1)); }; // if there is more than a none option, show the dropdown @@ -171,25 +191,31 @@ const AcceleratorProfileSelectField: React.FC { if (isPlaceholder) { // none - setAcceleratorProfileState('useExisting', false); - setAcceleratorProfileState('acceleratorProfile', undefined); - setAcceleratorProfileState('count', 0); + setSelectedAcceleratorProfile('useExistingSettings', false); + setSelectedAcceleratorProfile('profile', undefined); + setSelectedAcceleratorProfile('count', 0); } else if (key === 'use-existing') { // use existing settings - setAcceleratorProfileState('useExisting', true); - setAcceleratorProfileState('acceleratorProfile', undefined); - setAcceleratorProfileState('count', 0); + setSelectedAcceleratorProfile('useExistingSettings', true); + setSelectedAcceleratorProfile('profile', undefined); + setSelectedAcceleratorProfile('count', 0); } else { // normal flow - setAcceleratorProfileState('count', 1); - setAcceleratorProfileState('useExisting', false); - setAcceleratorProfileState( - 'acceleratorProfile', - acceleratorProfiles.find((ac) => ac.metadata.name === key), + setSelectedAcceleratorProfile('count', 1); + setSelectedAcceleratorProfile('useExistingSettings', false); + setSelectedAcceleratorProfile( + 'profile', + acceleratorProfileState.acceleratorProfiles.find( + (ac) => ac.metadata.name === key, + ), ); } }} @@ -206,7 +232,7 @@ const AcceleratorProfileSelectField: React.FC )} - {acceleratorProfile && ( + {selectedAcceleratorProfile.profile && ( @@ -214,7 +240,7 @@ const AcceleratorProfileSelectField: React.FC onStep(1)} @@ -222,7 +248,7 @@ const AcceleratorProfileSelectField: React.FC { if (isHTMLInputElement(event.target)) { const newSize = Number(event.target.value); - setAcceleratorProfileState('count', Math.max(newSize, 1)); + setSelectedAcceleratorProfile('count', Math.max(newSize, 1)); } }} /> diff --git a/frontend/src/pages/notebookController/screens/server/NotebookServerDetails.tsx b/frontend/src/pages/notebookController/screens/server/NotebookServerDetails.tsx index 814a6215cb..4181a67cf9 100644 --- a/frontend/src/pages/notebookController/screens/server/NotebookServerDetails.tsx +++ b/frontend/src/pages/notebookController/screens/server/NotebookServerDetails.tsx @@ -28,7 +28,7 @@ const NotebookServerDetails: React.FC = () => { const { images, loaded } = useWatchImages(); const [isExpanded, setExpanded] = React.useState(false); const { dashboardConfig } = useAppContext(); - const [acceleratorProfile] = useNotebookAcceleratorProfile(notebook); + const acceleratorProfile = useNotebookAcceleratorProfile(notebook); const container: PodContainer | undefined = notebook?.spec.template.spec.containers.find( (currentContainer) => currentContainer.name === notebook.metadata.name, @@ -112,12 +112,12 @@ const NotebookServerDetails: React.FC = () => { {acceleratorProfile.acceleratorProfile ? acceleratorProfile.acceleratorProfile.spec.displayName - : acceleratorProfile.useExisting + : acceleratorProfile.unknownProfileDetected ? 'Unknown' : 'None'} - {!acceleratorProfile.useExisting && ( + {!acceleratorProfile.unknownProfileDetected && ( Number of accelerators {acceleratorProfile.count} diff --git a/frontend/src/pages/notebookController/screens/server/SpawnerPage.tsx b/frontend/src/pages/notebookController/screens/server/SpawnerPage.tsx index 975518d13e..15151e3b20 100644 --- a/frontend/src/pages/notebookController/screens/server/SpawnerPage.tsx +++ b/frontend/src/pages/notebookController/screens/server/SpawnerPage.tsx @@ -42,6 +42,7 @@ import useNotebookAcceleratorProfile from '~/pages/projects/screens/detail/noteb import { SupportedArea, useIsAreaAvailable } from '~/concepts/areas'; import { fireFormTrackingEvent } from '~/concepts/analyticsTracking/segmentIOUtils'; import { TrackingOutcome } from '~/concepts/analyticsTracking/trackingProperties'; +import useGenericObjectState from '~/utilities/useGenericObjectState'; import SizeSelectField from './SizeSelectField'; import useSpawnerNotebookModalState from './useSpawnerNotebookModalState'; import BrowserTabPreferenceCheckbox from './BrowserTabPreferenceCheckbox'; @@ -49,7 +50,9 @@ import EnvironmentVariablesRow from './EnvironmentVariablesRow'; import ImageSelector from './ImageSelector'; import { usePreferredNotebookSize } from './usePreferredNotebookSize'; import StartServerModal from './StartServerModal'; -import AcceleratorProfileSelectField from './AcceleratorProfileSelectField'; +import AcceleratorProfileSelectField, { + AcceleratorProfileSelectFieldState, +} from './AcceleratorProfileSelectField'; import '~/pages/notebookController/NotebookController.scss'; @@ -72,11 +75,17 @@ const SpawnerPage: React.FC = () => { tag: undefined, }); const { selectedSize, setSelectedSize, sizes } = usePreferredNotebookSize(); - const [acceleratorProfile, setAcceleratorProfile] = - useNotebookAcceleratorProfile(currentUserNotebook); + const acceleratorProfile = useNotebookAcceleratorProfile(currentUserNotebook); const [variableRows, setVariableRows] = React.useState([]); const [submitError, setSubmitError] = React.useState(null); + const [selectedAcceleratorProfile, setSelectedAcceleratorProfile] = + useGenericObjectState({ + profile: undefined, + count: 0, + useExistingSettings: false, + }); + const disableSubmit = createInProgress || variableRows.some(({ errors }) => Object.keys(errors).length > 0); @@ -236,10 +245,12 @@ const SpawnerPage: React.FC = () => { outcome: TrackingOutcome.submit, accelerator: acceleratorProfile.acceleratorProfile ? `${acceleratorProfile.acceleratorProfile.spec.displayName} (${acceleratorProfile.acceleratorProfile.metadata.name}): ${acceleratorProfile.acceleratorProfile.spec.identifier}` - : acceleratorProfile.useExisting + : acceleratorProfile.unknownProfileDetected ? 'Unknown' : 'None', - acceleratorCount: acceleratorProfile.useExisting ? undefined : acceleratorProfile.count, + acceleratorCount: acceleratorProfile.unknownProfileDetected + ? undefined + : acceleratorProfile.count, lastSelectedSize: selectedSize.name, lastSelectedImage: `${selectedImageTag.image?.name}:${selectedImageTag.tag?.name}`, }); @@ -322,7 +333,8 @@ const SpawnerPage: React.FC = () => { /> diff --git a/frontend/src/pages/projects/notebook/NotebookStatusToggle.tsx b/frontend/src/pages/projects/notebook/NotebookStatusToggle.tsx index ed218796b6..654dede41f 100644 --- a/frontend/src/pages/projects/notebook/NotebookStatusToggle.tsx +++ b/frontend/src/pages/projects/notebook/NotebookStatusToggle.tsx @@ -28,7 +28,7 @@ const NotebookStatusToggle: React.FC = ({ isDisabled, }) => { const { notebook, isStarting, isRunning, isStopping, refresh } = notebookState; - const [acceleratorProfile] = useNotebookAcceleratorProfile(notebook); + const acceleratorProfile = useNotebookAcceleratorProfile(notebook); const { size } = useNotebookDeploymentSize(notebook); const [isOpenConfirm, setOpenConfirm] = React.useState(false); const [inProgress, setInProgress] = React.useState(false); @@ -55,10 +55,12 @@ const NotebookStatusToggle: React.FC = ({ (action: 'started' | 'stopped') => { fireFormTrackingEvent(`Workbench ${action === 'started' ? 'Started' : 'Stopped'}`, { outcome: TrackingOutcome.submit, - acceleratorCount: acceleratorProfile.useExisting ? undefined : acceleratorProfile.count, + acceleratorCount: acceleratorProfile.unknownProfileDetected + ? undefined + : acceleratorProfile.count, accelerator: acceleratorProfile.acceleratorProfile ? `${acceleratorProfile.acceleratorProfile.spec.displayName} (${acceleratorProfile.acceleratorProfile.metadata.name}): ${acceleratorProfile.acceleratorProfile.spec.identifier}` - : acceleratorProfile.useExisting + : acceleratorProfile.unknownProfileDetected ? 'Unknown' : 'None', lastSelectedSize: diff --git a/frontend/src/pages/projects/screens/detail/notebooks/useNotebookAcceleratorProfile.ts b/frontend/src/pages/projects/screens/detail/notebooks/useNotebookAcceleratorProfile.ts index 79e282d9ff..6343b628c7 100644 --- a/frontend/src/pages/projects/screens/detail/notebooks/useNotebookAcceleratorProfile.ts +++ b/frontend/src/pages/projects/screens/detail/notebooks/useNotebookAcceleratorProfile.ts @@ -3,11 +3,10 @@ import { Notebook } from '~/types'; import useAcceleratorProfileState, { AcceleratorProfileState, } from '~/utilities/useAcceleratorProfileState'; -import { GenericObjectState } from '~/utilities/useGenericObjectState'; const useNotebookAcceleratorProfile = ( notebook?: NotebookKind | Notebook | null, -): GenericObjectState => { +): AcceleratorProfileState => { const name = notebook?.metadata.annotations?.['opendatahub.io/accelerator-name']; const resources = notebook?.spec.template.spec.containers.find( (container) => container.name === notebook.metadata.name, diff --git a/frontend/src/pages/projects/screens/spawner/SpawnerFooter.tsx b/frontend/src/pages/projects/screens/spawner/SpawnerFooter.tsx index de095ceafc..22978b9fd3 100644 --- a/frontend/src/pages/projects/screens/spawner/SpawnerFooter.tsx +++ b/frontend/src/pages/projects/screens/spawner/SpawnerFooter.tsx @@ -84,14 +84,15 @@ const SpawnerFooter: React.FC = ({ editNotebook, existingDataConnections, ); - const afterStart = (name: string, type: 'created' | 'updated') => { - const { acceleratorProfile, notebookSize, image } = startNotebookData; + const { selectedAcceleratorProfile, notebookSize, image } = startNotebookData; const tep: FormTrackingEventProperties = { - acceleratorCount: acceleratorProfile.useExisting ? undefined : acceleratorProfile.count, - accelerator: acceleratorProfile.acceleratorProfile - ? `${acceleratorProfile.acceleratorProfile.spec.displayName} (${acceleratorProfile.acceleratorProfile.metadata.name}): ${acceleratorProfile.acceleratorProfile.spec.identifier}` - : acceleratorProfile.useExisting + acceleratorCount: selectedAcceleratorProfile.useExistingSettings + ? undefined + : selectedAcceleratorProfile.count, + accelerator: selectedAcceleratorProfile.profile + ? `${selectedAcceleratorProfile.profile.spec.displayName} (${selectedAcceleratorProfile.profile.metadata.name}): ${selectedAcceleratorProfile.profile.spec.identifier}` + : selectedAcceleratorProfile.useExistingSettings ? 'Unknown' : 'None', lastSelectedSize: notebookSize.name, diff --git a/frontend/src/pages/projects/screens/spawner/SpawnerPage.tsx b/frontend/src/pages/projects/screens/spawner/SpawnerPage.tsx index d64e5afcd4..790da0f874 100644 --- a/frontend/src/pages/projects/screens/spawner/SpawnerPage.tsx +++ b/frontend/src/pages/projects/screens/spawner/SpawnerPage.tsx @@ -21,10 +21,13 @@ import useNotebookImageData from '~/pages/projects/screens/detail/notebooks/useN import NotebookRestartAlert from '~/pages/projects/components/NotebookRestartAlert'; import useWillNotebooksRestart from '~/pages/projects/notebook/useWillNotebooksRestart'; import CanEnableElyraPipelinesCheck from '~/concepts/pipelines/elyra/CanEnableElyraPipelinesCheck'; -import AcceleratorProfileSelectField from '~/pages/notebookController/screens/server/AcceleratorProfileSelectField'; +import AcceleratorProfileSelectField, { + AcceleratorProfileSelectFieldState, +} from '~/pages/notebookController/screens/server/AcceleratorProfileSelectField'; import useNotebookAcceleratorProfile from '~/pages/projects/screens/detail/notebooks/useNotebookAcceleratorProfile'; import { NotebookImageAvailability } from '~/pages/projects/screens/detail/notebooks/const'; import { getDescriptionFromK8sResource, getDisplayNameFromK8sResource } from '~/concepts/k8s/utils'; +import useGenericObjectState from '~/utilities/useGenericObjectState'; import { SpawnerPageSectionID } from './types'; import { ScrollableSelectorID, SpawnerPageSectionTitles } from './const'; import SpawnerFooter from './SpawnerFooter'; @@ -72,6 +75,13 @@ const SpawnerPage: React.FC = ({ existingNotebook }) => { existingNotebook, ); + const [selectedAcceleratorProfile, setSelectedAcceleratorProfile] = + useGenericObjectState({ + profile: undefined, + count: 0, + useExistingSettings: false, + }); + const restartNotebooks = useWillNotebooksRestart([existingNotebook?.metadata.name || '']); React.useEffect(() => { @@ -94,8 +104,7 @@ const SpawnerPage: React.FC = ({ existingNotebook }) => { } }, [data, loaded, loadError]); - const [notebookAcceleratorProfileState, setNotebookAcceleratorProfileState] = - useNotebookAcceleratorProfile(existingNotebook); + const notebookAcceleratorProfileState = useNotebookAcceleratorProfile(existingNotebook); React.useEffect(() => { if (selectedImage.imageStream) { @@ -181,8 +190,9 @@ const SpawnerPage: React.FC = ({ existingNotebook }) => { /> = ({ existingNotebook }) => { projectName: currentProject.metadata.name, image: selectedImage, notebookSize: selectedSize, - acceleratorProfile: notebookAcceleratorProfileState, + initialAcceleratorProfile: notebookAcceleratorProfileState, + selectedAcceleratorProfile, volumes: [], volumeMounts: [], existingTolerations: existingNotebook?.spec.template.spec.tolerations || [], diff --git a/frontend/src/pages/projects/types.ts b/frontend/src/pages/projects/types.ts index 4ffdceb253..e8e6be0de3 100644 --- a/frontend/src/pages/projects/types.ts +++ b/frontend/src/pages/projects/types.ts @@ -12,6 +12,7 @@ import { import { ValueOf } from '~/typeHelpers'; import { AWSSecretKind } from '~/k8sTypes'; import { AcceleratorProfileState } from '~/utilities/useAcceleratorProfileState'; +import { AcceleratorProfileSelectFieldState } from '~/pages/notebookController/screens/server/AcceleratorProfileSelectField'; import { AwsKeys } from './dataConnections/const'; export type UpdateObjectAtPropAndValue = (propKey: keyof T, propValue: ValueOf) => void; @@ -65,7 +66,8 @@ export type StartNotebookData = { projectName: string; notebookName: string; notebookSize: NotebookSize; - acceleratorProfile: AcceleratorProfileState; + initialAcceleratorProfile: AcceleratorProfileState; + selectedAcceleratorProfile: AcceleratorProfileSelectFieldState; image: ImageStreamAndVersion; volumes?: Volume[]; volumeMounts?: VolumeMount[]; diff --git a/frontend/src/utilities/tolerations.ts b/frontend/src/utilities/tolerations.ts index 44ab2962d5..c4ba281faf 100644 --- a/frontend/src/utilities/tolerations.ts +++ b/frontend/src/utilities/tolerations.ts @@ -2,6 +2,7 @@ import { Patch } from '@openshift/dynamic-plugin-sdk-utils'; import _ from 'lodash-es'; import { Toleration, TolerationEffect, TolerationOperator, TolerationSettings } from '~/types'; import { DashboardConfigKind, NotebookKind } from '~/k8sTypes'; +import { AcceleratorProfileSelectFieldState } from '~/pages/notebookController/screens/server/AcceleratorProfileSelectField'; import { AcceleratorProfileState } from './useAcceleratorProfileState'; export type TolerationChanges = { @@ -11,24 +12,25 @@ export type TolerationChanges = { export const determineTolerations = ( tolerationSettings?: TolerationSettings, - acceleratorProfileState?: AcceleratorProfileState, + initialAcceleratorProfile?: AcceleratorProfileState, + selectedAcceleratorProfile?: AcceleratorProfileSelectFieldState, existingTolerations?: Toleration[], ): Toleration[] => { let tolerations = existingTolerations || []; // remove old accelerator tolerations if they exist - if (acceleratorProfileState?.initialAcceleratorProfile) { + if (initialAcceleratorProfile?.acceleratorProfile) { tolerations = tolerations.filter( (t) => - !acceleratorProfileState.initialAcceleratorProfile?.spec.tolerations?.some((t2) => + !initialAcceleratorProfile.acceleratorProfile?.spec.tolerations?.some((t2) => _.isEqual(t2, t), ), ); } // add new accelerator tolerations if they exist - if (acceleratorProfileState?.acceleratorProfile?.spec.tolerations) { - tolerations.push(...acceleratorProfileState.acceleratorProfile.spec.tolerations); + if (selectedAcceleratorProfile?.profile?.spec.tolerations) { + tolerations.push(...selectedAcceleratorProfile.profile.spec.tolerations); } // remove duplicated tolerations @@ -61,6 +63,7 @@ export const computeNotebooksTolerations = ( const settings = determineTolerations( dashboardConfig.spec.notebookController?.notebookTolerationSettings, undefined, + undefined, tolerations, ); diff --git a/frontend/src/utilities/useAcceleratorProfileState.ts b/frontend/src/utilities/useAcceleratorProfileState.ts index 239de80f51..d4798d6c89 100644 --- a/frontend/src/utilities/useAcceleratorProfileState.ts +++ b/frontend/src/utilities/useAcceleratorProfileState.ts @@ -9,127 +9,126 @@ import { TolerationEffect, TolerationOperator, } from '~/types'; -import useGenericObjectState, { GenericObjectState } from '~/utilities/useGenericObjectState'; import { getAcceleratorProfileCount, isEnumMember } from '~/utilities/utils'; export type AcceleratorProfileState = { - acceleratorProfile?: AcceleratorProfileKind; acceleratorProfiles: AcceleratorProfileKind[]; - initialAcceleratorProfile?: AcceleratorProfileKind; - useExisting: boolean; count: number; - additionalOptions?: { - useExisting?: boolean; - useDisabled?: AcceleratorProfileKind; - }; -}; +} & ( + | { acceleratorProfile?: AcceleratorProfileKind; unknownProfileDetected: false } + | { + acceleratorProfile: undefined; + unknownProfileDetected: true; + } +); const useAcceleratorProfileState = ( resources?: ContainerResources, tolerations?: Toleration[], existingAcceleratorProfileName?: string, -): GenericObjectState => { - const [acceleratorProfileState, setData, resetData] = - useGenericObjectState({ - acceleratorProfile: undefined, - acceleratorProfiles: [], - initialAcceleratorProfile: undefined, - count: 0, - useExisting: false, - }); - +): AcceleratorProfileState => { const { dashboardNamespace } = useDashboardNamespace(); - const [acceleratorProfiles, loaded, loadError, refresh] = - useAcceleratorProfiles(dashboardNamespace); - - React.useEffect(() => { - if (loaded && !loadError) { - setData('acceleratorProfiles', acceleratorProfiles); - - // Exit early if no resources = not in edit mode - if (!resources) { - return; - } + const [acceleratorProfiles, loaded, loadError] = useAcceleratorProfiles(dashboardNamespace); + return React.useMemo(() => { + if (!loaded || loadError) { + return { + acceleratorProfiles: [], + acceleratorProfile: undefined, + count: 0, + unknownProfileDetected: false, + }; + } + // Exit early if no resources = not in edit mode + if (resources) { const acceleratorProfile = acceleratorProfiles.find( (cr) => cr.metadata.name === existingAcceleratorProfileName, ); if (acceleratorProfile) { - setData('acceleratorProfile', acceleratorProfile); - setData('initialAcceleratorProfile', acceleratorProfile); - setData('count', getAcceleratorProfileCount(acceleratorProfile, resources)); - if (!acceleratorProfile.spec.enabled) { - setData('additionalOptions', { useDisabled: acceleratorProfile }); - } - } else { - // check if there is accelerator usage in the container - // this is to handle the case where the accelerator is disabled, deleted, or empty - const possibleAcceleratorRequests = Object.entries(resources.requests ?? {}) - .filter(([key]) => !isEnumMember(key, ContainerResourceAttributes)) - .map(([key, value]) => ({ identifier: key, count: value })); - if (possibleAcceleratorRequests.length > 0) { - // check if they are just using the nvidia.com/gpu - // if so, lets migrate them over to using the migrated-gpu accelerator profile if it exists - const nvidiaAcceleratorRequests = possibleAcceleratorRequests.find( - (request) => request.identifier === 'nvidia.com/gpu', - ); + return { + acceleratorProfiles, + acceleratorProfile, + count: getAcceleratorProfileCount(acceleratorProfile, resources), + unknownProfileDetected: false, + }; + } + // check if there is accelerator usage in the container + // this is to handle the case where the accelerator is disabled, deleted, or empty + const possibleAcceleratorRequests = Object.entries(resources.requests ?? {}) + .filter(([key]) => !isEnumMember(key, ContainerResourceAttributes)) + .map(([key, value]) => ({ identifier: key, count: value })); + if (possibleAcceleratorRequests.length > 0) { + // check if they are just using the nvidia.com/gpu + // if so, lets migrate them over to using the migrated-gpu accelerator profile if it exists + const nvidiaAcceleratorRequests = possibleAcceleratorRequests.find( + (request) => request.identifier === 'nvidia.com/gpu', + ); - if ( - nvidiaAcceleratorRequests && - tolerations?.some( - (toleration) => - toleration.key === 'nvidia.com/gpu' && - toleration.operator === 'Exists' && - toleration.effect === 'NoSchedule', - ) - ) { - const migratedAcceleratorProfile = acceleratorProfiles.find( - (cr) => cr.metadata.name === 'migrated-gpu', - ); + if ( + nvidiaAcceleratorRequests && + tolerations?.some( + (toleration) => + toleration.key === 'nvidia.com/gpu' && + toleration.operator === 'Exists' && + toleration.effect === 'NoSchedule', + ) + ) { + const migratedAcceleratorProfile = acceleratorProfiles.find( + (cr) => cr.metadata.name === 'migrated-gpu', + ); - if (migratedAcceleratorProfile) { - setData('acceleratorProfile', migratedAcceleratorProfile); - setData('initialAcceleratorProfile', migratedAcceleratorProfile); - setData('count', Number(nvidiaAcceleratorRequests.count ?? 0)); - if (!migratedAcceleratorProfile.spec.enabled) { - setData('additionalOptions', { useDisabled: acceleratorProfile }); - } - } else { - // create a fake accelerator to use - const fakeAcceleratorProfile: AcceleratorProfileKind = { - apiVersion: 'dashboard.opendatahub.io/v1', - kind: 'AcceleratorProfile', - metadata: { - name: 'migrated-gpu', - }, - spec: { - identifier: 'nvidia.com/gpu', - displayName: 'NVIDIA GPU', - enabled: true, - tolerations: [ - { - key: 'nvidia.com/gpu', - operator: TolerationOperator.EXISTS, - effect: TolerationEffect.NO_SCHEDULE, - }, - ], + if (migratedAcceleratorProfile) { + return { + acceleratorProfiles, + acceleratorProfile: migratedAcceleratorProfile, + count: getAcceleratorProfileCount(migratedAcceleratorProfile, resources), + unknownProfileDetected: false, + }; + } + // create a fake accelerator to use + const fakeAcceleratorProfile: AcceleratorProfileKind = { + apiVersion: 'dashboard.opendatahub.io/v1', + kind: 'AcceleratorProfile', + metadata: { + name: 'migrated-gpu', + }, + spec: { + identifier: 'nvidia.com/gpu', + displayName: 'NVIDIA GPU', + enabled: true, + tolerations: [ + { + key: 'nvidia.com/gpu', + operator: TolerationOperator.EXISTS, + effect: TolerationEffect.NO_SCHEDULE, }, - }; + ], + }, + }; - setData('acceleratorProfile', fakeAcceleratorProfile); - setData('acceleratorProfiles', [fakeAcceleratorProfile, ...acceleratorProfiles]); - setData('initialAcceleratorProfile', fakeAcceleratorProfile); - setData('count', Number(nvidiaAcceleratorRequests.count ?? 0)); - } - } else { - // fallback to using the existing accelerator - setData('useExisting', true); - setData('additionalOptions', { useExisting: true }); - } + return { + acceleratorProfiles: [fakeAcceleratorProfile, ...acceleratorProfiles], + acceleratorProfile: fakeAcceleratorProfile, + count: Number(nvidiaAcceleratorRequests.count ?? 0), + unknownProfileDetected: false, + }; } + return { + acceleratorProfiles, + acceleratorProfile: undefined, + count: 0, + unknownProfileDetected: true, + }; } } + + return { + acceleratorProfiles, + acceleratorProfile: undefined, + count: 0, + unknownProfileDetected: false, + }; }, [ acceleratorProfiles, loaded, @@ -137,15 +136,7 @@ const useAcceleratorProfileState = ( resources, tolerations, existingAcceleratorProfileName, - setData, ]); - - const resetDataAndRefresh = React.useCallback(() => { - resetData(); - refresh(); - }, [refresh, resetData]); - - return [acceleratorProfileState, setData, resetDataAndRefresh]; }; export default useAcceleratorProfileState; From c6bd57dce8d537f073cb3fbc97dff4efc46fe9ea Mon Sep 17 00:00:00 2001 From: Yulia Krimerman Date: Wed, 21 Aug 2024 16:38:06 -0400 Subject: [PATCH 04/14] removed the use of Patch Body (#3110) --- .../modelRegistry/modelVersionArchive.cy.ts | 12 ---- .../registeredModelArchive.cy.ts | 16 ----- .../ModelVersionDetailsHeaderActions.tsx | 6 +- .../ModelVersionDetailsView.tsx | 23 +++---- .../ModelVersions/ModelDetailsView.tsx | 21 +++---- .../ModelVersionsHeaderActions.tsx | 6 +- .../ModelVersions/ModelVersionsTableRow.tsx | 11 ++-- .../ModelVersionArchiveDetails.tsx | 6 +- .../RegisteredModelTableRow.tsx | 11 ++-- .../RegisteredModelArchiveDetails.tsx | 6 +- .../screens/__tests__/utils.spec.ts | 63 ------------------- .../src/pages/modelRegistry/screens/utils.ts | 32 ---------- 12 files changed, 40 insertions(+), 173 deletions(-) diff --git a/frontend/src/__tests__/cypress/cypress/tests/mocked/modelRegistry/modelVersionArchive.cy.ts b/frontend/src/__tests__/cypress/cypress/tests/mocked/modelRegistry/modelVersionArchive.cy.ts index 7435cf7d63..a15d58f1b8 100644 --- a/frontend/src/__tests__/cypress/cypress/tests/mocked/modelRegistry/modelVersionArchive.cy.ts +++ b/frontend/src/__tests__/cypress/cypress/tests/mocked/modelRegistry/modelVersionArchive.cy.ts @@ -190,10 +190,7 @@ describe('Restoring archive version', () => { cy.wait('@versionRestored').then((interception) => { expect(interception.request.body).to.eql({ - author: 'Test author', - customProperties: {}, state: 'LIVE', - description: 'Description of model version', }); }); }); @@ -219,10 +216,7 @@ describe('Restoring archive version', () => { cy.wait('@versionRestored').then((interception) => { expect(interception.request.body).to.eql({ - author: 'Test author', - customProperties: {}, state: 'LIVE', - description: 'Description of model version', }); }); }); @@ -252,10 +246,7 @@ describe('Archiving version', () => { archiveVersionModal.findArchiveButton().should('be.enabled').click(); cy.wait('@versionArchived').then((interception) => { expect(interception.request.body).to.eql({ - author: 'Test author', - customProperties: {}, state: 'ARCHIVED', - description: 'Description of model version', }); }); }); @@ -285,10 +276,7 @@ describe('Archiving version', () => { archiveVersionModal.findArchiveButton().should('be.enabled').click(); cy.wait('@versionArchived').then((interception) => { expect(interception.request.body).to.eql({ - author: 'Test author', - customProperties: {}, state: 'ARCHIVED', - description: 'Description of model version', }); }); }); diff --git a/frontend/src/__tests__/cypress/cypress/tests/mocked/modelRegistry/registeredModelArchive.cy.ts b/frontend/src/__tests__/cypress/cypress/tests/mocked/modelRegistry/registeredModelArchive.cy.ts index 1c8d9acf98..47fffc29cb 100644 --- a/frontend/src/__tests__/cypress/cypress/tests/mocked/modelRegistry/registeredModelArchive.cy.ts +++ b/frontend/src/__tests__/cypress/cypress/tests/mocked/modelRegistry/registeredModelArchive.cy.ts @@ -167,11 +167,7 @@ describe('Restoring archive model', () => { cy.wait('@modelRestored').then((interception) => { expect(interception.request.body).to.eql({ - customProperties: {}, - description: '', - externalID: '1234132asdfasdf', state: 'LIVE', - owner: 'Author 1', }); }); }); @@ -197,11 +193,7 @@ describe('Restoring archive model', () => { cy.wait('@modelRestored').then((interception) => { expect(interception.request.body).to.eql({ - customProperties: {}, - description: '', - externalID: '1234132asdfasdf', state: 'LIVE', - owner: 'Author 1', }); }); }); @@ -231,11 +223,7 @@ describe('Archiving model', () => { archiveModelModal.findArchiveButton().should('be.enabled').click(); cy.wait('@modelArchived').then((interception) => { expect(interception.request.body).to.eql({ - customProperties: {}, - description: '', - externalID: '1234132asdfasdf', state: 'ARCHIVED', - owner: 'Author 1', }); }); }); @@ -265,11 +253,7 @@ describe('Archiving model', () => { archiveModelModal.findArchiveButton().should('be.enabled').click(); cy.wait('@modelArchived').then((interception) => { expect(interception.request.body).to.eql({ - customProperties: {}, - description: '', - externalID: '1234132asdfasdf', state: 'ARCHIVED', - owner: 'Author 1', }); }); }); diff --git a/frontend/src/pages/modelRegistry/screens/ModelVersionDetails/ModelVersionDetailsHeaderActions.tsx b/frontend/src/pages/modelRegistry/screens/ModelVersionDetails/ModelVersionDetailsHeaderActions.tsx index b8b41d4690..f74f4b69b1 100644 --- a/frontend/src/pages/modelRegistry/screens/ModelVersionDetails/ModelVersionDetailsHeaderActions.tsx +++ b/frontend/src/pages/modelRegistry/screens/ModelVersionDetails/ModelVersionDetailsHeaderActions.tsx @@ -4,7 +4,6 @@ import { useNavigate } from 'react-router'; import { ArchiveModelVersionModal } from '~/pages/modelRegistry/screens/components/ArchiveModelVersionModal'; import { ModelRegistryContext } from '~/concepts/modelRegistry/context/ModelRegistryContext'; import { ModelVersion, ModelState } from '~/concepts/modelRegistry/types'; -import { getPatchBodyForModelVersion } from '~/pages/modelRegistry/screens/utils'; import { ModelRegistrySelectorContext } from '~/concepts/modelRegistry/context/ModelRegistrySelectorContext'; import { modelVersionArchiveDetailsUrl, @@ -92,8 +91,9 @@ const ModelVersionsDetailsHeaderActions: React.FC diff --git a/frontend/src/pages/modelRegistry/screens/ModelVersionDetails/ModelVersionDetailsView.tsx b/frontend/src/pages/modelRegistry/screens/ModelVersionDetails/ModelVersionDetailsView.tsx index c7c5a84e54..ac602a30d8 100644 --- a/frontend/src/pages/modelRegistry/screens/ModelVersionDetails/ModelVersionDetailsView.tsx +++ b/frontend/src/pages/modelRegistry/screens/ModelVersionDetails/ModelVersionDetailsView.tsx @@ -5,11 +5,7 @@ import DashboardDescriptionListGroup from '~/components/DashboardDescriptionList import EditableTextDescriptionListGroup from '~/components/EditableTextDescriptionListGroup'; import EditableLabelsDescriptionListGroup from '~/components/EditableLabelsDescriptionListGroup'; import ModelPropertiesDescriptionListGroup from '~/pages/modelRegistry/screens/ModelPropertiesDescriptionListGroup'; -import { - getLabels, - getPatchBodyForModelVersion, - mergeUpdatedLabels, -} from '~/pages/modelRegistry/screens/utils'; +import { getLabels, mergeUpdatedLabels } from '~/pages/modelRegistry/screens/utils'; import useModelArtifactsByVersionId from '~/concepts/modelRegistry/apiHooks/useModelArtifactsByVersionId'; import { ModelRegistryContext } from '~/concepts/modelRegistry/context/ModelRegistryContext'; import ModelTimestamp from '~/pages/modelRegistry/screens/components/ModelTimestamp'; @@ -44,8 +40,9 @@ const ModelVersionDetailsView: React.FC = ({ apiState.api .patchModelVersion( {}, - // TODO remove the getPatchBody* functions when https://issues.redhat.com/browse/RHOAIENG-6652 is resolved - getPatchBodyForModelVersion(mv, { description: value }), + { + description: value, + }, mv.id, ) .then(refresh) @@ -58,10 +55,9 @@ const ModelVersionDetailsView: React.FC = ({ apiState.api .patchModelVersion( {}, - // TODO remove the getPatchBody* functions when https://issues.redhat.com/browse/RHOAIENG-6652 is resolved - getPatchBodyForModelVersion(mv, { + { customProperties: mergeUpdatedLabels(mv.customProperties, editedLabels), - }), + }, mv.id, ) .then(refresh) @@ -71,12 +67,7 @@ const ModelVersionDetailsView: React.FC = ({ customProperties={mv.customProperties} saveEditedCustomProperties={(editedProperties) => apiState.api - .patchModelVersion( - {}, - // TODO remove the getPatchBody* functions when https://issues.redhat.com/browse/RHOAIENG-6652 is resolved - getPatchBodyForModelVersion(mv, { customProperties: editedProperties }), - mv.id, - ) + .patchModelVersion({}, { customProperties: editedProperties }, mv.id) .then(refresh) } /> diff --git a/frontend/src/pages/modelRegistry/screens/ModelVersions/ModelDetailsView.tsx b/frontend/src/pages/modelRegistry/screens/ModelVersions/ModelDetailsView.tsx index 1bef66780c..bdf7f79801 100644 --- a/frontend/src/pages/modelRegistry/screens/ModelVersions/ModelDetailsView.tsx +++ b/frontend/src/pages/modelRegistry/screens/ModelVersions/ModelDetailsView.tsx @@ -6,11 +6,7 @@ import DashboardDescriptionListGroup from '~/components/DashboardDescriptionList import EditableTextDescriptionListGroup from '~/components/EditableTextDescriptionListGroup'; import EditableLabelsDescriptionListGroup from '~/components/EditableLabelsDescriptionListGroup'; import ModelTimestamp from '~/pages/modelRegistry/screens/components/ModelTimestamp'; -import { - getLabels, - getPatchBodyForRegisteredModel, - mergeUpdatedLabels, -} from '~/pages/modelRegistry/screens/utils'; +import { getLabels, mergeUpdatedLabels } from '~/pages/modelRegistry/screens/utils'; import ModelPropertiesDescriptionListGroup from '~/pages/modelRegistry/screens/ModelPropertiesDescriptionListGroup'; type ModelDetailsViewProps = { @@ -36,8 +32,9 @@ const ModelDetailsView: React.FC = ({ registeredModel: rm apiState.api .patchRegisteredModel( {}, - // TODO remove the getPatchBody* functions when https://issues.redhat.com/browse/RHOAIENG-6652 is resolved - getPatchBodyForRegisteredModel(rm, { description: value }), + { + description: value, + }, rm.id, ) .then(refresh) @@ -50,10 +47,9 @@ const ModelDetailsView: React.FC = ({ registeredModel: rm apiState.api .patchRegisteredModel( {}, - // TODO remove the getPatchBody* functions when https://issues.redhat.com/browse/RHOAIENG-6652 is resolved - getPatchBodyForRegisteredModel(rm, { + { customProperties: mergeUpdatedLabels(rm.customProperties, editedLabels), - }), + }, rm.id, ) .then(refresh) @@ -65,8 +61,9 @@ const ModelDetailsView: React.FC = ({ registeredModel: rm apiState.api .patchRegisteredModel( {}, - // TODO remove the getPatchBody* functions when https://issues.redhat.com/browse/RHOAIENG-6652 is resolved - getPatchBodyForRegisteredModel(rm, { customProperties: editedProperties }), + { + customProperties: editedProperties, + }, rm.id, ) .then(refresh) diff --git a/frontend/src/pages/modelRegistry/screens/ModelVersions/ModelVersionsHeaderActions.tsx b/frontend/src/pages/modelRegistry/screens/ModelVersions/ModelVersionsHeaderActions.tsx index 0ab9c8826b..3bc6ccb344 100644 --- a/frontend/src/pages/modelRegistry/screens/ModelVersions/ModelVersionsHeaderActions.tsx +++ b/frontend/src/pages/modelRegistry/screens/ModelVersions/ModelVersionsHeaderActions.tsx @@ -10,7 +10,6 @@ import { import { useNavigate } from 'react-router'; import { ModelRegistryContext } from '~/concepts/modelRegistry/context/ModelRegistryContext'; import { ArchiveRegisteredModelModal } from '~/pages/modelRegistry/screens/components/ArchiveRegisteredModelModal'; -import { getPatchBodyForRegisteredModel } from '~/pages/modelRegistry/screens/utils'; import { registeredModelsUrl } from '~/pages/modelRegistry/screens/routeUtils'; import { ModelRegistrySelectorContext } from '~/concepts/modelRegistry/context/ModelRegistrySelectorContext'; import { RegisteredModel, ModelState } from '~/concepts/modelRegistry/types'; @@ -70,8 +69,9 @@ const ModelVersionsHeaderActions: React.FC = ({ apiState.api .patchRegisteredModel( {}, - // TODO remove the getPatchBody* functions when https://issues.redhat.com/browse/RHOAIENG-6652 is resolved - getPatchBodyForRegisteredModel(rm, { state: ModelState.ARCHIVED }), + { + state: ModelState.ARCHIVED, + }, rm.id, ) .then(() => navigate(registeredModelsUrl(preferredModelRegistry?.metadata.name))) diff --git a/frontend/src/pages/modelRegistry/screens/ModelVersions/ModelVersionsTableRow.tsx b/frontend/src/pages/modelRegistry/screens/ModelVersions/ModelVersionsTableRow.tsx index 8a1eb60916..fc97221cb6 100644 --- a/frontend/src/pages/modelRegistry/screens/ModelVersions/ModelVersionsTableRow.tsx +++ b/frontend/src/pages/modelRegistry/screens/ModelVersions/ModelVersionsTableRow.tsx @@ -13,7 +13,6 @@ import { } from '~/pages/modelRegistry/screens/routeUtils'; import { ArchiveModelVersionModal } from '~/pages/modelRegistry/screens/components/ArchiveModelVersionModal'; import { ModelRegistryContext } from '~/concepts/modelRegistry/context/ModelRegistryContext'; -import { getPatchBodyForModelVersion } from '~/pages/modelRegistry/screens/utils'; import { RestoreModelVersionModal } from '~/pages/modelRegistry/screens/components/RestoreModelVersionModal'; import DeployRegisteredModelModal from '~/pages/modelRegistry/screens/components/DeployRegisteredModelModal'; @@ -98,8 +97,9 @@ const ModelVersionsTableRow: React.FC = ({ apiState.api .patchModelVersion( {}, - // TODO remove the getPatchBody* functions when https://issues.redhat.com/browse/RHOAIENG-6652 is resolved - getPatchBodyForModelVersion(mv, { state: ModelState.ARCHIVED }), + { + state: ModelState.ARCHIVED, + }, mv.id, ) .then(refresh) @@ -127,8 +127,9 @@ const ModelVersionsTableRow: React.FC = ({ apiState.api .patchModelVersion( {}, - // TODO remove the getPatchBody* functions when https://issues.redhat.com/browse/RHOAIENG-6652 is resolved - getPatchBodyForModelVersion(mv, { state: ModelState.LIVE }), + { + state: ModelState.LIVE, + }, mv.id, ) .then(() => diff --git a/frontend/src/pages/modelRegistry/screens/ModelVersionsArchive/ModelVersionArchiveDetails.tsx b/frontend/src/pages/modelRegistry/screens/ModelVersionsArchive/ModelVersionArchiveDetails.tsx index 970675b24b..19c3f5319d 100644 --- a/frontend/src/pages/modelRegistry/screens/ModelVersionsArchive/ModelVersionArchiveDetails.tsx +++ b/frontend/src/pages/modelRegistry/screens/ModelVersionsArchive/ModelVersionArchiveDetails.tsx @@ -10,7 +10,6 @@ import { ModelVersionDetailsTab } from '~/pages/modelRegistry/screens/ModelVersi import ModelVersionDetailsTabs from '~/pages/modelRegistry/screens/ModelVersionDetails/ModelVersionDetailsTabs'; import { RestoreModelVersionModal } from '~/pages/modelRegistry/screens/components/RestoreModelVersionModal'; import { ModelRegistryContext } from '~/concepts/modelRegistry/context/ModelRegistryContext'; -import { getPatchBodyForModelVersion } from '~/pages/modelRegistry/screens/utils'; import { ModelState } from '~/concepts/modelRegistry/types'; import useInferenceServices from '~/pages/modelServing/useInferenceServices'; import useServingRuntimes from '~/pages/modelServing/useServingRuntimes'; @@ -92,8 +91,9 @@ const ModelVersionsArchiveDetails: React.FC = apiState.api .patchModelVersion( {}, - // TODO remove the getPatchBody* functions when https://issues.redhat.com/browse/RHOAIENG-6652 is resolved - getPatchBodyForModelVersion(mv, { state: ModelState.LIVE }), + { + state: ModelState.LIVE, + }, mv.id, ) .then(() => diff --git a/frontend/src/pages/modelRegistry/screens/RegisteredModels/RegisteredModelTableRow.tsx b/frontend/src/pages/modelRegistry/screens/RegisteredModels/RegisteredModelTableRow.tsx index 86eafda0bf..573203a1fc 100644 --- a/frontend/src/pages/modelRegistry/screens/RegisteredModels/RegisteredModelTableRow.tsx +++ b/frontend/src/pages/modelRegistry/screens/RegisteredModels/RegisteredModelTableRow.tsx @@ -12,7 +12,6 @@ import { import ModelLabels from '~/pages/modelRegistry/screens/components/ModelLabels'; import { ModelRegistryContext } from '~/concepts/modelRegistry/context/ModelRegistryContext'; import { ArchiveRegisteredModelModal } from '~/pages/modelRegistry/screens/components/ArchiveRegisteredModelModal'; -import { getPatchBodyForRegisteredModel } from '~/pages/modelRegistry/screens/utils'; import { RestoreRegisteredModelModal } from '~/pages/modelRegistry/screens/components/RestoreRegisteredModel'; type RegisteredModelTableRowProps = { @@ -86,8 +85,9 @@ const RegisteredModelTableRow: React.FC = ({ apiState.api .patchRegisteredModel( {}, - // TODO remove the getPatchBody* functions when https://issues.redhat.com/browse/RHOAIENG-6652 is resolved - getPatchBodyForRegisteredModel(rm, { state: ModelState.ARCHIVED }), + { + state: ModelState.ARCHIVED, + }, rm.id, ) .then(refresh) @@ -101,8 +101,9 @@ const RegisteredModelTableRow: React.FC = ({ apiState.api .patchRegisteredModel( {}, - // TODO remove the getPatchBody* functions when https://issues.redhat.com/browse/RHOAIENG-6652 is resolved - getPatchBodyForRegisteredModel(rm, { state: ModelState.LIVE }), + { + state: ModelState.LIVE, + }, rm.id, ) .then(() => diff --git a/frontend/src/pages/modelRegistry/screens/RegisteredModelsArchive/RegisteredModelArchiveDetails.tsx b/frontend/src/pages/modelRegistry/screens/RegisteredModelsArchive/RegisteredModelArchiveDetails.tsx index 2488e1a200..90f1676808 100644 --- a/frontend/src/pages/modelRegistry/screens/RegisteredModelsArchive/RegisteredModelArchiveDetails.tsx +++ b/frontend/src/pages/modelRegistry/screens/RegisteredModelsArchive/RegisteredModelArchiveDetails.tsx @@ -6,7 +6,6 @@ import { ModelRegistrySelectorContext } from '~/concepts/modelRegistry/context/M import { registeredModelUrl } from '~/pages/modelRegistry/screens/routeUtils'; import useRegisteredModelById from '~/concepts/modelRegistry/apiHooks/useRegisteredModelById'; import { ModelRegistryContext } from '~/concepts/modelRegistry/context/ModelRegistryContext'; -import { getPatchBodyForRegisteredModel } from '~/pages/modelRegistry/screens/utils'; import { ModelState } from '~/concepts/modelRegistry/types'; import { RestoreRegisteredModelModal } from '~/pages/modelRegistry/screens/components/RestoreRegisteredModel'; import ModelVersionsTabs from '~/pages/modelRegistry/screens/ModelVersions/ModelVersionsTabs'; @@ -84,8 +83,9 @@ const RegisteredModelsArchiveDetails: React.FC diff --git a/frontend/src/pages/modelRegistry/screens/__tests__/utils.spec.ts b/frontend/src/pages/modelRegistry/screens/__tests__/utils.spec.ts index 6fd4bc05c8..88aea0175c 100644 --- a/frontend/src/pages/modelRegistry/screens/__tests__/utils.spec.ts +++ b/frontend/src/pages/modelRegistry/screens/__tests__/utils.spec.ts @@ -15,7 +15,6 @@ import { getProperties, mergeUpdatedProperty, mergeUpdatedLabels, - getPatchBody, filterRegisteredModels, } from '~/pages/modelRegistry/screens/utils'; import { SearchType } from '~/concepts/dashboard/DashboardSearchField'; @@ -262,68 +261,6 @@ describe('mergeUpdatedProperty', () => { }); }); -describe('getPatchBody', () => { - it('returns a given RegisteredModel with id/name/timestamps removed, customProperties updated and other values unchanged', () => { - const registeredModel = mockRegisteredModel({ - id: '1', - owner: 'Author 1', - name: 'test-model', - description: 'Description here', - labels: [], - state: ModelState.LIVE, - }); - const result = getPatchBody( - registeredModel, - { - customProperties: { - label1: { string_value: '', metadataType: ModelRegistryMetadataType.STRING }, - }, - }, - [], - ); - expect(result).toEqual({ - description: 'Description here', - customProperties: { - label1: { string_value: '', metadataType: ModelRegistryMetadataType.STRING }, - }, - owner: 'Author 1', - state: ModelState.LIVE, - externalID: '1234132asdfasdf', - } satisfies Partial); - }); - - it('returns a given ModelVersion with id/name/timestamps removed, description updated and other values unchanged', () => { - const modelVersion = mockModelVersion({ - author: 'Test author', - registeredModelId: '1', - }); - const result = getPatchBody(modelVersion, { description: 'New description' }, []); - expect(result).toEqual({ - author: 'Test author', - registeredModelId: '1', - description: 'New description', - customProperties: {}, - state: ModelState.LIVE, - } satisfies Partial); - }); - - it('excludes given additional properties', () => { - const modelVersion = mockModelVersion({ - author: 'Test author', - registeredModelId: '1', - }); - const result = getPatchBody(modelVersion, { description: 'New description' }, [ - 'registeredModelId', - ]); - expect(result).toEqual({ - author: 'Test author', - description: 'New description', - customProperties: {}, - state: ModelState.LIVE, - } satisfies Partial); - }); -}); - describe('filterModelVersions', () => { const modelVersions: ModelVersion[] = [ mockModelVersion({ name: 'Test 1', state: ModelState.ARCHIVED }), diff --git a/frontend/src/pages/modelRegistry/screens/utils.ts b/frontend/src/pages/modelRegistry/screens/utils.ts index fdf5297d65..f6cad50fe4 100644 --- a/frontend/src/pages/modelRegistry/screens/utils.ts +++ b/frontend/src/pages/modelRegistry/screens/utils.ts @@ -1,6 +1,5 @@ import { SearchType } from '~/concepts/dashboard/DashboardSearchField'; import { - ModelRegistryBase, ModelRegistryCustomProperties, ModelRegistryMetadataType, ModelRegistryStringCustomProperties, @@ -76,37 +75,6 @@ export const mergeUpdatedProperty = ( return customPropertiesCopy; }; -// Returns a patch payload for a Model Registry object, retaining all mutable fields but excluding internal fields to prevent errors -// TODO this will not be necessary if the backend eventually merges objects on PATCH requests. See https://issues.redhat.com/browse/RHOAIENG-6652 -export const getPatchBody = ( - existingObj: T, - updates: Partial, - otherExcludedKeys: (keyof T)[], -): Partial => { - const objCopy = { ...existingObj }; - const excludedKeys: (keyof T)[] = [ - 'id', - 'name', - 'createTimeSinceEpoch', - 'lastUpdateTimeSinceEpoch', - ...otherExcludedKeys, - ]; - excludedKeys.forEach((key) => { - delete objCopy[key]; - }); - return { ...objCopy, ...updates }; -}; - -export const getPatchBodyForRegisteredModel = ( - existing: RegisteredModel, - updates: Partial, -): Partial => getPatchBody(existing, updates, []); - -export const getPatchBodyForModelVersion = ( - existing: ModelVersion, - updates: Partial, -): Partial => getPatchBody(existing, updates, ['registeredModelId']); - export const filterModelVersions = ( unfilteredModelVersions: ModelVersion[], search: string, From af2099ab97a5eeb4e8229e5ed688deb9101536f6 Mon Sep 17 00:00:00 2001 From: "Heiko W. Rupp" Date: Wed, 21 Aug 2024 22:49:06 +0200 Subject: [PATCH 05/14] RHOAIENG-9480 Update useTrackUser for Developer Sandbox (#3107) --- .../api/groups-config/groupsConfigUtil.ts | 6 +++--- backend/src/routes/api/notebooks/utils.ts | 4 ++-- .../routes/api/status/adminAllowedUsers.ts | 5 +++-- backend/src/routes/api/status/statusUtils.ts | 5 +++-- backend/src/types.ts | 1 + backend/src/utils/fileUtils.ts | 8 +++---- backend/src/utils/notebookUtils.ts | 4 ++-- backend/src/utils/route-security.ts | 19 +++++++++-------- backend/src/utils/userUtils.ts | 19 ++++++++++++----- .../analyticsTracking/segmentIOUtils.tsx | 6 ++++-- .../analyticsTracking/trackingProperties.ts | 2 +- .../analyticsTracking/useSegmentTracking.ts | 3 ++- .../analyticsTracking/useTrackUser.ts | 21 ++++++++++++------- frontend/src/redux/types.ts | 1 + 14 files changed, 63 insertions(+), 41 deletions(-) diff --git a/backend/src/routes/api/groups-config/groupsConfigUtil.ts b/backend/src/routes/api/groups-config/groupsConfigUtil.ts index ee504b4bd8..8c125ebca4 100644 --- a/backend/src/routes/api/groups-config/groupsConfigUtil.ts +++ b/backend/src/routes/api/groups-config/groupsConfigUtil.ts @@ -8,7 +8,7 @@ import { KubeFastifyInstance, } from '../../../types'; import { getAllGroups, getGroupsCR, updateGroupsCR } from '../../../utils/groupsUtils'; -import { getUserName } from '../../../utils/userUtils'; +import { getUserInfo } from '../../../utils/userUtils'; import { isUserAdmin } from '../../../utils/adminUtils'; const SYSTEM_AUTHENTICATED = 'system:authenticated'; @@ -34,8 +34,8 @@ export const updateGroupsConfig = async ( const { customObjectsApi } = fastify.kube; const { namespace } = fastify.kube; - const username = await getUserName(fastify, request); - const isAdmin = await isUserAdmin(fastify, username, namespace); + const userInfo = await getUserInfo(fastify, request); + const isAdmin = await isUserAdmin(fastify, userInfo.userName, namespace); if (!isAdmin) { const error = createError(403, 'Error updating groups, user needs to be admin'); diff --git a/backend/src/routes/api/notebooks/utils.ts b/backend/src/routes/api/notebooks/utils.ts index d1bb712cf0..09f90e4afa 100644 --- a/backend/src/routes/api/notebooks/utils.ts +++ b/backend/src/routes/api/notebooks/utils.ts @@ -2,7 +2,7 @@ import { V1ContainerStatus, V1Pod, V1PodList } from '@kubernetes/client-node'; import { FastifyRequest } from 'fastify'; import { isHttpError } from '../../../utils'; import { KubeFastifyInstance, Notebook, NotebookData } from '../../../types'; -import { getUserName } from '../../../utils/userUtils'; +import { getUserInfo } from '../../../utils/userUtils'; import { createNotebook, generateNotebookNameFromUsername, @@ -69,7 +69,7 @@ export const enableNotebook = async ( ): Promise => { const notebookData = request.body; const { notebookNamespace } = getNamespaces(fastify); - const username = request.body.username || (await getUserName(fastify, request)); + const username = request.body.username || (await getUserInfo(fastify, request)).userName; const name = generateNotebookNameFromUsername(username); const url = request.headers.origin; diff --git a/backend/src/routes/api/status/adminAllowedUsers.ts b/backend/src/routes/api/status/adminAllowedUsers.ts index bd68b4ed81..89663678f7 100644 --- a/backend/src/routes/api/status/adminAllowedUsers.ts +++ b/backend/src/routes/api/status/adminAllowedUsers.ts @@ -1,6 +1,6 @@ import { FastifyRequest } from 'fastify'; import { KubeFastifyInstance } from '../../../types'; -import { getUserName } from '../../../utils/userUtils'; +import { getUserInfo } from '../../../utils/userUtils'; import { getAdminUserList, getAllowedUserList, @@ -66,7 +66,8 @@ export const getAllowedUsers = async ( request: FastifyRequest<{ Params: { namespace: string } }>, ): Promise => { const { namespace } = request.params; - const currentUser = await getUserName(fastify, request); + const userInfo = await getUserInfo(fastify, request); + const currentUser = userInfo.userName; const isAdmin = await isUserAdmin(fastify, currentUser, namespace); if (!isAdmin) { // Privileged call -- return nothing diff --git a/backend/src/routes/api/status/statusUtils.ts b/backend/src/routes/api/status/statusUtils.ts index c5d110fdc4..956cd51414 100644 --- a/backend/src/routes/api/status/statusUtils.ts +++ b/backend/src/routes/api/status/statusUtils.ts @@ -1,6 +1,6 @@ import { FastifyRequest } from 'fastify'; import { KubeFastifyInstance, KubeStatus } from '../../../types'; -import { getUserName } from '../../../utils/userUtils'; +import { getUserInfo } from '../../../utils/userUtils'; import { createCustomError } from '../../../utils/requestUtils'; import { isUserAdmin, isUserAllowed } from '../../../utils/adminUtils'; import { isImpersonating } from '../../../devFlags'; @@ -19,7 +19,7 @@ export const status = async ( const { server } = currentCluster; - const userName = await getUserName(fastify, request); + const { userName, userID } = await getUserInfo(fastify, request); const isAdmin = await isUserAdmin(fastify, userName, namespace); const isAllowed = isAdmin ? true : await isUserAllowed(fastify, userName); @@ -37,6 +37,7 @@ export const status = async ( currentUser, namespace, userName, + userID, clusterID, clusterBranding, isAdmin, diff --git a/backend/src/types.ts b/backend/src/types.ts index 3f62b32c40..e11a848ad7 100644 --- a/backend/src/types.ts +++ b/backend/src/types.ts @@ -276,6 +276,7 @@ export type KubeStatus = { currentUser: User; namespace: string; userName: string | string[]; + userID?: string; clusterID: string; clusterBranding: string; isAdmin: boolean; diff --git a/backend/src/utils/fileUtils.ts b/backend/src/utils/fileUtils.ts index cdde729da7..6777c6d432 100644 --- a/backend/src/utils/fileUtils.ts +++ b/backend/src/utils/fileUtils.ts @@ -1,7 +1,7 @@ import * as fs from 'fs'; import { KubeFastifyInstance, OauthFastifyRequest } from '../types'; import { LOG_DIR } from './constants'; -import { getUserName } from './userUtils'; +import { getUserInfo } from './userUtils'; import { getNamespaces } from './notebookUtils'; import { isUserAdmin } from './adminUtils'; @@ -27,13 +27,13 @@ export const logRequestDetails = ( }; const writeLogAsync = async () => { - const username = await getUserName(fastify, request); + const userInfo = await getUserInfo(fastify, request); const { dashboardNamespace } = getNamespaces(fastify); - const isAdmin = await isUserAdmin(fastify, username, dashboardNamespace); + const isAdmin = await isUserAdmin(fastify, userInfo.userName, dashboardNamespace); writeAdminLog(fastify, { ...data, - user: username, + user: userInfo.userName, isAdmin: isAdmin, }); }; diff --git a/backend/src/utils/notebookUtils.ts b/backend/src/utils/notebookUtils.ts index 319d4851d9..71c8397eac 100644 --- a/backend/src/utils/notebookUtils.ts +++ b/backend/src/utils/notebookUtils.ts @@ -20,7 +20,7 @@ import { TolerationOperator, VolumeMount, } from '../types'; -import { getUserName, usernameTranslate } from './userUtils'; +import { getUserInfo, usernameTranslate } from './userUtils'; import { createCustomError } from './requestUtils'; import { PatchUtils, @@ -389,7 +389,7 @@ export const stopNotebook = async ( Body: NotebookData; }>, ): Promise => { - const username = request.body.username || (await getUserName(fastify, request)); + const username = request.body.username || (await getUserInfo(fastify, request)).userName; const name = generateNotebookNameFromUsername(username); const { notebookNamespace } = getNamespaces(fastify); diff --git a/backend/src/utils/route-security.ts b/backend/src/utils/route-security.ts index 149518472a..66c560a78b 100644 --- a/backend/src/utils/route-security.ts +++ b/backend/src/utils/route-security.ts @@ -1,5 +1,5 @@ import { FastifyReply, FastifyRequest } from 'fastify'; -import { getOpenshiftUser, getUserName, usernameTranslate } from './userUtils'; +import { getOpenshiftUser, getUserInfo, usernameTranslate } from './userUtils'; import { createCustomError } from './requestUtils'; import { isUserAdmin } from './adminUtils'; import { getNamespaces } from './notebookUtils'; @@ -18,9 +18,9 @@ const testAdmin = async ( request: OauthFastifyRequest, needsAdmin: boolean, ): Promise => { - const username = await getUserName(fastify, request); + const userInfo = await getUserInfo(fastify, request); const { dashboardNamespace } = getNamespaces(fastify); - const isAdmin = await isUserAdmin(fastify, username, dashboardNamespace); + const isAdmin = await isUserAdmin(fastify, userInfo.userName, dashboardNamespace); if (isAdmin) { // User is an admin, pass to caller that we can bypass some logic return true; @@ -29,7 +29,7 @@ const testAdmin = async ( if (needsAdmin) { // Not an admin, route needs one -- reject fastify.log.error( - `A Non-Admin User (${username}) made a request against an endpoint that requires an admin.`, + `A Non-Admin User (${userInfo.userName}) made a request against an endpoint that requires an admin.`, ); throw createCustomError( 'Not Admin', @@ -68,8 +68,8 @@ const requestSecurityGuard = async ( name?: string, ): Promise => { const { notebookNamespace, dashboardNamespace } = getNamespaces(fastify); - const username = await getUserName(fastify, request); - const translatedUsername = usernameTranslate(username); + const userInfo = await getUserInfo(fastify, request); + const translatedUsername = usernameTranslate(userInfo.userName); const isReadRequest = request.method.toLowerCase() === 'get'; // Check to see if a request was made against one of our namespaces @@ -208,11 +208,12 @@ const handleSecurityOnRouteData = async ( } // Not getting a resource, mutating something that is not verify-able theirs -- log the user encase of malicious behaviour - const username = await getUserName(fastify, request); + const userInfo = await getUserInfo(fastify, request); const { dashboardNamespace } = getNamespaces(fastify); - const isAdmin = await isUserAdmin(fastify, username, dashboardNamespace); + const isAdmin = await isUserAdmin(fastify, userInfo.userName, dashboardNamespace); fastify.log.warn( - `${isAdmin ? 'Admin ' : ''}User ${username} interacted with a resource that was not secure.`, + `${isAdmin ? 'Admin ' : ''}User + ${userInfo.userName} interacted with a resource that was not secure.`, ); } }; diff --git a/backend/src/utils/userUtils.ts b/backend/src/utils/userUtils.ts index ade0f44fbb..6d7dc28a58 100644 --- a/backend/src/utils/userUtils.ts +++ b/backend/src/utils/userUtils.ts @@ -29,6 +29,9 @@ export type OpenShiftUser = { name: string; uid: string; resourceVersion: string; + annotations?: { + 'toolchain.dev.openshift.com/sso-user-id': string; + }; }; fullName: string; identities: string[]; @@ -86,21 +89,27 @@ export const getUser = async ( } }; -export const getUserName = async ( +export const getUserInfo = async ( fastify: KubeFastifyInstance, request: FastifyRequest, -): Promise => { +): Promise<{ userName: string; userID: string }> => { const { currentUser } = fastify.kube; try { const userOauth = await getUser(fastify, request); - return userOauth.metadata.name; + return { + userName: userOauth.metadata.name, + userID: userOauth.metadata.annotations?.['toolchain.dev.openshift.com/sso-user-id'], + }; } catch (e) { if (DEV_MODE) { if (isImpersonating()) { - return DEV_IMPERSONATE_USER ?? ''; + return { userName: DEV_IMPERSONATE_USER ?? '', userID: undefined }; } - return (currentUser.username || currentUser.name).split('/')[0]; + return { + userName: (currentUser.username || currentUser.name).split('/')[0], + userID: undefined, + }; } fastify.log.error(`Failed to retrieve username: ${errorHandler(e)}`); const error = createCustomError( diff --git a/frontend/src/concepts/analyticsTracking/segmentIOUtils.tsx b/frontend/src/concepts/analyticsTracking/segmentIOUtils.tsx index f55556ba6b..006d541a92 100644 --- a/frontend/src/concepts/analyticsTracking/segmentIOUtils.tsx +++ b/frontend/src/concepts/analyticsTracking/segmentIOUtils.tsx @@ -72,7 +72,9 @@ export const firePageEvent = (): void => { const clusterID = window.clusterID ?? ''; if (DEV_MODE) { /* eslint-disable-next-line no-console */ - console.log(`Page event triggered for version ${INTERNAL_DASHBOARD_VERSION}`); + console.log( + `Page event triggered for version ${INTERNAL_DASHBOARD_VERSION} : ${window.location.pathname}`, + ); } else if (window.analytics) { window.analytics.page( undefined, @@ -108,6 +110,6 @@ export const fireIdentifyEvent = (properties: IdentifyEventProperties): void => isAdmin: properties.isAdmin, canCreateProjects: properties.canCreateProjects, }; - window.analytics.identify(properties.anonymousID, traits); + window.analytics.identify(properties.userID, traits); } }; diff --git a/frontend/src/concepts/analyticsTracking/trackingProperties.ts b/frontend/src/concepts/analyticsTracking/trackingProperties.ts index 61a56add1e..06b87121da 100644 --- a/frontend/src/concepts/analyticsTracking/trackingProperties.ts +++ b/frontend/src/concepts/analyticsTracking/trackingProperties.ts @@ -4,7 +4,7 @@ export type ODHSegmentKey = { export type IdentifyEventProperties = { isAdmin: boolean; - anonymousID?: string; + userID?: string; canCreateProjects: boolean; }; diff --git a/frontend/src/concepts/analyticsTracking/useSegmentTracking.ts b/frontend/src/concepts/analyticsTracking/useSegmentTracking.ts index 7c5473d832..3c955f2809 100644 --- a/frontend/src/concepts/analyticsTracking/useSegmentTracking.ts +++ b/frontend/src/concepts/analyticsTracking/useSegmentTracking.ts @@ -10,8 +10,9 @@ export const useSegmentTracking = (): void => { const { segmentKey, loaded, loadError } = useWatchSegmentKey(); const { dashboardConfig } = useAppContext(); const username = useAppSelector((state) => state.user); + const userID = useAppSelector((state) => state.userID); const clusterID = useAppSelector((state) => state.clusterID); - const [userProps, uPropsLoaded] = useTrackUser(username); + const [userProps, uPropsLoaded] = useTrackUser(username, userID); const disableTrackingConfig = dashboardConfig.spec.dashboardConfig.disableTracking; React.useEffect(() => { diff --git a/frontend/src/concepts/analyticsTracking/useTrackUser.ts b/frontend/src/concepts/analyticsTracking/useTrackUser.ts index 27429837c4..6590223a62 100644 --- a/frontend/src/concepts/analyticsTracking/useTrackUser.ts +++ b/frontend/src/concepts/analyticsTracking/useTrackUser.ts @@ -4,9 +4,12 @@ import { useAccessReview } from '~/api'; import { AccessReviewResourceAttributes } from '~/k8sTypes'; import { IdentifyEventProperties } from '~/concepts/analyticsTracking/trackingProperties'; -export const useTrackUser = (username?: string): [IdentifyEventProperties, boolean] => { +export const useTrackUser = ( + username?: string, + ssoUserID?: string, +): [IdentifyEventProperties, boolean] => { const { isAdmin } = useUser(); - const [anonymousId, setAnonymousId] = React.useState(undefined); + const [userID, setUserID] = React.useState(ssoUserID); const createReviewResource: AccessReviewResourceAttributes = { group: 'project.openshift.io', @@ -26,9 +29,11 @@ export const useTrackUser = (username?: string): [IdentifyEventProperties, boole return aId; }; - computeAnonymousUserId().then((val) => { - setAnonymousId(val); - }); + if (!ssoUserID) { + computeAnonymousUserId().then((val) => { + setUserID(val); + }); + } // compute anonymousId only once // eslint-disable-next-line react-hooks/exhaustive-deps }, []); @@ -37,10 +42,10 @@ export const useTrackUser = (username?: string): [IdentifyEventProperties, boole () => ({ isAdmin, canCreateProjects: allowCreate, - anonymousID: anonymousId, + userID, }), - [isAdmin, allowCreate, anonymousId], + [isAdmin, allowCreate, userID], ); - return [props, acLoaded && !!anonymousId]; + return [props, acLoaded && !!userID]; }; diff --git a/frontend/src/redux/types.ts b/frontend/src/redux/types.ts index b678114914..736b06eafd 100644 --- a/frontend/src/redux/types.ts +++ b/frontend/src/redux/types.ts @@ -42,6 +42,7 @@ export type AppState = { // user state isAdmin?: boolean; user?: string; + userID?: string; userLoading: boolean; userError?: Error | null; isImpersonating?: boolean; From dc4599e28f53671645cc13a4430aabc176cf9b07 Mon Sep 17 00:00:00 2001 From: Juntao Wang <37624318+DaoDaoNoCode@users.noreply.github.com> Date: Thu, 22 Aug 2024 08:58:32 -0400 Subject: [PATCH 06/14] Show only active experiments on create/duplicate schedules page (#3106) --- .../mocked/pipelines/pipelineCreateRuns.cy.ts | 47 +++++++++++++++++-- .../pipelines/content/createRun/RunForm.tsx | 1 - .../ProjectAndExperimentSection.tsx | 13 +---- .../content/createRun/useRunFormData.ts | 25 ++++------ 4 files changed, 53 insertions(+), 33 deletions(-) diff --git a/frontend/src/__tests__/cypress/cypress/tests/mocked/pipelines/pipelineCreateRuns.cy.ts b/frontend/src/__tests__/cypress/cypress/tests/mocked/pipelines/pipelineCreateRuns.cy.ts index 118301d4dc..7ad01744bd 100644 --- a/frontend/src/__tests__/cypress/cypress/tests/mocked/pipelines/pipelineCreateRuns.cy.ts +++ b/frontend/src/__tests__/cypress/cypress/tests/mocked/pipelines/pipelineCreateRuns.cy.ts @@ -550,7 +550,8 @@ describe('Pipeline create runs', () => { it('creates a schedule', () => { createScheduleRunCommonTest(); - createSchedulePage.findExperimentSelect().should('contain.text', 'Default'); + // Default is archived, so it should not pre-select the default + createSchedulePage.findExperimentSelect().should('contain.text', 'Select an experiment'); createSchedulePage.findExperimentSelect().should('not.be.disabled').click(); createSchedulePage.selectExperimentByName('Test experiment 1'); createSchedulePage @@ -588,6 +589,8 @@ describe('Pipeline create runs', () => { it('creates a schedule with trigger type cron without whitespace', () => { // Fill out the form with a schedule and submit createScheduleRunCommonTest(); + createSchedulePage.findExperimentSelect().should('not.be.disabled').click(); + createSchedulePage.selectExperimentByName('Test experiment 1'); createSchedulePage.findScheduledRunTypeSelector().findSelectOption('Cron').click(); createSchedulePage.findScheduledRunCron().fill('@every 5m'); createSchedulePage @@ -608,10 +611,10 @@ describe('Pipeline create runs', () => { }, trigger: { cron_schedule: { cron: '@every 5m' } }, max_concurrency: '10', - mode: 'DISABLE', + mode: 'ENABLE', no_catchup: false, service_account: '', - experiment_id: 'default', + experiment_id: 'experiment-1', }); }); @@ -623,6 +626,8 @@ describe('Pipeline create runs', () => { it('creates a schedule with trigger type cron with whitespace', () => { createScheduleRunCommonTest(); + createSchedulePage.findExperimentSelect().should('not.be.disabled').click(); + createSchedulePage.selectExperimentByName('Test experiment 1'); createSchedulePage.findScheduledRunTypeSelector().findSelectOption('Cron').click(); createSchedulePage.findScheduledRunCron().fill('@every 5m '); createSchedulePage @@ -643,10 +648,10 @@ describe('Pipeline create runs', () => { }, trigger: { cron_schedule: { cron: '@every 5m' } }, max_concurrency: '10', - mode: 'DISABLE', + mode: 'ENABLE', no_catchup: false, service_account: '', - experiment_id: 'default', + experiment_id: 'experiment-1', }); }); }); @@ -730,6 +735,38 @@ describe('Pipeline create runs', () => { ); }); + it('duplicates a schedule with an archived experiment', () => { + const [mockRecurringRun] = initialMockRecurringRuns; + const mockExperiment = { ...mockExperiments[0], storage_state: StorageStateKF.ARCHIVED }; + + // Mock experiments, pipelines & versions for form select dropdowns + cloneSchedulePage.mockGetExperiments(projectName, mockExperiments); + cloneSchedulePage.mockGetPipelines(projectName, [mockPipeline]); + cloneSchedulePage.mockGetPipelineVersions( + projectName, + [mockPipelineVersion], + mockPipelineVersion.pipeline_id, + ); + cloneSchedulePage.mockGetRecurringRun(projectName, mockRecurringRun); + cloneSchedulePage.mockGetPipelineVersion(projectName, mockPipelineVersion); + cloneSchedulePage.mockGetPipeline(projectName, mockPipeline); + cloneSchedulePage.mockGetExperiment(projectName, mockExperiment); + + // Navigate to clone run page for a given schedule + cy.visitWithLogin(`/experiments/${projectName}/experiment-1/runs`); + pipelineRunsGlobal.findSchedulesTab().click(); + pipelineRecurringRunTable + .getRowByName(mockRecurringRun.display_name) + .findKebabAction('Duplicate') + .click(); + verifyRelativeURL( + `/experiments/${projectName}/experiment-1/schedules/clone/${mockRecurringRun.recurring_run_id}`, + ); + + // Verify pre-populated values & submit + cloneSchedulePage.findExperimentSelect().should('have.text', 'Select an experiment'); + }); + it('shows cron & periodic fields', () => { visitLegacyRunsPage(); diff --git a/frontend/src/concepts/pipelines/content/createRun/RunForm.tsx b/frontend/src/concepts/pipelines/content/createRun/RunForm.tsx index 050cb22d41..65a25c2a25 100644 --- a/frontend/src/concepts/pipelines/content/createRun/RunForm.tsx +++ b/frontend/src/concepts/pipelines/content/createRun/RunForm.tsx @@ -106,7 +106,6 @@ const RunForm: React.FC = ({ data, onValueChange, isCloned }) => { projectName={getDisplayNameFromK8sResource(data.project)} value={data.experiment} onChange={(experiment) => onValueChange('experiment', experiment)} - isSchedule={isSchedule} /> void; - isSchedule: boolean; }; const ProjectAndExperimentSection: React.FC = ({ projectName, value, onChange, - isSchedule, }) => { const isExperimentsAvailable = useIsAreaAvailable(SupportedArea.PIPELINE_EXPERIMENTS).status; @@ -44,11 +39,7 @@ const ProjectAndExperimentSection: React.FC = - {isSchedule ? ( - - ) : ( - - )} + { - // on create run page, we always check the experiment archived state - // no matter it's duplicated or carried from the create schedules pages - if (formData.runType.type === RunTypeOption.ONE_TRIGGER) { - if (formData.experiment) { - if (formData.experiment.storage_state === StorageStateKF.ARCHIVED) { - setFormValue('experiment', null); - } - } else if (experiment) { - if (experiment.storage_state === StorageStateKF.ARCHIVED) { - setFormValue('experiment', null); - } else { - setFormValue('experiment', experiment); - } + if (formData.experiment) { + if (formData.experiment.storage_state === StorageStateKF.ARCHIVED) { + setFormValue('experiment', null); + } + } else if (experiment) { + if (experiment.storage_state === StorageStateKF.ARCHIVED) { + setFormValue('experiment', null); + } else { + setFormValue('experiment', experiment); } - } else if (!formData.experiment && experiment) { - // else, on create schedules page, we do what we did before - setFormValue('experiment', experiment); } }, [formData.experiment, setFormValue, experiment, formData.runType.type]); }; From e35546a9052314d0b7c2d11ea4d06b2426c018e4 Mon Sep 17 00:00:00 2001 From: Juntao Wang <37624318+DaoDaoNoCode@users.noreply.github.com> Date: Thu, 22 Aug 2024 15:43:04 -0400 Subject: [PATCH 07/14] Adapt model deployment modals to the latest design (#3102) --- .../cypress/cypress/pages/modelServing.ts | 6 +- .../components/DeployRegisteredModelModal.tsx | 14 +- .../DataConnectionExistingField.tsx | 4 +- .../DataConnectionFolderPathField.tsx | 1 + .../InferenceServiceFrameworkSection.tsx | 7 +- .../InferenceServiceNameSection.tsx | 23 +-- .../InferenceServiceServingRuntimeSection.tsx | 3 +- .../ManageInferenceServiceModal.tsx | 2 +- .../InferenceServiceModal/ProjectSelector.tsx | 5 +- .../AuthServingRuntimeSection.tsx | 39 +++--- .../ServingRuntimeSizeSection.tsx | 27 ++-- .../ServingRuntimeTemplateSection.tsx | 4 +- .../ServingRuntimeTokenSection.tsx | 132 +++++++++--------- .../KServeAutoscalerReplicaSection.tsx | 75 +++++----- .../kServeModal/ManageKServeModal.tsx | 130 ++++++++--------- .../server/AcceleratorProfileSelectField.tsx | 3 +- 16 files changed, 237 insertions(+), 238 deletions(-) diff --git a/frontend/src/__tests__/cypress/cypress/pages/modelServing.ts b/frontend/src/__tests__/cypress/cypress/pages/modelServing.ts index 16566af7e3..fc8f649392 100644 --- a/frontend/src/__tests__/cypress/cypress/pages/modelServing.ts +++ b/frontend/src/__tests__/cypress/cypress/pages/modelServing.ts @@ -117,7 +117,7 @@ class InferenceServiceModal extends Modal { findExistingConnectionSelect() { return this.find() - .findByRole('group', { name: 'Model location' }) + .findByRole('group', { name: 'Source model location' }) .findByRole('button', { name: 'Options menu' }); } @@ -202,9 +202,7 @@ class ServingRuntimeModal extends Modal { } findModelServerSizeSelect() { - return this.find() - .findByRole('group', { name: 'Compute resources per replica' }) - .findByTestId('model-server-size-selection'); + return this.find().findByTestId('model-server-size-selection'); } findModelServerReplicasMinusButton() { diff --git a/frontend/src/pages/modelRegistry/screens/components/DeployRegisteredModelModal.tsx b/frontend/src/pages/modelRegistry/screens/components/DeployRegisteredModelModal.tsx index e0593b5ccb..659cb1b12a 100644 --- a/frontend/src/pages/modelRegistry/screens/components/DeployRegisteredModelModal.tsx +++ b/frontend/src/pages/modelRegistry/screens/components/DeployRegisteredModelModal.tsx @@ -1,5 +1,5 @@ import React from 'react'; -import { Alert, Button, Form, Modal, Spinner } from '@patternfly/react-core'; +import { Alert, Button, Form, FormSection, Modal, Spinner } from '@patternfly/react-core'; import { ModelVersion } from '~/concepts/modelRegistry/types'; import { ProjectKind } from '~/k8sTypes'; import useProjectErrorForRegisteredModel from '~/pages/modelRegistry/screens/RegisteredModels/useProjectErrorForRegisteredModel'; @@ -92,11 +92,13 @@ const DeployRegisteredModelModal: React.FC = ({ ) : !deployInfoLoaded ? ( ) : ( - + + + )} diff --git a/frontend/src/pages/modelServing/screens/projects/InferenceServiceModal/DataConnectionExistingField.tsx b/frontend/src/pages/modelServing/screens/projects/InferenceServiceModal/DataConnectionExistingField.tsx index f2ab552f28..0f95a8a1aa 100644 --- a/frontend/src/pages/modelServing/screens/projects/InferenceServiceModal/DataConnectionExistingField.tsx +++ b/frontend/src/pages/modelServing/screens/projects/InferenceServiceModal/DataConnectionExistingField.tsx @@ -74,9 +74,8 @@ const DataConnectionExistingField: React.FC = ( )} - + ({ @@ -84,6 +83,7 @@ const DataConnectionExistingField: React.FC = ( dropdownLabel: getLabeledOption(connection), label: getDataConnectionDisplayName(connection.dataConnection), }))} + toggleProps={{ id: 'inference-service-data-connection' }} value={data.storage.dataConnection} toggleLabel={ selectedDataConnection ? getLabeledOption(selectedDataConnection) : placeholderText diff --git a/frontend/src/pages/modelServing/screens/projects/InferenceServiceModal/DataConnectionFolderPathField.tsx b/frontend/src/pages/modelServing/screens/projects/InferenceServiceModal/DataConnectionFolderPathField.tsx index 3d71404df3..0cb29251e9 100644 --- a/frontend/src/pages/modelServing/screens/projects/InferenceServiceModal/DataConnectionFolderPathField.tsx +++ b/frontend/src/pages/modelServing/screens/projects/InferenceServiceModal/DataConnectionFolderPathField.tsx @@ -46,6 +46,7 @@ const DataConnectionFolderPathField: React.FC + { const name = framework.version ? `${framework.name} - ${framework.version}` diff --git a/frontend/src/pages/modelServing/screens/projects/InferenceServiceModal/InferenceServiceNameSection.tsx b/frontend/src/pages/modelServing/screens/projects/InferenceServiceModal/InferenceServiceNameSection.tsx index d5ded68074..9a030d189a 100644 --- a/frontend/src/pages/modelServing/screens/projects/InferenceServiceModal/InferenceServiceNameSection.tsx +++ b/frontend/src/pages/modelServing/screens/projects/InferenceServiceModal/InferenceServiceNameSection.tsx @@ -24,7 +24,7 @@ const InferenceServiceNameSection: React.FC = const validated = !isNameValid ? 'warning' : 'default'; return ( - + = onChange={(e, name) => setData('name', name)} validated={validated} /> - {validated === 'warning' && ( - - - }> - Cannot exceed 253 characters - - - - )} + + + } + > + {isNameValid + ? 'This is the name of the inference service created when the model is deployed' + : 'Cannot exceed 253 characters'} + + + ); }; diff --git a/frontend/src/pages/modelServing/screens/projects/InferenceServiceModal/InferenceServiceServingRuntimeSection.tsx b/frontend/src/pages/modelServing/screens/projects/InferenceServiceModal/InferenceServiceServingRuntimeSection.tsx index 869cf2a90f..c62eaa2ebd 100644 --- a/frontend/src/pages/modelServing/screens/projects/InferenceServiceModal/InferenceServiceServingRuntimeSection.tsx +++ b/frontend/src/pages/modelServing/screens/projects/InferenceServiceModal/InferenceServiceServingRuntimeSection.tsx @@ -49,7 +49,7 @@ const InferenceServiceServingRuntimeSection: React.FC< } return ( - + = /> - + = ({ const { projects } = React.useContext(ProjectsContext); return ( - +