diff --git a/frontend/src/pages/modelRegistry/screens/RegisterModel/RegisterModel.tsx b/frontend/src/pages/modelRegistry/screens/RegisterModel/RegisterModel.tsx index 7281e65af3..9708071a7c 100644 --- a/frontend/src/pages/modelRegistry/screens/RegisterModel/RegisterModel.tsx +++ b/frontend/src/pages/modelRegistry/screens/RegisterModel/RegisterModel.tsx @@ -19,10 +19,11 @@ 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'; @@ -30,20 +31,37 @@ import { MR_CHARACTER_LIMIT } 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 ( @@ -114,11 +132,13 @@ 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..cf9245d3c3 --- /dev/null +++ b/frontend/src/pages/modelRegistry/screens/RegisterModel/RegisterModelErrors.tsx @@ -0,0 +1,113 @@ +import React from 'react'; +import { Alert, AlertActionCloseButton, StackItem } from '@patternfly/react-core'; + +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 === 'Register model' && errorName === 'modelVersion') { + return ( + <> + {showAlert && ( + + setShowAlert(false)} />} + /> + + )} + + + {submitError.message} + + + + ); + } + + if (submitLabel === 'Register new version' && errorName === 'modelVersion') { + return ( + + + {submitError.message} + + + ); + } + + if (submitLabel === 'Register model' && errorName === 'modelArtifact') { + return ( + <> + {showAlert && ( + + setShowAlert(false)} />} + /> + + )} + + + {submitError.message} + + + + ); + } + + if (submitLabel === 'Register new version' && errorName === 'modelArtifact') { + 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 f6bbf50b1d..555f7144d3 100644 --- a/frontend/src/pages/modelRegistry/screens/RegisterModel/RegisterVersion.tsx +++ b/frontend/src/pages/modelRegistry/screens/RegisterModel/RegisterVersion.tsx @@ -16,10 +16,11 @@ 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'; @@ -27,14 +28,16 @@ import { usePrefillRegisterVersionFields } from './usePrefillRegisterVersionFiel 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 +51,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 @@ -128,12 +145,13 @@ 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 e4742b118a..3725a2de6b 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/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..f2301e5e48 100644 --- a/frontend/src/pages/modelRegistry/screens/RegisterModel/utils.ts +++ b/frontend/src/pages/modelRegistry/screens/RegisterModel/utils.ts @@ -16,36 +16,50 @@ import { import { 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.registeredModel = 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.modelVersion = 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.modelArtifact = e; + } + } + + return { data: { modelVersion, modelArtifact }, errors }; }; const isSubmitDisabledForCommonFields = (formData: RegistrationCommonFormData): boolean => {