Add register model page for model catalog #3781
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
ppadti
force-pushed
the
RHOAIENG-19060
branch
from
8e3c562 to
93b1bc9
Compare
|
@yih-wang could you please take a look at the screenshots attached? Also, let me know what do you think about the placeholder text of model registry selector. |
ppadti
force-pushed
the
RHOAIENG-19060
branch
from
93b1bc9 to
b3f8dde
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3781 +/- ##
==========================================
+ Coverage 84.66% 84.70% +0.03%
==========================================
Files 1517 1519 +2
Lines 35134 35231 +97
Branches 9836 9872 +36
==========================================
+ Hits 29747 29843 +96
- Misses 5387 5388 +1
... and 6 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
|
Thanks for the updates @ppadti! Look great! |
Thanks @yih-wang for taking a look at the screenshots. |
|
@ppadti I see. Yes I agree with you that this shouldn't belong to this issue then. cc @angesDing @simrandhaliw, we might need a consistency issue to address this change as a whole. |
ppadti
force-pushed
the
RHOAIENG-19060
branch
from
b3f8dde to
8240040
Compare
|
i'm getting errors when i try to submit the form, is there something i am missing? you mention we won't be able to see duplicate name error, but that's all i'm seeing: if i use also kind of curious why in your recording, the MR looks disabled. i'm able to change it. |
Sorry, I think the PR description was not clear enough to test this. Updated the description let me know if you need anything else. |
ppadti
force-pushed
the
RHOAIENG-19060
branch
from
8240040 to
56da92f
Compare
ppadti
force-pushed
the
RHOAIENG-19060
branch
from
56da92f to
b46964b
Compare
@yih-wang I'm a little surprised to see that guidance - that's how it used to work and it was changed fairly recently (this past November) to the current disabled-single-option behavior (PR by @DaoDaoNoCode , issue). I'll comment in the doc. |
| @@ -46,6 +47,8 @@ export const createModelVersionForRegisteredModel = | |||
| opts: K8sAPIOptions, | |||
| registeredModelId: string, | |||
| data: CreateModelVersionData, | |||
| customProperties: ModelRegistryCustomProperties, | |||
There was a problem hiding this comment.
Can we rename this to modelCustomProperties? It could be confusing since these are passed when creating the version but they are for the model. I wonder if there's a way we can avoid passing them, to be honest.
| export const bumpModelVersionTimestamp = async ( | ||
| api: ModelRegistryAPIs, | ||
| modelVersionId: string, | ||
| customProperties: ModelRegistryCustomProperties, |
There was a problem hiding this comment.
Ok. @ppadti please don't kill me. Looking more at this I think it's not good for us to have to pass in the customProperties in bumpModelVersionTimestamp and bumpRegisteredModelTimestamp. If they are accidentally passed as {} for some reason or passed the wrong thing (model/version properties mixed up), we'll have a problem.
If it's practical, I think it would be better to refactor both of these utils so that they don't take customProperties or ids, but they take the entire current copy of the object they're updating, and then use the customProperties from it. So we'd have bumpModelVersionTimestamp(api, modelVersion) which uses ...modelVersion.customProperties, bumpRegisteredModelTimestamp(api, registeredModel) which uses ...registeredModel.customProperties, and bumpBothTimestamps(api, modelVersion, registeredModel)`.
That would require making sure everywhere we call all these we have the full model/version object in scope, but I think we do for the most part. The one place I think we don't is inside createModelVersionForRegisteredModel in frontend/src/api/modelRegistry/custom.ts. We only call that in one place though (the registerVersion util in frontend/src/pages/modelRegistry/screens/RegisterModel/utils.ts), and we could either just pass in the whole registeredModel there or lift out the bump so it happens in registerVersion and not in the underlying api util, whichever you think is cleaner.
I think this way we'll save ourselves some potential future headaches and also make the code a bit readable, right now it's not immediately clear why we'd be passing around customProperties like this.
| const [registeredModelName, setRegisteredModelName] = React.useState<string>(''); | ||
| const [versionName, setVersionName] = React.useState<string>(''); | ||
| const [errorName, setErrorName] = React.useState<string | undefined>(undefined); |
There was a problem hiding this comment.
Hey - it's ok not to block this PR on this since I know this is duplicated from the RegisterModel and RegisterVersion pages, but I think this state is a little confusing. I know it exists because when there's an error submitting we need to show the names of the resources that were attempted to be created even if the form has since changed, but with these names it looks like duplicated state that we might try to use expecting it to be the form values. I would love to rename these to something like submittedRegisteredModelName and submittedVersionName, and maybe errorName (and the ErrorName type) should be registrationErrorType or something.
We probably also want to see what we can deduplicate between these 3 pages... that definitely belongs in a followup tech debt issue though.
| key, | ||
| encodeURIComponent(value).replace(/\./g, '%252E'), | ||
| ]), | ||
| Object.entries(params).map(([key, value]) => [key, encodeURIComponent(value)]), |
There was a problem hiding this comment.
Nice, this is way better, thanks.
| }; | ||
|
|
||
| const ModelRegistrySelector: React.FC<ModelRegistrySelectorProps> = ({ | ||
| modelRegistry, | ||
| onSelection, | ||
| primary, | ||
| isCatalogModel, |
There was a problem hiding this comment.
I think this prop can just be called isFullWidth, there may be other forms in the future other than catalog where we want to use the ModelRegistrySelector.
| @@ -43,6 +44,7 @@ const ModelVersionDetailsView: React.FC<ModelVersionDetailsViewProps> = ({ | |||
| const modelArtifact = modelArtifacts.items.length ? modelArtifacts.items[0] : null; | |||
| const { apiState } = React.useContext(ModelRegistryContext); | |||
| const storageFields = uriToStorageFields(modelArtifact?.uri || ''); | |||
| const [rm] = useRegisteredModelById(mv.registeredModelId); | |||
There was a problem hiding this comment.
We should pull in loaded, loadError and refresh from this fetch hook and make sure we handle them properly.
We can create composite variables for:
const loaded = modelArtifactsLoaded && registeredModelLoaded;
const loadError = modelArtifactsLoadError || registeredModelLoadError;
const refresh = () => {
refreshModelArtifacts();
refreshRegisteredModel();
};and then use those any place we currently use modelArtifactsLoaded, modelArtifactsLoadError and refreshModelArtifacts here.
| export const isRegisterCatalogModelSubmitDisabled = ( | ||
| formData: RegisterCatalogModelFormData, | ||
| ): boolean => | ||
| !formData.modelRegistry || | ||
| !formData.modelName || | ||
| isSubmitDisabledForCommonFields(formData) || | ||
| !isNameValid(formData.modelName); |
There was a problem hiding this comment.
Since this form state extends the register model form state, I think this can just be
isRegisterModelSubmitDisabled(formData, []) || !formData.modelRegistryand then we should add a comment explaining that the [] there is temporary and linking to the future issue for fixing the isModelNameExisting validation on the catalog registration page. Or if you prefer, have a registeredModels param here and pass in [] when calling isRegisterCatalogModelSubmitDisabled, and put the comment there.
| @@ -51,6 +52,7 @@ const DeployRegisteredModelModal: React.FC<DeployRegisteredModelModalProps> = ({ | |||
| const [dataConnections] = useDataConnections(selectedProject?.metadata.name); | |||
| const [connections] = useConnections(selectedProject?.metadata.name, true); | |||
| const error = platformError || projectError; | |||
| const [rm] = useRegisteredModelById(modelVersion.registeredModelId); | |||
There was a problem hiding this comment.
Same deal here with the loading/error states. We should combine the loading/error here with deployInfoLoaded and deployInfoError below.
| @@ -68,12 +70,21 @@ const DeployRegisteredModelModal: React.FC<DeployRegisteredModelModalProps> = ({ | |||
| modelRegistryApi.api, | |||
| modelVersion.id, | |||
| modelVersion.registeredModelId, | |||
| rm?.customProperties || {}, | |||
There was a problem hiding this comment.
Also (and this applies to everywhere we'll pass in the objects to these bump utils) since they're not optional, we shouldn't fall back to a blank value like this. We can just narrow the type within the scope of calling these utils so we know they're defined, like by wrapping the call in a if (rm). In this case, we can edit the early return a few lines above this (currently if (!modelVersion.registeredModelId)) so it returns early if rm isn't defined.
Closes: RHOAIENG-19060 and RHOAIENG-20245
Description
This PR aims to add Register model page for model catalog. Also, fixed https://issues.redhat.com/browse/RHOAIENG-20245
Screen.Recording.2025-02-20.at.2.01.01.PM.mov
NOTE:
How Has This Been Tested?
Go to the model catalog, click a model, and see that there is a Register button in the top right corner.
Verify that the Register button is disabled with a tooltip if there are no model registries the user can access.
With model registries available, click the Register button. See that you are taken to a register form that shows the correct model name, prefills the correct URI and if more than one model registries are present you will be able to select it. If you have only one model registry, then the model registry will auto selected and the field will be disabled.
Verify that you are able to select a model registry, fill out the form and register a model. Verify that the registered model and version contain all the expected details based on the mockup.
Test Impact
Updated unit and added new cypress tests for register model and model details page.
Request review criteria:
Self checklist (all need to be checked):
If you have UI changes:
After the PR is posted & before it merges:
main