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

Add character length limit to both model name and version name #3377

Merged
merged 1 commit into from
Nov 4, 2024
Merged
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
Original file line number Diff line number Diff line change
Expand Up @@ -182,6 +182,7 @@ describe('Register model page', () => {
});

it('Creates expected resources on submit in object storage mode', () => {
const veryLongName = 'Test name'.repeat(15); // A string over 128 characters
registerModelPage.findFormField(FormFieldSelector.MODEL_NAME).type('Test model name');
registerModelPage
.findFormField(FormFieldSelector.MODEL_DESCRIPTION)
Expand All @@ -201,7 +202,17 @@ describe('Register model page', () => {
registerModelPage
.findFormField(FormFieldSelector.LOCATION_PATH)
.type('demo-models/flan-t5-small-caikit');
registerModelPage.findSubmitButton().should('be.enabled');

registerModelPage.findFormField(FormFieldSelector.MODEL_NAME).clear().type(veryLongName);
registerModelPage.findSubmitButton().should('be.disabled');
registerModelPage.findFormField(FormFieldSelector.VERSION_NAME).clear().type(veryLongName);
registerModelPage.findFormField(FormFieldSelector.MODEL_NAME).clear().type('Test model name');
registerModelPage.findSubmitButton().should('be.disabled');
registerModelPage
.findFormField(FormFieldSelector.VERSION_NAME)
.clear()
.type('Test version name');
registerModelPage.findSubmitButton().click();

cy.wait('@createRegisteredModel').then((interception) => {
Expand All @@ -224,7 +235,7 @@ describe('Register model page', () => {
});
cy.wait('@createModelArtifact').then((interception) => {
expect(interception.request.body).to.containSubset({
name: 'Test model name-Test version name-artifact',
name: 'Test version name',
description: 'Test version description',
customProperties: {},
state: ModelArtifactState.LIVE,
Expand Down Expand Up @@ -292,7 +303,7 @@ describe('Register model page', () => {
});
cy.wait('@createModelArtifact').then((interception) => {
expect(interception.request.body).to.containSubset({
name: 'Test model name-Test version name-artifact',
name: 'Test version name',
description: 'Test version description',
customProperties: {},
state: ModelArtifactState.LIVE,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -350,6 +350,7 @@ describe('Register model page with no preselected model', () => {
});

it('Creates expected resources on submit in object storage mode', () => {
const veryLongName = 'Test name'.repeat(15); // A string over 128 characters
registerVersionPage.selectRegisteredModel('Test model 1');
registerVersionPage.findFormField(FormFieldSelector.VERSION_NAME).type('Test version name');
registerVersionPage
Expand All @@ -359,6 +360,14 @@ describe('Register model page with no preselected model', () => {
.findFormField(FormFieldSelector.LOCATION_PATH)
.type('demo-models/flan-t5-small-caikit');

registerVersionPage.findFormField(FormFieldSelector.VERSION_NAME).clear().type(veryLongName);
registerVersionPage.findSubmitButton().should('be.disabled');
registerVersionPage
.findFormField(FormFieldSelector.VERSION_NAME)
.clear()
.type('Test version name');

registerVersionPage.findSubmitButton().should('be.enabled');
registerVersionPage.findSubmitButton().click();

cy.wait('@createModelVersion').then((interception) => {
Expand All @@ -373,7 +382,7 @@ describe('Register model page with no preselected model', () => {
});
cy.wait('@createModelArtifact').then((interception) => {
expect(interception.request.body).to.containSubset({
name: 'Test model 1-Test version name-artifact',
name: 'Test version name',
description: 'Test version description',
customProperties: {},
state: ModelArtifactState.LIVE,
Expand Down Expand Up @@ -428,7 +437,7 @@ describe('Register model page with no preselected model', () => {
});
cy.wait('@createModelArtifact').then((interception) => {
expect(interception.request.body).to.containSubset({
name: 'Test model 1-Test version name-artifact',
name: 'Test version name',
description: 'Test version description',
customProperties: {},
state: ModelArtifactState.LIVE,
Expand Down Expand Up @@ -513,7 +522,7 @@ describe('Register model page with preselected model', () => {
});
cy.wait('@createModelArtifact').then((interception) => {
expect(interception.request.body).to.containSubset({
name: 'Test model 1-Test version name-artifact',
name: 'Test version name',
description: 'Test version description',
customProperties: {},
state: ModelArtifactState.LIVE,
Expand Down Expand Up @@ -568,7 +577,7 @@ describe('Register model page with preselected model', () => {
});
cy.wait('@createModelArtifact').then((interception) => {
expect(interception.request.body).to.containSubset({
name: 'Test model 1-Test version name-artifact',
name: 'Test version name',
description: 'Test version description',
customProperties: {},
state: ModelArtifactState.LIVE,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,9 @@ import {
BreadcrumbItem,
Form,
FormGroup,
FormHelperText,
HelperText,
HelperTextItem,
PageSection,
Stack,
StackItem,
Expand All @@ -17,11 +20,12 @@ import FormSection from '~/components/pf-overrides/FormSection';
import ApplicationsPage from '~/pages/ApplicationsPage';
import { modelRegistryUrl, registeredModelUrl } from '~/pages/modelRegistry/screens/routeUtils';
import { useRegisterModelData } from './useRegisterModelData';
import { isRegisterModelSubmitDisabled, registerModel } from './utils';
import { isNameValid, isRegisterModelSubmitDisabled, registerModel } from './utils';
import RegistrationCommonFormSections from './RegistrationCommonFormSections';
import { useRegistrationCommonState } from './useRegistrationCommonState';
import PrefilledModelRegistryField from './PrefilledModelRegistryField';
import RegistrationFormFooter from './RegistrationFormFooter';
import { MR_CHARACTER_LIMIT } from './const';

const RegisterModel: React.FC = () => {
const { modelRegistry: mrName } = useParams();
Expand All @@ -31,6 +35,7 @@ const RegisterModel: React.FC = () => {
useRegistrationCommonState();

const [formData, setData] = useRegisterModelData();
const isModelNameValid = isNameValid(formData.modelName);
const isSubmitDisabled = isSubmitting || isRegisterModelSubmitDisabled(formData);
const { modelName, modelDescription } = formData;

Expand Down Expand Up @@ -75,7 +80,17 @@ const RegisterModel: React.FC = () => {
name="model-name"
value={modelName}
onChange={(_e, value) => setData('modelName', value)}
validated={isModelNameValid ? 'default' : 'error'}
/>
{!isModelNameValid && (
<FormHelperText>
<HelperText>
<HelperTextItem variant="error">
Cannot exceed {MR_CHARACTER_LIMIT} characters
</HelperTextItem>
</HelperText>
</FormHelperText>
)}
</FormGroup>
<FormGroup label="Model description" fieldId="model-description">
<TextArea
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@ import FormSection from '~/components/pf-overrides/FormSection';
import { ModelVersion } from '~/concepts/modelRegistry/types';
import { ModelLocationType, RegistrationCommonFormData } from './useRegisterModelData';
import { ConnectionModal } from './ConnectionModal';
import { MR_CHARACTER_LIMIT } from './const';
import { isNameValid } from './utils';

type RegistrationCommonFormSectionsProps<D extends RegistrationCommonFormData> = {
formData: D;
Expand All @@ -36,6 +38,7 @@ const RegistrationCommonFormSections = <D extends RegistrationCommonFormData>({
latestVersion,
}: RegistrationCommonFormSectionsProps<D>): React.ReactNode => {
const [isAutofillModalOpen, setAutofillModalOpen] = React.useState(false);
const isVersionNameValid = isNameValid(formData.versionName);

const connectionDataMap: Record<
string,
Expand Down Expand Up @@ -86,14 +89,24 @@ const RegistrationCommonFormSections = <D extends RegistrationCommonFormData>({
name="version-name"
value={versionName}
onChange={(_e, value) => setData('versionName', value)}
validated={isVersionNameValid ? 'default' : 'error'}
/>
{latestVersion && (
<FormHelperText>
<FormHelperText>
{latestVersion && (
<HelperText>
<HelperTextItem>Current version is {latestVersion.name}</HelperTextItem>
<HelperTextItem variant="indeterminate">
Current version is {latestVersion.name}
</HelperTextItem>
</HelperText>
</FormHelperText>
)}
)}
{!isVersionNameValid && (
<HelperText>
<HelperTextItem variant="error">
Cannot exceed {MR_CHARACTER_LIMIT} characters
</HelperTextItem>
</HelperText>
)}
</FormHelperText>
</FormGroup>
<FormGroup label="Version description" fieldId="version-description">
<TextArea
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export const MR_CHARACTER_LIMIT = 128;
12 changes: 9 additions & 3 deletions frontend/src/pages/modelRegistry/screens/RegisterModel/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import {
RegisterVersionFormData,
RegistrationCommonFormData,
} from './useRegisterModelData';
import { MR_CHARACTER_LIMIT } from './const';

export type RegisterModelCreatedResources = RegisterVersionCreatedResources & {
registeredModel: RegisteredModel;
Expand Down Expand Up @@ -66,7 +67,7 @@ export const registerVersion = async (
},
);
const modelArtifact = await apiState.api.createModelArtifactForModelVersion({}, modelVersion.id, {
name: `${registeredModel.name}-${formData.versionName}-artifact`,
name: `${formData.versionName}`,
description: formData.versionDescription,
customProperties: {},
state: ModelArtifactState.LIVE,
Expand Down Expand Up @@ -103,12 +104,17 @@ const isSubmitDisabledForCommonFields = (formData: RegistrationCommonFormData):
!versionName ||
(modelLocationType === ModelLocationType.URI && !modelLocationURI) ||
(modelLocationType === ModelLocationType.ObjectStorage &&
(!modelLocationBucket || !modelLocationEndpoint || !modelLocationPath))
(!modelLocationBucket || !modelLocationEndpoint || !modelLocationPath)) ||
!isNameValid(versionName)
);
};

export const isRegisterModelSubmitDisabled = (formData: RegisterModelFormData): boolean =>
!formData.modelName || isSubmitDisabledForCommonFields(formData);
!formData.modelName ||
isSubmitDisabledForCommonFields(formData) ||
!isNameValid(formData.modelName);

export const isRegisterVersionSubmitDisabled = (formData: RegisterVersionFormData): boolean =>
!formData.registeredModelId || isSubmitDisabledForCommonFields(formData);

export const isNameValid = (name: string): boolean => name.length <= MR_CHARACTER_LIMIT;
Loading