Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(MR): 12864 disable archive for mr/mv if has deploys #3292

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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 @@
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 @@
: {
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