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

Open
wants to merge 13 commits into
base: main
Choose a base branch
from
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
36 changes: 28 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,25 @@ export const createModelVersionForRegisteredModel =
),
);

// Use the established timestamp update utility
await bumpRegisteredModelTimestamp(
{
patchRegisteredModel: (apiOpts, patchData, id) =>
handleModelRegistryFailures(
proxyPATCH(
hostpath,
`/api/model_registry/${MODEL_REGISTRY_API_VERSION}/registered_models/${id}`,
patchData,
apiOpts,
),
),
},
registeredModelId,
);
Copy link
Contributor

Choose a reason for hiding this comment

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

You might be able to just use the patchRegisteredModel exported from this same file below (already in scope inside this function), it returns the same function you're writing here.

    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 +214,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 +228,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,37 @@
import * as React from 'react';
import { CreateModelVersionData, ModelVersion, ModelState } from '~/concepts/modelRegistry/types';
import { useModelRegistryAPI } from '~/concepts/modelRegistry/context/ModelRegistryContext';

export const useModelVersionCreation = (): {
YuliaKrimerman marked this conversation as resolved.
Show resolved Hide resolved
createVersionWithTimestampUpdate: (
registeredModelId: string,
data: CreateModelVersionData,
opts?: Record<string, unknown>,
) => Promise<ModelVersion>;
apiAvailable: boolean;
} => {
const { api, apiAvailable } = useModelRegistryAPI();

const createVersionWithTimestampUpdate = React.useCallback(
async (
registeredModelId: string,
data: CreateModelVersionData,
opts: Record<string, unknown> = {},
): Promise<ModelVersion> => {
const newVersion = await api.createModelVersionForRegisteredModel(
opts,
registeredModelId,
data,
);
await api.patchRegisteredModel(opts, { state: ModelState.LIVE }, registeredModelId);

return newVersion;
},
[api],
);

return {
createVersionWithTimestampUpdate,
apiAvailable,
};
};
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import { ContextResourceData, CustomWatchK8sResult } from '~/types';
import { TemplateKind } from '~/k8sTypes';
import { DEFAULT_CONTEXT_DATA, DEFAULT_LIST_WATCH_RESULT } from '~/utilities/const';
import useTemplateDisablement from '~/pages/modelServing/customServingRuntimes/useTemplateDisablement';
import { ModelVersion, RegisteredModel } from '~/concepts/modelRegistry/types';
import useModelRegistryAPIState, { ModelRegistryAPIState } from './useModelRegistryAPIState';

export type ModelRegistryContextType = {
Expand Down Expand Up @@ -67,6 +68,11 @@ export const ModelRegistryContextProvider = conditionalArea<ModelRegistryContext

type UseModelRegistryAPI = ModelRegistryAPIState & {
refreshAllAPI: () => void;
patchModelVersion: (modelVersionId: string, updates: Partial<ModelVersion>) => Promise<void>;
patchRegisteredModel: (
registeredModelId: string,
updates: Partial<RegisteredModel>,
) => Promise<void>;
};

export const useModelRegistryAPI = (): UseModelRegistryAPI => {
Expand All @@ -75,5 +81,9 @@ export const useModelRegistryAPI = (): UseModelRegistryAPI => {
return {
refreshAllAPI,
...apiState,
patchModelVersion: (modelVersionId: string, updates: Partial<ModelVersion>) =>
apiState.api.patchModelVersion({}, updates, modelVersionId).then(() => undefined),
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 not sure I understand the purpose of the changes in this file. Why add these patch functions to the root of the returned object when they're already returned under .api.* which you use internally here? Why then and then do nothing in the callback?

patchRegisteredModel: (registeredModelId: string, updates: Partial<RegisteredModel>) =>
apiState.api.patchRegisteredModel({}, updates, registeredModelId).then(() => undefined),
};
};
70 changes: 70 additions & 0 deletions frontend/src/concepts/modelRegistry/utils/updateTimestamps.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
import {
ModelRegistryAPIs,
ModelState,
ModelRegistryMetadataType,
} from '~/concepts/modelRegistry/types';

type MinimalModelRegistryAPI = Pick<ModelRegistryAPIs, 'patchRegisteredModel'>;

export const bumpModelVersionTimestamp = async (
api: ModelRegistryAPIs,
modelVersionId: string,
): Promise<void> => {
if (!modelVersionId) {
throw new Error('Model version ID is required');
}

try {
await api.patchModelVersion({}, { state: ModelState.LIVE }, modelVersionId);
} catch (error) {
throw new Error(
`Failed to update model version timestamp: ${
error instanceof Error ? error.message : String(error)
}`,
);
}
};

export const bumpRegisteredModelTimestamp = async (
api: MinimalModelRegistryAPI,
registeredModelId: string,
): Promise<void> => {
if (!registeredModelId) {
throw new Error('Registered model ID is required');
}

try {
const currentTime = new Date().toISOString();
await api.patchRegisteredModel(
{},
{
state: ModelState.LIVE,
customProperties: {
_lastModified: {
metadataType: ModelRegistryMetadataType.STRING,
// eslint-disable-next-line camelcase
string_value: currentTime,
},
},
Comment on lines +42 to +48
Copy link
Contributor

Choose a reason for hiding this comment

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

What's up with this customProperties piece? I don't think it should be necessary, and it'll show up in the properties table in the UI

Screenshot 2024-12-19 at 4 26 57 PM

I think all we need is the { state } part.

Also... and this part is more annoying... I wonder if it's ok to just pass ModelState.LIVE here, technically if you called this on an archived model it would un-archive it. but since the UI also prevents editing archived stuff I think we're ok. We may just want a comment explaining that here.

Copy link
Contributor

@mturley mturley Dec 19, 2024

Choose a reason for hiding this comment

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

Thinking more - if the _lastModified addition here was because the { state } alone wasn't enough to automatically bump the version, that's an issue we should raise again with the backend team. If we get blocked on this and it looks like it won't make 2.17 we could remove it from the strat scope. Or if we want a temporary workaround we can leave this, but we should maybe filter it out of the properties list so it doesn't look confusing, and open a followup issue we link in comments here and where that filter is added.

},
registeredModelId,
);
} catch (error) {
throw new Error(
`Failed to update registered model timestamp: ${
error instanceof Error ? error.message : String(error)
}`,
);
}
};

export const bumpBothTimestamps = async (
api: ModelRegistryAPIs,
modelVersionId: string,
registeredModelId: string,
): Promise<void> => {
await Promise.all([
bumpModelVersionTimestamp(api, modelVersionId),
bumpRegisteredModelTimestamp(api, registeredModelId),
]);
};
Loading
Loading