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

Conversation

ppadti
Copy link
Contributor

@ppadti ppadti commented Oct 24, 2024

Closes: RHOAIENG-12844

Description

This PR aims to add character length limit to both model name and version name in Register model and Register version forms. Also to use the given model name for the RegisteredModel and the given version name for both the ModelVersion and the ModelArtifact.

Screenshot 2024-10-25 at 3 49 09 PM Screenshot 2024-10-25 at 3 48 31 PM Screenshot 2024-10-25 at 3 53 23 PM

How Has This Been Tested?

  1. In model registry, click on Register model.
  2. check the helper text "Cannot exceed 128 characters" is under model name and version name field.
  3. Try to type in a long string, length greater than 128, you will see an error and it will disable the Register model button.
  4. Same goes for Register version form, try to Register a version with lang name and you will see the error in helper text and Register version button will be disabled.
  5. For Register version, when we select a model we will have multiple helper text under version name.
    Ref: Doc

Test Impact

Added cypress tests.

Request review criteria:

Self checklist (all need to be checked):

  • The developer has manually tested the changes and verified that the changes work
  • Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
  • The developer has added tests or explained why testing cannot be added (unit or cypress tests for related changes)

If you have UI changes:

  • Included any necessary screenshots or gifs if it was a UI change.
  • Included tags to the UX team if it was a UI/UX change.

After the PR is posted & before it merges:

  • The developer has tested their solution on a cluster by using the image produced by the PR to main

@ppadti
Copy link
Contributor Author

ppadti commented Oct 24, 2024

@yih-wang could you please take a look at the above screenshots?

Copy link

codecov bot commented Oct 24, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 85.91%. Comparing base (96e06a2) to head (d17b4da).
Report is 59 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3377      +/-   ##
==========================================
+ Coverage   85.01%   85.91%   +0.90%     
==========================================
  Files        1327     1339      +12     
  Lines       29770    30296     +526     
  Branches     8149     8365     +216     
==========================================
+ Hits        25308    26029     +721     
+ Misses       4462     4267     -195     
Files with missing lines Coverage Δ
...elRegistry/screens/RegisterModel/RegisterModel.tsx 100.00% <100.00%> (ø)
...s/RegisterModel/RegistrationCommonFormSections.tsx 95.74% <100.00%> (+0.39%) ⬆️
...pages/modelRegistry/screens/RegisterModel/const.ts 100.00% <100.00%> (ø)
...pages/modelRegistry/screens/RegisterModel/utils.ts 96.66% <100.00%> (+0.51%) ⬆️

... and 167 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 96e06a2...d17b4da. Read the comment docs.

@yih-wang
Copy link

yih-wang commented Oct 24, 2024

@ppadti thanks for the updates!
I know the new validation guidelines suggest having the char limit displayed as help text under the field. However, looking at how it looks under the version field, I feel it's not conveying the info effectively:

  • The current version name is not something that can be validated, so we shouldn't use the multi-condition validation pattern here.
  • However, having both help texts displayed as two lines of plain text also feels strange.
  • Also, the char limit isn't too important here IMO because a user will rarely touch the 128 char limit. It's basically a mechanism we add to avoid registration failure.
    So I'm wondering are we supposed to always have the char limit displayed as help text, or should it be optional? @angesDing @simrandhaliw

I personally prefer not to display the char limit as help text here (for both model name and version name), but only show the error message when user exceeds the limitation.

@manaswinidas
Copy link
Contributor

manaswinidas commented Oct 24, 2024

@yih-wang I see two help texts in the Create Project modal and both are plain text too.

Screenshot 2024-10-24 at 11 47 37 PM

@yih-wang
Copy link

yih-wang commented Oct 25, 2024

@manaswinidas the two help text in project creation are both conditions to be validated. So they both have that 'dynamic validation status' at the very beginning to show whether the condition is satisfied. However, in our case, current version name is xxx is not a validation condition. We can never tell that when it's satisfied or not. It's just a reference for users to create the next version name. So it doesn't make sense to have it in that 'condition to be validated' format.

I chat with @angesDing and @simrandhaliw offline and they both agree that the char limit is not essential to display as help text in our case. We are likely to make improvements to the guidelines to make sure it's clear that the help text for char limit is optional.

So, @ppadti for both model name and version name, let's remove the char limit as the help text, and only have it as error message when the user input exceeds the char limit. Basically just copy what we have for MR labels. For version name, there could be both help text and error message (see the image below). I'm not sure which one would be on top by default, but I'm fine with both ways. We should just follow PF's default and avoid any customization.
image

@ppadti
Copy link
Contributor Author

ppadti commented Oct 25, 2024

@manaswinidas the two help text in project creation are both conditions to be validated. So they both have that 'dynamic validation status' at the very beginning to show whether the condition is satisfied. However, in our case, current version name is xxx is not a validation condition. We can never tell that when it's satisfied or not. It's just a reference for users to create the next version name. So it doesn't make sense to have it in that 'condition to be validated' format.

I chat with @angesDing and @simrandhaliw offline and they both agree that the char limit is not essential to display as help text in our case. We are likely to make improvements to the guidelines to make sure it's clear that the help text for char limit is optional.

So, @ppadti for both model name and version name, let's remove the char limit as the help text, and only have it as error message when the user input exceeds the char limit. Basically just copy what we have for MR labels. For version name, there could be both help text and error message (see the image below). I'm not sure which one would be on top by default, but I'm fine with both ways. We should just follow PF's default and avoid any customization. image

@yih-wang just wanted a small clarification, here in label we show error icon before error text Label text can't exceed 63 characters
Screenshot 2024-10-25 at 2 32 02 PM
that is missing in img that you shared above. So you want me to keep the error icon before error text under model name and version name field like this:
Screenshot 2024-10-25 at 2 34 18 PM

@ppadti
Copy link
Contributor Author

ppadti commented Oct 25, 2024

oh, in Register version form with icon it looks odd,
Screenshot 2024-10-25 at 2 55 50 PM

so I will go ahead with the error text without error icon as you mentioned above @yih-wang sorry for the confusion here...Thanks!!

@yih-wang
Copy link

@ppadti Seems the new guidelines don't include the error icon in the error messages. So I think the error icon we have for label error will also be removed when we apply the new rules across the console.

@ppadti
Copy link
Contributor Author

ppadti commented Oct 25, 2024

@yih-wang I have updated the screenshots, take a look them when you have time.

Copy link
Contributor

@manaswinidas manaswinidas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

small nit, otherwise lgtm

Copy link
Contributor

@mturley mturley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just one nitpick, otherwise looks good. If you disagree with this let me know, we could leave it as-is.

@yih-wang
Copy link

LGTM, thanks for the updates, @ppadti !

Copy link
Contributor

@mturley mturley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks @ppadti !

Copy link
Contributor

openshift-ci bot commented Nov 4, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gitdallas, mturley

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved label Nov 4, 2024
@mturley mturley dismissed manaswinidas’s stale review November 4, 2024 18:31

Comments were addressed

@openshift-merge-bot openshift-merge-bot bot merged commit 0c5a5d9 into opendatahub-io:main Nov 4, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants