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

Archive and restore registered models #2836

Conversation

manaswinidas
Copy link
Contributor

@manaswinidas manaswinidas commented May 22, 2024

Closes: https://issues.redhat.com/browse/RHOAIENG-6637
Closes: https://issues.redhat.com/browse/RHOAIENG-6638

Description

Added table and modals for archiving and restoring registered models.

@yih-wang here's a screen recording for you to take a look at.

Screen.Recording.2024-05-30.at.9.mp4

How Has This Been Tested?

  1. Go to Registered models table and click on any table item kebab menu and click on Archive model, a modal appears to ask for confirmation and once that is done, a toast notification appears to confirm the same.
  2. You can now click on View archived models on the Registered models table header kebab and view archived models table.
  3. You can also archive model after clicking in any model hyperlink and from the Actions header action, click on Archive model to see the modal and archive it.
  4. To restore model, go to Archived models table, click on any table item kebab menu and click on Restore model, a modal appears to confirm restoration and a toast notification appears.
  5. You can also restore a model from the Model Details page, click on any model hyperlink on the Archived models table and click on Restore on the header, this will lead to a modal confirmation as in Step 4.

Test Impact

Added cypress and unit tests, updated mocks

Request review criteria:

Self checklist (all need to be checked):

  • The developer has manually tested the changes and verified that the changes work
  • Commits have been squashed into descriptive, self-contained units of work (e.g. 'WIP' and 'Implements feedback' style messages have been removed)
  • 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 (find relevant UX in the SMEs section).

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

@openshift-ci openshift-ci bot requested review from dpanshug and lucferbux May 22, 2024 14:03
@manaswinidas
Copy link
Contributor Author

/hold please merge #2815 first

@openshift-ci openshift-ci bot added the do-not-merge/hold This PR is hold for some reason label May 22, 2024
@openshift-merge-robot openshift-merge-robot added the needs-rebase PR needs to be rebased label May 29, 2024
@openshift-merge-robot openshift-merge-robot removed the needs-rebase PR needs to be rebased label May 29, 2024
@manaswinidas manaswinidas removed the do-not-merge/hold This PR is hold for some reason label May 29, 2024
Copy link

codecov bot commented May 29, 2024

Codecov Report

Attention: Patch coverage is 83.82353% with 33 lines in your changes are missing coverage. Please review.

Project coverage is 77.54%. Comparing base (3bee930) to head (5ee88bd).
Report is 2 commits behind head on main.

Current head 5ee88bd differs from pull request most recent head 0408177

Please upload reports for the commit 0408177 to get more accurate results.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2836      +/-   ##
==========================================
+ Coverage   77.44%   77.54%   +0.10%     
==========================================
  Files        1110     1117       +7     
  Lines       23521    23689     +168     
  Branches     5937     5963      +26     
==========================================
+ Hits        18215    18369     +154     
- Misses       5306     5320      +14     
Files Coverage Δ
frontend/src/concepts/modelRegistry/types.ts 100.00% <100.00%> (ø)
...nd/src/pages/modelRegistry/ModelRegistryRoutes.tsx 66.66% <ø> (ø)
.../src/pages/modelRegistry/screens/ModelRegistry.tsx 75.00% <100.00%> (+12.50%) ⬆️
...ersionDetails/ModelVersionDetailsHeaderActions.tsx 86.95% <ø> (ø)
...try/screens/ModelVersions/ModelVersionListView.tsx 76.31% <ø> (ø)
...elRegistry/screens/ModelVersions/ModelVersions.tsx 94.11% <100.00%> (+0.36%) ⬆️
...ry/screens/ModelVersions/ModelVersionsTableRow.tsx 96.87% <ø> (ø)
...odelVersionsArchive/ModelVersionArchiveDetails.tsx 100.00% <ø> (ø)
...s/RegisteredModels/RegisteredModelsTableColumns.ts 33.33% <ø> (ø)
...tend/src/pages/modelRegistry/screens/routeUtils.ts 94.73% <100.00%> (+3.07%) ⬆️
... and 13 more

... and 5 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 3bee930...0408177. Read the comment docs.

@manaswinidas manaswinidas force-pushed the archive-registered-models branch 3 times, most recently from d4271be to 8728ebd Compare May 30, 2024 12:13
@yih-wang
Copy link

@manaswinidas Thanks for all the work! They look really great! Several comments -

  • The action dropdown on the model details page should be right-aligned with the Action button to avoid exceeding the page border. Now in the screencap, after the action button is clicked, the button jumps to the left to fit the screen border.
  • After the model is archived (no matter from the models list, or the model details page), the screen will always navigate to the refreshed model list.
  • The header of the view archived models' page should be 'Archived models of [registryName]'
  • This may not be an issue from this pr, but the toolbar of the table is wrapped into two lines in the screencap. We should find a better way to handle the small screen layout, e.g. change the pagination from standard to compact for small screens. How do the other tables across the console handle this? Should this be a global RFE issue rather than only be fixed here?

@manaswinidas
Copy link
Contributor Author

manaswinidas commented May 30, 2024

@yih-wang Thanks for pointing these out.

The 2nd and 3rd points are taken care of. The screencap is replaced in the PR description.

1st point) Is there a way to achieve this? Are you talking about the position prop? I tried doing that but the button still jumps for me.

4th point) I saw this on the projects page, so I guess there might be a need for the global RFE(not sure about this)
Screenshot 2024-05-30 at 9 20 25 PM

@manaswinidas
Copy link
Contributor Author

This button jumping issue is now fixed.

Screen.Recording.2024-05-30.at.11.mp4

@openshift-merge-robot openshift-merge-robot added the needs-rebase PR needs to be rebased label May 30, 2024
@yih-wang
Copy link

@manaswinidas The updates look good! One more comment to the latest video you attached -

After the model has been archived and the page returns to the models list, if there are no models anymore, the empty state shows a 'find nothing' state. It should show the empty state for no registered model instead.

@openshift-merge-robot openshift-merge-robot removed the needs-rebase PR needs to be rebased label May 31, 2024
@manaswinidas
Copy link
Contributor Author

@lucferbux Are we removing internal-registry? The change that Haley pointed out seems unrelated to my changes. She is referring to this screencap.

@lucferbux
Copy link
Contributor

@lucferbux Are we removing internal-registry? The change that Haley pointed out seems unrelated to my changes. She is referring to this screencap.

Yes @yih-wang is right, you are not doing a search, you are showing an empty table of archive models, it's not related to internal registry but rather to the archived modeles we have there, so it should display the screen haley is pointing out

@manaswinidas
Copy link
Contributor Author

manaswinidas commented May 31, 2024

@lucferbux The search page isn't the archived models page but the registered models list page(the one we see when we first click on Model Registry from the left nav). I'm looking into it. I don't see it on main, so there indeed is an issue there.
Screenshot 2024-05-31 at 7 04 02 PM

@manaswinidas
Copy link
Contributor Author

I was getting this because all models in internal-registry do not have a state, as it's optional. I kept thinking there's something wrong with the logic

Screenshot 2024-05-31 at 10 11 56 PM

@manaswinidas manaswinidas force-pushed the archive-registered-models branch 4 times, most recently from 893bea4 to 41e5a7e Compare May 31, 2024 18:45
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.

Everything LGTM except one stray console.log!

@manaswinidas manaswinidas force-pushed the archive-registered-models branch 2 times, most recently from 5ee88bd to 23cdd00 Compare June 3, 2024 11:07
Copy link
Contributor

@dpanshug dpanshug left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci openshift-ci bot added the lgtm label Jun 3, 2024
@mturley
Copy link
Contributor

mturley commented Jun 3, 2024

/approve

Copy link
Contributor

openshift-ci bot commented Jun 3, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dpanshug, 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 Jun 3, 2024
@openshift-merge-bot openshift-merge-bot bot merged commit 20d32e9 into opendatahub-io:main Jun 3, 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.

6 participants