Skip to content

Commit

Permalink
fix(MR): disable archive for mr/mv if has deploys
Browse files Browse the repository at this point in the history
Signed-off-by: gitdallas <[email protected]>
  • Loading branch information
gitdallas committed Oct 15, 2024
1 parent c4ae140 commit 284829b
Show file tree
Hide file tree
Showing 10 changed files with 135 additions and 42 deletions.
2 changes: 2 additions & 0 deletions frontend/src/__mocks__/mockInferenceServiceK8sResource.ts
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,8 @@ export const mockInferenceServiceK8sResource = ({
generation: 1,
labels: {
name,
[KnownLabels.REGISTERED_MODEL_ID]: '1',
[KnownLabels.MODEL_VERSION_ID]: '3',
[KnownLabels.DASHBOARD_RESOURCE]: 'true',
...(kserveInternalLabel && { 'networking.knative.dev/visibility': 'cluster-local' }),
},
Expand Down
Original file line number Diff line number Diff line change
@@ -1,8 +1,17 @@
/* eslint-disable camelcase */
import { mockDscStatus, mockK8sResourceList } from '~/__mocks__';
import {
mockDscStatus,
mockInferenceServiceK8sResource,
mockK8sResourceList,
mockProjectK8sResource,
} from '~/__mocks__';
import { mockDashboardConfig } from '~/__mocks__/mockDashboardConfig';
import { mockRegisteredModelList } from '~/__mocks__/mockRegisteredModelsList';
import { ServiceModel } from '~/__tests__/cypress/cypress/utils/models';
import {
InferenceServiceModel,
ProjectModel,
ServiceModel,
} from '~/__tests__/cypress/cypress/utils/models';
import { mockModelVersionList } from '~/__mocks__/mockModelVersionList';
import { mockModelVersion } from '~/__mocks__/mockModelVersion';
import type { ModelVersion } from '~/concepts/modelRegistry/types';
Expand Down Expand Up @@ -272,6 +281,20 @@ describe('Archiving version', () => {
});
});

it('Cannot archive version that has a deployment from versions table', () => {
cy.interceptK8sList(ProjectModel, mockK8sResourceList([mockProjectK8sResource({})]));
cy.interceptK8sList(
InferenceServiceModel,
mockK8sResourceList([mockInferenceServiceK8sResource({})]),
);
initIntercepts({});

modelVersionArchive.visitModelVersionList();

const modelVersionRow = modelRegistry.getModelVersionRow('model version 3');
modelVersionRow.findKebabAction('Archive model version').should('have.attr', 'aria-disabled');
});

it('Archive version from versions details', () => {
cy.interceptOdh(
'PATCH /api/service/modelregistry/:serviceName/api/model_registry/:apiVersion/model_versions/:modelVersionId',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,11 @@ const ModelVersionsDetails: React.FC<ModelVersionsDetailProps> = ({ tab, ...page
/>
</FlexItem>
<FlexItem>
<ModelVersionsDetailsHeaderActions mv={mv} refresh={refresh} />
<ModelVersionsDetailsHeaderActions
mv={mv}
hasDeployment={inferenceServices.data.length > 0}
refresh={refresh}
/>
</FlexItem>
</Flex>
)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,11 +13,13 @@ import DeployRegisteredModelModal from '~/pages/modelRegistry/screens/components

interface ModelVersionsDetailsHeaderActionsProps {
mv: ModelVersion;
hasDeployment?: boolean;
refresh: () => void;
}

const ModelVersionsDetailsHeaderActions: React.FC<ModelVersionsDetailsHeaderActionsProps> = ({
mv,
hasDeployment = false,
refresh,
}) => {
const { apiState } = React.useContext(ModelRegistryContext);
Expand Down Expand Up @@ -60,10 +62,14 @@ const ModelVersionsDetailsHeaderActions: React.FC<ModelVersionsDetailsHeaderActi
Deploy
</DropdownItem>
<DropdownItem
isAriaDisabled={hasDeployment}
id="archive-version-button"
aria-label="Archive version"
key="archive-version-button"
onClick={() => setIsArchiveModalOpen(true)}
tooltipProps={
hasDeployment ? { content: 'Deployed versions cannot be archived' } : undefined
}
ref={tooltipRef}
>
Archive version
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@ import useRegisteredModelById from '~/concepts/modelRegistry/apiHooks/useRegiste
import { ModelRegistrySelectorContext } from '~/concepts/modelRegistry/context/ModelRegistrySelectorContext';
import { ModelState } from '~/concepts/modelRegistry/types';
import { registeredModelArchiveDetailsUrl } from '~/pages/modelRegistry/screens/routeUtils';
import { useMakeFetchObject } from '~/utilities/useMakeFetchObject';
import useInferenceServices from '~/pages/modelServing/useInferenceServices';
import ModelVersionsTabs from './ModelVersionsTabs';
import ModelVersionsHeaderActions from './ModelVersionsHeaderActions';
import { ModelVersionsTab } from './const';
Expand All @@ -27,6 +29,7 @@ const ModelVersions: React.FC<ModelVersionsProps> = ({ tab, ...pageProps }) => {
const loadError = mvLoadError || rmLoadError;
const loaded = mvLoaded && rmLoaded;
const navigate = useNavigate();
const inferenceServices = useMakeFetchObject(useInferenceServices(undefined, rmId));

useEffect(() => {
if (rm?.state === ModelState.ARCHIVED) {
Expand All @@ -52,7 +55,11 @@ const ModelVersions: React.FC<ModelVersionsProps> = ({ tab, ...pageProps }) => {
</Breadcrumb>
}
title={rm?.name}
headerAction={rm && <ModelVersionsHeaderActions rm={rm} />}
headerAction={
rm && (
<ModelVersionsHeaderActions hasDeployments={!!inferenceServices.data.length} rm={rm} />
)
}
description={<Truncate content={rm?.description || ''} />}
loadError={loadError}
loaded={loaded}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,13 @@ import { RegisteredModel, ModelState } from '~/concepts/modelRegistry/types';

interface ModelVersionsHeaderActionsProps {
rm: RegisteredModel;
hasDeployments?: boolean;
}

const ModelVersionsHeaderActions: React.FC<ModelVersionsHeaderActionsProps> = ({ rm }) => {
const ModelVersionsHeaderActions: React.FC<ModelVersionsHeaderActionsProps> = ({
rm,
hasDeployments = false,
}) => {
const { apiState } = React.useContext(ModelRegistryContext);
const { preferredModelRegistry } = React.useContext(ModelRegistrySelectorContext);

Expand Down Expand Up @@ -56,6 +60,12 @@ const ModelVersionsHeaderActions: React.FC<ModelVersionsHeaderActionsProps> = ({
key="archive-model-button"
onClick={() => setIsArchiveModalOpen(true)}
ref={tooltipRef}
isAriaDisabled={hasDeployments}
tooltipProps={
hasDeployments
? { content: 'Models with deployed versions cannot be archived.' }
: undefined
}
>
Archive model
</DropdownItem>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,11 @@ import * as React from 'react';
import { Table } from '~/components/table';
import { ModelVersion } from '~/concepts/modelRegistry/types';
import DashboardEmptyTableView from '~/concepts/dashboard/DashboardEmptyTableView';
import ModelVersionsTableRow from './ModelVersionsTableRow';
import useInferenceServices from '~/pages/modelServing/useInferenceServices';
import { useMakeFetchObject } from '~/utilities/useMakeFetchObject';
import { KnownLabels } from '~/k8sTypes';
import { mvColumns } from './ModelVersionsTableColumns';
import ModelVersionsTableRow from './ModelVersionsTableRow';

type ModelVersionsTableProps = {
clearFilters: () => void;
Expand All @@ -18,25 +21,36 @@ const ModelVersionsTable: React.FC<ModelVersionsTableProps> = ({
toolbarContent,
isArchiveModel,
refresh,
}) => (
<Table
data-testid="model-versions-table"
data={modelVersions}
columns={mvColumns}
toolbarContent={toolbarContent}
defaultSortColumn={3}
enablePagination
onClearFilters={clearFilters}
emptyTableView={<DashboardEmptyTableView onClearFilters={clearFilters} />}
rowRenderer={(mv) => (
<ModelVersionsTableRow
key={mv.name}
modelVersion={mv}
isArchiveModel={isArchiveModel}
refresh={refresh}
/>
)}
/>
);
}) => {
const inferenceServices = useMakeFetchObject(
useInferenceServices(undefined, modelVersions[0].registeredModelId),
);
return (
<Table
data-testid="model-versions-table"
data={modelVersions}
columns={mvColumns}
toolbarContent={toolbarContent}
defaultSortColumn={3}
enablePagination
onClearFilters={clearFilters}
emptyTableView={<DashboardEmptyTableView onClearFilters={clearFilters} />}
rowRenderer={(mv) => {
const hasDeployment = !!inferenceServices.data.some(
(s) => s.metadata.labels?.[KnownLabels.MODEL_VERSION_ID] === mv.id,
);
return (
<ModelVersionsTableRow
hasDeployment={hasDeployment}
key={mv.name}
modelVersion={mv}
isArchiveModel={isArchiveModel}
refresh={refresh}
/>
);
}}
/>
);
};

export default ModelVersionsTable;
Original file line number Diff line number Diff line change
Expand Up @@ -21,13 +21,15 @@ type ModelVersionsTableRowProps = {
modelVersion: ModelVersion;
isArchiveRow?: boolean;
isArchiveModel?: boolean;
hasDeployment?: boolean;
refresh: () => void;
};

const ModelVersionsTableRow: React.FC<ModelVersionsTableRowProps> = ({
modelVersion: mv,
isArchiveRow,
isArchiveModel,
hasDeployment = false,
refresh,
}) => {
const navigate = useNavigate();
Expand All @@ -52,6 +54,10 @@ const ModelVersionsTableRow: React.FC<ModelVersionsTableRowProps> = ({
{
title: 'Archive model version',
onClick: () => setIsArchiveModalOpen(true),
isAriaDisabled: hasDeployment,
tooltipProps: hasDeployment
? { content: 'Deployed versions cannot be archived' }
: undefined,
},
];

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,11 @@ import * as React from 'react';
import { Table } from '~/components/table';
import { RegisteredModel } from '~/concepts/modelRegistry/types';
import DashboardEmptyTableView from '~/concepts/dashboard/DashboardEmptyTableView';
import { rmColumns } from './RegisteredModelsTableColumns';
import useInferenceServices from '~/pages/modelServing/useInferenceServices';
import { useMakeFetchObject } from '~/utilities/useMakeFetchObject';
import { KnownLabels } from '~/k8sTypes';
import RegisteredModelTableRow from './RegisteredModelTableRow';
import { rmColumns } from './RegisteredModelsTableColumns';

type RegisteredModelTableProps = {
clearFilters: () => void;
Expand All @@ -16,20 +19,32 @@ const RegisteredModelTable: React.FC<RegisteredModelTableProps> = ({
registeredModels,
toolbarContent,
refresh,
}) => (
<Table
data-testid="registered-model-table"
data={registeredModels}
columns={rmColumns}
toolbarContent={toolbarContent}
defaultSortColumn={2}
onClearFilters={clearFilters}
enablePagination
emptyTableView={<DashboardEmptyTableView onClearFilters={clearFilters} />}
rowRenderer={(rm) => (
<RegisteredModelTableRow key={rm.name} registeredModel={rm} refresh={refresh} />
)}
/>
);
}) => {
const inferenceServices = useMakeFetchObject(useInferenceServices());
const hasDeploys = (rmId: string) =>
!!inferenceServices.data.some(
(s) => s.metadata.labels?.[KnownLabels.REGISTERED_MODEL_ID] === rmId,
);
return (
<Table
data-testid="registered-model-table"
data={registeredModels}
columns={rmColumns}
toolbarContent={toolbarContent}
defaultSortColumn={2}
onClearFilters={clearFilters}
enablePagination
emptyTableView={<DashboardEmptyTableView onClearFilters={clearFilters} />}
rowRenderer={(rm) => (
<RegisteredModelTableRow
key={rm.name}
hasDeploys={hasDeploys(rm.id)}
registeredModel={rm}
refresh={refresh}
/>
)}
/>
);
};

export default RegisteredModelTable;
Original file line number Diff line number Diff line change
Expand Up @@ -19,12 +19,14 @@ import { ModelVersionsTab } from '~/pages/modelRegistry/screens/ModelVersions/co
type RegisteredModelTableRowProps = {
registeredModel: RegisteredModel;
isArchiveRow?: boolean;
hasDeploys?: boolean;
refresh: () => void;
};

const RegisteredModelTableRow: React.FC<RegisteredModelTableRowProps> = ({
registeredModel: rm,
isArchiveRow,
hasDeploys = false,
refresh,
}) => {
const { apiState } = React.useContext(ModelRegistryContext);
Expand Down Expand Up @@ -57,6 +59,10 @@ const RegisteredModelTableRow: React.FC<RegisteredModelTableRowProps> = ({
: {
title: 'Archive model',
onClick: () => setIsArchiveModalOpen(true),
isAriaDisabled: hasDeploys,
tooltipProps: hasDeploys
? { content: 'Models with deployed versions cannot be archived.' }

Check warning on line 64 in frontend/src/pages/modelRegistry/screens/RegisteredModels/RegisteredModelTableRow.tsx

View check run for this annotation

Codecov / codecov/patch

frontend/src/pages/modelRegistry/screens/RegisteredModels/RegisteredModelTableRow.tsx#L64

Added line #L64 was not covered by tests
: undefined,
},
];

Expand Down

0 comments on commit 284829b

Please sign in to comment.