From 5fc14a683fcbd5e46bdbc6bb796dbab0aec2b5b5 Mon Sep 17 00:00:00 2001 From: "opensearch-trigger-bot[bot]" <98922864+opensearch-trigger-bot[bot]@users.noreply.github.com> Date: Wed, 2 Aug 2023 17:34:05 +0800 Subject: [PATCH] Add source field to distinguish local and external model (#239) (#240) * Revert "feat: exclude remote model for admin UI (#225)" This reverts commit ba01d34b60735de75ba122e7041bac9a4b7a1380. Signed-off-by: Lin Wang * feat: add source field to distinguish local and external model Signed-off-by: Lin Wang * feat: add miss display words Signed-off-by: Lin Wang --------- Signed-off-by: Lin Wang (cherry picked from commit 63c7a5a56cdc4c09d03a9d3e512af17f5f1b05f4) Co-authored-by: Lin Wang --- public/apis/model.ts | 1 - .../monitoring/model_deployment_table.tsx | 11 +++- .../tests/model_deployment_table.test.tsx | 28 ++++++-- .../monitoring/tests/use_monitoring.test.ts | 65 ++++++++++++++----- .../components/monitoring/use_monitoring.ts | 3 +- .../__tests__/preview_panel.test.tsx | 4 +- public/components/preview_panel/index.tsx | 5 +- server/routes/model_router.ts | 4 +- server/services/model_service.ts | 1 - server/services/utils/model.ts | 21 ++---- 10 files changed, 94 insertions(+), 49 deletions(-) diff --git a/public/apis/model.ts b/public/apis/model.ts index 040feb25..daf0689c 100644 --- a/public/apis/model.ts +++ b/public/apis/model.ts @@ -30,7 +30,6 @@ export class Model { size: number; states?: MODEL_STATE[]; nameOrId?: string; - exclude?: 'REMOTE_MODEL'; }) { return InnerHttpProvider.getHttp().get(MODEL_API_ENDPOINT, { query, diff --git a/public/components/monitoring/model_deployment_table.tsx b/public/components/monitoring/model_deployment_table.tsx index 7ebf9c0f..6e18c99b 100644 --- a/public/components/monitoring/model_deployment_table.tsx +++ b/public/components/monitoring/model_deployment_table.tsx @@ -80,7 +80,7 @@ export const ModelDeploymentTable = ({ { field: 'model_state', name: 'Status', - width: '37.5%', + width: '34.5%', sortable: true, truncateText: true, render: ( @@ -124,10 +124,17 @@ export const ModelDeploymentTable = ({ ); }, }, + { + field: 'source', + name: 'Source', + width: '7%', + sortable: false, + truncateText: true, + }, { field: 'id', name: 'Model ID', - width: '25%', + width: '21%', sortable: true, render: (id: string) => ( ) => { notRespondingNodesCount: 2, planningNodesCount: 3, planningWorkerNodes: [], + source: 'Local', }, { id: 'model-2-id', @@ -27,6 +28,7 @@ const setup = (props?: Partial) => { notRespondingNodesCount: 0, planningNodesCount: 3, planningWorkerNodes: [], + source: 'Local', }, { id: 'model-3-id', @@ -35,6 +37,7 @@ const setup = (props?: Partial) => { notRespondingNodesCount: 3, planningNodesCount: 3, planningWorkerNodes: [], + source: 'External', }, ], pagination: { currentPage: 1, pageSize: 10, totalRecords: 100 }, @@ -122,11 +125,26 @@ describe('', () => { expect(within(cells[2] as HTMLElement).getByText('on 3 of 3 nodes')).toBeInTheDocument(); }); - it('should render Model ID at third column and copy to clipboard after text clicked', async () => { + it('should display source name at third column', () => { + const columnIndex = 2; + setup(); + const header = screen.getAllByRole('columnheader')[columnIndex]; + const columnContent = header + .closest('table') + ?.querySelectorAll(`tbody tr td:nth-child(${columnIndex + 1})`); + expect(within(header).getByText('Source')).toBeInTheDocument(); + expect(columnContent?.length).toBe(3); + const cells = columnContent!; + expect(within(cells[0] as HTMLElement).getByText('Local')).toBeInTheDocument(); + expect(within(cells[1] as HTMLElement).getByText('Local')).toBeInTheDocument(); + expect(within(cells[2] as HTMLElement).getByText('External')).toBeInTheDocument(); + }); + + it('should render Model ID at forth column and copy to clipboard after text clicked', async () => { const execCommandOrigin = document.execCommand; document.execCommand = jest.fn(() => true); - const columnIndex = 2; + const columnIndex = 3; setup(); const header = screen.getAllByRole('columnheader')[columnIndex]; const columnContent = header @@ -146,7 +164,7 @@ describe('', () => { }); it('should render Action column and call onViewDetail with the model item of the current table row', async () => { - const columnIndex = 3; + const columnIndex = 4; const onViewDetailMock = jest.fn(); const { finalProps } = setup({ onViewDetail: onViewDetailMock, @@ -258,7 +276,7 @@ describe('', () => { }, }); - await userEvent.click(within(screen.getAllByRole('columnheader')[2]).getByText('Model ID')); + await userEvent.click(within(screen.getAllByRole('columnheader')[3]).getByText('Model ID')); expect(finalProps.onChange).toHaveBeenCalledWith( expect.objectContaining({ sort: { @@ -277,7 +295,7 @@ describe('', () => { }} /> ); - await userEvent.click(within(screen.getAllByRole('columnheader')[2]).getByText('Model ID')); + await userEvent.click(within(screen.getAllByRole('columnheader')[3]).getByText('Model ID')); expect(finalProps.onChange).toHaveBeenCalledWith( expect.objectContaining({ sort: { diff --git a/public/components/monitoring/tests/use_monitoring.test.ts b/public/components/monitoring/tests/use_monitoring.test.ts index 2fd1ae1c..99168b35 100644 --- a/public/components/monitoring/tests/use_monitoring.test.ts +++ b/public/components/monitoring/tests/use_monitoring.test.ts @@ -124,6 +124,54 @@ describe('useMonitoring', () => { }); await waitFor(() => expect(Model.prototype.search).toHaveBeenCalledTimes(2)); }); + + it('should return consistent deployedModels', async () => { + jest.spyOn(Model.prototype, 'search').mockRestore(); + const searchMock = jest.spyOn(Model.prototype, 'search').mockResolvedValue({ + data: [ + { + id: 'model-1-id', + name: 'model-1-name', + current_worker_node_count: 1, + planning_worker_node_count: 3, + algorithm: 'TEXT_EMBEDDING', + model_state: '', + model_version: '', + planning_worker_nodes: ['node1', 'node2', 'node3'], + }, + { + id: 'model-2-id', + name: 'model-2-name', + current_worker_node_count: 1, + planning_worker_node_count: 3, + algorithm: 'REMOTE', + model_state: '', + model_version: '', + planning_worker_nodes: ['node1', 'node2', 'node3'], + }, + ], + total_models: 2, + }); + const { result, waitFor } = renderHook(() => useMonitoring()); + + await waitFor(() => { + expect(result.current.deployedModels).toEqual( + expect.arrayContaining([ + expect.objectContaining({ + id: 'model-1-id', + name: 'model-1-name', + respondingNodesCount: 1, + notRespondingNodesCount: 2, + planningNodesCount: 3, + planningWorkerNodes: ['node1', 'node2', 'node3'], + }), + expect.objectContaining({ source: 'External' }), + ]) + ); + }); + + searchMock.mockRestore(); + }); }); describe('useMonitoring.pageStatus', () => { @@ -195,21 +243,4 @@ describe('useMonitoring.pageStatus', () => { await waitFor(() => expect(result.current.pageStatus).toBe('empty')); }); - - it('should return consistent deployedModels', async () => { - const { result, waitFor } = renderHook(() => useMonitoring()); - - await waitFor(() => - expect(result.current.deployedModels).toEqual([ - { - id: 'model-1-id', - name: 'model-1-name', - respondingNodesCount: 1, - notRespondingNodesCount: 2, - planningNodesCount: 3, - planningWorkerNodes: ['node1', 'node2', 'node3'], - }, - ]) - ); - }); }); diff --git a/public/components/monitoring/use_monitoring.ts b/public/components/monitoring/use_monitoring.ts index 34907b5e..3ed509d1 100644 --- a/public/components/monitoring/use_monitoring.ts +++ b/public/components/monitoring/use_monitoring.ts @@ -47,7 +47,6 @@ const fetchDeployedModels = async (params: Params) => { ? [MODEL_STATE.loadFailed, MODEL_STATE.loaded, MODEL_STATE.partiallyLoaded] : states, sort: [`${params.sort.field}-${params.sort.direction}`], - exclude: 'REMOTE_MODEL', }); const totalPages = Math.ceil(result.total_models / params.pageSize); return { @@ -64,6 +63,7 @@ const fetchDeployedModels = async (params: Params) => { current_worker_node_count: workerCount, planning_worker_node_count: planningCount, planning_worker_nodes: planningWorkerNodes, + algorithm, }) => { return { id, @@ -75,6 +75,7 @@ const fetchDeployedModels = async (params: Params) => { ? planningCount - workerCount : undefined, planningWorkerNodes, + source: algorithm === 'REMOTE' ? 'External' : 'Local', }; } ), diff --git a/public/components/preview_panel/__tests__/preview_panel.test.tsx b/public/components/preview_panel/__tests__/preview_panel.test.tsx index 95e75965..08ca1584 100644 --- a/public/components/preview_panel/__tests__/preview_panel.test.tsx +++ b/public/components/preview_panel/__tests__/preview_panel.test.tsx @@ -13,6 +13,7 @@ const MODEL = { id: 'id1', name: 'test', planningWorkerNodes: ['node-1', 'node-2', 'node-3'], + source: 'External', }; function setup({ model = MODEL, onClose = jest.fn() }) { @@ -26,10 +27,11 @@ describe('', () => { jest.clearAllMocks(); }); - it('should render id and name in panel', () => { + it('should render id, name and source in panel', () => { setup({}); expect(screen.getByText('test')).toBeInTheDocument(); expect(screen.getByText('id1')).toBeInTheDocument(); + expect(screen.getByText('External')).toBeInTheDocument(); }); it('should call onClose when close panel', async () => { diff --git a/public/components/preview_panel/index.tsx b/public/components/preview_panel/index.tsx index 6f051292..a51e2487 100644 --- a/public/components/preview_panel/index.tsx +++ b/public/components/preview_panel/index.tsx @@ -31,6 +31,7 @@ export interface PreviewModel { name: string; id: string; planningWorkerNodes: string[]; + source: string; } interface Props { @@ -39,7 +40,7 @@ interface Props { } export const PreviewPanel = ({ onClose, model }: Props) => { - const { id, name } = model; + const { id, name, source } = model; const { data, loading } = useFetcher(APIProvider.getAPI('profile').getModel, id); const nodes = useMemo(() => { if (loading) { @@ -101,6 +102,8 @@ export const PreviewPanel = ({ onClose, model }: Props) => { + Source + {source} Model status by node diff --git a/server/routes/model_router.ts b/server/routes/model_router.ts index 94a8512e..822df4c2 100644 --- a/server/routes/model_router.ts +++ b/server/routes/model_router.ts @@ -42,12 +42,11 @@ export const modelRouter = (router: IRouter) => { ), states: schema.maybe(schema.oneOf([schema.arrayOf(modelStateSchema), modelStateSchema])), nameOrId: schema.maybe(schema.string()), - exclude: schema.maybe(schema.literal('REMOTE_MODEL')), }), }, }, async (context, request) => { - const { from, size, sort, states, nameOrId, exclude } = request.query; + const { from, size, sort, states, nameOrId } = request.query; try { const payload = await ModelService.search({ client: context.core.opensearch.client, @@ -56,7 +55,6 @@ export const modelRouter = (router: IRouter) => { sort: typeof sort === 'string' ? [sort] : sort, states: typeof states === 'string' ? [states] : states, nameOrId, - exclude, }); return opensearchDashboardsResponseFactory.ok({ body: payload }); } catch (err) { diff --git a/server/services/model_service.ts b/server/services/model_service.ts index 35480209..2b98dfdf 100644 --- a/server/services/model_service.ts +++ b/server/services/model_service.ts @@ -43,7 +43,6 @@ export class ModelService { sort?: ModelSearchSort[]; states?: MODEL_STATE[]; nameOrId?: string; - exclude?: 'REMOTE_MODEL'; }) { const { body: { hits }, diff --git a/server/services/utils/model.ts b/server/services/utils/model.ts index e347fd33..f7b585c4 100644 --- a/server/services/utils/model.ts +++ b/server/services/utils/model.ts @@ -9,11 +9,9 @@ import { generateTermQuery } from './query'; export const generateModelSearchQuery = ({ states, nameOrId, - exclude, }: { states?: MODEL_STATE[]; nameOrId?: string; - exclude?: 'REMOTE_MODEL'; }) => ({ bool: { must: [ @@ -35,21 +33,10 @@ export const generateModelSearchQuery = ({ ] : []), ], - must_not: [ - { - exists: { - field: 'chunk_number', - }, + must_not: { + exists: { + field: 'chunk_number', }, - ...(exclude === 'REMOTE_MODEL' - ? [ - { - term: { - algorithm: 'REMOTE', - }, - }, - ] - : []), - ], + }, }, });