Merge pull request #1936 from DaoDaoNoCode/upstream-issue-1353

Check serving runtime kind at creation time
opendatahub-io · Oct 17, 2023 · d1228e0 · d1228e0
2 parents a9b3aae + 2078e9f
commit d1228e0
Show file tree

Hide file tree

Showing 6 changed files with 147 additions and 20 deletions.
diff --git a/frontend/src/__mocks__/mockServingRuntimeTemplateK8sResource.ts b/frontend/src/__mocks__/mockServingRuntimeTemplateK8sResource.ts
@@ -92,3 +92,79 @@ export const mockTemplateK8sResource = ({
   ],
   parameters: [],
 });
+
+export const mockInvalidTemplateK8sResource = ({
+  name = 'test-model-invalid',
+  namespace = 'opendatahub',
+}: MockResourceConfigType): TemplateKind => ({
+  apiVersion: 'template.openshift.io/v1',
+  kind: 'Template',
+  metadata: {
+    name: 'template-ar2pcd',
+    namespace,
+    uid: '31277020-b60a-40c9-91bc-5ee3e2bb25ed',
+    resourceVersion: '164740436',
+    creationTimestamp: '2023-05-03T21:58:17Z',
+    labels: {
+      'opendatahub.io/dashboard': 'true',
+    },
+    annotations: {
+      tags: 'new-one,servingruntime',
+    },
+  },
+  objects: [
+    {
+      apiVersion: 'serving.kserve.io/v1alpha1',
+      kind: 'ServingRuntime',
+      metadata: {
+        name,
+        annotations: {
+          'openshift.io/display-name': 'New OVMS Server Invalid',
+        },
+        labels: {
+          'opendatahub.io/dashboard': 'true',
+        },
+      },
+      spec: {
+        builtInAdapter: {
+          memBufferBytes: 134217728,
+          modelLoadingTimeoutMillis: 90000,
+          runtimeManagementPort: 8888,
+          serverType: 'ovms',
+        },
+        containers: [
+          {
+            args: [
+              '--port=8001',
+              '--rest_port=8888',
+              '--config_path=/models/model_config_list.json',
+              '--file_system_poll_wait_seconds=0',
+              '--grpc_bind_address=127.0.0.1',
+              '--rest_bind_address=127.0.0.1',
+              '--target_device=NVIDIA',
+            ],
+            image:
+              'quay.io/modh/openvino-model-server@sha256:c89f76386bc8b59f0748cf173868e5beef21ac7d2f78dada69089c4d37c44116',
+            name: 'ovms',
+            resources: {
+              limits: {
+                cpu: '0',
+                memory: '0Gi',
+              },
+              requests: {
+                cpu: '0',
+                memory: '0Gi',
+              },
+            },
+          },
+        ],
+        grpcDataEndpoint: 'port:8001',
+        grpcEndpoint: 'port:8085',
+        multiModel: true,
+        protocolVersions: ['grpc-v1'],
+        replicas: 1,
+      },
+    },
+  ],
+  parameters: [],
+});
diff --git a/frontend/src/__tests__/integration/pages/modelServing/ServingRuntimeList.spec.ts b/frontend/src/__tests__/integration/pages/modelServing/ServingRuntimeList.spec.ts
@@ -94,6 +94,7 @@ test('Add model server', async ({ page }) => {
   await page.getByLabel('Model server name *').fill('Test Name');
   await page.locator('#serving-runtime-template-selection').click();
   await page.getByRole('menuitem', { name: 'New OVMS Server' }).click();
+  await expect(page.getByRole('menuitem', { name: 'New OVMS Server Invalid' })).toBeHidden();
   await expect(page.getByRole('button', { name: 'Add', exact: true })).toBeEnabled();
 
   // test the if the alert is visible when route is external while token is not set

diff --git a/frontend/src/__tests__/integration/pages/modelServing/ServingRuntimeList.stories.tsx b/frontend/src/__tests__/integration/pages/modelServing/ServingRuntimeList.stories.tsx
@@ -20,7 +20,10 @@ import {
 import { mockProjectK8sResource } from '~/__mocks__/mockProjectK8sResource';
 import { mockPVCK8sResource } from '~/__mocks__/mockPVCK8sResource';
 import ProjectsRoutes from '~/concepts/projects/ProjectsRoutes';
-import { mockTemplateK8sResource } from '~/__mocks__/mockServingRuntimeTemplateK8sResource';
+import {
+  mockInvalidTemplateK8sResource,
+  mockTemplateK8sResource,
+} from '~/__mocks__/mockServingRuntimeTemplateK8sResource';
 import { mockDashboardConfig } from '~/__mocks__/mockDashboardConfig';
 import { mockStatus } from '~/__mocks__/mockStatus';
 import useDetectUser from '~/utilities/useDetectUser';
@@ -107,7 +110,15 @@ export default {
         ),
         rest.get(
           '/api/k8s/apis/template.openshift.io/v1/namespaces/opendatahub/templates',
-          (req, res, ctx) => res(ctx.json(mockK8sResourceList([mockTemplateK8sResource({})]))),
+          (req, res, ctx) =>
+            res(
+              ctx.json(
+                mockK8sResourceList([
+                  mockTemplateK8sResource({}),
+                  mockInvalidTemplateK8sResource({}),
+                ]),
+              ),
+            ),
         ),
         rest.get(
           '/api/k8s/apis/opendatahub.io/v1alpha/namespaces/opendatahub/odhdashboardconfigs/odh-dashboard-config',

diff --git a/frontend/src/pages/modelServing/customServingRuntimes/CustomServingRuntimeAddTemplate.tsx b/frontend/src/pages/modelServing/customServingRuntimes/CustomServingRuntimeAddTemplate.tsx
@@ -24,6 +24,7 @@ import {
 import {
   getServingRuntimeDisplayNameFromTemplate,
   getServingRuntimeNameFromTemplate,
+  isServingRuntimeKind,
 } from './utils';
 import { CustomServingRuntimeContext } from './CustomServingRuntimeContext';
 
@@ -122,10 +123,10 @@ const CustomServingRuntimeAddTemplate: React.FC<CustomServingRuntimeAddTemplateP
             <Alert
               isInline
               variant="danger"
-              title="Error"
+              title={error.name}
               actionClose={<AlertActionCloseButton onClose={() => setError(undefined)} />}
             >
-              <p>{error.message}</p>
+              {error.message}
             </Alert>
           </StackItem>
         )}
@@ -138,6 +139,14 @@ const CustomServingRuntimeAddTemplate: React.FC<CustomServingRuntimeAddTemplateP
                 id="create-button"
                 isLoading={loading}
                 onClick={() => {
+                  try {
+                    isServingRuntimeKind(YAML.parse(code));
+                  } catch (e) {
+                    if (e instanceof Error) {
+                      setError(e);
+                    }
+                    return;
+                  }
                   setIsLoading(true);
                   // TODO: Revert back to pass through api once we migrate admin panel
                   const onClickFunc = existingTemplate

diff --git a/frontend/src/pages/modelServing/customServingRuntimes/utils.ts b/frontend/src/pages/modelServing/customServingRuntimes/utils.ts
@@ -41,29 +41,44 @@ export const getServingRuntimeDisplayNameFromTemplate = (template: TemplateKind)
 export const getServingRuntimeNameFromTemplate = (template: TemplateKind) =>
   template.objects[0].metadata.name;
 
-export const isServingRuntimeKind = (obj: K8sResourceCommon): obj is ServingRuntimeKind =>
-  obj.kind === 'ServingRuntime' &&
-  obj.spec?.containers !== undefined &&
-  obj.spec?.supportedModelFormats !== undefined;
+const createServingRuntimeCustomError = (name: string, message: string) => {
+  const error = new Error(message);
+  error.name = name;
+  return error;
+};
+
+export const isServingRuntimeKind = (obj: K8sResourceCommon): obj is ServingRuntimeKind => {
+  if (obj.kind !== 'ServingRuntime') {
+    throw createServingRuntimeCustomError('Invalid parameter', 'kind: must be ServingRuntime.');
+  }
+  if (!obj.spec?.containers) {
+    throw createServingRuntimeCustomError('Missing parameter', 'spec.containers: is required.');
+  }
+  if (!obj.spec?.supportedModelFormats) {
+    throw createServingRuntimeCustomError(
+      'Missing parameter',
+      'spec.supportedModelFormats: is required.',
+    );
+  }
+  return true;
+};
 
 export const getServingRuntimeFromName = (
   templateName: string,
-  templateList?: TemplateKind[],
+  templateList: TemplateKind[] = [],
 ): ServingRuntimeKind | undefined => {
-  if (!templateList) {
-    return undefined;
-  }
   const template = templateList.find((t) => getServingRuntimeNameFromTemplate(t) === templateName);
-  if (!template) {
-    return undefined;
-  }
   return getServingRuntimeFromTemplate(template);
 };
 
 export const getServingRuntimeFromTemplate = (
-  template: TemplateKind,
+  template?: TemplateKind,
 ): ServingRuntimeKind | undefined => {
-  if (!isServingRuntimeKind(template.objects[0])) {
+  try {
+    if (!template || !isServingRuntimeKind(template.objects[0])) {
+      return undefined;
+    }
+  } catch (e) {
     return undefined;
   }
   return template.objects[0];

diff --git a/...pages/modelServing/screens/projects/ServingRuntimeModal/ServingRuntimeTemplateSection.tsx b/...pages/modelServing/screens/projects/ServingRuntimeModal/ServingRuntimeTemplateSection.tsx
@@ -6,6 +6,7 @@ import { TemplateKind } from '~/k8sTypes';
 import {
   getServingRuntimeDisplayNameFromTemplate,
   getServingRuntimeNameFromTemplate,
+  isServingRuntimeKind,
 } from '~/pages/modelServing/customServingRuntimes/utils';
 import { isCompatibleWithAccelerator } from '~/pages/projects/screens/spawner/spawnerUtils';
 import SimpleDropdownSelect from '~/components/SimpleDropdownSelect';
@@ -26,7 +27,19 @@ const ServingRuntimeTemplateSection: React.FC<ServingRuntimeTemplateSectionProps
   isEditing,
   acceleratorState,
 }) => {
-  const options = templates.map((template) => ({
+  const filteredTemplates = React.useMemo(
+    () =>
+      templates.filter((template) => {
+        try {
+          return isServingRuntimeKind(template.objects[0]);
+        } catch (e) {
+          return false;
+        }
+      }),
+    [templates],
+  );
+
+  const options = filteredTemplates.map((template) => ({
     key: getServingRuntimeNameFromTemplate(template),
     selectedLabel: getServingRuntimeDisplayNameFromTemplate(template),
     label: (
@@ -59,12 +72,14 @@ const ServingRuntimeTemplateSection: React.FC<ServingRuntimeTemplateSectionProps
         <FormGroup label="Serving runtime" fieldId="serving-runtime-selection" isRequired>
           <SimpleDropdownSelect
             isFullWidth
-            isDisabled={isEditing || templates.length === 0}
+            isDisabled={isEditing || filteredTemplates.length === 0}
             id="serving-runtime-template-selection"
             aria-label="Select a template"
             options={options}
             placeholder={
-              isEditing || templates.length === 0 ? data.servingRuntimeTemplateName : 'Select one'
+              isEditing || filteredTemplates.length === 0
+                ? data.servingRuntimeTemplateName
+                : 'Select one'
             }
             value={data.servingRuntimeTemplateName ?? ''}
             onChange={(name) => {