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

Sorting on models page by Last Modified is reflecting changes in versions as well #3597

Merged
41 changes: 38 additions & 3 deletions frontend/src/__tests__/cypress/cypress/support/commands/odh.ts
Original file line number Diff line number Diff line change
Expand Up @@ -342,9 +342,7 @@ declare global {
) => Cypress.Chainable<null>) &
((
type: 'GET /api/service/modelregistry/:serviceName/api/model_registry/:apiVersion/model_versions/:modelVersionId',
options: {
path: { serviceName: string; apiVersion: string; modelVersionId: number };
},
options: { path: { serviceName: string; apiVersion: string; modelVersionId: number } },
response: OdhResponse<ModelVersion>,
) => Cypress.Chainable<null>) &
((
Expand Down Expand Up @@ -686,6 +684,43 @@ declare global {
};
},
response: OdhResponse<NimServingResponse>,
) => Cypress.Chainable<null>) &
((
type: 'PATCH /api/service/modelregistry/:serviceName/api/model_registry/:apiVersion/registered_models/:registeredModelId',
options: {
path: {
serviceName: string;
apiVersion: string;
registeredModelId: string | number;
};
},
response: OdhResponse<RegisteredModel>,
) => Cypress.Chainable<null>) &
((
type: string,
options: {
method: 'GET';
path: {
serviceName: string;
apiVersion: string;
modelVersionId: string;
};
},
response: OdhResponse<ModelVersion>,
) => Cypress.Chainable<null>) &
((
type: 'POST /api/service/modelregistry/:serviceName/api/model_registry/:apiVersion/model_versions/:modelVersionId/artifacts',
options: {
path: { serviceName: string; apiVersion: string; modelVersionId: string };
},
response: OdhResponse<ModelArtifact>,
) => Cypress.Chainable<null>) &
((
type: 'PATCH /api/service/modelregistry/:serviceName/api/model_registry/:apiVersion/model_versions/:modelVersionId',
options: {
path: { serviceName: string; apiVersion: string; modelVersionId: string };
},
response: OdhResponse<ModelVersion>,
) => Cypress.Chainable<null>);
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -478,6 +478,18 @@ describe('Model version details', () => {
mockModelArtifact({}),
).as('updateModelFormat');

cy.interceptOdh(
'PATCH /api/service/modelregistry/:serviceName/api/model_registry/:apiVersion/registered_models/:registeredModelId',
{
path: {
serviceName: 'modelregistry-sample',
apiVersion: MODEL_REGISTRY_API_VERSION,
registeredModelId: '1',
},
},
mockRegisteredModel({}),
);

modelVersionDetails.findSourceModelFormat('edit').click();
modelVersionDetails
.findSourceModelFormat('group')
Expand Down Expand Up @@ -506,6 +518,18 @@ describe('Model version details', () => {
mockModelArtifact({}),
).as('updateModelVersion');

cy.interceptOdh(
'PATCH /api/service/modelregistry/:serviceName/api/model_registry/:apiVersion/registered_models/:registeredModelId',
{
path: {
serviceName: 'modelregistry-sample',
apiVersion: MODEL_REGISTRY_API_VERSION,
registeredModelId: '1',
},
},
mockRegisteredModel({}),
);

modelVersionDetails.findSourceModelVersion('edit').click();
modelVersionDetails.findSourceModelVersion('group').find('input').clear().type('2.0.0');
modelVersionDetails.findSourceModelVersion('save').click();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,30 @@ const initIntercepts = () => {
},
mockModelArtifact(),
).as('createModelArtifact');

cy.interceptOdh(
'PATCH /api/service/modelregistry/:serviceName/api/model_registry/:apiVersion/registered_models/:registeredModelId',
{
path: {
serviceName: 'modelregistry-sample',
apiVersion: MODEL_REGISTRY_API_VERSION,
registeredModelId: '1',
},
},
mockRegisteredModel({ id: '1', name: 'Test model name' }),
).as('updateRegisteredModel');

cy.interceptOdh(
'PATCH /api/service/modelregistry/:serviceName/api/model_registry/:apiVersion/model_versions/:modelVersionId',
{
path: {
serviceName: 'modelregistry-sample',
apiVersion: MODEL_REGISTRY_API_VERSION,
modelVersionId: '2',
},
},
mockModelVersion({ id: '2', name: 'Test version name' }),
).as('updateModelVersion');
};

describe('Register model page', () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -236,6 +236,31 @@ const initIntercepts = () => {
},
mockModelArtifact(),
).as('createModelArtifact');

// Add intercepts for timestamp updates
cy.interceptOdh(
'PATCH /api/service/modelregistry/:serviceName/api/model_registry/:apiVersion/registered_models/:registeredModelId',
{
path: {
serviceName: 'modelregistry-sample',
apiVersion: MODEL_REGISTRY_API_VERSION,
registeredModelId: '1',
},
},
mockRegisteredModel({ id: '1', name: 'Test model name' }),
).as('updateRegisteredModel');

cy.interceptOdh(
'PATCH /api/service/modelregistry/:serviceName/api/model_registry/:apiVersion/model_versions/:modelVersionId',
{
path: {
serviceName: 'modelregistry-sample',
apiVersion: MODEL_REGISTRY_API_VERSION,
modelVersionId: '6',
},
},
mockModelVersion({ id: '6', name: 'Test version name' }),
).as('updateModelVersion');
};

describe('Register model page with no preselected model', () => {
Expand Down
2 changes: 1 addition & 1 deletion frontend/src/api/modelRegistry/__tests__/custom.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,7 @@ describe('createModelVersionForRegisteredModel', () => {
state: ModelState.LIVE,
customProperties: {},
}),
).toBe(mockResultPromise);
).toEqual(mockResultPromise);
expect(proxyCREATEMock).toHaveBeenCalledTimes(1);
expect(proxyCREATEMock).toHaveBeenCalledWith(
'hostPath',
Expand Down
25 changes: 17 additions & 8 deletions frontend/src/api/modelRegistry/custom.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,10 @@ import {
RegisteredModelList,
RegisteredModel,
} from '~/concepts/modelRegistry/types';
import { MODEL_REGISTRY_API_VERSION } from '~/concepts/modelRegistry/const';
import { bumpRegisteredModelTimestamp } from '~/concepts/modelRegistry/utils/updateTimestamps';
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 =
Expand Down Expand Up @@ -41,12 +42,12 @@ export const createModelVersion =
);
export const createModelVersionForRegisteredModel =
(hostpath: string) =>
(
async (
opts: K8sAPIOptions,
registeredModelId: string,
data: CreateModelVersionData,
): Promise<ModelVersion> =>
handleModelRegistryFailures(
): Promise<ModelVersion> => {
const newVersion = await handleModelRegistryFailures<ModelVersion>(
proxyCREATE(
hostpath,
`/api/model_registry/${MODEL_REGISTRY_API_VERSION}/registered_models/${registeredModelId}/versions`,
Expand All @@ -56,6 +57,14 @@ export const createModelVersionForRegisteredModel =
),
);

await bumpRegisteredModelTimestamp(
{ patchRegisteredModel: patchRegisteredModel(hostpath) },
registeredModelId,
);

return newVersion;
};

export const createModelArtifact =
(hostPath: string) =>
(opts: K8sAPIOptions, data: CreateModelArtifactData): Promise<ModelArtifact> =>
Expand Down Expand Up @@ -194,7 +203,7 @@ export const patchRegisteredModel =
data: Partial<RegisteredModel>,
registeredModelId: string,
): Promise<RegisteredModel> =>
handleModelRegistryFailures(
handleModelRegistryFailures<RegisteredModel>(
proxyPATCH(
hostPath,
`/api/model_registry/${MODEL_REGISTRY_API_VERSION}/registered_models/${registeredModelId}`,
Expand All @@ -208,12 +217,12 @@ export const patchModelVersion =
(
opts: K8sAPIOptions,
data: Partial<ModelVersion>,
modelversionId: string,
modelVersionId: string,
): Promise<ModelVersion> =>
handleModelRegistryFailures(
handleModelRegistryFailures<ModelVersion>(
proxyPATCH(
hostPath,
`/api/model_registry/${MODEL_REGISTRY_API_VERSION}/model_versions/${modelversionId}`,
`/api/model_registry/${MODEL_REGISTRY_API_VERSION}/model_versions/${modelVersionId}`,
data,
opts,
),
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,127 @@
import {
ModelRegistryAPIs,
ModelState,
ModelRegistryMetadataType,
ModelVersion,
RegisteredModel,
} from '~/concepts/modelRegistry/types';
import {
bumpModelVersionTimestamp,
bumpRegisteredModelTimestamp,
bumpBothTimestamps,
} from '~/concepts/modelRegistry/utils/updateTimestamps';

describe('updateTimestamps', () => {
const mockApi = jest.mocked<ModelRegistryAPIs>({
createRegisteredModel: jest.fn(),
createModelVersion: jest.fn(),
createModelVersionForRegisteredModel: jest.fn(),
createModelArtifact: jest.fn(),
createModelArtifactForModelVersion: jest.fn(),
getRegisteredModel: jest.fn(),
getModelVersion: jest.fn(),
getModelArtifact: jest.fn(),
listModelArtifacts: jest.fn(),
listModelVersions: jest.fn(),
listRegisteredModels: jest.fn(),
getModelVersionsByRegisteredModel: jest.fn(),
getModelArtifactsByModelVersion: jest.fn(),
patchRegisteredModel: jest.fn(),
patchModelVersion: jest.fn(),
patchModelArtifact: jest.fn(),
});
const fakeModelVersionId = 'test-model-version-id';
const fakeRegisteredModelId = 'test-registered-model-id';

beforeEach(() => {
jest.spyOn(Date.prototype, 'toISOString').mockReturnValue('2024-01-01T00:00:00.000Z');
});

describe('bumpModelVersionTimestamp', () => {
it('should successfully update model version timestamp', async () => {
await bumpModelVersionTimestamp(mockApi, fakeModelVersionId);

expect(mockApi.patchModelVersion).toHaveBeenCalledWith(
{},
{ state: ModelState.LIVE },
fakeModelVersionId,
);
});

it('should throw error if modelVersionId is empty', async () => {
await expect(bumpModelVersionTimestamp(mockApi, '')).rejects.toThrow(
'Model version ID is required',
);
});

it('should handle API errors appropriately', async () => {
const errorMessage = 'API Error';
// Use proper type for mock function
const mockFn = mockApi.patchModelVersion;
mockFn.mockRejectedValue(new Error(errorMessage));

await expect(bumpModelVersionTimestamp(mockApi, fakeModelVersionId)).rejects.toThrow(
`Failed to update model version timestamp: ${errorMessage}`,
);
});
});

describe('bumpRegisteredModelTimestamp', () => {
it('should successfully update registered model timestamp', async () => {
await bumpRegisteredModelTimestamp(mockApi, fakeRegisteredModelId);

expect(mockApi.patchRegisteredModel).toHaveBeenCalledWith(
{},
{
state: ModelState.LIVE,
customProperties: {
_lastModified: {
metadataType: ModelRegistryMetadataType.STRING,
// eslint-disable-next-line camelcase
string_value: '2024-01-01T00:00:00.000Z',
},
},
},
fakeRegisteredModelId,
);
});

it('should throw error if registeredModelId is empty', async () => {
await expect(bumpRegisteredModelTimestamp(mockApi, '')).rejects.toThrow(
'Registered model ID is required',
);
});

it('should handle API errors appropriately', async () => {
const errorMessage = 'API Error';
// Use proper type for mock function
const mockFn = mockApi.patchRegisteredModel;
mockFn.mockRejectedValue(new Error(errorMessage));

await expect(bumpRegisteredModelTimestamp(mockApi, fakeRegisteredModelId)).rejects.toThrow(
`Failed to update registered model timestamp: ${errorMessage}`,
);
});
});

describe('bumpBothTimestamps', () => {
it('should update both timestamps successfully', async () => {
mockApi.patchModelVersion.mockResolvedValue({} as ModelVersion);
mockApi.patchRegisteredModel.mockResolvedValue({} as RegisteredModel);

await bumpBothTimestamps(mockApi, fakeModelVersionId, fakeRegisteredModelId);

expect(mockApi.patchModelVersion).toHaveBeenCalled();
expect(mockApi.patchRegisteredModel).toHaveBeenCalled();
});

it('should handle errors from either update', async () => {
const errorMessage = 'API Error';
mockApi.patchModelVersion.mockRejectedValue(new Error(errorMessage));

await expect(
bumpBothTimestamps(mockApi, fakeModelVersionId, fakeRegisteredModelId),
).rejects.toThrow();
});
});
});
Loading
Loading