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

"Source model format" and "source model format version" should be editable #3369

Merged
merged 13 commits into from
Oct 25, 2024
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,14 @@ class ModelVersionDetails {
return cy.findByTestId('model-version-description');
}

findSourceModelFormat(subComponent: 'group' | 'edit' | 'save' | 'cancel') {
return cy.findByTestId(`source-model-format-${subComponent}`);
}

findSourceModelVersion(subComponent: 'group' | 'edit' | 'save' | 'cancel') {
return cy.findByTestId(`source-model-version-${subComponent}`);
}

findMoreLabelsButton() {
return cy.findByTestId('label-group').find('button');
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -642,6 +642,13 @@ declare global {
((
type: 'POST /api/modelRegistryRoleBindings',
response: OdhResponse<RoleBindingKind>,
) => Cypress.Chainable<null>) &
((
type: 'PATCH /api/service/modelregistry/:serviceName/api/model_registry/:apiVersion/model_artifacts/:artifactId',
options: {
path: { serviceName: string; apiVersion: string; artifactId: string };
},
response: OdhResponse<ModelArtifact>,
) => Cypress.Chainable<null>);
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import {
mockProjectK8sResource,
mockDscStatus,
} from '~/__mocks__';
import { mockModelArtifact } from '~/__mocks__/mockModelArtifact';

import {
InferenceServiceModel,
Expand Down Expand Up @@ -375,4 +376,63 @@ describe('Model version details', () => {
modelServingGlobal.getModelRow('Test Inference Service').should('exist');
});
});

describe('Model Version Details', () => {
beforeEach(() => {
initIntercepts();
modelVersionDetails.visit();
});

it('should update source model format', () => {
cy.interceptOdh(
'PATCH /api/service/modelregistry/:serviceName/api/model_registry/:apiVersion/model_artifacts/:artifactId',
{
path: {
serviceName: 'modelregistry-sample',
apiVersion: MODEL_REGISTRY_API_VERSION,
artifactId: '1',
},
},
mockModelArtifact({}),
).as('updateModelFormat');

modelVersionDetails.findSourceModelFormat('edit').click();
modelVersionDetails
.findSourceModelFormat('group')
.find('input')
.clear()
.type('UpdatedFormat');
modelVersionDetails.findSourceModelFormat('save').click();

cy.wait('@updateModelFormat').then((interception) => {
expect(interception.request.body).to.deep.equal({
modelFormatName: 'UpdatedFormat',
});
});
});

it('should update source model version', () => {
cy.interceptOdh(
'PATCH /api/service/modelregistry/:serviceName/api/model_registry/:apiVersion/model_artifacts/:artifactId',
{
path: {
serviceName: 'modelregistry-sample',
apiVersion: MODEL_REGISTRY_API_VERSION,
artifactId: '1',
},
},
mockModelArtifact({}),
).as('updateModelVersion');

modelVersionDetails.findSourceModelVersion('edit').click();
modelVersionDetails.findSourceModelVersion('group').find('input').clear().type('2.0.0');
modelVersionDetails.findSourceModelVersion('save').click();

cy.wait('@updateModelVersion').then((interception) => {
expect(interception.request.body).to.deep.equal({
modelFormatVersion: '2.0.0',
});
});
});
});
});
2 changes: 1 addition & 1 deletion frontend/src/components/DashboardDescriptionListGroup.scss
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
.odh-custom-description-list-term-with-action > span {
/* Workaround for missing functionality in PF DescriptionList, see https://github.com/patternfly/patternfly/issues/6583 */
width: 100%;
}
}
16 changes: 12 additions & 4 deletions frontend/src/components/DashboardDescriptionListGroup.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,9 @@ type EditableProps = {
onEditClick: () => void;
onSaveEditsClick: () => void;
onDiscardEditsClick: () => void;
editButtonTestId?: string;
saveButtonTestId?: string;
cancelButtonTestId?: string;
};

export type DashboardDescriptionListGroupProps = {
Expand All @@ -32,6 +35,7 @@ export type DashboardDescriptionListGroupProps = {
isEmpty?: boolean;
contentWhenEmpty?: React.ReactNode;
children: React.ReactNode;
groupTestId?: string;
} & (({ isEditable: true } & EditableProps) | ({ isEditable?: false } & Partial<EditableProps>));

const DashboardDescriptionListGroup: React.FC<DashboardDescriptionListGroupProps> = (props) => {
Expand All @@ -49,9 +53,13 @@ const DashboardDescriptionListGroup: React.FC<DashboardDescriptionListGroupProps
onSaveEditsClick,
onDiscardEditsClick,
children,
groupTestId,
editButtonTestId,
saveButtonTestId,
cancelButtonTestId,
} = props;
return (
<DescriptionListGroup>
<DescriptionListGroup data-testid={groupTestId}>
{action || isEditable ? (
<DescriptionListTerm className="odh-custom-description-list-term-with-action">
<Split>
Expand All @@ -62,7 +70,7 @@ const DashboardDescriptionListGroup: React.FC<DashboardDescriptionListGroupProps
<ActionList isIconList>
<ActionListItem>
<Button
data-testid={`save-edit-button-${title}`}
data-testid={saveButtonTestId}
aria-label={`Save edits to ${title}`}
variant="link"
onClick={onSaveEditsClick}
Expand All @@ -73,7 +81,7 @@ const DashboardDescriptionListGroup: React.FC<DashboardDescriptionListGroupProps
</ActionListItem>
<ActionListItem>
<Button
data-testid={`discard-edit-button-${title}`}
data-testid={cancelButtonTestId}
aria-label={`Discard edits to ${title} `}
variant="plain"
onClick={onDiscardEditsClick}
Expand All @@ -85,7 +93,7 @@ const DashboardDescriptionListGroup: React.FC<DashboardDescriptionListGroupProps
</ActionList>
) : (
<Button
data-testid={`edit-button-${title}`}
data-testid={editButtonTestId}
aria-label={`Edit ${title}`}
isInline
variant="link"
Expand Down
45 changes: 31 additions & 14 deletions frontend/src/components/EditableTextDescriptionListGroup.tsx
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import * as React from 'react';
import { ExpandableSection, TextArea } from '@patternfly/react-core';
import { ExpandableSection, TextArea, TextInput } from '@patternfly/react-core';
import DashboardDescriptionListGroup, {
DashboardDescriptionListGroupProps,
} from './DashboardDescriptionListGroup';
Expand All @@ -10,8 +10,9 @@ type EditableTextDescriptionListGroupProps = Pick<
> & {
value: string;
saveEditedValue: (value: string) => Promise<void>;
testid?: string;
baseTestId?: string;
isArchive?: boolean;
editableVariant: 'TextInput' | 'TextArea';
};

const EditableTextDescriptionListGroup: React.FC<EditableTextDescriptionListGroupProps> = ({
Expand All @@ -20,7 +21,8 @@ const EditableTextDescriptionListGroup: React.FC<EditableTextDescriptionListGrou
value,
isArchive,
saveEditedValue,
testid,
baseTestId,
editableVariant,
}) => {
const [isEditing, setIsEditing] = React.useState(false);
const [unsavedValue, setUnsavedValue] = React.useState(value);
Expand All @@ -34,17 +36,32 @@ const EditableTextDescriptionListGroup: React.FC<EditableTextDescriptionListGrou
isEditable={!isArchive}
isEditing={isEditing}
isSavingEdits={isSavingEdits}
groupTestId={`${baseTestId}-group`}
editButtonTestId={`${baseTestId}-edit`}
saveButtonTestId={`${baseTestId}-save`}
cancelButtonTestId={`${baseTestId}-cancel`}
contentWhenEditing={
<TextArea
autoFocus
data-testid={`edit-text-area-${title}`}
aria-label={`Text box for editing ${title}`}
value={unsavedValue}
onChange={(_event, v) => setUnsavedValue(v)}
isDisabled={isSavingEdits}
rows={24}
resizeOrientation="vertical"
/>
editableVariant === 'TextInput' ? (
<TextInput
autoFocus
data-testid={`${baseTestId}-input`}
aria-label={`Text input for editing ${title}`}
value={unsavedValue}
onChange={(_event, v) => setUnsavedValue(v)}
isDisabled={isSavingEdits}
/>
) : (
<TextArea
autoFocus
data-testid={`${baseTestId}-input`}
aria-label={`Text box for editing ${title}`}
value={unsavedValue}
onChange={(_event, v) => setUnsavedValue(v)}
isDisabled={isSavingEdits}
rows={24}
resizeOrientation="vertical"
/>
)
}
onEditClick={() => {
setUnsavedValue(value);
Expand All @@ -65,7 +82,7 @@ const EditableTextDescriptionListGroup: React.FC<EditableTextDescriptionListGrou
}}
>
<ExpandableSection
data-testid={testid}
data-testid={baseTestId}
variant="truncate"
truncateMaxLines={12}
toggleText={isTextExpanded ? 'Show less' : 'Show more'}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,9 +24,11 @@ const ModelVersionDetailsView: React.FC<ModelVersionDetailsViewProps> = ({
isArchiveVersion,
refresh,
}) => {
const [modelArtifact] = useModelArtifactsByVersionId(mv.id);
// TODO handle loading state / error for artifacts here?
const [modelArtifacts, , , refreshModelArtifacts] = useModelArtifactsByVersionId(mv.id);
const modelArtifact = modelArtifacts.items.length ? modelArtifacts.items[0] : null;
const { apiState } = React.useContext(ModelRegistryContext);
const storageFields = uriToObjectStorageFields(modelArtifact.items[0]?.uri || '');
const storageFields = uriToObjectStorageFields(modelArtifact?.uri || '');

return (
<Flex
Expand All @@ -37,7 +39,8 @@ const ModelVersionDetailsView: React.FC<ModelVersionDetailsViewProps> = ({
<FlexItem flex={{ default: 'flex_1' }}>
<DescriptionList isFillColumns>
<EditableTextDescriptionListGroup
testid="model-version-description"
editableVariant="TextArea"
baseTestId="model-version-description"
isArchive={isArchiveVersion}
title="Description"
contentWhenEmpty="No description"
Expand Down Expand Up @@ -99,7 +102,7 @@ const ModelVersionDetailsView: React.FC<ModelVersionDetailsViewProps> = ({
<>
<DashboardDescriptionListGroup
title="Endpoint"
isEmpty={modelArtifact.size === 0 || !storageFields.endpoint}
isEmpty={modelArtifacts.size === 0 || !storageFields.endpoint}
contentWhenEmpty="No endpoint"
>
<InlineTruncatedClipboardCopy
Expand All @@ -109,7 +112,7 @@ const ModelVersionDetailsView: React.FC<ModelVersionDetailsViewProps> = ({
</DashboardDescriptionListGroup>
<DashboardDescriptionListGroup
title="Region"
isEmpty={modelArtifact.size === 0 || !storageFields.region}
isEmpty={modelArtifacts.size === 0 || !storageFields.region}
contentWhenEmpty="No region"
>
<InlineTruncatedClipboardCopy
Expand All @@ -119,7 +122,7 @@ const ModelVersionDetailsView: React.FC<ModelVersionDetailsViewProps> = ({
</DashboardDescriptionListGroup>
<DashboardDescriptionListGroup
title="Bucket"
isEmpty={modelArtifact.size === 0 || !storageFields.bucket}
isEmpty={modelArtifacts.size === 0 || !storageFields.bucket}
contentWhenEmpty="No bucket"
>
<InlineTruncatedClipboardCopy
Expand All @@ -129,7 +132,7 @@ const ModelVersionDetailsView: React.FC<ModelVersionDetailsViewProps> = ({
</DashboardDescriptionListGroup>
<DashboardDescriptionListGroup
title="Path"
isEmpty={modelArtifact.size === 0 || !storageFields.path}
isEmpty={modelArtifacts.size === 0 || !storageFields.path}
contentWhenEmpty="No path"
>
<InlineTruncatedClipboardCopy
Expand All @@ -143,12 +146,12 @@ const ModelVersionDetailsView: React.FC<ModelVersionDetailsViewProps> = ({
<>
<DashboardDescriptionListGroup
title="URI"
isEmpty={modelArtifact.size === 0 || !modelArtifact.items[0].uri}
isEmpty={modelArtifacts.size === 0 || !modelArtifact?.uri}
contentWhenEmpty="No URI"
>
<InlineTruncatedClipboardCopy
testId="storage-uri"
textToCopy={modelArtifact.items[0]?.uri || ''}
textToCopy={modelArtifact?.uri || ''}
/>
</DashboardDescriptionListGroup>
</>
Expand All @@ -158,20 +161,36 @@ const ModelVersionDetailsView: React.FC<ModelVersionDetailsViewProps> = ({
Source model format
</Title>
<DescriptionList isFillColumns>
<DashboardDescriptionListGroup
title="Name"
isEmpty={modelArtifact.size === 0 || !modelArtifact.items[0].modelFormatName}
contentWhenEmpty="No source model format"
>
{modelArtifact.items[0]?.modelFormatName}
</DashboardDescriptionListGroup>
<DashboardDescriptionListGroup
<EditableTextDescriptionListGroup
editableVariant="TextInput"
baseTestId="source-model-format"
isArchive={isArchiveVersion}
value={modelArtifact?.modelFormatName || ''}
saveEditedValue={(value) =>
apiState.api
.patchModelArtifact({}, { modelFormatName: value }, modelArtifact?.id || '')
.then(() => {
refreshModelArtifacts();
})
}
title="Model Format"
contentWhenEmpty="No model format specified"
/>
<EditableTextDescriptionListGroup
editableVariant="TextInput"
baseTestId="source-model-version"
value={modelArtifact?.modelFormatVersion || ''}
isArchive={isArchiveVersion}
saveEditedValue={(newVersion) =>
apiState.api
.patchModelArtifact({}, { modelFormatVersion: newVersion }, modelArtifact?.id || '')
.then(() => {
refreshModelArtifacts();
})
}
title="Version"
isEmpty={modelArtifact.size === 0 || !modelArtifact.items[0].modelFormatVersion}
contentWhenEmpty="No source model format version"
>
{modelArtifact.items[0]?.modelFormatVersion}
</DashboardDescriptionListGroup>
/>
</DescriptionList>
<DescriptionList isFillColumns style={{ marginTop: '2em' }}>
<DashboardDescriptionListGroup
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ const ModelDetailsView: React.FC<ModelDetailsViewProps> = ({
<FlexItem flex={{ default: 'flex_1' }}>
<DescriptionList isFillColumns>
<EditableTextDescriptionListGroup
editableVariant="TextArea"
title="Description"
isArchive={isArchiveModel}
contentWhenEmpty="No description"
Expand Down
Loading