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.

Sign up for GitHub

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

Jump to bottom

Update archived models and archived versions components #3206

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Update archived models and archived versions components #3206

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
4 changes: 4 additions & 0 deletions frontend/src/__tests__/cypress/cypress/pages/modelRegistry.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,10 @@ class ModelRegistry {
cy.findByTestId('empty-model-versions').should('exist');
}

shouldArchveModelVersionsEmpty() {
cy.findByTestId('empty-archive-model-versions').should('exist');
}

shouldModelRegistrySelectorExist() {
cy.findByTestId('model-registry-selector-dropdown').should('exist');
}
Expand Down
6 changes: 6 additions & 0 deletions frontend/src/__tests__/cypress/cypress/pages/modelRegistry/registeredModelArchive.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,12 @@ class ModelArchive {
cy.visit(`/modelRegistry/${preferredModelRegistry}/registeredModels/archive/${rmId}`);
}

visitArchiveModelVersionList() {
const rmId = '2';
const preferredModelRegistry = 'modelregistry-sample';
cy.visit(`/modelRegistry/${preferredModelRegistry}/registeredModels/archive/${rmId}/versions`);
}

visitModelList() {
cy.visit('/modelRegistry/modelregistry-sample');
this.wait();
Expand Down
42 changes: 40 additions & 2 deletions ...end/src/__tests__/cypress/cypress/tests/mocked/modelRegistry/registeredModelArchive.cy.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ const initIntercepts = ({
mockRegisteredModel({ id: '4', name: 'model 4' }),
],
modelVersions = [
mockModelVersion({ author: 'Author 1' }),
mockModelVersion({ author: 'Author 1', registeredModelId: '2' }),
mockModelVersion({ name: 'model version' }),
],
}: HandlersProps) => {
Expand All @@ -74,13 +74,25 @@ const initIntercepts = ({
mockRegisteredModelList({ items: registeredModels }),
);

cy.interceptOdh(
`GET /api/service/modelregistry/:serviceName/api/model_registry/:apiVersion/model_versions/:modelVersionId`,
{
path: {
serviceName: 'modelregistry-sample',
apiVersion: MODEL_REGISTRY_API_VERSION,
modelVersionId: 1,
},
},
mockModelVersion({ id: '1', name: 'Version 2' }),
);

cy.interceptOdh(
`GET /api/service/modelregistry/:serviceName/api/model_registry/:apiVersion/registered_models/:registeredModelId/versions`,
{
path: {
serviceName: 'modelregistry-sample',
apiVersion: MODEL_REGISTRY_API_VERSION,
registeredModelId: 1,
registeredModelId: 2,
},
},
mockModelVersionList({ items: modelVersions }),
Expand Down Expand Up @@ -123,6 +135,32 @@ describe('Model archive list', () => {
registeredModelArchive.findArchiveModelTable().should('be.visible');
});

it('Archived model with no versions', () => {
initIntercepts({ modelVersions: [] });
registeredModelArchive.visit();
verifyRelativeURL('/modelRegistry/modelregistry-sample/registeredModels/archive');
registeredModelArchive.findArchiveModelBreadcrumbItem().contains('Archived models');
const archiveModelRow = registeredModelArchive.getRow('model 2');
archiveModelRow.findName().contains('model 2').click();
modelRegistry.shouldArchveModelVersionsEmpty();
});

it('Archived model flow', () => {
initIntercepts({});
registeredModelArchive.visitArchiveModelVersionList();
verifyRelativeURL('/modelRegistry/modelregistry-sample/registeredModels/archive/2/versions');

modelRegistry.findModelVersionsTable().should('be.visible');
modelRegistry.findModelVersionsTableRows().should('have.length', 2);
const version = modelRegistry.getModelVersionRow('model version');
version.findModelVersionName().contains('model version').click();
verifyRelativeURL(
'/modelRegistry/modelregistry-sample/registeredModels/archive/2/versions/1/details',
);
cy.go('back');
verifyRelativeURL('/modelRegistry/modelregistry-sample/registeredModels/archive/2/versions');
});

it('Archive models list', () => {
initIntercepts({});
registeredModelArchive.visit();
Expand Down
4 changes: 3 additions & 1 deletion frontend/src/components/EditableLabelsDescriptionListGroup.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,13 +22,15 @@ type EditableTextDescriptionListGroupProps = Partial<
labels: string[];
saveEditedLabels: (labels: string[]) => Promise<unknown>;
allExistingKeys?: string[];
isArchive?: boolean;
};

const EditableLabelsDescriptionListGroup: React.FC<EditableTextDescriptionListGroupProps> = ({
title = 'Labels',
contentWhenEmpty = 'No labels',
labels,
saveEditedLabels,
isArchive,
allExistingKeys = labels,
}) => {
const [isEditing, setIsEditing] = React.useState(false);
Expand Down Expand Up @@ -98,7 +100,7 @@ const EditableLabelsDescriptionListGroup: React.FC<EditableTextDescriptionListGr
title={title}
isEmpty={labels.length === 0}
contentWhenEmpty={contentWhenEmpty}
isEditable
isEditable={!isArchive}
isEditing={isEditing}
isSavingEdits={isSavingEdits}
contentWhenEditing={
Expand Down
4 changes: 3 additions & 1 deletion frontend/src/components/EditableTextDescriptionListGroup.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,12 +11,14 @@ type EditableTextDescriptionListGroupProps = Pick<
value: string;
saveEditedValue: (value: string) => Promise<void>;
testid?: string;
isArchive?: boolean;
};

const EditableTextDescriptionListGroup: React.FC<EditableTextDescriptionListGroupProps> = ({
title,
contentWhenEmpty,
value,
isArchive,
saveEditedValue,
testid,
}) => {
Expand All @@ -29,7 +31,7 @@ const EditableTextDescriptionListGroup: React.FC<EditableTextDescriptionListGrou
title={title}
isEmpty={!value}
contentWhenEmpty={contentWhenEmpty}
isEditable
isEditable={!isArchive}
isEditing={isEditing}
isSavingEdits={isSavingEdits}
contentWhenEditing={
Expand Down
20 changes: 20 additions & 0 deletions frontend/src/pages/modelRegistry/ModelRegistryRoutes.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import RegisteredModelsArchiveDetails from './screens/RegisteredModelsArchive/Re
import RegisterModel from './screens/RegisterModel/RegisterModel';
import RegisterVersion from './screens/RegisterModel/RegisterVersion';
import { modelRegistryUrl } from './screens/routeUtils';
import ArchiveModelVersionDetails from './screens/ModelVersionsArchive/ArchiveModelVersionDetails';

const ModelRegistryRoutes: React.FC = () => (
<Routes>
Expand Down Expand Up @@ -91,6 +92,25 @@ const ModelRegistryRoutes: React.FC = () => (
<RegisteredModelsArchiveDetails tab={ModelVersionsTab.VERSIONS} empty={false} />
}
/>
<Route path="versions/:modelVersionId">
<Route index element={<Navigate to={ModelVersionDetailsTab.DETAILS} replace />} />
<Route
path={ModelVersionDetailsTab.DETAILS}
element={
<ArchiveModelVersionDetails tab={ModelVersionDetailsTab.DETAILS} empty={false} />
}
/>
<Route
path={ModelVersionDetailsTab.DEPLOYMENTS}
element={
<ArchiveModelVersionDetails
tab={ModelVersionDetailsTab.DEPLOYMENTS}
empty={false}
/>
}
/>
<Route path="*" element={<Navigate to="." />} />
</Route>
<Route path="*" element={<Navigate to="." />} />
</Route>
<Route path="*" element={<Navigate to="." />} />
Expand Down
27 changes: 16 additions & 11 deletions frontend/src/pages/modelRegistry/screens/ModelPropertiesDescriptionListGroup.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,11 +11,13 @@ import { getProperties, mergeUpdatedProperty } from './utils';

type ModelPropertiesDescriptionListGroupProps = {
customProperties: ModelRegistryCustomProperties;
isArchive?: boolean;
saveEditedCustomProperties: (properties: ModelRegistryCustomProperties) => Promise<unknown>;
};

const ModelPropertiesDescriptionListGroup: React.FC<ModelPropertiesDescriptionListGroupProps> = ({
customProperties = {},
isArchive,
saveEditedCustomProperties,
}) => {
const [editingPropertyKeys, setEditingPropertyKeys] = React.useState<string[]>([]);
Expand Down Expand Up @@ -51,16 +53,18 @@ const ModelPropertiesDescriptionListGroup: React.FC<ModelPropertiesDescriptionLi
<DashboardDescriptionListGroup
title="Properties"
action={
<Button
isInline
variant="link"
icon={<PlusCircleIcon />}
iconPosition="start"
isDisabled={isAdding || isSavingEdits}
onClick={() => setIsAdding(true)}
>
Add property
</Button>
!isArchive && (
<Button
isInline
variant="link"
icon={<PlusCircleIcon />}
iconPosition="start"
isDisabled={isAdding || isSavingEdits}
onClick={() => setIsAdding(true)}
>
Add property
</Button>
)
}
isEmpty={!isAdding && keys.length === 0}
contentWhenEmpty="No properties"
Expand All @@ -70,13 +74,14 @@ const ModelPropertiesDescriptionListGroup: React.FC<ModelPropertiesDescriptionLi
<Tr>
<Th>Key {isEditingSomeRow && requiredAsterisk}</Th>
<Th>Value {isEditingSomeRow && requiredAsterisk}</Th>
<Th />
<Th screenReaderText="Actions" />
</Tr>
</Thead>
<Tbody>
{shownKeys.map((key) => (
<ModelPropertiesTableRow
key={key}
isArchive={isArchive}
keyValuePair={{ key, value: filteredProperties[key].string_value }}
allExistingKeys={allExistingKeys}
isEditing={editingPropertyKeys.includes(key)}
Expand Down
78 changes: 41 additions & 37 deletions frontend/src/pages/modelRegistry/screens/ModelPropertiesTableRow.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ type ModelPropertiesTableRowProps = {
allExistingKeys: string[];
setIsEditing: (isEditing: boolean) => void;
isSavingEdits: boolean;
isArchive?: boolean;
setIsSavingEdits: (isSaving: boolean) => void;
saveEditedProperty: (oldKey: string, newPair: KeyValuePair) => Promise<unknown>;
} & EitherNotBoth<
Expand All @@ -38,6 +39,7 @@ const ModelPropertiesTableRow: React.FC<ModelPropertiesTableRowProps> = ({
setIsEditing,
isSavingEdits,
setIsSavingEdits,
isArchive,
saveEditedProperty,
}) => {
const { key, value } = keyValuePair;
Expand Down Expand Up @@ -143,43 +145,45 @@ const ModelPropertiesTableRow: React.FC<ModelPropertiesTableRowProps> = ({
</ExpandableSection>
)}
</Td>
<Td isActionCell width={10}>
{isEditing ? (
<ActionList isIconList>
<ActionListItem>
<Button
data-testid={`save-edit-button-property-${key}`}
aria-label={`Save edits to property with key ${key}`}
variant="link"
onClick={onSaveEditsClick}
isDisabled={isSavingEdits || !unsavedKey || !unsavedValue || !!keyValidationError}
>
<CheckIcon />
</Button>
</ActionListItem>
<ActionListItem>
<Button
data-testid={`discard-edit-button-property-${key}`}
aria-label={`Discard edits to property with key ${key}`}
variant="plain"
onClick={onDiscardEditsClick}
isDisabled={isSavingEdits}
>
<TimesIcon />
</Button>
</ActionListItem>
</ActionList>
) : (
<ActionsColumn
isDisabled={isSavingEdits}
popperProps={{ direction: 'up' }}
items={[
{ title: 'Edit', onClick: onEditClick, isDisabled: isSavingEdits },
{ title: 'Delete', onClick: onDeleteClick, isDisabled: isSavingEdits },
]}
/>
)}
</Td>
{!isArchive && (
<Td isActionCell width={10}>
{isEditing ? (
<ActionList isIconList>
<ActionListItem>
<Button
data-testid={`save-edit-button-property-${key}`}
aria-label={`Save edits to property with key ${key}`}
variant="link"
onClick={onSaveEditsClick}
isDisabled={isSavingEdits || !unsavedKey || !unsavedValue || !!keyValidationError}
>
<CheckIcon />
</Button>
</ActionListItem>
<ActionListItem>
<Button
data-testid={`discard-edit-button-property-${key}`}
aria-label={`Discard edits to property with key ${key}`}
variant="plain"
onClick={onDiscardEditsClick}
isDisabled={isSavingEdits}
>
<TimesIcon />
</Button>
</ActionListItem>
</ActionList>
) : (
<ActionsColumn
isDisabled={isSavingEdits}
popperProps={{ direction: 'up' }}
items={[
{ title: 'Edit', onClick: onEditClick, isDisabled: isSavingEdits },
{ title: 'Delete', onClick: onDeleteClick, isDisabled: isSavingEdits },
]}
/>
)}
</Td>
)}
</Tr>
);
};
Expand Down
30 changes: 28 additions & 2 deletions frontend/src/pages/modelRegistry/screens/ModelVersionDetails/ModelVersionDetails.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,15 +1,21 @@
import React from 'react';
import React, { useEffect } from 'react';
import { useNavigate, useParams } from 'react-router';
import { Breadcrumb, BreadcrumbItem, Flex, FlexItem, Truncate } from '@patternfly/react-core';
import { Link } from 'react-router-dom';
import ApplicationsPage from '~/pages/ApplicationsPage';
import useModelVersionById from '~/concepts/modelRegistry/apiHooks/useModelVersionById';
import { ModelRegistrySelectorContext } from '~/concepts/modelRegistry/context/ModelRegistrySelectorContext';
import { modelVersionUrl, registeredModelUrl } from '~/pages/modelRegistry/screens/routeUtils';
import {
archiveModelVersionDetailsUrl,
modelVersionArchiveDetailsUrl,
modelVersionUrl,
registeredModelUrl,
} from '~/pages/modelRegistry/screens/routeUtils';
import useRegisteredModelById from '~/concepts/modelRegistry/apiHooks/useRegisteredModelById';
import useInferenceServices from '~/pages/modelServing/useInferenceServices';
import useServingRuntimes from '~/pages/modelServing/useServingRuntimes';
import { useMakeFetchObject } from '~/utilities/useMakeFetchObject';
import { ModelState } from '~/concepts/modelRegistry/types';
import { ModelVersionDetailsTab } from './const';
import ModelVersionsDetailsHeaderActions from './ModelVersionDetailsHeaderActions';
import ModelVersionDetailsTabs from './ModelVersionDetailsTabs';
Expand Down Expand Up @@ -41,6 +47,26 @@ const ModelVersionsDetails: React.FC<ModelVersionsDetailProps> = ({ tab, ...page
servingRuntimes.refresh();
}, [inferenceServices, servingRuntimes, refreshModelVersion]);

useEffect(() => {
if (rm?.state === ModelState.ARCHIVED && mv?.id) {
navigate(
archiveModelVersionDetailsUrl(
mv.id,
mv.registeredModelId,
preferredModelRegistry?.metadata.name,
),
);
} else if (mv?.state === ModelState.ARCHIVED) {
navigate(
modelVersionArchiveDetailsUrl(
mv.id,
mv.registeredModelId,
preferredModelRegistry?.metadata.name,
),
);
}
});

Comment on lines +50 to +68
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should all these new useEffects have dependency arrays? I'm not sure whether it matters since the if conditions are the same perf impact as comparing dependencies across renders, but it may still be better to not run the effects every render in case we modify them later.

return (
<ApplicationsPage
{...pageProps}
Expand Down
Loading
Loading