Sorting on models page by Last Modified is reflecting changes in versions as well #3597
+407
−47
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| 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, | ||
| }, | ||
| }, | ||
| }, | ||
| 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), | ||
| ]); | ||
| }; | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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
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.LIVEhere, 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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thinking more - if the
_lastModifiedaddition 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.