Set up model registry context and apiHooks #360

lucferbux · 2024-09-09T08:30:14Z

Description

This is a really big PR, added some views to test the context and apiHooks cause there were some issues that I couldn't test until we could hook them into views.

Model Registry Context

Two new contexts, one for switching between model registries and support federation and the other to load the API interface once we tap into the Model Registry

Api Hooks

Defined api hooks to support fetching from the defined API

Model Registry Empty View and MR Selector

Added Model Registry Selector Context to cover all the states fetching MR data before loading the main page
Adapted routes to support dynamic loading of contexts
Implemented the toolbar selector to change between Model Registries and support federation

How Has This Been Tested?

Added both unit testing for apiHooks, error states and other util functions.
Enhanced integration testing with the first page of model registry.

Merge criteria:

All the commits have been signed-off (To pass the DCO check)

The commits have meaningful messages; the author will squash them after approval or in case of manual merges will ask to merge with squash.
Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
The developer has manually tested the changes and verified that the changes work.
Code changes follow the kubeflow contribution guidelines.

If you have UI changes

The developer has added tests or explained why testing cannot be added.
Included any necessary screenshots or gifs if it was a UI change.
Verify that UI/UX changes conform the UX guidelines for Kubeflow.

lucferbux · 2024-09-09T08:36:08Z

Test are failing, I opened the PR cause I wanted to ask why it's not working as expected

lucferbux · 2024-09-11T11:25:18Z

clients/ui/bff/api/app.go

@@ -17,7 +18,7 @@ const (
 	ModelRegistryId      = "model_registry_id"
 	RegisteredModelId    = "registered_model_id"
 	HealthCheckPath      = PathPrefix + "/healthcheck"
-	ModelRegistry        = PathPrefix + "/model-registry"
+	ModelRegistry        = PathPrefix + "/model_registry"


@alexcreasy @ederign @Griffin-Sullivan I've just updated this enpoint cause it wasn't using the api specification and it was making the dev env to fail, I can leave this out of the PR if you guys wanna add it into other PR.

I'm fine with this, but could you update the BFF readme so the examples are correct?

Fixed, thanks a lot for the review!!

lucferbux · 2024-09-11T11:25:49Z

clients/ui/frontend/config/webpack.dev.js

@@ -18,7 +18,6 @@ module.exports = merge(common('development'), {
    host: HOST,
    port: PORT,
    historyApiFallback: true,
-    open: true,


I've just removed the auto open, we disabled it in other projects cause some people in the community tend to dislike it.

Thanks... auto open drives me nuts because I'm lame and use Safari as my default browser but like to develop in chrome :D

lucferbux · 2024-09-11T11:27:11Z

clients/ui/frontend/src/app/api/k8s.ts

-  (opts: APIOptions): Promise<ModelRegistry> =>
-    handleRestFailures(restGET(hostPath, `/api/${BFF_API_VERSION}/model_registry`, {}, opts));
+  (opts: APIOptions): Promise<ModelRegistry[]> =>
+    handleRestFailures(restGET(hostPath, `/api/${BFF_API_VERSION}/model_registry`, {}, opts)).then(


Just added here the conversion to make it easier to process in the rest of the dashboard.

lucferbux · 2024-09-11T11:27:54Z

clients/ui/frontend/src/app/api/service.ts


 export const createRegisteredModel =
-  (hostPath: string, mrName: string) =>
+  (hostPath: string) =>


It made more sense passing most of the path as hostPath rather than parameters.

lucferbux · 2024-09-11T11:29:09Z

clients/ui/frontend/src/app/api/useAPIState.ts

@@ -0,0 +1,32 @@
+import * as React from 'react';


Refactor of useApi for the proxy

Signed-off-by: lucferbux <[email protected]>

Griffin-Sullivan

I'm fine with the changes. Lots going on here so can't give a super great review but tests are passing, it works for me locally, and I don't want to get held up on this when we have more things depending on it.

Griffin-Sullivan · 2024-09-12T19:05:41Z

clients/ui/frontend/src/app/AppRoutes.tsx

@@ -42,7 +42,7 @@ export const useAdminSettings = (): NavDataItem[] => {
 export const useNavData = (): NavDataItem[] => [
  {
    label: 'Model Registry',
-    path: '/',
+    path: '/modelRegistry',


is it normal to do camel case for URLs?

This is a valid point, im just porting midstream code but we can refactor in the future.

yeah, agree with Griffin, but no need to change right now.

Signed-off-by: lucferbux <[email protected]>

alexcreasy · 2024-09-12T21:20:40Z

/lgtm

alexcreasy · 2024-09-12T21:21:48Z

/approve

google-oss-prow · 2024-09-12T21:21:54Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alexcreasy, Griffin-Sullivan

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:

~~clients/ui/OWNERS~~ [alexcreasy]

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

google-oss-prow bot requested review from alexcreasy, andreyvelich and ederign September 9, 2024 08:30

google-oss-prow bot added the size/XL label Sep 9, 2024

lucferbux force-pushed the mod-arch-10611 branch from 75118d0 to f33b037 Compare September 9, 2024 08:30

github-actions bot added the Area/UI label Sep 9, 2024

google-oss-prow bot added size/XXL and removed size/XL labels Sep 11, 2024

lucferbux commented Sep 11, 2024

View reviewed changes

lucferbux force-pushed the mod-arch-10611 branch from e57758c to 8538a76 Compare September 11, 2024 11:31

Set up model registry context and apiHooks

0821826

Signed-off-by: lucferbux <[email protected]>

lucferbux force-pushed the mod-arch-10611 branch from 8538a76 to d49c850 Compare September 12, 2024 17:13

Griffin-Sullivan approved these changes Sep 12, 2024

View reviewed changes

Add selector core loader

c08bd10

Signed-off-by: lucferbux <[email protected]>

lucferbux force-pushed the mod-arch-10611 branch from d49c850 to c08bd10 Compare September 12, 2024 19:51

google-oss-prow bot assigned alexcreasy Sep 12, 2024

google-oss-prow bot added the lgtm label Sep 12, 2024

google-oss-prow bot added the approved label Sep 12, 2024

google-oss-prow bot merged commit cc6455f into kubeflow:main Sep 12, 2024
15 checks passed

tarilabs mentioned this pull request Sep 13, 2024

[pull] main from kubeflow:main opendatahub-io/model-registry#121

Merged

This was referenced Nov 21, 2024

Add lucferbux as Member of Kubeflow org kubeflow/internal-acls#725

Closed

adds @lucferbux as a member kubeflow/internal-acls#726

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Set up model registry context and apiHooks #360

Set up model registry context and apiHooks #360

lucferbux commented Sep 9, 2024 •

edited

Loading

lucferbux commented Sep 9, 2024

lucferbux Sep 11, 2024

alexcreasy Sep 12, 2024

lucferbux Sep 12, 2024

lucferbux Sep 11, 2024

alexcreasy Sep 12, 2024

lucferbux Sep 11, 2024

lucferbux Sep 11, 2024

lucferbux Sep 11, 2024

Griffin-Sullivan left a comment

Griffin-Sullivan Sep 12, 2024

lucferbux Sep 12, 2024

alexcreasy Sep 12, 2024

alexcreasy commented Sep 12, 2024

alexcreasy commented Sep 12, 2024

google-oss-prow bot commented Sep 12, 2024

Set up model registry context and apiHooks #360

Set up model registry context and apiHooks #360

Conversation

lucferbux commented Sep 9, 2024 • edited Loading

Description

Model Registry Context

Api Hooks

Model Registry Empty View and MR Selector

How Has This Been Tested?

Merge criteria:

lucferbux commented Sep 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Griffin-Sullivan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexcreasy commented Sep 12, 2024

alexcreasy commented Sep 12, 2024

google-oss-prow bot commented Sep 12, 2024

lucferbux commented Sep 9, 2024 •

edited

Loading