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.

Sign up for GitHub

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
merged 15 commits into from
Jan 6, 2025
Merged

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

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
537f7c2
working version
YuliaKrimerman Dec 17, 2024
52a8b05
Remove not-needed
YuliaKrimerman Dec 17, 2024
4583fc8
working version
YuliaKrimerman Dec 18, 2024
7277c59
fixed lint
YuliaKrimerman Dec 19, 2024
72d36df
fixed side effect for label
YuliaKrimerman Dec 19, 2024
3df03c7
removed files
YuliaKrimerman Dec 19, 2024
82c76d0
fixed rebase
YuliaKrimerman Dec 19, 2024
0105b6f
fixing
YuliaKrimerman Dec 19, 2024
db0a896
addressed comments
YuliaKrimerman Dec 20, 2024
c01978e
filtering and fixes
YuliaKrimerman Dec 20, 2024
419ab6d
cleanup
YuliaKrimerman Dec 20, 2024
bb185fd
more comments
YuliaKrimerman Dec 20, 2024
1d7aba9
tests
YuliaKrimerman Jan 3, 2025
637fa40
added Jira issue in comment
YuliaKrimerman Jan 6, 2025
76773f5
added comment
YuliaKrimerman Jan 6, 2025
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
2 changes: 1 addition & 1 deletion frontend/src/api/modelRegistry/__tests__/custom.spec.ts
View file
Open in desktop
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
View file
Open in desktop
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
37 changes: 37 additions & 0 deletions frontend/src/concepts/modelRegistry/apiHooks/useModelVersionCreation.ts
View file
Open in desktop
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,
};
};
10 changes: 10 additions & 0 deletions frontend/src/concepts/modelRegistry/context/ModelRegistryContext.tsx
View file
Open in desktop
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
View file
Open in desktop
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,
},
},
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 •
edited
Loading

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's add a comment here explaining the workaround and linking to https://issues.redhat.com/browse/RHOAIENG-17614

},
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),
]);
};
67 changes: 40 additions & 27 deletions frontend/src/pages/modelRegistry/screens/ModelVersionDetails/ModelVersionDetailsView.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import { ModelRegistryContext } from '~/concepts/modelRegistry/context/ModelRegi
import ModelTimestamp from '~/pages/modelRegistry/screens/components/ModelTimestamp';
import { uriToObjectStorageFields } from '~/concepts/modelRegistry/utils';
import InlineTruncatedClipboardCopy from '~/components/InlineTruncatedClipboardCopy';
import { bumpBothTimestamps } from '~/concepts/modelRegistry/utils/updateTimestamps';

type ModelVersionDetailsViewProps = {
modelVersion: ModelVersion;
Expand All @@ -47,6 +48,28 @@ const ModelVersionDetailsView: React.FC<ModelVersionDetailsViewProps> = ({
</Bullseye>
);
}
const handleVersionUpdate = async (updatePromise: Promise<unknown>): Promise<void> => {
await updatePromise;
YuliaKrimerman marked this conversation as resolved.
Show resolved Hide resolved

if (!mv.registeredModelId) {
return;
}

await bumpBothTimestamps(apiState.api, mv.id, mv.registeredModelId);
Copy link
Contributor

Choose a reason for hiding this comment

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

Since the version was just updated, you only need to bump the model timestamp, not both here.

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 tried- It didn't work - need both of them .

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand though, the updatePromise you pass in is already calling patchModelVersion with a description/labels, why do we need to patch it again to get the timestamp to change? Are you sure?

refresh();
};

const handleArtifactUpdate = async (updatePromise: Promise<unknown>): Promise<void> => {
try {
await updatePromise;
await bumpBothTimestamps(apiState.api, mv.id, mv.registeredModelId);
refreshModelArtifacts();
} catch (error) {
throw new Error(
`Failed to update artifact: ${error instanceof Error ? error.message : String(error)}`,
);
}
};

return (
<Flex
Expand All @@ -64,15 +87,7 @@ const ModelVersionDetailsView: React.FC<ModelVersionDetailsViewProps> = ({
contentWhenEmpty="No description"
value={mv.description || ''}
saveEditedValue={(value) =>
apiState.api
.patchModelVersion(
{},
{
description: value,
},
mv.id,
)
.then(refresh)
handleVersionUpdate(apiState.api.patchModelVersion({}, { description: value }, mv.id))
}
/>
<EditableLabelsDescriptionListGroup
Expand All @@ -82,15 +97,13 @@ const ModelVersionDetailsView: React.FC<ModelVersionDetailsViewProps> = ({
title="Labels"
contentWhenEmpty="No labels"
onLabelsChange={(editedLabels) =>
apiState.api
.patchModelVersion(
handleVersionUpdate(
apiState.api.patchModelVersion(
{},
{
customProperties: mergeUpdatedLabels(mv.customProperties, editedLabels),
},
{ customProperties: mergeUpdatedLabels(mv.customProperties, editedLabels) },
mv.id,
)
.then(refresh)
),
)
}
data-testid="model-version-labels"
/>
Expand Down Expand Up @@ -195,11 +208,13 @@ const ModelVersionDetailsView: React.FC<ModelVersionDetailsViewProps> = ({
isArchive={isArchiveVersion}
value={modelArtifact?.modelFormatName || ''}
saveEditedValue={(value) =>
apiState.api
.patchModelArtifact({}, { modelFormatName: value }, modelArtifact?.id || '')
.then(() => {
refreshModelArtifacts();
})
handleArtifactUpdate(
apiState.api.patchModelArtifact(
{},
{ modelFormatName: value },
modelArtifact?.id || '',
),
)
}
title="Model Format"
contentWhenEmpty="No model format specified"
Expand All @@ -210,15 +225,13 @@ const ModelVersionDetailsView: React.FC<ModelVersionDetailsViewProps> = ({
value={modelArtifact?.modelFormatVersion || ''}
isArchive={isArchiveVersion}
saveEditedValue={(newVersion) =>
apiState.api
.patchModelArtifact(
handleArtifactUpdate(
apiState.api.patchModelArtifact(
{},
{ modelFormatVersion: newVersion },
modelArtifact?.id || '',
)
.then(() => {
refreshModelArtifacts();
})
),
)
}
title="Version"
contentWhenEmpty="No source model format version"
Expand Down
26 changes: 25 additions & 1 deletion frontend/src/pages/modelRegistry/screens/ModelVersions/ModelVersionsTable.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
import * as React from 'react';
import { useCallback } from 'react';
import { useParams } from 'react-router-dom';
import { useModelRegistryAPI } from '~/concepts/modelRegistry/context/ModelRegistryContext';
import { Table } from '~/components/table';
import { ModelVersion } from '~/concepts/modelRegistry/types';
import { ModelVersion, ModelState } from '~/concepts/modelRegistry/types';
import DashboardEmptyTableView from '~/concepts/dashboard/DashboardEmptyTableView';
import useInferenceServices from '~/pages/modelServing/useInferenceServices';
import { useMakeFetchObject } from '~/utilities/useMakeFetchObject';
Expand All @@ -25,10 +27,31 @@ const ModelVersionsTable: React.FC<ModelVersionsTableProps> = ({
}) => {
const { registeredModelId } = useParams();
const inferenceServices = useMakeFetchObject(useInferenceServices(undefined, registeredModelId));
const modelRegistryApi = useModelRegistryAPI();

const handleAfterDeploy = useCallback(
async (modelVersionId: string) => {
if (!registeredModelId) {
return;
}

try {
await Promise.all([
modelRegistryApi.patchModelVersion(modelVersionId, { state: ModelState.LIVE }),
modelRegistryApi.patchRegisteredModel(registeredModelId, { state: ModelState.LIVE }),
]);
Copy link
Contributor

Choose a reason for hiding this comment

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

If you remove the changes you made to ModelRegistryContext, you can just rewrite these calls using the stuff already returned by useModelRegistryAPI. You just need an extra .api.* and to rearrange your args a bit:

        await Promise.all([
          modelRegistryApi.api.patchModelVersion({}, { state: ModelState.LIVE }, modelVersionId),
          modelRegistryApi.api.patchRegisteredModel(
            {},
            { state: ModelState.LIVE },
            registeredModelId,
          ),
        ]);

I wonder if the reason for these extra functions was because you didn't see them exposed on the root of the returned modelRegistryApi object here? I think this is why in other places we name that variable apiState and then call apiState.api.patch.... It's structured that way so we can attach extra stuff like refreshAllAPI to it but the core .api object already included here should contain all you need.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also - as mentioned elsewhere, since you'll need this same behavior for both of the places where we can deploy (the <DeployRegisteredModelModal> is also rendered in the ModelVersionDetailsHeaderActions component) you may not want to pass down a handleAfterDeploy from all the way up here (and have to duplicate it). Maybe you can just write this logic inside DeployRegisteredModelModal itself? That component is only used for model registry (it's a wrapper around the stuff we reuse in model serving) so it should always be bumping these timestamps. You can call useModelRegistryAPI(); in there. Then you could remove some of the extra plumbing you added to make handleAfterDeploy work.

Lastly, you can probably use your new util bumpBothTimestamps instead of repeating the patches here.

} catch (error) {
throw new Error('Failed to update timestamps after deployment');
}
},
[registeredModelId, modelRegistryApi],
);

const hasDeploys = (mvId: string) =>
!!inferenceServices.data.some(
(s) => s.metadata.labels?.[KnownLabels.MODEL_VERSION_ID] === mvId,
);

return (
<Table
data-testid="model-versions-table"
Expand All @@ -46,6 +69,7 @@ const ModelVersionsTable: React.FC<ModelVersionsTableProps> = ({
modelVersion={mv}
isArchiveModel={isArchiveModel}
refresh={refresh}
onAfterDeploy={handleAfterDeploy}
/>
)}
/>
Expand Down
Loading
Loading