Skip to content

Commit 8fc7313

Browse files
manaswinidasmanaswinidas
authored
Fix artifacts fetched twice on model version details page (#1333)
* Fix artifacts fetched twice on model version details page Signed-off-by: manaswinidas <[email protected]> * Fix Cypress tests Signed-off-by: manaswinidas <[email protected]> --------- Signed-off-by: manaswinidas <[email protected]>
1 parent a70a022 commit 8fc7313

File tree

9 files changed

+111
-41
lines changed

9 files changed

+111
-41
lines changed

‎clients/ui/frontend/src/__tests__/cypress/cypress/tests/mocked/modelRegistry/modelVersionArchive.cy.ts‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@ import {
1616
} from '~/__tests__/cypress/cypress/pages/modelRegistryView/modelVersionArchive';
1717
import { MODEL_REGISTRY_API_VERSION } from '~/__tests__/cypress/cypress/support/commands/api';
1818
import { ToastNotification } from '~/__tests__/cypress/cypress/pages/components/Notification';
19+
import { mockModelArtifactList } from '~/__mocks__/mockModelArtifactList';
1920

2021
type HandlersProps = {
2122
registeredModelsSize?: number;
@@ -147,6 +148,18 @@ const initIntercepts = ({
147148
},
148149
mockModelVersion({ id: '3', name: 'model version 3', state: ModelState.LIVE }),
149150
);
151+
152+
cy.interceptApi(
153+
`GET /api/:apiVersion/model_registry/:modelRegistryName/model_versions/:modelVersionId/artifacts`,
154+
{
155+
path: {
156+
modelRegistryName: 'modelregistry-sample',
157+
apiVersion: MODEL_REGISTRY_API_VERSION,
158+
modelVersionId: 3,
159+
},
160+
},
161+
mockModelArtifactList({}),
162+
);
150163
};
151164

152165
describe('Model version archive list', () => {

‎clients/ui/frontend/src/app/pages/modelRegistry/screens/ModelVersionDetails/ModelVersionDetails.tsx‎

Lines changed: 24 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import { ApplicationsPage } from 'mod-arch-shared';
66
import { ModelRegistrySelectorContext } from '~/app/context/ModelRegistrySelectorContext';
77
import useRegisteredModelById from '~/app/hooks/useRegisteredModelById';
88
import useModelVersionById from '~/app/hooks/useModelVersionById';
9+
import useModelArtifactsByVersionId from '~/app/hooks/useModelArtifactsByVersionId';
910
import { ModelState } from '~/app/types';
1011
import {
1112
archiveModelVersionDetailsUrl,
@@ -33,10 +34,16 @@ const ModelVersionsDetails: React.FC<ModelVersionsDetailProps> = ({ tab, ...page
3334
const { modelVersionId: mvId, registeredModelId: rmId } = useParams();
3435
const [rm] = useRegisteredModelById(rmId);
3536
const [mv, mvLoaded, mvLoadError, refreshModelVersion] = useModelVersionById(mvId);
37+
const [modelArtifacts, modelArtifactsLoaded, modelArtifactsLoadError, refreshModelArtifacts] =
38+
useModelArtifactsByVersionId(mvId);
3639

3740
const refresh = React.useCallback(() => {
3841
refreshModelVersion();
39-
}, [refreshModelVersion]);
42+
refreshModelArtifacts();
43+
}, [refreshModelVersion, refreshModelArtifacts]);
44+
45+
const loaded = mvLoaded && modelArtifactsLoaded;
46+
const loadError = mvLoadError || modelArtifactsLoadError;
4047

4148
useEffect(() => {
4249
if (rm?.state === ModelState.ARCHIVED && mv?.id) {
@@ -74,7 +81,7 @@ const ModelVersionsDetails: React.FC<ModelVersionsDetailProps> = ({ tab, ...page
7481
}
7582
title={mv?.name}
7683
headerAction={
77-
mvLoaded &&
84+
loaded &&
7885
mv && (
7986
<Flex
8087
spaceItems={{ default: 'spaceItemsMd' }}
@@ -90,17 +97,28 @@ const ModelVersionsDetails: React.FC<ModelVersionsDetailProps> = ({ tab, ...page
9097
/>
9198
</FlexItem>
9299
<FlexItem>
93-
<ModelVersionsDetailsHeaderActions mv={mv} refresh={refresh} />
100+
<ModelVersionsDetailsHeaderActions
101+
mv={mv}
102+
refresh={refresh}
103+
modelArtifacts={modelArtifacts}
104+
/>
94105
</FlexItem>
95106
</Flex>
96107
)
97108
}
98109
description={<Truncate content={mv?.description || ''} />}
99-
loadError={mvLoadError}
100-
loaded={mvLoaded}
110+
loadError={loadError}
111+
loaded={loaded}
101112
provideChildrenPadding
102113
>
103-
{mv !== null && <ModelVersionDetailsTabs tab={tab} modelVersion={mv} refresh={refresh} />}
114+
{mv !== null && (
115+
<ModelVersionDetailsTabs
116+
tab={tab}
117+
modelVersion={mv}
118+
refresh={refresh}
119+
modelArtifacts={modelArtifacts}
120+
/>
121+
)}
104122
</ApplicationsPage>
105123
);
106124
};

‎clients/ui/frontend/src/app/pages/modelRegistry/screens/ModelVersionDetails/ModelVersionDetailsHeaderActions.tsx‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@ import {
1010
ActionListItem,
1111
} from '@patternfly/react-core';
1212
import { useNavigate } from 'react-router';
13-
import { ModelState, ModelVersion } from '~/app/types';
13+
import { ModelState, ModelVersion, ModelArtifactList } from '~/app/types';
1414
import { ModelRegistryContext } from '~/app/context/ModelRegistryContext';
1515
import { ModelRegistrySelectorContext } from '~/app/context/ModelRegistrySelectorContext';
1616
import { ArchiveModelVersionModal } from '~/app/pages/modelRegistry/screens/components/ArchiveModelVersionModal';
@@ -19,12 +19,15 @@ import { modelVersionListUrl } from '~/app/pages/modelRegistry/screens/routeUtil
1919
interface ModelVersionsDetailsHeaderActionsProps {
2020
mv: ModelVersion;
2121
refresh: () => void;
22+
modelArtifacts: ModelArtifactList;
2223
}
2324

2425
const ModelVersionsDetailsHeaderActions: React.FC<ModelVersionsDetailsHeaderActionsProps> = ({
2526
mv,
2627
// eslint-disable-next-line @typescript-eslint/no-unused-vars
2728
refresh,
29+
// eslint-disable-next-line @typescript-eslint/no-unused-vars
30+
modelArtifacts,
2831
}) => {
2932
const { apiState } = React.useContext(ModelRegistryContext);
3033
const { preferredModelRegistry } = React.useContext(ModelRegistrySelectorContext);

‎clients/ui/frontend/src/app/pages/modelRegistry/screens/ModelVersionDetails/ModelVersionDetailsTabs.tsx‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
import * as React from 'react';
22
import { useNavigate } from 'react-router-dom';
33
import { PageSection, Tab, Tabs, TabTitleText } from '@patternfly/react-core';
4-
import { ModelVersion } from '~/app/types';
4+
import { ModelVersion, ModelArtifactList } from '~/app/types';
55
import { ModelVersionDetailsTabTitle, ModelVersionDetailsTab } from './const';
66
import ModelVersionDetailsView from './ModelVersionDetailsView';
77

@@ -10,13 +10,15 @@ type ModelVersionDetailTabsProps = {
1010
modelVersion: ModelVersion;
1111
isArchiveVersion?: boolean;
1212
refresh: () => void;
13+
modelArtifacts: ModelArtifactList;
1314
};
1415

1516
const ModelVersionDetailsTabs: React.FC<ModelVersionDetailTabsProps> = ({
1617
tab,
1718
modelVersion: mv,
1819
isArchiveVersion,
1920
refresh,
21+
modelArtifacts,
2022
}) => {
2123
const navigate = useNavigate();
2224
return (
@@ -42,6 +44,7 @@ const ModelVersionDetailsTabs: React.FC<ModelVersionDetailTabsProps> = ({
4244
modelVersion={mv}
4345
refresh={refresh}
4446
isArchiveVersion={isArchiveVersion}
47+
modelArtifacts={modelArtifacts}
4548
/>
4649
</PageSection>
4750
</Tab>

‎clients/ui/frontend/src/app/pages/modelRegistry/screens/ModelVersionDetails/ModelVersionDetailsView.tsx‎

Lines changed: 9 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -16,8 +16,7 @@ import {
1616
DashboardDescriptionListGroup,
1717
InlineTruncatedClipboardCopy,
1818
} from 'mod-arch-shared';
19-
import { ModelVersion } from '~/app/types';
20-
import useModelArtifactsByVersionId from '~/app/hooks/useModelArtifactsByVersionId';
19+
import { ModelVersion, ModelArtifactList } from '~/app/types';
2120
import { ModelRegistryContext } from '~/app/context/ModelRegistryContext';
2221
import { getLabels, mergeUpdatedLabels } from '~/app/pages/modelRegistry/screens/utils';
2322
import ModelPropertiesDescriptionListGroup from '~/app/pages/modelRegistry/screens/ModelPropertiesDescriptionListGroup';
@@ -30,28 +29,24 @@ type ModelVersionDetailsViewProps = {
3029
modelVersion: ModelVersion;
3130
isArchiveVersion?: boolean;
3231
refresh: () => void;
32+
modelArtifacts: ModelArtifactList;
3333
};
3434

3535
const ModelVersionDetailsView: React.FC<ModelVersionDetailsViewProps> = ({
3636
modelVersion: mv,
3737
isArchiveVersion,
3838
refresh,
39+
modelArtifacts,
3940
}) => {
40-
const [modelArtifacts, modelArtifactsLoaded, modelArtifactsLoadError, refreshModelArtifacts] =
41-
useModelArtifactsByVersionId(mv.id);
42-
4341
const modelArtifact = modelArtifacts.items.length ? modelArtifacts.items[0] : null;
4442
const { apiState } = React.useContext(ModelRegistryContext);
4543
const storageFields = uriToStorageFields(modelArtifact?.uri || '');
46-
const [registeredModel, registeredModelLoaded, registeredModelLoadError, refreshRegisteredModel] =
47-
useRegisteredModelById(mv.registeredModelId);
44+
const [registeredModel, registeredModelLoaded, registeredModelLoadError] = useRegisteredModelById(
45+
mv.registeredModelId,
46+
);
4847

49-
const loaded = modelArtifactsLoaded && registeredModelLoaded;
50-
const loadError = modelArtifactsLoadError || registeredModelLoadError;
51-
const refreshBoth = () => {
52-
refreshModelArtifacts();
53-
refreshRegisteredModel();
54-
};
48+
const loaded = registeredModelLoaded;
49+
const loadError = registeredModelLoadError;
5550

5651
if (!loaded) {
5752
return (
@@ -76,7 +71,7 @@ const ModelVersionDetailsView: React.FC<ModelVersionDetailsViewProps> = ({
7671
await updatePromise;
7772
if (registeredModel) {
7873
await bumpBothTimestamps(apiState.api, registeredModel, mv);
79-
refreshBoth();
74+
refresh();
8075
}
8176
} catch (error) {
8277
throw new Error(

‎clients/ui/frontend/src/app/pages/modelRegistry/screens/ModelVersions/ModelVersionsTableRow.tsx‎

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,10 @@ const ModelVersionsTableRow: React.FC<ModelVersionsTableRowProps> = ({
3434
const { preferredModelRegistry } = React.useContext(ModelRegistrySelectorContext);
3535
const { apiState } = React.useContext(ModelRegistryContext);
3636

37+
// TODO: Fetch model artifacts for when deploy functionality is enabled
38+
// const [modelArtifacts, modelArtifactsLoaded, modelArtifactsLoadError] =
39+
// useModelArtifactsByVersionId(mv.id);
40+
3741
const [isArchiveModalOpen, setIsArchiveModalOpen] = React.useState(false);
3842
const [isRestoreModalOpen, setIsRestoreModalOpen] = React.useState(false);
3943

@@ -119,6 +123,23 @@ const ModelVersionsTableRow: React.FC<ModelVersionsTableRowProps> = ({
119123
modelVersionName={mv.name}
120124
/>
121125
) : null}
126+
{/* TODO: [Model Serving] Uncomment when model serving is available */}
127+
{/* NOTE: When uncommenting, pass modelArtifacts prop to avoid duplicate fetching */}
128+
{/* {isDeployModalOpen ? (
129+
<DeployRegisteredModelModal
130+
onSubmit={() => {
131+
navigate(
132+
modelVersionDeploymentsUrl(
133+
mv.id,
134+
mv.registeredModelId,
135+
preferredModelRegistry.metadata.name,
136+
),
137+
);
138+
}}
139+
onCancel={() => setIsDeployModalOpen(false)}
140+
modelVersion={mv}
141+
/>
142+
) : null} */}
122143
{isRestoreModalOpen ? (
123144
<RestoreModelVersionModal
124145
onCancel={() => setIsRestoreModalOpen(false)}

‎clients/ui/frontend/src/app/pages/modelRegistry/screens/ModelVersionsArchive/ArchiveModelVersionDetails.tsx‎

Lines changed: 15 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ import { ApplicationsPage } from 'mod-arch-shared';
55
import { ModelRegistrySelectorContext } from '~/app/context/ModelRegistrySelectorContext';
66
import useRegisteredModelById from '~/app/hooks/useRegisteredModelById';
77
import useModelVersionById from '~/app/hooks/useModelVersionById';
8+
import useModelArtifactsByVersionId from '~/app/hooks/useModelArtifactsByVersionId';
89
import { ModelState } from '~/app/types';
910
import { modelVersionUrl } from '~/app/pages/modelRegistry/screens/routeUtils';
1011
import ModelVersionDetailsTabs from '~/app/pages/modelRegistry/screens/ModelVersionDetails/ModelVersionDetailsTabs';
@@ -26,8 +27,18 @@ const ArchiveModelVersionDetails: React.FC<ArchiveModelVersionDetailsProps> = ({
2627
const { modelVersionId: mvId, registeredModelId: rmId } = useParams();
2728
const [rm] = useRegisteredModelById(rmId);
2829
const [mv, mvLoaded, mvLoadError, refreshModelVersion] = useModelVersionById(mvId);
30+
const [modelArtifacts, modelArtifactsLoaded, modelArtifactsLoadError, refreshModelArtifacts] =
31+
useModelArtifactsByVersionId(mvId);
2932
const navigate = useNavigate();
3033

34+
const refresh = React.useCallback(() => {
35+
refreshModelVersion();
36+
refreshModelArtifacts();
37+
}, [refreshModelVersion, refreshModelArtifacts]);
38+
39+
const loaded = mvLoaded && modelArtifactsLoaded;
40+
const loadError = mvLoadError || modelArtifactsLoadError;
41+
3142
useEffect(() => {
3243
if (rm?.state === ModelState.LIVE && mv?.id) {
3344
navigate(modelVersionUrl(mv.id, mv.registeredModelId, preferredModelRegistry?.name));
@@ -64,16 +75,17 @@ const ArchiveModelVersionDetails: React.FC<ArchiveModelVersionDetailsProps> = ({
6475
</Tooltip>
6576
}
6677
description={<Truncate content={mv?.description || ''} />}
67-
loadError={mvLoadError}
68-
loaded={mvLoaded}
78+
loadError={loadError}
79+
loaded={loaded}
6980
provideChildrenPadding
7081
>
7182
{mv !== null && (
7283
<ModelVersionDetailsTabs
7384
isArchiveVersion
7485
tab={tab}
7586
modelVersion={mv}
76-
refresh={refreshModelVersion}
87+
refresh={refresh}
88+
modelArtifacts={modelArtifacts}
7789
/>
7890
)}
7991
</ApplicationsPage>

‎clients/ui/frontend/src/app/pages/modelRegistry/screens/ModelVersionsArchive/ModelVersionArchiveDetails.tsx‎

Lines changed: 15 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import { ModelRegistrySelectorContext } from '~/app/context/ModelRegistrySelecto
66
import { ModelRegistryContext } from '~/app/context/ModelRegistryContext';
77
import useRegisteredModelById from '~/app/hooks/useRegisteredModelById';
88
import useModelVersionById from '~/app/hooks/useModelVersionById';
9+
import useModelArtifactsByVersionId from '~/app/hooks/useModelArtifactsByVersionId';
910
import { ModelState } from '~/app/types';
1011
import {
1112
archiveModelVersionDetailsUrl,
@@ -35,8 +36,18 @@ const ModelVersionsArchiveDetails: React.FC<ModelVersionsArchiveDetailsProps> =
3536
const { modelVersionId: mvId, registeredModelId: rmId } = useParams();
3637
const [rm] = useRegisteredModelById(rmId);
3738
const [mv, mvLoaded, mvLoadError, refreshModelVersion] = useModelVersionById(mvId);
39+
const [modelArtifacts, modelArtifactsLoaded, modelArtifactsLoadError, refreshModelArtifacts] =
40+
useModelArtifactsByVersionId(mvId);
3841
const [isRestoreModalOpen, setIsRestoreModalOpen] = React.useState(false);
3942

43+
const refresh = React.useCallback(() => {
44+
refreshModelVersion();
45+
refreshModelArtifacts();
46+
}, [refreshModelVersion, refreshModelArtifacts]);
47+
48+
const loaded = mvLoaded && modelArtifactsLoaded;
49+
const loadError = mvLoadError || modelArtifactsLoadError;
50+
4051
useEffect(() => {
4152
if (rm?.state === ModelState.ARCHIVED && mv?.id) {
4253
navigate(
@@ -72,16 +83,17 @@ const ModelVersionsArchiveDetails: React.FC<ModelVersionsArchiveDetailsProps> =
7283
</Button>
7384
}
7485
description={<Truncate content={mv?.description || ''} />}
75-
loadError={mvLoadError}
76-
loaded={mvLoaded}
86+
loadError={loadError}
87+
loaded={loaded}
7788
provideChildrenPadding
7889
>
7990
{mv !== null && (
8091
<ModelVersionDetailsTabs
8192
isArchiveVersion
8293
tab={tab}
8394
modelVersion={mv}
84-
refresh={refreshModelVersion}
95+
refresh={refresh}
96+
modelArtifacts={modelArtifacts}
8597
/>
8698
)}
8799
</ApplicationsPage>

clients/ui/frontend/src/app/hooks/useModelVersionTuningData.ts renamed to clients/ui/frontend/src/app/pages/modelRegistry/screens/getModelVersionTuningData.ts

Lines changed: 6 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -1,13 +1,14 @@
1-
import { useContext } from 'react';
2-
import type { ModelVersion, RegisteredModel } from '~/app/types';
3-
import useModelArtifactsByVersionId from '~/app/hooks/useModelArtifactsByVersionId';
1+
import type { ModelVersion, RegisteredModel, ModelArtifactList, ModelRegistry } from '~/app/types';
42
import { getServerAddress } from '~/app/pages/modelRegistry/screens/utils';
5-
import { ModelRegistrySelectorContext } from '~/app/context/ModelRegistrySelectorContext';
63

7-
export const useModelVersionTuningData = (
4+
// Utility function to get model version tuning data without hook dependencies
5+
export const getModelVersionTuningData = (
86
modelVersionId: string | null,
97
modelVersion: ModelVersion | null,
108
registeredModel: RegisteredModel | null,
9+
artifacts: ModelArtifactList,
10+
preferredModelRegistry: ModelRegistry | undefined,
11+
modelRegistries: ModelRegistry[],
1112
): {
1213
tuningData: {
1314
modelRegistryName: string;
@@ -19,14 +20,8 @@ export const useModelVersionTuningData = (
1920
inputModelLocationUri: string;
2021
outputModelRegistryApiUrl: string;
2122
} | null;
22-
loaded: boolean;
23-
loadError: Error | null;
2423
} => {
25-
const { preferredModelRegistry, modelRegistries } = useContext(ModelRegistrySelectorContext);
2624
const registryService = modelRegistries.find((s) => s.name === preferredModelRegistry?.name);
27-
28-
const [artifacts, loaded, loadError] = useModelArtifactsByVersionId(modelVersionId || undefined);
29-
3025
const inputModelLocationUri = artifacts.items[0]?.uri;
3126
const modelRegistryDisplayName = registryService ? registryService.displayName : '';
3227
const outputModelRegistryApiUrl = registryService ? getServerAddress(registryService) : '';
@@ -45,7 +40,5 @@ export const useModelVersionTuningData = (
4540
outputModelRegistryApiUrl,
4641
}
4742
: null,
48-
loaded,
49-
loadError: loadError || null,
5043
};
5144
};

0 commit comments

Comments
 (0)