Update archived models and archived versions components #3206

ppadti · 2024-09-13T08:21:08Z

Closes: RHOAIENG-11509

Description

This PR aims to update

Archived models to read-only by removing the toolbar actions, toolbar kebab and kebab menu from the table.

Archived model details page to read-only by removing all the actions.

Archived version details page to read-only by removing all the actions.

implementation for versions of archived model.

Empty state for version with archived model

How Has This Been Tested?

Archived models details page:
- Archive a model
- Go to the view archived models
- see the archived model versions tab and details tab
Archived version details page:
- Archive a version
- Go to view archived version in model details
- Go to the details page of the archived version
Version details page of archived model:
- Archive a model with versions
- Go to a version inside it

Test Impact

Added cypress test.

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 · 2024-09-13T09:34:00Z

frontend/src/pages/modelRegistry/screens/ModelVersions/ModelVersionListView.tsx

+    if (isArchiveModel) {
+      return (
+        <EmptyModelRegistryState
+          testid="empty-archive-model-versions"
+          title="No versions"
+          description={`${rm?.name} has no registered versions.`}
+        />
+      );
+    }


@mturley @yih-wang I am not sure, should we have Empty state for versions of Archived models? for now, I have just re-used the one - empty state for versions by removing the primary action. Let me know your thoughts on this?

Had discussion with @yih-wang and I will update the Empty state for version with archived model.

codecov · 2024-09-13T10:25:02Z

Codecov Report

Attention: Patch coverage is 87.69231% with 16 lines in your changes missing coverage. Please review.

Project coverage is 85.30%. Comparing base (d0b5ce9) to head (1690e07).

Files with missing lines	Patch %	Lines
.../modelRegistry/screens/ModelPropertiesTableRow.tsx	0.00%	6 Missing ⚠️
...try/screens/ModelVersions/ModelVersionListView.tsx	89.47%	4 Missing ⚠️
...ns/ModelVersionDetails/ModelVersionDetailsView.tsx	71.42%	2 Missing ⚠️
...egistry/screens/ModelVersions/ModelDetailsView.tsx	81.81%	2 Missing ⚠️
...ry/screens/ModelVersions/ModelVersionsTableRow.tsx	90.00%	2 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #3206   +/-   ##
=======================================
  Coverage   85.30%   85.30%           
=======================================
  Files        1267     1269    +2     
  Lines       27830    27913   +83     
  Branches     7406     7440   +34     
=======================================
+ Hits        23739    23811   +72     
- Misses       4091     4102   +11

Files with missing lines	Coverage Δ
...rc/components/EditableTextDescriptionListGroup.tsx	`64.28% <100.00%> (+1.32%)`	⬆️
...nd/src/pages/modelRegistry/ModelRegistryRoutes.tsx	`100.00% <ø> (ø)`
...ry/screens/ModelPropertiesDescriptionListGroup.tsx	`80.00% <100.00%> (+1.42%)`	⬆️
...ns/ModelVersionDetails/ModelVersionDetailsTabs.tsx	`100.00% <100.00%> (ø)`
...istry/screens/ModelVersions/ModelVersionsTable.tsx	`100.00% <100.00%> (ø)`
...gistry/screens/ModelVersions/ModelVersionsTabs.tsx	`100.00% <100.00%> (ø)`
...odelVersionsArchive/ArchiveModelVersionDetails.tsx	`100.00% <100.00%> (ø)`
...nsArchive/ArchiveModelVersionDetailsBreadcrumb.tsx	`100.00% <100.00%> (ø)`
...odelVersionsArchive/ModelVersionArchiveDetails.tsx	`100.00% <ø> (ø)`
...redModelsArchive/RegisteredModelArchiveDetails.tsx	`100.00% <ø> (+3.84%)`	⬆️
... and 6 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 d0b5ce9...1690e07. Read the comment docs.

mturley

@ppadti there's one fix missing here - it is possible to see the wrong view for a model or version by going to the URL as if it was archived when it's not or vice versa. We should have the pages check whether the model and/or version is actually archived or not archived, and redirect to the correct place if you're at the wrong URL. This is a corner case, but it can come up if you have an old tab open on a model or version before its archived state changes or if you bookmark one.

Example:

If I have a model that is not archived and go its details page, that URL is:
/modelRegistry/modelregistry-sample/registeredModels/13/versions
and if I edit that to:
/modelRegistry/modelregistry-sample/registeredModels/archive/13/versions
I incorrectly see the archived view even though the model is not archived. The page should redirect me back to that first URL.
If I then archive that model and navigate to it, then change its URL to remove /archive/, we get the opposite problem. Either way it should redirect me to the URL corresponding to the real model state.

The same is true of the archived / non-archived version pages. They should detect which page you should be on based on whether model or version are archived and redirect you there if you aren't already there.

A few other comments below as well, but the rest LGTM! Thanks for the hard work on this.

frontend/src/pages/modelRegistry/screens/ModelVersionDetails/ModelVersionDetailsView.tsx

frontend/src/pages/modelRegistry/screens/ModelVersions/ModelDetailsView.tsx

mturley · 2024-09-18T21:35:07Z

frontend/src/pages/modelRegistry/screens/ModelVersions/ModelVersionListView.tsx

+          title={`All the versions have been archived along with the model by ${
+            rm?.owner ?? '--'
+          } on  ${


@yih-wang @vconzola I don't think we can actually display who the models/versions were archived by. This code will display that they have been archived by whoever is the model owner, which may not be true if multiple users have access to the registry and someone else archived it. The API currently has no way to record who made a change unless we change the owner when someone archives something, which doesn't seem right.

I think maybe we need to instead not name a user here and say "All the versions have been archived along with the model on [date]". What do you think?

In the future we could ask the MR team to add a lastModifiedBy field to the API for us to track who made changes.

That makes sense to me since we don't record any model activities for now.

I think maybe we need to instead not name a user here and say "All the versions have been archived along with the model on [date]". What do you think?

Sounds good!

ppadti · 2024-09-19T08:13:03Z

@ppadti there's one fix missing here - it is possible to see the wrong view for a model or version by going to the URL as if it was archived when it's not or vice versa. We should have the pages check whether the model and/or version is actually archived or not archived, and redirect to the correct place if you're at the wrong URL. This is a corner case, but it can come up if you have an old tab open on a model or version before its archived state changes or if you bookmark one.

Example:

If I have a model that is not archived and go its details page, that URL is: /modelRegistry/modelregistry-sample/registeredModels/13/versions and if I edit that to: /modelRegistry/modelregistry-sample/registeredModels/archive/13/versions I incorrectly see the archived view even though the model is not archived. The page should redirect me back to that first URL. If I then archive that model and navigate to it, then change its URL to remove /archive/, we get the opposite problem. Either way it should redirect me to the URL corresponding to the real model state.

The same is true of the archived / non-archived version pages. They should detect which page you should be on based on whether model or version are archived and redirect you there if you aren't already there.

ahh... Some how I missed this, I thought only the archived models logic is wrong for versions under that. I will update this. Thanks!!

openshift-ci · 2024-09-19T13:39:21Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from mturley. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

frontend/OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

mturley

LGTM except the one question here and the test failures @ppadti

mturley · 2024-09-20T15:48:43Z

frontend/src/pages/modelRegistry/screens/ModelVersionDetails/ModelVersionDetails.tsx

+  useEffect(() => {
+    if (rm?.state === ModelState.ARCHIVED && mv?.id) {
+      navigate(
+        archiveModelVersionDetailsUrl(
+          mv.id,
+          mv.registeredModelId,
+          preferredModelRegistry?.metadata.name,
+        ),
+      );
+    } else if (mv?.state === ModelState.ARCHIVED) {
+      navigate(
+        modelVersionArchiveDetailsUrl(
+          mv.id,
+          mv.registeredModelId,
+          preferredModelRegistry?.metadata.name,
+        ),
+      );
+    }
+  });


Should all these new useEffects have dependency arrays? I'm not sure whether it matters since the if conditions are the same perf impact as comparing dependencies across renders, but it may still be better to not run the effects every render in case we modify them later.

openshift-ci bot requested review from alexcreasy and lucferbux September 13, 2024 08:21

ppadti force-pushed the RHOAIENG-11509 branch 2 times, most recently from 467d440 to 416e81d Compare September 13, 2024 09:27

ppadti commented Sep 13, 2024

View reviewed changes

ppadti force-pushed the RHOAIENG-11509 branch from 416e81d to 0c991e1 Compare September 13, 2024 09:44

ppadti force-pushed the RHOAIENG-11509 branch from 0c991e1 to 1690e07 Compare September 13, 2024 11:55

ppadti changed the title ~~Upadte archived models and archived versions components~~ Update archived models and archived versions components Sep 16, 2024

mturley requested changes Sep 18, 2024

View reviewed changes

openshift-ci bot assigned mturley Sep 18, 2024

Upadte archived models and archived versions components

1d0d692

ppadti force-pushed the RHOAIENG-11509 branch from 1690e07 to 1d0d692 Compare September 19, 2024 13:39

mturley reviewed Sep 20, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update archived models and archived versions components #3206

Update archived models and archived versions components #3206

ppadti commented Sep 13, 2024 •

edited

Loading

ppadti Sep 13, 2024

ppadti Sep 13, 2024

codecov bot commented Sep 13, 2024 •

edited

Loading

mturley left a comment

mturley Sep 18, 2024

yih-wang Sep 19, 2024 •

edited

Loading

ppadti commented Sep 19, 2024

openshift-ci bot commented Sep 19, 2024

mturley left a comment

mturley Sep 20, 2024

Update archived models and archived versions components #3206

Are you sure you want to change the base?

Update archived models and archived versions components #3206

Conversation

ppadti commented Sep 13, 2024 • edited Loading

Description

How Has This Been Tested?

Test Impact

Request review criteria:

ppadti Sep 13, 2024

Choose a reason for hiding this comment

ppadti Sep 13, 2024

Choose a reason for hiding this comment

codecov bot commented Sep 13, 2024 • edited Loading

Codecov Report

mturley left a comment

Choose a reason for hiding this comment

mturley Sep 18, 2024

Choose a reason for hiding this comment

yih-wang Sep 19, 2024 • edited Loading

Choose a reason for hiding this comment

ppadti commented Sep 19, 2024

openshift-ci bot commented Sep 19, 2024

mturley left a comment

Choose a reason for hiding this comment

mturley Sep 20, 2024

Choose a reason for hiding this comment

ppadti commented Sep 13, 2024 •

edited

Loading

codecov bot commented Sep 13, 2024 •

edited

Loading

yih-wang Sep 19, 2024 •

edited

Loading