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

Bug fixes in model registry #3284

Merged
merged 4 commits into from
Oct 11, 2024

Conversation

manaswinidas
Copy link
Contributor

@manaswinidas manaswinidas commented Oct 1, 2024

Closes: RHOAIENG-13119
Closes: RHOAIENG-13280
Closes: RHOAIENG-13061
Closes: RHOAIENG-13053
Closes: RHOAIENG-13046

Description

"Settings" breadcrumb item removed from Manage permissions page
Screenshot 2024-10-01 at 11 04 12 PM

"Settings" breadcrumb item removed from the internal Serving Runtimes admin page.
Screenshot 2024-10-01 at 11 03 57 PM

"Source model format version" field added to Model version details page
Screenshot 2024-10-07 at 5 48 17 PM

"Select a model registry" changed to "All model registries" in dropdown
Screenshot 2024-10-01 at 10 18 51 PM

PlusIcon changed to SearchIcon in Archived models empty state page
Screenshot 2024-10-01 at 10 17 14 PM

PlusIcon changed to SearchIcon in Archived versions empty state page
Screenshot 2024-10-07 at 5 48 43 PM

Register Version: 'clear input value' button working fixed on Model Name field
https://github.com/user-attachments/assets/ae439469-6701-4e5e-8f49-1e2076ef57db

How Has This Been Tested?

Tested locally via UI.

  1. Check for the internal pages in "Manage Permissions" page in Model registry settings and Serving runtimes admin page > "Settings" removed from the breadcrumb items
  2. "Source model format version" field added to Model version details page
  3. Check for the above changes in the pages mentioned

Test Impact

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

Copy link

codecov bot commented Oct 1, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.88%. Comparing base (c4e2940) to head (8e81504).
Report is 17 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3284      +/-   ##
==========================================
+ Coverage   84.74%   84.88%   +0.13%     
==========================================
  Files        1308     1309       +1     
  Lines       29242    29327      +85     
  Branches     7936     8002      +66     
==========================================
+ Hits        24781    24893     +112     
+ Misses       4461     4434      -27     
Files with missing lines Coverage Δ
...es/modelRegistry/screens/ModelRegistrySelector.tsx 61.01% <ø> (ø)
...ns/ModelVersionDetails/ModelVersionDetailsView.tsx 90.32% <100.00%> (+0.66%) ⬆️
...elVersionsArchive/ModelVersionsArchiveListView.tsx 73.91% <ø> (ø)
.../screens/RegisterModel/RegisteredModelSelector.tsx 100.00% <100.00%> (ø)
...dModelsArchive/RegisteredModelsArchiveListView.tsx 73.91% <ø> (ø)
...delRegistrySettings/ModelRegistriesPermissions.tsx 96.77% <ø> (ø)
...ervingRuntimes/CustomServingRuntimeAddTemplate.tsx 92.77% <ø> (ø)

... and 87 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 c4e2940...8e81504. Read the comment docs.

@YuliaKrimerman
Copy link
Contributor

/lgtm Verified locally, works as needed

@yih-wang
Copy link

yih-wang commented Oct 8, 2024

Hey @manaswinidas, thanks for making all the updates! They look great!
Some layout changes in this mockup haven't been addressed by this PR. I know that doesn't belong to the original scope of RHOAIENG-13119. Will the layout changes be addressed separately?

@manaswinidas
Copy link
Contributor Author

@yih-wang The layout issues will be addressed as a part of https://issues.redhat.com/browse/RHOAIENG-13062 - added this in the description

Comment on lines 156 to 158
<Title style={{ marginTop: '1em' }} headingLevel={TextVariants.h3}>
Source model format
</Title>
Copy link
Contributor

Choose a reason for hiding this comment

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

@manaswinidas this is a real nitpick, but can you follow the same pattern above with the "Model location" title where we close the </DescriptionList> before the Title and then open a new <DescriptionList isFillColumns>? And then can we add some additional margin between the two source model format fields and the Author field, since it's not part of Source model format? Maybe putting those last few fields in their own DescriptionList would also do that. Right now you can't tell where the "Source model format" section ends.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added DescriptionList with margin.

Introducing aDescriptionList between version and author without margin:
Screenshot 2024-10-11 at 1 29 04 PM

Introducing aDescriptionList between version and author with 2em margin(current):
Screenshot 2024-10-11 at 1 37 11 PM

WithoutDescriptionList between version and author:
Screenshot 2024-10-11 at 1 25 29 PM

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 @manaswinidas ! Nice to knock all these out

Copy link
Contributor

openshift-ci bot commented Oct 11, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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

@mturley mturley dismissed ppadti’s stale review October 11, 2024 14:58

Comments were addressed

@openshift-merge-bot openshift-merge-bot bot merged commit bd03c0b into opendatahub-io:main Oct 11, 2024
8 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