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

Set up model registry context and apiHooks #360

Merged
merged 2 commits into from
Sep 12, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 12 additions & 12 deletions clients/ui/bff/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -57,27 +57,27 @@ make docker-build
| URL Pattern | Handler | Action |
|------------------------------------------------------------------------------------|-------------------------|----------------------------------------------|
| GET /v1/healthcheck | HealthcheckHandler | Show application information. |
| GET /v1/model-registry | ModelRegistryHandler | Get all model registries, |
| GET /v1/model-registry/{model_registry_id}/registered_models | RegisteredModelsHandler | Gets a list of all RegisteredModel entities. |
| POST /v1/model-registry/{model_registry_id}/registered_models | RegisteredModelsHandler | Create a RegisteredModel entity. |
| GET /v1/model-registry/{model_registry_id}/registered_models/{registered_model_id} | RegisteredModelHandler | Get a RegisteredModel entity by ID |
| GET /v1/model_registry | ModelRegistryHandler | Get all model registries, |
| GET /v1/model_registry/{model_registry_id}/registered_models | RegisteredModelsHandler | Gets a list of all RegisteredModel entities. |
| POST /v1/model_registry/{model_registry_id}/registered_models | RegisteredModelsHandler | Create a RegisteredModel entity. |
| GET /v1/model_registry/{model_registry_id}/registered_models/{registered_model_id} | RegisteredModelHandler | Get a RegisteredModel entity by ID |

### Sample local calls
```
# GET /v1/healthcheck
curl -i localhost:4000/api/v1/healthcheck
```
```
# GET /v1/model-registry
curl -i localhost:4000/api/v1/model-registry
# GET /v1/model_registry
curl -i localhost:4000/api/v1/model_registry
```
```
# GET /v1/model-registry/{model_registry_id}/registered_models
curl -i localhost:4000/api/v1/model-registry/model-registry/registered_models
# GET /v1/model_registry/{model_registry_id}/registered_models
curl -i localhost:4000/api/v1/model_registry/model_registry_1/registered_models
```
```
#POST /v1/model-registry/{model_registry_id}/registered_models
curl -i -X POST "http://localhost:4000/api/v1/model-registry/model-registry/registered_models" \
#POST /v1/model_registry/{model_registry_id}/registered_models
curl -i -X POST "http://localhost:4000/api/v1/model_registry/model_registry/registered_models" \
-H "Content-Type: application/json" \
-d '{
"customProperties": {
Expand All @@ -94,6 +94,6 @@ curl -i -X POST "http://localhost:4000/api/v1/model-registry/model-registry/regi
}'
```
```
# GET /v1/model-registry/{model_registry_id}/registered_models/{registered_model_id}
curl -i localhost:4000/api/v1/model-registry/model-registry/registered_models/1
# GET /v1/model_registry/{model_registry_id}/registered_models/{registered_model_id}
curl -i localhost:4000/api/v1/model_registry/model_registry/registered_models/1
```
7 changes: 4 additions & 3 deletions clients/ui/bff/api/app.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,14 @@ package api

import (
"fmt"
"log/slog"
"net/http"

"github.com/julienschmidt/httprouter"
"github.com/kubeflow/model-registry/ui/bff/config"
"github.com/kubeflow/model-registry/ui/bff/data"
"github.com/kubeflow/model-registry/ui/bff/integrations"
"github.com/kubeflow/model-registry/ui/bff/internals/mocks"
"log/slog"
"net/http"
)

const (
Expand All @@ -17,7 +18,7 @@ const (
ModelRegistryId = "model_registry_id"
RegisteredModelId = "registered_model_id"
HealthCheckPath = PathPrefix + "/healthcheck"
ModelRegistry = PathPrefix + "/model-registry"
ModelRegistry = PathPrefix + "/model_registry"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed, thanks a lot for the review!!

RegisteredModelsPath = ModelRegistry + "/:" + ModelRegistryId + "/registered_models"
RegisteredModelPath = RegisteredModelsPath + "/:" + RegisteredModelId
)
Expand Down
1 change: 0 additions & 1 deletion clients/ui/frontend/config/webpack.dev.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@ module.exports = merge(common('development'), {
host: HOST,
port: PORT,
historyApiFallback: true,
open: true,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

static: {
directory: path.resolve(relativeDir, 'dist'),
},
Expand Down
34 changes: 34 additions & 0 deletions clients/ui/frontend/src/__mocks__/mockModelArtifact.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
import { ModelArtifact, ModelArtifactState } from '~/app/types';

type MockModelArtifact = {
id?: string;
name?: string;
uri?: string;
state?: ModelArtifactState;
author?: string;
};

export const mockModelArtifact = ({
id = '1',
name = 'test',
uri = 'test',
state = ModelArtifactState.LIVE,
author = 'Author 1',
}: MockModelArtifact): ModelArtifact => ({
id,
name,
externalID: '1234132asdfasdf',
description: '',
createTimeSinceEpoch: '1710404288975',
lastUpdateTimeSinceEpoch: '1710404288975',
customProperties: {},
uri,
state,
author,
modelFormatName: 'test',
storageKey: 'test',
storagePath: 'test',
modelFormatVersion: 'test',
serviceAccountName: 'test',
artifactType: 'test',
});
17 changes: 17 additions & 0 deletions clients/ui/frontend/src/__mocks__/mockModelRegistry.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
import { ModelRegistry } from '~/app/types';

type MockModelRegistry = {
name?: string;
description?: string;
displayName?: string;
};

export const mockModelRegistry = ({
name = 'modelregistry-sample',
description = 'New model registry',
displayName = 'Model Registry Sample',
}: MockModelRegistry): ModelRegistry => ({
name,
description,
displayName,
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
/* eslint-disable camelcase */
import { ModelRegistryResponse } from '~/app/types';

export const mockModelRegistryResponse = ({
model_registry = [],
}: Partial<ModelRegistryResponse>): ModelRegistryResponse => ({
model_registry,
});
36 changes: 36 additions & 0 deletions clients/ui/frontend/src/__mocks__/mockModelVersion.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
import { ModelVersion, ModelState } from '~/app/types';
import { createModelRegistryLabelsObject } from './utils';

type MockModelVersionType = {
author?: string;
id?: string;
registeredModelId?: string;
name?: string;
labels?: string[];
state?: ModelState;
description?: string;
createTimeSinceEpoch?: string;
lastUpdateTimeSinceEpoch?: string;
};

export const mockModelVersion = ({
author = 'Test author',
registeredModelId = '1',
name = 'new model version',
labels = [],
id = '1',
state = ModelState.LIVE,
description = 'Description of model version',
createTimeSinceEpoch = '1712234877179',
lastUpdateTimeSinceEpoch = '1712234877179',
}: MockModelVersionType): ModelVersion => ({
author,
createTimeSinceEpoch,
customProperties: createModelRegistryLabelsObject(labels),
id,
lastUpdateTimeSinceEpoch,
name,
state,
registeredModelId,
description,
});
11 changes: 11 additions & 0 deletions clients/ui/frontend/src/__mocks__/mockModelVersionList.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
/* eslint-disable camelcase */
import { ModelVersionList } from '~/app/types';

export const mockModelVersionList = ({
items = [],
}: Partial<ModelVersionList>): ModelVersionList => ({
items,
nextPageToken: '',
pageSize: 0,
size: items.length,
});
53 changes: 53 additions & 0 deletions clients/ui/frontend/src/__mocks__/mockRegisteredModelsList.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
import { RegisteredModelList } from '~/app/types';
import { mockRegisteredModel } from './mockRegisteredModel';

export const mockRegisteredModelList = ({
size = 5,
items = [
mockRegisteredModel({ name: 'test-1' }),
mockRegisteredModel({ name: 'test-2' }),
mockRegisteredModel({
name: 'Fraud detection model',
description:
'A machine learning model trained to detect fraudulent transactions in financial data',
labels: [
'Financial data',
'Fraud detection',
'Test label',
'Machine learning',
'Next data to be overflow',
],
}),
mockRegisteredModel({
name: 'Credit Scoring',
labels: [
'Credit Score Predictor',
'Creditworthiness scoring system',
'Default Risk Analyzer',
'Portfolio Management',
'Risk Assessment',
],
}),
mockRegisteredModel({
name: 'Label modal',
description:
'A machine learning model trained to detect fraudulent transactions in financial data',
labels: [
'Testing label',
'Financial data',
'Fraud detection',
'Long label data to be truncated abc abc abc abc abc abc abc abc abc abc abc abc abc abc abc abc abc abc abc abc abc abc abc abc abc abc abc abc abc abc abc abc abc abc abc abc abc abc abc abc abc abc abc abc abc abc abc abc abc abc',
'Machine learning',
'Next data to be overflow',
'Label x',
'Label y',
'Label z',
],
}),
],
}: Partial<RegisteredModelList>): RegisteredModelList => ({
items,
nextPageToken: '',
pageSize: 0,
size,
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
class AppChrome {
visit() {
cy.visit('/');
this.wait();
}

private wait() {
cy.get('#dashboard-page-main');
cy.testA11y();
}

// TODO: implement when authorization is enabled
// shouldBeUnauthorized() {
// cy.findByTestId('unauthorized-error');
// return this;
// }

findNavToggle() {
return cy.get('#page-nav-toggle');
}

findSideBar() {
return cy.get('#page-sidebar');
}

findNavSection(name: string) {
return this.findSideBar().findByRole('button', { name });
}

findNavItem(name: string, section?: string) {
if (section) {
this.findNavSection(section)
// do not fail if the section is not found
.should('have.length.at.least', 0)
.then(($el) => {
if ($el.attr('aria-expanded') === 'false') {
cy.wrap($el).click();
}
});
}
return this.findSideBar().findByRole('link', { name });
}
}

export const appChrome = new AppChrome();
11 changes: 0 additions & 11 deletions clients/ui/frontend/src/__tests__/cypress/cypress/pages/home.ts

This file was deleted.

Loading