From caa0381a6fb8eacdc4fbae939abd9196aa1bce51 Mon Sep 17 00:00:00 2001 From: ppadti Date: Mon, 2 Dec 2024 21:45:10 +0530 Subject: [PATCH] Improve error message for model registration --- .../screens/RegisterModel/RegisterModel.tsx | 46 ++++-- .../RegisterModel/RegisterModelErrors.tsx | 114 +++++++++++++++ .../screens/RegisterModel/RegisterVersion.tsx | 45 ++++-- .../RegisterModel/RegistrationFormFooter.tsx | 36 ++--- .../screens/RegisterModel/const.ts | 11 ++ .../useRegistrationCommonState.ts | 41 ------ .../screens/RegisterModel/utils.ts | 135 +++++++++++------- 7 files changed, 288 insertions(+), 140 deletions(-) create mode 100644 frontend/src/pages/modelRegistry/screens/RegisterModel/RegisterModelErrors.tsx delete mode 100644 frontend/src/pages/modelRegistry/screens/RegisterModel/useRegistrationCommonState.ts diff --git a/frontend/src/pages/modelRegistry/screens/RegisterModel/RegisterModel.tsx b/frontend/src/pages/modelRegistry/screens/RegisterModel/RegisterModel.tsx index 124fb3bb22..d79db22e9e 100644 --- a/frontend/src/pages/modelRegistry/screens/RegisterModel/RegisterModel.tsx +++ b/frontend/src/pages/modelRegistry/screens/RegisterModel/RegisterModel.tsx @@ -19,31 +19,49 @@ import { Link } from 'react-router-dom'; import FormSection from '~/components/pf-overrides/FormSection'; import ApplicationsPage from '~/pages/ApplicationsPage'; import { modelRegistryUrl, registeredModelUrl } from '~/pages/modelRegistry/screens/routeUtils'; +import { ModelRegistryContext } from '~/concepts/modelRegistry/context/ModelRegistryContext'; +import { useAppSelector } from '~/redux/hooks'; import { useRegisterModelData } from './useRegisterModelData'; 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'; +import { MR_CHARACTER_LIMIT, SubmitLabel } from './const'; const RegisterModel: React.FC = () => { const { modelRegistry: mrName } = useParams(); const navigate = useNavigate(); - - const { isSubmitting, submitError, setSubmitError, handleSubmit, apiState, author } = - useRegistrationCommonState(); - + const { apiState } = React.useContext(ModelRegistryContext); + const author = useAppSelector((state) => state.user || ''); + const [isSubmitting, setIsSubmitting] = React.useState(false); + const [submitError, setSubmitError] = React.useState(undefined); const [formData, setData] = useRegisterModelData(); const isModelNameValid = isNameValid(formData.modelName); const isSubmitDisabled = isSubmitting || isRegisterModelSubmitDisabled(formData); const { modelName, modelDescription } = formData; + const [registeredModelName, setRegisteredModelName] = React.useState(''); + const [versionName, setVersionName] = React.useState(''); + const [errorName, setErrorName] = React.useState(undefined); + + const handleSubmit = async () => { + setIsSubmitting(true); + setSubmitError(undefined); - const onSubmit = () => - handleSubmit(async () => { - const { registeredModel } = await registerModel(apiState, formData, author); + const { + data: { registeredModel, modelVersion, modelArtifact }, + errors, + } = await registerModel(apiState, formData, author); + if (registeredModel && modelVersion && modelArtifact) { navigate(registeredModelUrl(registeredModel.id, mrName)); - }); + } else if (Object.keys(errors).length > 0) { + setIsSubmitting(false); + setRegisteredModelName(formData.modelName); + setVersionName(formData.versionName); + const resourceName = Object.keys(errors)[0]; + setErrorName(resourceName); + setSubmitError(errors[resourceName]); + } + }; const onCancel = () => navigate(modelRegistryUrl(mrName)); return ( @@ -110,13 +128,15 @@ const RegisterModel: React.FC = () => { diff --git a/frontend/src/pages/modelRegistry/screens/RegisterModel/RegisterModelErrors.tsx b/frontend/src/pages/modelRegistry/screens/RegisterModel/RegisterModelErrors.tsx new file mode 100644 index 0000000000..d14b8548ff --- /dev/null +++ b/frontend/src/pages/modelRegistry/screens/RegisterModel/RegisterModelErrors.tsx @@ -0,0 +1,114 @@ +import React from 'react'; +import { Alert, AlertActionCloseButton, StackItem } from '@patternfly/react-core'; +import { ErrorName, SubmitLabel } from './const'; + +type RegisterModelErrorProp = { + submitLabel: string; + submitError: Error; + errorName?: string; + versionName?: string; + modelName?: string; +}; + +const RegisterModelErrors: React.FC = ({ + submitLabel, + submitError, + errorName, + versionName = '', + modelName = '', +}) => { + const [showAlert, setShowAlert] = React.useState(true); + + if (submitLabel === SubmitLabel.REGISTER_MODEL && errorName === ErrorName.MODEL_VERSION) { + return ( + <> + {showAlert && ( + + setShowAlert(false)} />} + /> + + )} + + + {submitError.message} + + + + ); + } + + if (submitLabel === SubmitLabel.REGISTER_VERSION && errorName === ErrorName.MODEL_VERSION) { + return ( + + + {submitError.message} + + + ); + } + + if (submitLabel === SubmitLabel.REGISTER_MODEL && errorName === ErrorName.MODEL_ARTIFACT) { + return ( + <> + {showAlert && ( + + setShowAlert(false)} />} + /> + + )} + + + {submitError.message} + + + + ); + } + + if (submitLabel === SubmitLabel.REGISTER_VERSION && errorName === ErrorName.MODEL_ARTIFACT) { + return ( + <> + {showAlert && ( + + setShowAlert(false)} />} + /> + + )} + + + {submitError.message} + + + + ); + } + + return ( + + + {submitError.message} + + + ); +}; +export default RegisterModelErrors; diff --git a/frontend/src/pages/modelRegistry/screens/RegisterModel/RegisterVersion.tsx b/frontend/src/pages/modelRegistry/screens/RegisterModel/RegisterVersion.tsx index 4046a5ab25..07c1feac1c 100644 --- a/frontend/src/pages/modelRegistry/screens/RegisterModel/RegisterVersion.tsx +++ b/frontend/src/pages/modelRegistry/screens/RegisterModel/RegisterVersion.tsx @@ -16,25 +16,29 @@ import ApplicationsPage from '~/pages/ApplicationsPage'; import { modelRegistryUrl, registeredModelUrl } from '~/pages/modelRegistry/screens/routeUtils'; import useRegisteredModels from '~/concepts/modelRegistry/apiHooks/useRegisteredModels'; import { filterLiveModels } from '~/concepts/modelRegistry/utils'; +import { ModelRegistryContext } from '~/concepts/modelRegistry/context/ModelRegistryContext'; +import { useAppSelector } from '~/redux/hooks'; import { useRegisterVersionData } from './useRegisterModelData'; import { isRegisterVersionSubmitDisabled, registerVersion } from './utils'; import RegistrationCommonFormSections from './RegistrationCommonFormSections'; -import { useRegistrationCommonState } from './useRegistrationCommonState'; import PrefilledModelRegistryField from './PrefilledModelRegistryField'; import RegistrationFormFooter from './RegistrationFormFooter'; import RegisteredModelSelector from './RegisteredModelSelector'; import { usePrefillRegisterVersionFields } from './usePrefillRegisterVersionFields'; +import { SubmitLabel } from './const'; const RegisterVersion: React.FC = () => { const { modelRegistry: mrName, registeredModelId: prefilledRegisteredModelId } = useParams(); - const navigate = useNavigate(); - - const { isSubmitting, submitError, setSubmitError, handleSubmit, apiState, author } = - useRegistrationCommonState(); - + const { apiState } = React.useContext(ModelRegistryContext); + const author = useAppSelector((state) => state.user || ''); + const [isSubmitting, setIsSubmitting] = React.useState(false); const [formData, setData] = useRegisterVersionData(prefilledRegisteredModelId); const isSubmitDisabled = isSubmitting || isRegisterVersionSubmitDisabled(formData); + const [submitError, setSubmitError] = React.useState(undefined); + const [errorName, setErrorName] = React.useState(undefined); + const [versionName, setVersionName] = React.useState(''); + const { registeredModelId } = formData; const [allRegisteredModels, loadedRegisteredModels, loadRegisteredModelsError] = @@ -48,15 +52,29 @@ const RegisterVersion: React.FC = () => { setData, }); - const onSubmit = () => { + const handleSubmit = async () => { if (!registeredModel) { return; // We shouldn't be able to hit this due to form validation } - handleSubmit(async () => { - await registerVersion(apiState, registeredModel, formData, author); + setIsSubmitting(true); + setSubmitError(undefined); + + const { + data: { modelVersion, modelArtifact }, + errors, + } = await registerVersion(apiState, registeredModel, formData, author); + + if (modelVersion && modelArtifact) { navigate(registeredModelUrl(registeredModel.id, mrName)); - }); + } else if (Object.keys(errors).length > 0) { + const resourceName = Object.keys(errors)[0]; + setVersionName(formData.versionName); + setErrorName(resourceName); + setSubmitError(errors[resourceName]); + setIsSubmitting(false); + } }; + const onCancel = () => navigate( prefilledRegisteredModelId && registeredModel @@ -125,13 +143,14 @@ const RegisterVersion: React.FC = () => { diff --git a/frontend/src/pages/modelRegistry/screens/RegisterModel/RegistrationFormFooter.tsx b/frontend/src/pages/modelRegistry/screens/RegisterModel/RegistrationFormFooter.tsx index 86ca64789f..2974121aa4 100644 --- a/frontend/src/pages/modelRegistry/screens/RegisterModel/RegistrationFormFooter.tsx +++ b/frontend/src/pages/modelRegistry/screens/RegisterModel/RegistrationFormFooter.tsx @@ -1,46 +1,40 @@ import React from 'react'; -import { - PageSection, - Stack, - StackItem, - Alert, - AlertActionCloseButton, - ActionGroup, - Button, -} from '@patternfly/react-core'; +import { PageSection, Stack, StackItem, ActionGroup, Button } from '@patternfly/react-core'; +import RegisterModelErrors from './RegisterModelErrors'; type RegistrationFormFooterProps = { submitLabel: string; submitError?: Error; - setSubmitError: (e?: Error) => void; isSubmitDisabled: boolean; isSubmitting: boolean; onSubmit: () => void; onCancel: () => void; + errorName?: string; + versionName?: string; + modelName?: string; }; const RegistrationFormFooter: React.FC = ({ submitLabel, submitError, - setSubmitError, isSubmitDisabled, isSubmitting, onSubmit, onCancel, + errorName, + versionName, + modelName, }) => ( {submitError && ( - - setSubmitError(undefined)} />} - > - {submitError.message} - - + )} diff --git a/frontend/src/pages/modelRegistry/screens/RegisterModel/const.ts b/frontend/src/pages/modelRegistry/screens/RegisterModel/const.ts index 360de6224e..a2ca8196b8 100644 --- a/frontend/src/pages/modelRegistry/screens/RegisterModel/const.ts +++ b/frontend/src/pages/modelRegistry/screens/RegisterModel/const.ts @@ -1 +1,12 @@ export const MR_CHARACTER_LIMIT = 128; + +export enum SubmitLabel { + REGISTER_MODEL = 'Register model', + REGISTER_VERSION = 'Register new version', +} + +export enum ErrorName { + REGISTERED_MODEL = 'registeredModel', + MODEL_VERSION = 'modelVersion', + MODEL_ARTIFACT = 'modelArtifact', +} diff --git a/frontend/src/pages/modelRegistry/screens/RegisterModel/useRegistrationCommonState.ts b/frontend/src/pages/modelRegistry/screens/RegisterModel/useRegistrationCommonState.ts deleted file mode 100644 index ae962edd3e..0000000000 --- a/frontend/src/pages/modelRegistry/screens/RegisterModel/useRegistrationCommonState.ts +++ /dev/null @@ -1,41 +0,0 @@ -import React from 'react'; -import { ModelRegistryContext } from '~/concepts/modelRegistry/context/ModelRegistryContext'; -import { ModelRegistryAPIState } from '~/concepts/modelRegistry/context/useModelRegistryAPIState'; -import { useAppSelector } from '~/redux/hooks'; - -type RegistrationCommonState = { - isSubmitting: boolean; - setIsSubmitting: React.Dispatch>; - submitError: Error | undefined; - setSubmitError: React.Dispatch>; - handleSubmit: (doSubmit: () => Promise) => void; - apiState: ModelRegistryAPIState; - author: string; -}; - -export const useRegistrationCommonState = (): RegistrationCommonState => { - const [isSubmitting, setIsSubmitting] = React.useState(false); - const [submitError, setSubmitError] = React.useState(undefined); - - const { apiState } = React.useContext(ModelRegistryContext); - const author = useAppSelector((state) => state.user || ''); - - const handleSubmit = (doSubmit: () => Promise) => { - setIsSubmitting(true); - setSubmitError(undefined); - doSubmit().catch((e: Error) => { - setIsSubmitting(false); - setSubmitError(e); - }); - }; - - return { - isSubmitting, - setIsSubmitting, - submitError, - setSubmitError, - handleSubmit, - apiState, - author, - }; -}; diff --git a/frontend/src/pages/modelRegistry/screens/RegisterModel/utils.ts b/frontend/src/pages/modelRegistry/screens/RegisterModel/utils.ts index 8e37b74fb7..2c1686cfe7 100644 --- a/frontend/src/pages/modelRegistry/screens/RegisterModel/utils.ts +++ b/frontend/src/pages/modelRegistry/screens/RegisterModel/utils.ts @@ -13,39 +13,53 @@ import { RegisterVersionFormData, RegistrationCommonFormData, } from './useRegisterModelData'; -import { MR_CHARACTER_LIMIT } from './const'; +import { ErrorName, MR_CHARACTER_LIMIT } from './const'; export type RegisterModelCreatedResources = RegisterVersionCreatedResources & { - registeredModel: RegisteredModel; + registeredModel?: RegisteredModel; }; export type RegisterVersionCreatedResources = { - modelVersion: ModelVersion; - modelArtifact: ModelArtifact; + modelVersion?: ModelVersion; + modelArtifact?: ModelArtifact; }; export const registerModel = async ( apiState: ModelRegistryAPIState, formData: RegisterModelFormData, author: string, -): Promise => { - const registeredModel = await apiState.api.createRegisteredModel( - {}, - { - name: formData.modelName, - description: formData.modelDescription, - customProperties: {}, - owner: author, - state: ModelState.LIVE, - }, - ); - const { modelVersion, modelArtifact } = await registerVersion( - apiState, - registeredModel, - formData, - author, - ); - return { registeredModel, modelVersion, modelArtifact }; +): Promise<{ + data: RegisterModelCreatedResources; + errors: { [key: string]: Error | undefined }; +}> => { + let registeredModel; + const error: { [key: string]: Error | undefined } = {}; + try { + registeredModel = await apiState.api.createRegisteredModel( + {}, + { + name: formData.modelName, + description: formData.modelDescription, + customProperties: {}, + owner: author, + state: ModelState.LIVE, + }, + ); + } catch (e) { + if (e instanceof Error) { + error[ErrorName.REGISTERED_MODEL] = e; + } + return { data: { registeredModel }, errors: error }; + } + const { + data: { modelVersion, modelArtifact }, + errors, + } = await registerVersion(apiState, registeredModel, formData, author); + + return { + data: { registeredModel, modelVersion, modelArtifact }, + errors, + }; }; export const registerVersion = async ( @@ -53,42 +67,59 @@ export const registerVersion = async ( registeredModel: RegisteredModel, formData: Omit, author: string, -): Promise => { - const modelVersion = await apiState.api.createModelVersionForRegisteredModel( - {}, - registeredModel.id, - { +): Promise<{ + data: RegisterVersionCreatedResources; + errors: { [key: string]: Error | undefined }; +}> => { + let modelVersion; + let modelArtifact; + const errors: { [key: string]: Error | undefined } = {}; + try { + modelVersion = await apiState.api.createModelVersionForRegisteredModel({}, registeredModel.id, { name: formData.versionName, description: formData.versionDescription, customProperties: {}, state: ModelState.LIVE, author, registeredModelId: registeredModel.id, - }, - ); - const modelArtifact = await apiState.api.createModelArtifactForModelVersion({}, modelVersion.id, { - name: `${formData.versionName}`, - description: formData.versionDescription, - customProperties: {}, - state: ModelArtifactState.LIVE, - author, - modelFormatName: formData.sourceModelFormat, - modelFormatVersion: formData.sourceModelFormatVersion, - // TODO fill in the name of the data connection we used to prefill if we used one - // TODO this should be done as part of https://issues.redhat.com/browse/RHOAIENG-9914 - // storageKey: 'TODO', - uri: - formData.modelLocationType === ModelLocationType.ObjectStorage - ? objectStorageFieldsToUri({ - endpoint: formData.modelLocationEndpoint, - bucket: formData.modelLocationBucket, - region: formData.modelLocationRegion, - path: formData.modelLocationPath, - }) || '' // We'll only hit this case if required fields are empty strings, so form validation should catch it. - : formData.modelLocationURI, - artifactType: 'model-artifact', - }); - return { modelVersion, modelArtifact }; + }); + } catch (e) { + if (e instanceof Error) { + errors[ErrorName.MODEL_VERSION] = e; + } + return { data: { modelVersion, modelArtifact }, errors }; + } + + try { + modelArtifact = await apiState.api.createModelArtifactForModelVersion({}, modelVersion.id, { + name: `${formData.versionName}`, + description: formData.versionDescription, + customProperties: {}, + state: ModelArtifactState.LIVE, + author, + modelFormatName: formData.sourceModelFormat, + modelFormatVersion: formData.sourceModelFormatVersion, + // TODO fill in the name of the data connection we used to prefill if we used one + // TODO this should be done as part of https://issues.redhat.com/browse/RHOAIENG-9914 + // storageKey: 'TODO', + uri: + formData.modelLocationType === ModelLocationType.ObjectStorage + ? objectStorageFieldsToUri({ + endpoint: formData.modelLocationEndpoint, + bucket: formData.modelLocationBucket, + region: formData.modelLocationRegion, + path: formData.modelLocationPath, + }) || '' // We'll only hit this case if required fields are empty strings, so form validation should catch it. + : formData.modelLocationURI, + artifactType: 'model-artifact', + }); + } catch (e) { + if (e instanceof Error) { + errors[ErrorName.MODEL_ARTIFACT] = e; + } + } + + return { data: { modelVersion, modelArtifact }, errors }; }; const isSubmitDisabledForCommonFields = (formData: RegistrationCommonFormData): boolean => {