-
Notifications
You must be signed in to change notification settings - Fork 163
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
Model Details and Model Version Details: add editable properties section, set up API patches for description, labels and properties #2770
Conversation
d34a14e
to
e68c463
Compare
9488bd0
to
80fead9
Compare
frontend/src/pages/modelRegistry/screens/ModelPropertiesTableRow.tsx
Outdated
Show resolved
Hide resolved
frontend/src/pages/modelRegistry/screens/ModelPropertiesDescriptionListGroup.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. @mturley I think the way you're handling error cases is fine for now. I also like the enhancement you suggested in Slack. I assume that'll be in a future PR once UXD has a chance to review your proposal. |
@manaswinidas I'm working on the overflow in the Properties section, but for the Description textarea I do get a draggable control in the bottom-right on Chrome. Although it should probably default to that 24-line height, I can change that. |
80fead9
to
71d1ff5
Compare
@manaswinidas I addressed your feedback: the description textarea will now default to 24 lines and be resizable only vertically (I was missing the Really good catch there, thank you! |
/hold |
71d1ff5
to
d04fcab
Compare
d04fcab
to
b166ee9
Compare
Hi @mturley, thanks for all the work, they look really great! I got two comments regarding the current screens, but feel free to close this pr and address the comments in future enhancements.
|
/unhold there are ux comments so we might need to update the pr but we are not waiting on any other PR |
Hi @yih-wang, Thanks for the feedback! Let's make sure to follow up with a UX issue / discussion to improve this after this PR.
|
f262de5
to
2ef4e63
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, so far everything seems great, I think there was still a discussion open right? But so far it looks great, just a small nit about refreshing data, we should use the right refreshing state for each component
frontend/src/pages/modelRegistry/screens/ModelVersionDetails/ModelVersionDetailsView.tsx
Outdated
Show resolved
Hide resolved
2ef4e63
to
0cd9400
Compare
…ion, set up API patches for description, labels and properties Signed-off-by: Mike Turley <[email protected]>
0cd9400
to
e136ed3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: gitdallas, lucferbux The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
FYI, we have these followup issues based on the parts of the above feedback and Slack discussions that were not addressed in this PR:
|
Closes RHOAIENG-2235 and RHOAIENG-2238
Followup to #2760
Description
ModelPropertiesDescriptionListGroup
component which provides view/add/edit/delete controlsRegisteredModel
, they work withModelRegistryCustomProperties
objects and genericT extends ModelRegistryBase
objects, so it should be easy to reuse these for the ModelVersion detail page.getPatchBody
utility in order to make sure the request isn't clearing other fields due to the way patches are performed in the API. We must supply all editable fields when patching an object even if we aren't modifying them all, but we can't supply some fields like the create/edit timestamps or we'll get 400 errors.Demo screencaps
Editing the Description of a RegisteredModel
Screen.Recording.2024-05-02.at.3.22.26.PM.mov
Editing the Labels of a RegisteredModel
Screen.Recording.2024-05-02.at.4.00.16.PM.mov
Editing the Properties of a RegisteredModel
Screen.Recording.2024-05-02.at.4.02.56.PM.mov
Input validation screencaps
When adding a new label, in addition to the existing validation that the label text must be no longer than 63 characters, we also now validate that a newly added label is unique (does not match any existing key in
customProperties
, whether that is a label or the key of a property in the table below). If the label is not unique, the modal submission is blocked and an error message appears:When editing an existing label, the same validation is applied. However, with the current beta version of PF editable labels, there is no way to express input validation errors while editing a label's text. As a workaround for now, if the user attempts to use an invalid label (either longer than 63 characters or matching an existing label/property), when they click away the change is not applied and the label remains as it was before they clicked it:
Screen.Recording.2024-05-02.at.3.31.09.PM.mov
When adding or editing a property, the same validation is applied to the property key. Errors in this case appear inline below the key input field:
cc @yih-wang @vconzola
How Has This Been Tested?
Go to
Model Registry
in the nav, click a registered model and click the Details tab. Play around with the edit controls on the Description, Labels and Properties sections. (see the screen recordings above)Test Impact
Unit tests have been added for the new utility functions
mergeUpdatedLabels
,getProperties
,mergeUpdatedProperty
andgetPatchBody
. Cypress tests for all these views are to come soon.Request review criteria:
Self checklist (all need to be checked):
If you have UI changes:
After the PR is posted & before it merges:
main