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

Improve error message for model registration #3534

Merged
merged 1 commit into from
Dec 10, 2024

Conversation

ppadti
Copy link
Contributor

@ppadti ppadti commented Dec 2, 2024

Closes: RHOAIENG-13908

Description

This PR aims to improve error messages for model registration as mentioned in this doc

In Register model page:

  1. Model failed to create:
Screen.Recording.2024-12-03.at.8.24.30.PM.mov
  1. Model created but Version failed to create:
Screen.Recording.2024-12-03.at.8.56.51.PM.mov
  1. Model and version created but artifact failed to create:
Screen.Recording.2024-12-03.at.8.57.32.PM.mov

In Register new version page:

  1. Version failed to create:
Screen.Recording.2024-12-03.at.8.11.59.PM.mov
  1. Version created but artifact failed to create:
Screen.Recording.2024-12-03.at.8.12.38.PM.mov

How Has This Been Tested?

Its bit difficult to test as we fixed long name error here.

To test all these states mentioned in this doc locally

  • Try duplicate name while creating model/ version to get failed model/ version error
  • revert this commit to see version failure error
  • To get artifact failure error, modify this line to long name.

Test Impact

NA, Already covered by other PR and few cases can not be reproduced.

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 Dec 2, 2024

@yih-wang could you please take a look at the screen recordings?
Also, I have a question. when we have two alert to display, should both of them have action to close separately or only one with error alert (like below image). I am not sure what should be the expected behaviour here. Could you please provide your suggestion for this.

Screenshot 2024-12-02 at 9 39 14 PM

Copy link

codecov bot commented Dec 2, 2024

Codecov Report

Attention: Patch coverage is 52.94118% with 48 lines in your changes missing coverage. Please review.

Project coverage is 85.47%. Comparing base (70e0d4e) to head (caa0381).
Report is 11 commits behind head on main.

Files with missing lines Patch % Lines
...stry/screens/RegisterModel/RegisterModelErrors.tsx 3.84% 25 Missing ⚠️
...pages/modelRegistry/screens/RegisterModel/utils.ts 66.66% 9 Missing ⚠️
...elRegistry/screens/RegisterModel/RegisterModel.tsx 65.00% 7 Missing ⚠️
...Registry/screens/RegisterModel/RegisterVersion.tsx 66.66% 6 Missing ⚠️
...y/screens/RegisterModel/RegistrationFormFooter.tsx 75.00% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3534      +/-   ##
==========================================
- Coverage   85.56%   85.47%   -0.09%     
==========================================
  Files        1372     1372              
  Lines       31211    31277      +66     
  Branches     8741     8761      +20     
==========================================
+ Hits        26705    26735      +30     
- Misses       4506     4542      +36     
Files with missing lines Coverage Δ
...pages/modelRegistry/screens/RegisterModel/const.ts 100.00% <100.00%> (ø)
...y/screens/RegisterModel/RegistrationFormFooter.tsx 92.85% <75.00%> (+8.24%) ⬆️
...Registry/screens/RegisterModel/RegisterVersion.tsx 78.72% <66.66%> (-9.52%) ⬇️
...elRegistry/screens/RegisterModel/RegisterModel.tsx 80.55% <65.00%> (-19.45%) ⬇️
...pages/modelRegistry/screens/RegisterModel/utils.ts 80.43% <66.66%> (-16.24%) ⬇️
...stry/screens/RegisterModel/RegisterModelErrors.tsx 3.84% <3.84%> (ø)

... and 3 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 70e0d4e...caa0381. Read the comment docs.

@yih-wang
Copy link

yih-wang commented Dec 3, 2024

@ppadti the videos look good, thank you!
Regarding your question of whether to include the close buttons - I've consulted @angesDing about how to align them with our new validation guidelines - the short answer is the error alert shouldn't include the close button, and the success alert should include it.

@ppadti ppadti force-pushed the RHOAIENG-13908 branch 2 times, most recently from d278732 to 935e4b7 Compare December 3, 2024 16:22
@ppadti
Copy link
Contributor Author

ppadti commented Dec 4, 2024

@yih-wang I have updated the screen recordings, please take a look at them when you have a moment.

@yih-wang
Copy link

yih-wang commented Dec 4, 2024

Thanks for the updates @ppadti! We discussed yesterday the issue where the name in the alert will change after the name in the form. Haven't seen that behavior in the videos. Has it been addressed?

@ppadti
Copy link
Contributor Author

ppadti commented Dec 4, 2024

Thanks for the updates @ppadti! We discussed yesterday the issue where the name in the alert will change after the name in the form. Haven't seen that behavior in the videos. Has it been addressed?

yes, If you look at the first recording (model failed to create), you can see that behaviour there. Also attaching another recording for that here:

Screen.Recording.2024-12-04.at.4.55.08.PM.mov

@yih-wang
Copy link

yih-wang commented Dec 4, 2024

@ppadti Ah yes, I see it now! It works well so I missed it... All look good to me!

@ppadti ppadti requested a review from mturley December 6, 2024 15:20
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.

@ppadti this LGTM, just needs rebase and conflicts resolved.

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.

/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.

Thanks @ppadti

Copy link
Contributor

openshift-ci bot commented Dec 10, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: manaswinidas, 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-merge-bot openshift-merge-bot bot merged commit a0640b8 into opendatahub-io:main Dec 10, 2024
6 checks passed
ConorOM1 pushed a commit to ConorOM1/odh-dashboard that referenced this pull request Dec 12, 2024
ConorOM1 pushed a commit to ConorOM1/odh-dashboard that referenced this pull request Dec 12, 2024
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