Skip to content

Commit

Permalink
Refactor model registry routes and configuration
Browse files Browse the repository at this point in the history
  • Loading branch information
lucferbux committed Apr 17, 2024
1 parent 99fe109 commit ca11336
Show file tree
Hide file tree
Showing 11 changed files with 127 additions and 110 deletions.
5 changes: 5 additions & 0 deletions .env.development
Original file line number Diff line number Diff line change
Expand Up @@ -14,3 +14,8 @@ DS_PIPELINE_DSPA_SERVICE_PORT=8443
TRUSTYAI_NAME=trustyai-service
TRUSTYAI_TAIS_SERVICE_HOST=localhost
TRUSTYAI_TAIS_SERVICE_PORT=9443


MODEL_REGISTRY_NAME=modelregistry-sample
MODEL_REGISTRY_SERVICE_HOST=localhost
MODEL_REGISTRY_SERVICE_PORT=8085
1 change: 1 addition & 0 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,7 @@ ifdef NAMESPACE
'oc port-forward -n ${NAMESPACE} svc/ds-pipeline-metadata-envoy-${DSPA_NAME} ${METADATA_ENVOY_SERVICE_PORT}:9090' \
'oc port-forward -n ${NAMESPACE} svc/ds-pipeline-${DSPA_NAME} ${DS_PIPELINE_DSPA_SERVICE_PORT}:8443' \
'oc port-forward -n ${NAMESPACE} svc/${TRUSTYAI_NAME}-tls ${TRUSTYAI_TAIS_SERVICE_PORT}:443'
'oc port-forward -n odh-model-registries svc/${MODEL_REGISTRY_NAME} ${MODEL_REGISTRY_SERVICE_PORT}:8080'
else
$(error Missing NAMESPACE variable)
endif
Expand Down
34 changes: 34 additions & 0 deletions backend/src/routes/api/service/modelregistry/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
import httpProxy from '@fastify/http-proxy';
import { KubeFastifyInstance } from '../../../../types';
import { DEV_MODE } from '../../../../utils/constants';
import { getParam, setParam } from '../../../../utils/proxy';

export default async (fastify: KubeFastifyInstance): Promise<void> => {
if (DEV_MODE) {
fastify.register(httpProxy, {
upstream: '',
prefix: '/:name',
rewritePrefix: '',
replyOptions: {
// preHandler must set the `upstream` param
getUpstream: (request) => getParam(request, 'upstream'),
},
preHandler: (request, _, done) => {
const name = getParam(request, 'name');

const upstream = DEV_MODE
? // Use port forwarding for local development:
// kubectl port-forward -n <namespace> svc/<service-name> <local.port>:<service.port>
`http://${process.env.MODEL_REGISTRY_SERVICE_HOST}:${process.env.MODEL_REGISTRY_SERVICE_PORT}`
: // Construct service URL
`http://${name}.odh-model-registries.svc.cluster.local:8080`;

// assign the `upstream` param so we can dynamically set the upstream URL for http-proxy
setParam(request, 'upstream', upstream);

fastify.log.info(`Proxy ${request.method} request ${request.url} to ${upstream}`);
done();
},
});
}
};
6 changes: 6 additions & 0 deletions backend/src/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1021,3 +1021,9 @@ export type TrustyAIKind = K8sResourceCommon & {
conditions?: K8sCondition[];
};
};

export type ModelRegistryKind = K8sResourceCommon & {
status?: {
conditions?: K8sCondition[];
};
};
4 changes: 2 additions & 2 deletions backend/src/utils/proxy.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,10 @@ import { DEV_MODE } from './constants';
import { createCustomError } from './requestUtils';
import { getAccessToken, getDirectCallOptions } from './directCallUtils';

const getParam = <F extends FastifyRequest<any, any>>(req: F, name: string) =>
export const getParam = <F extends FastifyRequest<any, any>>(req: F, name: string): string =>
(req.params as { [key: string]: string })[name];

const setParam = (req: FastifyRequest, name: string, value: string) =>
export const setParam = (req: FastifyRequest, name: string, value: string): string =>
((req.params as { [key: string]: string })[name] = value);

const notFoundError = (kind: string, name: string, e?: any, overrideMessage?: string) => {
Expand Down
27 changes: 14 additions & 13 deletions frontend/src/api/modelRegistry/__tests__/custom.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import {
patchModelVersion,
patchRegisteredModel,
} from '~/api/modelRegistry/custom';
import { MODEL_REGISTRY_API_VERSION } from '~/concepts/modelRegistry/const';

const mockProxyPromise = Promise.resolve();

Expand Down Expand Up @@ -56,7 +57,7 @@ describe('createRegisteredModel', () => {
expect(proxyCREATEMock).toHaveBeenCalledTimes(1);
expect(proxyCREATEMock).toHaveBeenCalledWith(
'hostPath',
'/api/model_registry/v1alpha2/registered_models',
`/api/model_registry/${MODEL_REGISTRY_API_VERSION}/registered_models`,
{
description: 'test',
externalID: '1',
Expand Down Expand Up @@ -88,7 +89,7 @@ describe('createModelVersion', () => {
expect(proxyCREATEMock).toHaveBeenCalledTimes(1);
expect(proxyCREATEMock).toHaveBeenCalledWith(
'hostPath',
'/api/model_registry/v1alpha2/model_versions',
`/api/model_registry/${MODEL_REGISTRY_API_VERSION}/model_versions`,
{
description: 'test',
externalID: '1',
Expand Down Expand Up @@ -126,7 +127,7 @@ describe('createModelArtifact', () => {
expect(proxyCREATEMock).toHaveBeenCalledTimes(1);
expect(proxyCREATEMock).toHaveBeenCalledWith(
'hostPath',
'/api/model_registry/v1alpha2/model_artifacts',
`/api/model_registry/${MODEL_REGISTRY_API_VERSION}/model_artifacts`,
{
description: 'test',
externalID: 'test',
Expand Down Expand Up @@ -154,7 +155,7 @@ describe('getRegisteredModel', () => {
expect(proxyGETMock).toHaveBeenCalledTimes(1);
expect(proxyGETMock).toHaveBeenCalledWith(
'hostPath',
'/api/model_registry/v1alpha2/registered_models/1',
`/api/model_registry/${MODEL_REGISTRY_API_VERSION}/registered_models/1`,
{},
K8sAPIOptionsMock,
);
Expand All @@ -169,7 +170,7 @@ describe('getModelVersion', () => {
expect(proxyGETMock).toHaveBeenCalledTimes(1);
expect(proxyGETMock).toHaveBeenCalledWith(
'hostPath',
'/api/model_registry/v1alpha2/model_versions/1',
`/api/model_registry/${MODEL_REGISTRY_API_VERSION}/model_versions/1`,
{},
K8sAPIOptionsMock,
);
Expand All @@ -184,7 +185,7 @@ describe('getModelArtifact', () => {
expect(proxyGETMock).toHaveBeenCalledTimes(1);
expect(proxyGETMock).toHaveBeenCalledWith(
'hostPath',
'/api/model_registry/v1alpha2/model_artifacts/1',
`/api/model_registry/${MODEL_REGISTRY_API_VERSION}/model_artifacts/1`,
{},
K8sAPIOptionsMock,
);
Expand All @@ -199,7 +200,7 @@ describe('getListRegisteredModels', () => {
expect(proxyGETMock).toHaveBeenCalledTimes(1);
expect(proxyGETMock).toHaveBeenCalledWith(
'hostPath',
'/api/model_registry/v1alpha2/registered_models',
`/api/model_registry/${MODEL_REGISTRY_API_VERSION}/registered_models`,
{},
K8sAPIOptionsMock,
);
Expand All @@ -214,7 +215,7 @@ describe('getListModelArtifacts', () => {
expect(proxyGETMock).toHaveBeenCalledTimes(1);
expect(proxyGETMock).toHaveBeenCalledWith(
'hostPath',
'/api/model_registry/v1alpha2/model_artifacts',
`/api/model_registry/${MODEL_REGISTRY_API_VERSION}/model_artifacts`,
{},
K8sAPIOptionsMock,
);
Expand All @@ -229,7 +230,7 @@ describe('getListModelVersions', () => {
expect(proxyGETMock).toHaveBeenCalledTimes(1);
expect(proxyGETMock).toHaveBeenCalledWith(
'hostPath',
'/api/model_registry/v1alpha2/model_versions',
`/api/model_registry/${MODEL_REGISTRY_API_VERSION}/model_versions`,
{},
K8sAPIOptionsMock,
);
Expand All @@ -244,7 +245,7 @@ describe('getModelVersionsByRegisteredModel', () => {
expect(proxyGETMock).toHaveBeenCalledTimes(1);
expect(proxyGETMock).toHaveBeenCalledWith(
'hostPath',
'/api/model_registry/v1alpha2/registered_models/1/versions',
`/api/model_registry/${MODEL_REGISTRY_API_VERSION}/registered_models/1/versions`,
{},
K8sAPIOptionsMock,
);
Expand All @@ -261,7 +262,7 @@ describe('patchRegisteredModel', () => {
expect(proxyPATCHMock).toHaveBeenCalledTimes(1);
expect(proxyPATCHMock).toHaveBeenCalledWith(
'hostPath',
'/api/model_registry/v1alpha2/registered_models/1',
`/api/model_registry/${MODEL_REGISTRY_API_VERSION}/registered_models/1`,
{ description: 'new test' },
K8sAPIOptionsMock,
);
Expand All @@ -278,7 +279,7 @@ describe('patchModelVersion', () => {
expect(proxyPATCHMock).toHaveBeenCalledTimes(1);
expect(proxyPATCHMock).toHaveBeenCalledWith(
'hostPath',
'/api/model_registry/v1alpha2/model_versions/1',
`/api/model_registry/${MODEL_REGISTRY_API_VERSION}/model_versions/1`,
{ description: 'new test' },
K8sAPIOptionsMock,
);
Expand All @@ -295,7 +296,7 @@ describe('patchModelArtifact', () => {
expect(proxyPATCHMock).toHaveBeenCalledTimes(1);
expect(proxyPATCHMock).toHaveBeenCalledWith(
'hostPath',
'/api/model_registry/v1alpha2/model_artifacts/1',
`/api/model_registry/${MODEL_REGISTRY_API_VERSION}/model_artifacts/1`,
{ description: 'new test' },
K8sAPIOptionsMock,
);
Expand Down
65 changes: 52 additions & 13 deletions frontend/src/api/modelRegistry/custom.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,27 +11,46 @@ import {
} from '~/concepts/modelRegistry/types';
import { proxyCREATE, proxyGET, proxyPATCH } from '~/api/proxyUtils';
import { K8sAPIOptions } from '~/k8sTypes';
import { MODEL_REGISTRY_API_VERSION } from '~/concepts/modelRegistry/const';
import { handleModelRegistryFailures } from './errorUtils';

export const createRegisteredModel =
(hostpath: string) =>
(opts: K8sAPIOptions, data: CreateRegisteredModelData): Promise<RegisteredModel> =>
handleModelRegistryFailures(
proxyCREATE(hostpath, '/api/model_registry/v1alpha2/registered_models', data, {}, opts),
proxyCREATE(
hostpath,
`/api/model_registry/${MODEL_REGISTRY_API_VERSION}/registered_models`,
data,
{},
opts,
),
);

export const createModelVersion =
(hostpath: string) =>
(opts: K8sAPIOptions, data: CreateModelVersionData): Promise<ModelVersion> =>
handleModelRegistryFailures(
proxyCREATE(hostpath, '/api/model_registry/v1alpha2/model_versions', data, {}, opts),
proxyCREATE(
hostpath,
`/api/model_registry/${MODEL_REGISTRY_API_VERSION}/model_versions`,
data,
{},
opts,
),
);

export const createModelArtifact =
(hostPath: string) =>
(opts: K8sAPIOptions, data: CreateModelArtifactData): Promise<ModelArtifact> =>
handleModelRegistryFailures(
proxyCREATE(hostPath, '/api/model_registry/v1alpha2/model_artifacts', data, {}, opts),
proxyCREATE(
hostPath,
`/api/model_registry/${MODEL_REGISTRY_API_VERSION}/model_artifacts`,
data,
{},
opts,
),
);

export const getRegisteredModel =
Expand All @@ -40,7 +59,7 @@ export const getRegisteredModel =
handleModelRegistryFailures(
proxyGET(
hostPath,
`/api/model_registry/v1alpha2/registered_models/${registeredModelID}`,
`/api/model_registry/${MODEL_REGISTRY_API_VERSION}/registered_models/${registeredModelID}`,
{},
opts,
),
Expand All @@ -50,7 +69,12 @@ export const getModelVersion =
(hostpath: string) =>
(opts: K8sAPIOptions, modelversionId: string): Promise<ModelVersion> =>
handleModelRegistryFailures(
proxyGET(hostpath, `/api/model_registry/v1alpha2/model_versions/${modelversionId}`, {}, opts),
proxyGET(
hostpath,
`/api/model_registry/${MODEL_REGISTRY_API_VERSION}/model_versions/${modelversionId}`,
{},
opts,
),
);

export const getModelArtifact =
Expand All @@ -59,7 +83,7 @@ export const getModelArtifact =
handleModelRegistryFailures(
proxyGET(
hostPath,
`/api/model_registry/v1alpha2/model_artifacts/${modelArtifactId}`,
`/api/model_registry/${MODEL_REGISTRY_API_VERSION}/model_artifacts/${modelArtifactId}`,
{},
opts,
),
Expand All @@ -69,21 +93,36 @@ export const getListModelArtifacts =
(hostpath: string) =>
(opts: K8sAPIOptions): Promise<ModelArtifactList> =>
handleModelRegistryFailures(
proxyGET(hostpath, `/api/model_registry/v1alpha2/model_artifacts`, {}, opts),
proxyGET(
hostpath,
`/api/model_registry/${MODEL_REGISTRY_API_VERSION}/model_artifacts`,
{},
opts,
),
);

export const getListModelVersions =
(hostpath: string) =>
(opts: K8sAPIOptions): Promise<ModelVersionList> =>
handleModelRegistryFailures(
proxyGET(hostpath, `/api/model_registry/v1alpha2/model_versions`, {}, opts),
proxyGET(
hostpath,
`/api/model_registry/${MODEL_REGISTRY_API_VERSION}/model_versions`,
{},
opts,
),
);

export const getListRegisteredModels =
(hostpath: string) =>
(opts: K8sAPIOptions): Promise<RegisteredModelList> =>
handleModelRegistryFailures(
proxyGET(hostpath, `/api/model_registry/v1alpha2/registered_models`, {}, opts),
proxyGET(
hostpath,
`/api/model_registry/${MODEL_REGISTRY_API_VERSION}/registered_models`,
{},
opts,
),
);

export const getModelVersionsByRegisteredModel =
Expand All @@ -92,7 +131,7 @@ export const getModelVersionsByRegisteredModel =
handleModelRegistryFailures(
proxyGET(
hostpath,
`/api/model_registry/v1alpha2/registered_models/${registeredmodelId}/versions`,
`/api/model_registry/${MODEL_REGISTRY_API_VERSION}/registered_models/${registeredmodelId}/versions`,
{},
opts,
),
Expand All @@ -108,7 +147,7 @@ export const patchRegisteredModel =
handleModelRegistryFailures(
proxyPATCH(
hostPath,
`/api/model_registry/v1alpha2/registered_models/${registeredModelID}`,
`/api/model_registry/${MODEL_REGISTRY_API_VERSION}/registered_models/${registeredModelID}`,
data,
opts,
),
Expand All @@ -124,7 +163,7 @@ export const patchModelVersion =
handleModelRegistryFailures(
proxyPATCH(
hostPath,
`/api/model_registry/v1alpha2/model_versions/${modelversionId}`,
`/api/model_registry/${MODEL_REGISTRY_API_VERSION}/model_versions/${modelversionId}`,
data,
opts,
),
Expand All @@ -140,7 +179,7 @@ export const patchModelArtifact =
handleModelRegistryFailures(
proxyPATCH(
hostPath,
`/api/model_registry/v1alpha2/model_artifacts/${modelartifactId}`,
`/api/model_registry/${MODEL_REGISTRY_API_VERSION}/model_artifacts/${modelartifactId}`,
data,
opts,
),
Expand Down
4 changes: 2 additions & 2 deletions frontend/src/concepts/modelRegistry/const.ts
Original file line number Diff line number Diff line change
@@ -1,2 +1,2 @@
export const MODEL_REGISTRY_DEFINITION_NAME = 'modelregistry-sample'; // TODO: harcoded value, we need to dynamically fetch this
export const MODEL_REGISTRY_DEFAULT_NAMESPACE = 'shared'; // TODO: harcoded value, we need to check which will be the default namespace
export const MODEL_REGISTRY_DEFAULT_NAMESPACE = 'odh-model-registries'; // The default namespace for the model registry service, controlled by the operator.
export const MODEL_REGISTRY_API_VERSION = 'v1alpha3';
Loading

0 comments on commit ca11336

Please sign in to comment.