From 7bc67423a727cd9829b9979faff2133a1b77669d Mon Sep 17 00:00:00 2001 From: James Liu Date: Mon, 14 Jun 2021 23:33:23 -0700 Subject: [PATCH 1/6] feat(frontend) Support Input/Output from MLMD for V2-compatible --- frontend/src/components/DetailsTable.tsx | 12 +- .../components/tabs/ExecutionTitle.test.tsx | 51 ++++ .../src/components/tabs/ExecutionTitle.tsx | 39 +++ .../components/tabs/InputOutputTab.test.tsx | 181 ++++++++++++++ .../src/components/tabs/InputOutputTab.tsx | 234 ++++++++++++++++++ frontend/src/components/tabs/MetricsTab.tsx | 19 +- frontend/src/lib/MlmdUtils.ts | 28 ++- frontend/src/pages/RunDetails.tsx | 77 +++--- 8 files changed, 577 insertions(+), 64 deletions(-) create mode 100644 frontend/src/components/tabs/ExecutionTitle.test.tsx create mode 100644 frontend/src/components/tabs/ExecutionTitle.tsx create mode 100644 frontend/src/components/tabs/InputOutputTab.test.tsx create mode 100644 frontend/src/components/tabs/InputOutputTab.tsx diff --git a/frontend/src/components/DetailsTable.tsx b/frontend/src/components/DetailsTable.tsx index 499520d698b..a337d31b269 100644 --- a/frontend/src/components/DetailsTable.tsx +++ b/frontend/src/components/DetailsTable.tsx @@ -14,15 +14,15 @@ * limitations under the License. */ -import * as React from 'react'; -import { stylesheet } from 'typestyle'; -import { color, spacing, commonCss } from '../Css'; -import { KeyValue } from '../lib/StaticGraphParser'; -import Editor from './Editor'; import 'brace'; import 'brace/ext/language_tools'; import 'brace/mode/json'; import 'brace/theme/github'; +import * as React from 'react'; +import { stylesheet } from 'typestyle'; +import { color, commonCss, spacing } from '../Css'; +import { KeyValue } from '../lib/StaticGraphParser'; +import Editor from './Editor'; export const css = stylesheet({ key: { @@ -71,7 +71,7 @@ const DetailsTable = (props: DetailsTableProps) => { const { fields, title, valueComponent: ValueComponent, valueComponentProps } = props; return ( - {!!title &&
{title}
} + {!!title &&
{title}
}
{fields.map((f, i) => { const [key, value] = f; diff --git a/frontend/src/components/tabs/ExecutionTitle.test.tsx b/frontend/src/components/tabs/ExecutionTitle.test.tsx new file mode 100644 index 00000000000..f3a9103740a --- /dev/null +++ b/frontend/src/components/tabs/ExecutionTitle.test.tsx @@ -0,0 +1,51 @@ +/* + * Copyright 2021 The Kubeflow Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import { Execution, Value } from '@kubeflow/frontend'; +import { render } from '@testing-library/react'; +import React from 'react'; +import { testBestPractices } from 'src/TestUtils'; +import { CommonTestWrapper } from 'src/TestWrapper'; +import { ExecutionTitle } from './ExecutionTitle'; + +testBestPractices(); +describe('ExecutionTitle', () => { + const execution = new Execution(); + const executionName = 'fake-execution'; + const executionId = 123; + beforeEach(() => { + execution.setId(executionId); + execution.getCustomPropertiesMap().set('task_name', new Value().setStringValue(executionName)); + }); + + it('Shows execution name', () => { + const { getByText } = render( + + + , + ); + getByText(executionName, { selector: 'a', exact: false }); + }); + + it('Shows execution description', () => { + const { getByText } = render( + + + , + ); + getByText('This step corresponds to execution'); + }); +}); diff --git a/frontend/src/components/tabs/ExecutionTitle.tsx b/frontend/src/components/tabs/ExecutionTitle.tsx new file mode 100644 index 00000000000..e7379e63472 --- /dev/null +++ b/frontend/src/components/tabs/ExecutionTitle.tsx @@ -0,0 +1,39 @@ +/* + * Copyright 2021 The Kubeflow Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import { Execution } from '@kubeflow/frontend'; +import React from 'react'; +import { Link } from 'react-router-dom'; +import { commonCss } from 'src/Css'; +import { ExecutionHelpers } from 'src/lib/MlmdUtils'; +import { RoutePageFactory } from '../Router'; + +interface ExecutionTitleProps { + execution: Execution; +} + +export function ExecutionTitle({ execution }: ExecutionTitleProps) { + return ( + <> +
+ This step corresponds to execution{' '} + + "{ExecutionHelpers.getName(execution)}". + +
+ + ); +} diff --git a/frontend/src/components/tabs/InputOutputTab.test.tsx b/frontend/src/components/tabs/InputOutputTab.test.tsx new file mode 100644 index 00000000000..57d9d458da9 --- /dev/null +++ b/frontend/src/components/tabs/InputOutputTab.test.tsx @@ -0,0 +1,181 @@ +/* + * Copyright 2021 The Kubeflow Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import { Artifact, Execution, Value } from '@kubeflow/frontend'; +import { render, waitFor } from '@testing-library/react'; +import { Struct } from 'google-protobuf/google/protobuf/struct_pb'; +import React from 'react'; +import * as mlmdUtils from 'src/lib/MlmdUtils'; +import { testBestPractices } from 'src/TestUtils'; +import { CommonTestWrapper } from 'src/TestWrapper'; +import InputOutputTab from './InputOutputTab'; + +const executionName = 'fake-execution'; +const artifactName = 'artifactName'; +const artifactUri = 'gs://test'; + +testBestPractices(); +describe('InoutOutputTab', () => { + it('shows execution title', () => { + const { getByText } = render( + + + , + ); + getByText(executionName, { selector: 'a', exact: false }); + }); + + it('shows Input/Output artifacts and parameters title', () => { + const { getAllByText, getByText } = render( + + + , + ); + getByText('Input'); + getByText('Output'); + expect(getAllByText('Parameters').length).toEqual(2); + expect(getAllByText('Artifacts').length).toEqual(2); + }); + + it('shows Input parameters with various types', async () => { + jest.spyOn(mlmdUtils, 'getOutputArtifactsInExecution').mockResolvedValueOnce([]); + jest.spyOn(mlmdUtils, 'getInputArtifactsInExecution').mockResolvedValueOnce([]); + + const execution = buildBasicExecution(); + execution + .getCustomPropertiesMap() + .set('thisKeyIsNotInput', new Value().setStringValue("value shouldn't show")); + execution + .getCustomPropertiesMap() + .set('input:stringkey', new Value().setStringValue('string input')); + execution.getCustomPropertiesMap().set('input:intkey', new Value().setIntValue(42)); + execution.getCustomPropertiesMap().set('input:doublekey', new Value().setDoubleValue(1.99)); + execution + .getCustomPropertiesMap() + .set( + 'input:structkey', + new Value().setStructValue(Struct.fromJavaScript({ struct: { key: 'value', num: 42 } })), + ); + execution + .getCustomPropertiesMap() + .set( + 'input:arraykey', + new Value().setStructValue(Struct.fromJavaScript({ list: ['a', 'b', 'c'] })), + ); + const { queryByText, getByText } = render( + + + , + ); + + getByText('stringkey'); + getByText('string input'); + getByText('intkey'); + getByText('42'); + getByText('doublekey'); + getByText('1.99'); + getByText('structkey'); + getByText('arraykey'); + expect(queryByText('thisKeyIsNotInput')).toBeNull(); + }); + + it('shows Output parameters with various types', async () => { + jest.spyOn(mlmdUtils, 'getOutputArtifactsInExecution').mockResolvedValueOnce([]); + jest.spyOn(mlmdUtils, 'getInputArtifactsInExecution').mockResolvedValueOnce([]); + + const execution = buildBasicExecution(); + execution + .getCustomPropertiesMap() + .set('thisKeyIsNotOutput', new Value().setStringValue("value shouldn't show")); + execution + .getCustomPropertiesMap() + .set('output:stringkey', new Value().setStringValue('string output')); + execution.getCustomPropertiesMap().set('output:intkey', new Value().setIntValue(42)); + execution.getCustomPropertiesMap().set('output:doublekey', new Value().setDoubleValue(1.99)); + execution + .getCustomPropertiesMap() + .set( + 'output:structkey', + new Value().setStructValue(Struct.fromJavaScript({ struct: { key: 'value', num: 42 } })), + ); + execution + .getCustomPropertiesMap() + .set( + 'output:arraykey', + new Value().setStructValue(Struct.fromJavaScript({ list: ['a', 'b', 'c'] })), + ); + const { queryByText, getByText } = render( + + + , + ); + + getByText('stringkey'); + getByText('string output'); + getByText('intkey'); + getByText('42'); + getByText('doublekey'); + getByText('1.99'); + getByText('structkey'); + getByText('arraykey'); + expect(queryByText('thisKeyIsNotOutput')).toBeNull(); + }); + + it('shows Input artifacts', async () => { + const artifact = buildArtifact(); + jest.spyOn(mlmdUtils, 'getInputArtifactsInExecution').mockResolvedValueOnce([artifact]); + jest.spyOn(mlmdUtils, 'getOutputArtifactsInExecution').mockResolvedValueOnce([]); + const { getByText } = render( + + + , + ); + + await waitFor(() => getByText(artifactName)); + await waitFor(() => getByText(artifactUri)); + }); + + it('shows Output artifacts', async () => { + const artifact = buildArtifact(); + jest.spyOn(mlmdUtils, 'getInputArtifactsInExecution').mockResolvedValueOnce([]); + jest.spyOn(mlmdUtils, 'getOutputArtifactsInExecution').mockResolvedValueOnce([artifact]); + const { getByText } = render( + + + , + ); + + await waitFor(() => getByText(artifactName)); + await waitFor(() => getByText(artifactUri)); + }); +}); + +function buildBasicExecution() { + const execution = new Execution(); + const executionId = 123; + + execution.setId(executionId); + execution.getCustomPropertiesMap().set('task_name', new Value().setStringValue(executionName)); + + return execution; +} + +function buildArtifact() { + const artifact = new Artifact(); + artifact.getCustomPropertiesMap().set('name', new Value().setStringValue(artifactName)); + artifact.setUri(artifactUri); + return artifact; +} diff --git a/frontend/src/components/tabs/InputOutputTab.tsx b/frontend/src/components/tabs/InputOutputTab.tsx new file mode 100644 index 00000000000..b559825f343 --- /dev/null +++ b/frontend/src/components/tabs/InputOutputTab.tsx @@ -0,0 +1,234 @@ +/* + * Copyright 2021 The Kubeflow Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import { + Artifact, + ArtifactCustomProperties, + ArtifactProperties, + Execution, + getMetadataValue, + getResourceProperty, +} from '@kubeflow/frontend'; +import { Struct } from 'google-protobuf/google/protobuf/struct_pb'; +import React from 'react'; +import { useQuery } from 'react-query'; +import { ErrorBoundary } from 'src/atoms/ErrorBoundary'; +import { color, commonCss, padding, spacing } from 'src/Css'; +import { getInputArtifactsInExecution, getOutputArtifactsInExecution } from 'src/lib/MlmdUtils'; +import { KeyValue } from 'src/lib/StaticGraphParser'; +import { stylesheet } from 'typestyle'; +import { ArtifactLink } from '../ArtifactLink'; +import Banner from '../Banner'; +import DetailsTable, { ValueComponentProps } from '../DetailsTable'; +import { ExecutionTitle } from './ExecutionTitle'; + +const css = stylesheet({ + key: { + color: color.strong, + flex: '0 0 50%', + fontWeight: 'bold', + maxWidth: 300, + }, + row: { + borderBottom: `1px solid ${color.divider}`, + display: 'flex', + padding: `${spacing.units(-5)}px ${spacing.units(-6)}px`, + }, + valueText: { + flexGrow: 1, + overflow: 'hidden', + overflowWrap: 'break-word', + }, +}); + +type ParamList = Array>; + +export interface IOTabProps { + execution: Execution; +} + +export function InputOutputTab({ execution }: IOTabProps) { + const executionId = execution.getId(); + + // Retrieves input and output artifacts from Metadata store. + const { + isSuccess: isOutputArtifactSuccess, + error: outputArtifactError, + data: outputArtifactData, + } = useQuery( + ['execution_output_artifact', { id: executionId }], + () => getOutputArtifactsInExecution(execution), + { staleTime: Infinity }, + ); + + const { + isSuccess: isInputArtifactSuccess, + error: inputArtifactError, + data: inputArtifactData, + } = useQuery( + ['execution_input_artifact', { id: executionId }], + () => getInputArtifactsInExecution(execution), + { staleTime: Infinity }, + ); + + // Restructs artifacts and parameters for visualization. + const inputParams: ParamList = extractInputFromExecution(execution); + const outputParams: ParamList = extractOutputFromExecution(execution); + let inputArtifacts: ParamList = []; + if (isInputArtifactSuccess && inputArtifactData) { + inputArtifacts = getArtifactParamList(inputArtifactData); + } + let outputArtifacts: ParamList = []; + if (isOutputArtifactSuccess && outputArtifactData) { + outputArtifacts = getArtifactParamList(outputArtifactData); + } + + return ( + +
+
+ + + {outputArtifactError && ( + + )} + {!outputArtifactError && inputArtifactError && ( + + )} + +
{'Input'}
+ + + + +
+
+ +
{'Output'}
+ + + +
+
+ + ); +} + +export default InputOutputTab; + +function extractInputFromExecution(execution: Execution): KeyValue[] { + return extractParamFromExecution(execution, /input:(?.+)/, 'inputName'); +} + +function extractOutputFromExecution(execution: Execution): KeyValue[] { + return extractParamFromExecution(execution, /output:(?.+)/, 'outputName'); +} + +function extractParamFromExecution( + execution: Execution, + pattern: RegExp, + groupName: string, +): KeyValue[] { + const result: KeyValue[] = []; + execution.getCustomPropertiesMap().forEach((value, key) => { + const found = key.match(pattern); + if (found?.groups?.[groupName]) { + result.push([found.groups[groupName], prettyPrintValue(getMetadataValue(value))]); + } + }); + return result; +} + +function prettyPrintValue(value: string | number | Struct | undefined): string { + if (value == null) { + return ''; + } + if (typeof value === 'string') { + return value; + } + if (typeof value === 'number') { + return JSON.stringify(value); + } + // value is Struct + const jsObject = value.toJavaScript(); + // When Struct is converted to js object, it may contain a top level "struct" + // or "list" key depending on its type, but the key is meaningless and we can + // omit it in visualization. + return JSON.stringify(jsObject?.struct || jsObject?.list || jsObject, null, 2); +} + +function getArtifactParamList(inputArtifactData: Artifact[]): ParamList { + return inputArtifactData.map(artifact => [ + (getResourceProperty(artifact, ArtifactProperties.NAME) || + getResourceProperty(artifact, ArtifactCustomProperties.NAME, true) || + '') as string, + artifact.getUri(), + ]); +} + +interface ArtifactPreviewProps extends ValueComponentProps { + fields: Array>; + title?: string; +} + +const ArtifactPreview: React.FC = (props: ArtifactPreviewProps) => { + const { fields, title } = props; + return ( + <> + {!!title &&
{title}
} +
+ {fields.map((f, i) => { + const [key, value] = f; + if (typeof value === 'string') { + return ( +
+ {key} + + {value ? : `${value || ''}`} + +
+ ); + } + return null; + })} +
+ + ); +}; diff --git a/frontend/src/components/tabs/MetricsTab.tsx b/frontend/src/components/tabs/MetricsTab.tsx index b83ea37fd66..93386a4b217 100644 --- a/frontend/src/components/tabs/MetricsTab.tsx +++ b/frontend/src/components/tabs/MetricsTab.tsx @@ -17,17 +17,12 @@ import { Artifact, ArtifactType, Execution } from '@kubeflow/frontend'; import * as React from 'react'; import { useQuery } from 'react-query'; -import { Link } from 'react-router-dom'; import { ErrorBoundary } from 'src/atoms/ErrorBoundary'; import { commonCss, padding } from 'src/Css'; -import { - ExecutionHelpers, - getArtifactTypes, - getOutputArtifactsInExecution, -} from 'src/lib/MlmdUtils'; +import { getArtifactTypes, getOutputArtifactsInExecution } from 'src/lib/MlmdUtils'; import Banner from '../Banner'; -import { RoutePageFactory } from '../Router'; import { MetricsVisualizations } from '../viewers/MetricsVisualizations'; +import { ExecutionTitle } from './ExecutionTitle'; type MetricsTabProps = { execution: Execution; @@ -88,15 +83,7 @@ export function MetricsTab({ execution }: MetricsTabProps) {
-
- This step corresponds to execution{' '} - - "{ExecutionHelpers.getName(execution)}". - -
+ {executionStateUnknown && } {!executionStateUnknown && !executionCompleted && ( diff --git a/frontend/src/lib/MlmdUtils.ts b/frontend/src/lib/MlmdUtils.ts index bd8cf8d6f06..d8a3859479a 100644 --- a/frontend/src/lib/MlmdUtils.ts +++ b/frontend/src/lib/MlmdUtils.ts @@ -177,10 +177,10 @@ function getStringValue(value?: string | number | Struct | null): string | undef return value; } -/** - * @throws error when network error or invalid data - */ -export async function getOutputArtifactsInExecution(execution: Execution): Promise { +async function getArtifactsInExecution( + execution: Execution, + event_filter: Event.Type | undefined, +): Promise { const executionId = execution.getId(); if (!executionId) { throw new Error('Execution must have an ID'); @@ -196,13 +196,13 @@ export async function getOutputArtifactsInExecution(execution: Execution): Promi throw err; } - const outputArtifactIds = res + const inputArtifactIds = res .getEventsList() - .filter(event => event.getType() === Event.Type.OUTPUT && event.getArtifactId()) + .filter(event => (!event_filter || event.getType() === event_filter) && event.getArtifactId()) .map(event => event.getArtifactId()); const artifactsRequest = new GetArtifactsByIDRequest(); - artifactsRequest.setArtifactIdsList(outputArtifactIds); + artifactsRequest.setArtifactIdsList(inputArtifactIds); let artifactsRes: GetArtifactsByIDResponse; try { artifactsRes = await Api.getInstance().metadataStoreService.getArtifactsByID(artifactsRequest); @@ -214,6 +214,20 @@ export async function getOutputArtifactsInExecution(execution: Execution): Promi return artifactsRes.getArtifactsList(); } +/** + * @throws error when network error or invalid data + */ +export async function getOutputArtifactsInExecution(execution: Execution): Promise { + return getArtifactsInExecution(execution, Event.Type.OUTPUT); +} + +/** + * @throws error when network error or invalid data + */ +export async function getInputArtifactsInExecution(execution: Execution): Promise { + return getArtifactsInExecution(execution, Event.Type.INPUT); +} + export async function getArtifactTypes(): Promise { const request = new GetArtifactTypesRequest(); let res: GetArtifactTypesResponse; diff --git a/frontend/src/pages/RunDetails.tsx b/frontend/src/pages/RunDetails.tsx index 1747f0876e6..014b4f18be0 100644 --- a/frontend/src/pages/RunDetails.tsx +++ b/frontend/src/pages/RunDetails.tsx @@ -21,6 +21,7 @@ import { flatten } from 'lodash'; import * as React from 'react'; import { Link, Redirect } from 'react-router-dom'; import { ExternalLink } from 'src/atoms/ExternalLink'; +import InputOutputTab from 'src/components/tabs/InputOutputTab'; import { MetricsTab } from 'src/components/tabs/MetricsTab'; import { GkeMetadata, GkeMetadataContext } from 'src/lib/GkeMetadata'; import { useNamespaceChangeEvent } from 'src/lib/KubeflowClient'; @@ -387,41 +388,47 @@ class RunDetails extends Page { isV2Pipeline(workflow) && selectedExecution && } - {sidepanelSelectedTab === SidePaneTab.INPUT_OUTPUT && ( -
- - - - - - - -
- )} + {sidepanelSelectedTab === SidePaneTab.INPUT_OUTPUT && + !isV2Pipeline(workflow) && ( +
+ + + + + + + +
+ )} + {sidepanelSelectedTab === SidePaneTab.INPUT_OUTPUT && + isV2Pipeline(workflow) && + selectedExecution && ( + + )} {sidepanelSelectedTab === SidePaneTab.TASK_DETAILS && (
From 0aedf30e43d3b526f552c6e5379a1f297a4277c9 Mon Sep 17 00:00:00 2001 From: James Liu Date: Mon, 14 Jun 2021 23:50:26 -0700 Subject: [PATCH 2/6] fix test --- frontend/src/components/DetailsTable.test.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frontend/src/components/DetailsTable.test.tsx b/frontend/src/components/DetailsTable.test.tsx index 1c1dcc7e132..d01ce26e79d 100644 --- a/frontend/src/components/DetailsTable.test.tsx +++ b/frontend/src/components/DetailsTable.test.tsx @@ -62,7 +62,7 @@ describe('DetailsTable', () => { expect(container).toMatchInlineSnapshot(`
some title
From 7999f36ccfde3f0be73bc679287d7b4c5e5389c5 Mon Sep 17 00:00:00 2001 From: James Liu Date: Wed, 16 Jun 2021 00:00:04 -0700 Subject: [PATCH 3/6] address nit comments --- .../components/tabs/ExecutionTitle.test.tsx | 10 +-- .../components/tabs/InputOutputTab.test.tsx | 68 +++++++++---------- .../src/components/tabs/InputOutputTab.tsx | 8 +-- 3 files changed, 43 insertions(+), 43 deletions(-) diff --git a/frontend/src/components/tabs/ExecutionTitle.test.tsx b/frontend/src/components/tabs/ExecutionTitle.test.tsx index f3a9103740a..f82c90aabd8 100644 --- a/frontend/src/components/tabs/ExecutionTitle.test.tsx +++ b/frontend/src/components/tabs/ExecutionTitle.test.tsx @@ -15,7 +15,7 @@ */ import { Execution, Value } from '@kubeflow/frontend'; -import { render } from '@testing-library/react'; +import { render, screen } from '@testing-library/react'; import React from 'react'; import { testBestPractices } from 'src/TestUtils'; import { CommonTestWrapper } from 'src/TestWrapper'; @@ -32,20 +32,20 @@ describe('ExecutionTitle', () => { }); it('Shows execution name', () => { - const { getByText } = render( + render( , ); - getByText(executionName, { selector: 'a', exact: false }); + screen.getByText(executionName, { selector: 'a', exact: false }); }); it('Shows execution description', () => { - const { getByText } = render( + render( , ); - getByText('This step corresponds to execution'); + screen.getByText('This step corresponds to execution'); }); }); diff --git a/frontend/src/components/tabs/InputOutputTab.test.tsx b/frontend/src/components/tabs/InputOutputTab.test.tsx index 57d9d458da9..f96bfa3ad3a 100644 --- a/frontend/src/components/tabs/InputOutputTab.test.tsx +++ b/frontend/src/components/tabs/InputOutputTab.test.tsx @@ -15,7 +15,7 @@ */ import { Artifact, Execution, Value } from '@kubeflow/frontend'; -import { render, waitFor } from '@testing-library/react'; +import { render, waitFor, screen } from '@testing-library/react'; import { Struct } from 'google-protobuf/google/protobuf/struct_pb'; import React from 'react'; import * as mlmdUtils from 'src/lib/MlmdUtils'; @@ -30,24 +30,24 @@ const artifactUri = 'gs://test'; testBestPractices(); describe('InoutOutputTab', () => { it('shows execution title', () => { - const { getByText } = render( + render( , ); - getByText(executionName, { selector: 'a', exact: false }); + screen.getByText(executionName, { selector: 'a', exact: false }); }); it('shows Input/Output artifacts and parameters title', () => { - const { getAllByText, getByText } = render( + render( , ); - getByText('Input'); - getByText('Output'); - expect(getAllByText('Parameters').length).toEqual(2); - expect(getAllByText('Artifacts').length).toEqual(2); + screen.getByText('Input'); + screen.getByText('Output'); + expect(screen.getAllByText('Parameters').length).toEqual(2); + expect(screen.getAllByText('Artifacts').length).toEqual(2); }); it('shows Input parameters with various types', async () => { @@ -75,21 +75,21 @@ describe('InoutOutputTab', () => { 'input:arraykey', new Value().setStructValue(Struct.fromJavaScript({ list: ['a', 'b', 'c'] })), ); - const { queryByText, getByText } = render( + render( , ); - getByText('stringkey'); - getByText('string input'); - getByText('intkey'); - getByText('42'); - getByText('doublekey'); - getByText('1.99'); - getByText('structkey'); - getByText('arraykey'); - expect(queryByText('thisKeyIsNotInput')).toBeNull(); + screen.getByText('stringkey'); + screen.getByText('string input'); + screen.getByText('intkey'); + screen.getByText('42'); + screen.getByText('doublekey'); + screen.getByText('1.99'); + screen.getByText('structkey'); + screen.getByText('arraykey'); + expect(screen.queryByText('thisKeyIsNotInput')).toBeNull(); }); it('shows Output parameters with various types', async () => { @@ -117,49 +117,49 @@ describe('InoutOutputTab', () => { 'output:arraykey', new Value().setStructValue(Struct.fromJavaScript({ list: ['a', 'b', 'c'] })), ); - const { queryByText, getByText } = render( + render( , ); - getByText('stringkey'); - getByText('string output'); - getByText('intkey'); - getByText('42'); - getByText('doublekey'); - getByText('1.99'); - getByText('structkey'); - getByText('arraykey'); - expect(queryByText('thisKeyIsNotOutput')).toBeNull(); + screen.getByText('stringkey'); + screen.getByText('string output'); + screen.getByText('intkey'); + screen.getByText('42'); + screen.getByText('doublekey'); + screen.getByText('1.99'); + screen.getByText('structkey'); + screen.getByText('arraykey'); + expect(screen.queryByText('thisKeyIsNotOutput')).toBeNull(); }); it('shows Input artifacts', async () => { const artifact = buildArtifact(); jest.spyOn(mlmdUtils, 'getInputArtifactsInExecution').mockResolvedValueOnce([artifact]); jest.spyOn(mlmdUtils, 'getOutputArtifactsInExecution').mockResolvedValueOnce([]); - const { getByText } = render( + render( , ); - await waitFor(() => getByText(artifactName)); - await waitFor(() => getByText(artifactUri)); + await waitFor(() => screen.getByText(artifactName)); + await waitFor(() => screen.getByText(artifactUri)); }); it('shows Output artifacts', async () => { const artifact = buildArtifact(); jest.spyOn(mlmdUtils, 'getInputArtifactsInExecution').mockResolvedValueOnce([]); jest.spyOn(mlmdUtils, 'getOutputArtifactsInExecution').mockResolvedValueOnce([artifact]); - const { getByText } = render( + render( , ); - await waitFor(() => getByText(artifactName)); - await waitFor(() => getByText(artifactUri)); + await waitFor(() => screen.getByText(artifactName)); + await waitFor(() => screen.getByText(artifactUri)); }); }); diff --git a/frontend/src/components/tabs/InputOutputTab.tsx b/frontend/src/components/tabs/InputOutputTab.tsx index b559825f343..b854e9888b4 100644 --- a/frontend/src/components/tabs/InputOutputTab.tsx +++ b/frontend/src/components/tabs/InputOutputTab.tsx @@ -85,8 +85,8 @@ export function InputOutputTab({ execution }: IOTabProps) { ); // Restructs artifacts and parameters for visualization. - const inputParams: ParamList = extractInputFromExecution(execution); - const outputParams: ParamList = extractOutputFromExecution(execution); + const inputParams = extractInputFromExecution(execution); + const outputParams = extractOutputFromExecution(execution); let inputArtifacts: ParamList = []; if (isInputArtifactSuccess && inputArtifactData) { inputArtifacts = getArtifactParamList(inputArtifactData); @@ -208,7 +208,7 @@ interface ArtifactPreviewProps extends ValueComponentProps { title?: string; } -const ArtifactPreview: React.FC = (props: ArtifactPreviewProps) => { +function ArtifactPreview(props: ArtifactPreviewProps) { const { fields, title } = props; return ( <> @@ -231,4 +231,4 @@ const ArtifactPreview: React.FC = (props: ArtifactPreviewP
); -}; +} From 83530f3856d5e1bb8a8b80e0677481e25fbb0ecb Mon Sep 17 00:00:00 2001 From: James Liu Date: Sat, 19 Jun 2021 13:54:07 -0700 Subject: [PATCH 4/6] Artifact Preview component, use events to get artifact name. --- frontend/src/TestUtils.ts | 8 +- .../src/components/ArtifactPreview.test.tsx | 123 +++++++++++++++ frontend/src/components/ArtifactPreview.tsx | 147 ++++++++++++++++++ .../components/tabs/InputOutputTab.test.tsx | 124 ++++++++++++--- .../src/components/tabs/InputOutputTab.tsx | 141 +++++------------ frontend/src/lib/Apis.ts | 2 +- frontend/src/lib/MlmdUtils.ts | 65 +++++--- frontend/src/pages/RunDetails.tsx | 9 +- 8 files changed, 468 insertions(+), 151 deletions(-) create mode 100644 frontend/src/components/ArtifactPreview.test.tsx create mode 100644 frontend/src/components/ArtifactPreview.tsx diff --git a/frontend/src/TestUtils.ts b/frontend/src/TestUtils.ts index c12af78e856..abdd8c01045 100644 --- a/frontend/src/TestUtils.ts +++ b/frontend/src/TestUtils.ts @@ -163,7 +163,13 @@ export function expectWarnings() { }; } -export const queryClientTest = new QueryClient(); +export const queryClientTest = new QueryClient({ + defaultOptions: { + queries: { + retry: false, + }, + }, +}); export function testBestPractices() { beforeEach(async () => { queryClientTest.clear(); diff --git a/frontend/src/components/ArtifactPreview.test.tsx b/frontend/src/components/ArtifactPreview.test.tsx new file mode 100644 index 00000000000..a64cf28bad8 --- /dev/null +++ b/frontend/src/components/ArtifactPreview.test.tsx @@ -0,0 +1,123 @@ +/* + * Copyright 2021 The Kubeflow Authors + * + * Licensed under the Apache License, Version 2.0 (the 'License'); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an 'AS IS' BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import { render, screen, waitFor } from '@testing-library/react'; +import React from 'react'; +import { CommonTestWrapper } from 'src/TestWrapper'; +import { Apis } from '../lib/Apis'; +import { testBestPractices } from '../TestUtils'; +import ArtifactPreview from './ArtifactPreview'; + +testBestPractices(); +describe('ArtifactPreview', () => { + it('handles undefined artifact', () => { + render( + + + , + ); + screen.getByText('Can not retrieve storage path from artifact uri: undefined'); + }); + + it('handles null artifact', () => { + render( + + + , + ); + screen.getByText('Can not retrieve storage path from artifact uri: null'); + }); + + it('handles empty artifact', () => { + expect(() => { + render( + + + , + ); + }).toThrow(new Error('Unsupported storage path: i am random path')); + }); + + it('handles invalid artifact: no bucket', async () => { + jest.spyOn(Apis, 'readFile').mockRejectedValue(new Error('server error: no bucket')); + + render( + + + , + ); + await waitFor(() => screen.getByText('Error in retrieving artifact preview.')); + }); + + it('handles gcs artifact', async () => { + jest.spyOn(Apis, 'readFile').mockResolvedValue('gcs preview'); + render( + + + , + ); + await waitFor(() => screen.getByText('gcs://bucket/key')); + await waitFor(() => screen.getByText('gcs preview')); + }); + + it('handles minio artifact with namespace', async () => { + jest.spyOn(Apis, 'readFile').mockResolvedValueOnce('minio content'); + render( + + + , + ); + await waitFor(() => screen.getByText('minio://bucket/key')); + await waitFor(() => + expect(screen.getByText('View All').getAttribute('href')).toEqual( + 'artifacts/get?source=minio&namespace=kubeflow&bucket=bucket&key=key', + ), + ); + }); + + it('handles artifact that previews with maxlines', async () => { + const data = `012\n345\n678\n910`; + jest.spyOn(Apis, 'readFile').mockResolvedValueOnce(data); + render( + + + , + ); + await waitFor(() => screen.getByText('minio://bucket/key')); + await waitFor(() => screen.getByText(`012 345 ...`)); + }); + + it('handles artifact that previews with maxbytes', async () => { + const data = `012\n345\n678\n910`; + jest.spyOn(Apis, 'readFile').mockResolvedValueOnce(data); + render( + + + , + ); + await waitFor(() => screen.getByText('minio://bucket/key')); + await waitFor(() => screen.getByText(`012 345 67 ...`)); + }); +}); diff --git a/frontend/src/components/ArtifactPreview.tsx b/frontend/src/components/ArtifactPreview.tsx new file mode 100644 index 00000000000..385dcfcf6a7 --- /dev/null +++ b/frontend/src/components/ArtifactPreview.tsx @@ -0,0 +1,147 @@ +/** + * Copyright 2021 The Kubeflow Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import React from 'react'; +import { useQuery } from 'react-query'; +import { ExternalLink } from 'src/atoms/ExternalLink'; +import { color } from 'src/Css'; +import { Apis } from 'src/lib/Apis'; +import WorkflowParser, { StoragePath } from 'src/lib/WorkflowParser'; +import { stylesheet } from 'typestyle'; +import Banner from './Banner'; +import { ValueComponentProps } from './DetailsTable'; + +const css = stylesheet({ + root: { + width: '100%', + }, + preview: { + maxHeight: 250, + overflowY: 'auto', + padding: 3, + backgroundColor: color.lightGrey, + }, + topDiv: { + display: 'flex', + justifyContent: 'space-between', + }, + separater: { + width: 20, // There's minimum 20px separation between URI and view button. + display: 'inline-block', + }, + viewLink: { + whiteSpace: 'nowrap', + }, +}); + +export interface ArtifactPreviewProps extends ValueComponentProps { + namespace?: string; + maxbytes?: number; + maxlines?: number; +} + +/** + * A component that renders a preview to an artifact with a link to the full content. + */ +const ArtifactPreview: React.FC = ({ + value, + namespace, + maxbytes = 255, + maxlines = 20, +}) => { + let storage: StoragePath | undefined; + if (value) { + storage = WorkflowParser.parseStoragePath(value); + } + + const { isSuccess, isError, data, error } = useQuery( + ['artifact_preview', value], + () => getPreview(storage, namespace, maxbytes, maxlines), + { staleTime: Infinity }, + ); + + if (!storage) { + return ( + + ); + } + + const linkText = Apis.buildArtifactUrl(storage); + const artifactDownloadUrl = Apis.buildReadFileUrl({ + path: storage, + namespace, + isDownload: true, + }); + const artifactViewUrl = Apis.buildReadFileUrl({ path: storage, namespace }); + + return ( +
+
+ + {linkText} + + + + View All + +
+ {isError && ( + + )} + {isSuccess && data && ( +
+ + 
{data}
+ +
+ )} +
+ ); +}; + +export default ArtifactPreview; + +async function getPreview( + storagePath: StoragePath | undefined, + namespace: string | undefined, + maxbytes: number, + maxlines?: number, +): Promise { + if (!storagePath) { + return ``; + } + // TODO how to handle binary data (can probably use magic number to id common mime types) + let data = await Apis.readFile(storagePath, namespace, maxbytes + 1); + // is preview === data and no maxlines + if (data.length <= maxbytes && (!maxlines || data.split('\n').length < maxlines)) { + return data; + } + // remove extra byte at the end (we requested maxbytes +1) + data = data.slice(0, maxbytes); + // check num lines + if (maxlines) { + data = data + .split('\n') + .slice(0, maxlines) + .join('\n') + .trim(); + } + return `${data}\n...`; +} diff --git a/frontend/src/components/tabs/InputOutputTab.test.tsx b/frontend/src/components/tabs/InputOutputTab.test.tsx index f96bfa3ad3a..b7d097adf44 100644 --- a/frontend/src/components/tabs/InputOutputTab.test.tsx +++ b/frontend/src/components/tabs/InputOutputTab.test.tsx @@ -14,34 +14,61 @@ * limitations under the License. */ -import { Artifact, Execution, Value } from '@kubeflow/frontend'; +import { + Api, + Artifact, + Execution, + Value, + Event, + GetArtifactsByIDResponse, + GetEventsByExecutionIDsResponse, +} from '@kubeflow/frontend'; import { render, waitFor, screen } from '@testing-library/react'; import { Struct } from 'google-protobuf/google/protobuf/struct_pb'; import React from 'react'; +import { Apis } from 'src/lib/Apis'; import * as mlmdUtils from 'src/lib/MlmdUtils'; import { testBestPractices } from 'src/TestUtils'; import { CommonTestWrapper } from 'src/TestWrapper'; import InputOutputTab from './InputOutputTab'; const executionName = 'fake-execution'; -const artifactName = 'artifactName'; -const artifactUri = 'gs://test'; +const artifactId = 100; +const artifactUri = 'gs://bucket/test'; +const artifactUriView = 'gcs://bucket/test'; +const inputArtifactName = 'input_artifact'; +const outputArtifactName = 'output_artifact'; +const namespace = 'namespace'; testBestPractices(); describe('InoutOutputTab', () => { it('shows execution title', () => { + jest + .spyOn(Api.getInstance().metadataStoreService, 'getEventsByExecutionIDs') + .mockResolvedValue(new GetEventsByExecutionIDsResponse()); + jest + .spyOn(Api.getInstance().metadataStoreService, 'getArtifactsByID') + .mockReturnValue(new GetArtifactsByIDResponse()); + render( - + , ); screen.getByText(executionName, { selector: 'a', exact: false }); }); it('shows Input/Output artifacts and parameters title', () => { + jest + .spyOn(Api.getInstance().metadataStoreService, 'getEventsByExecutionIDs') + .mockResolvedValue(new GetEventsByExecutionIDsResponse()); + jest + .spyOn(Api.getInstance().metadataStoreService, 'getArtifactsByID') + .mockReturnValue(new GetArtifactsByIDResponse()); + render( - + , ); screen.getByText('Input'); @@ -51,8 +78,12 @@ describe('InoutOutputTab', () => { }); it('shows Input parameters with various types', async () => { - jest.spyOn(mlmdUtils, 'getOutputArtifactsInExecution').mockResolvedValueOnce([]); - jest.spyOn(mlmdUtils, 'getInputArtifactsInExecution').mockResolvedValueOnce([]); + jest + .spyOn(Api.getInstance().metadataStoreService, 'getEventsByExecutionIDs') + .mockResolvedValue(new GetEventsByExecutionIDsResponse()); + jest + .spyOn(Api.getInstance().metadataStoreService, 'getArtifactsByID') + .mockReturnValue(new GetArtifactsByIDResponse()); const execution = buildBasicExecution(); execution @@ -77,7 +108,7 @@ describe('InoutOutputTab', () => { ); render( - + , ); @@ -93,8 +124,12 @@ describe('InoutOutputTab', () => { }); it('shows Output parameters with various types', async () => { - jest.spyOn(mlmdUtils, 'getOutputArtifactsInExecution').mockResolvedValueOnce([]); - jest.spyOn(mlmdUtils, 'getInputArtifactsInExecution').mockResolvedValueOnce([]); + jest + .spyOn(Api.getInstance().metadataStoreService, 'getEventsByExecutionIDs') + .mockResolvedValue(new GetEventsByExecutionIDsResponse()); + jest + .spyOn(Api.getInstance().metadataStoreService, 'getArtifactsByID') + .mockReturnValue(new GetArtifactsByIDResponse()); const execution = buildBasicExecution(); execution @@ -119,7 +154,7 @@ describe('InoutOutputTab', () => { ); render( - + , ); @@ -135,31 +170,49 @@ describe('InoutOutputTab', () => { }); it('shows Input artifacts', async () => { - const artifact = buildArtifact(); - jest.spyOn(mlmdUtils, 'getInputArtifactsInExecution').mockResolvedValueOnce([artifact]); - jest.spyOn(mlmdUtils, 'getOutputArtifactsInExecution').mockResolvedValueOnce([]); + jest.spyOn(Apis, 'readFile').mockResolvedValue('artifact preview'); + const getEventResponse = new GetEventsByExecutionIDsResponse(); + getEventResponse.getEventsList().push(buildInputEvent()); + jest + .spyOn(Api.getInstance().metadataStoreService, 'getEventsByExecutionIDs') + .mockResolvedValueOnce(getEventResponse); + const getArtifactsResponse = new GetArtifactsByIDResponse(); + getArtifactsResponse.getArtifactsList().push(buildArtifact()); + jest + .spyOn(Api.getInstance().metadataStoreService, 'getArtifactsByID') + .mockReturnValueOnce(getArtifactsResponse); + render( - + , ); - await waitFor(() => screen.getByText(artifactName)); - await waitFor(() => screen.getByText(artifactUri)); + await waitFor(() => screen.getByText(artifactUriView)); + await waitFor(() => screen.getByText(inputArtifactName)); }); it('shows Output artifacts', async () => { - const artifact = buildArtifact(); - jest.spyOn(mlmdUtils, 'getInputArtifactsInExecution').mockResolvedValueOnce([]); - jest.spyOn(mlmdUtils, 'getOutputArtifactsInExecution').mockResolvedValueOnce([artifact]); + jest.spyOn(Apis, 'readFile').mockResolvedValue('artifact preview'); + const getEventResponse = new GetEventsByExecutionIDsResponse(); + getEventResponse.getEventsList().push(buildOutputEvent()); + jest + .spyOn(Api.getInstance().metadataStoreService, 'getEventsByExecutionIDs') + .mockResolvedValueOnce(getEventResponse); + const getArtifactsResponse = new GetArtifactsByIDResponse(); + getArtifactsResponse.getArtifactsList().push(buildArtifact()); + jest + .spyOn(Api.getInstance().metadataStoreService, 'getArtifactsByID') + .mockReturnValueOnce(getArtifactsResponse); + render( - + , ); - await waitFor(() => screen.getByText(artifactName)); - await waitFor(() => screen.getByText(artifactUri)); + await waitFor(() => screen.getByText(artifactUriView)); + await waitFor(() => screen.getByText(outputArtifactName)); }); }); @@ -175,7 +228,30 @@ function buildBasicExecution() { function buildArtifact() { const artifact = new Artifact(); - artifact.getCustomPropertiesMap().set('name', new Value().setStringValue(artifactName)); + artifact.getCustomPropertiesMap(); artifact.setUri(artifactUri); + artifact.setId(artifactId); return artifact; } + +function buildInputEvent() { + const event = new Event(); + const path = new Event.Path(); + path.getStepsList().push(new Event.Path.Step().setKey(inputArtifactName)); + event + .setType(Event.Type.INPUT) + .setArtifactId(artifactId) + .setPath(path); + return event; +} + +function buildOutputEvent() { + const event = new Event(); + const path = new Event.Path(); + path.getStepsList().push(new Event.Path.Step().setKey(outputArtifactName)); + event + .setType(Event.Type.OUTPUT) + .setArtifactId(artifactId) + .setPath(path); + return event; +} diff --git a/frontend/src/components/tabs/InputOutputTab.tsx b/frontend/src/components/tabs/InputOutputTab.tsx index b854e9888b4..b85dc5fe84b 100644 --- a/frontend/src/components/tabs/InputOutputTab.tsx +++ b/frontend/src/components/tabs/InputOutputTab.tsx @@ -14,73 +14,38 @@ * limitations under the License. */ -import { - Artifact, - ArtifactCustomProperties, - ArtifactProperties, - Execution, - getMetadataValue, - getResourceProperty, -} from '@kubeflow/frontend'; +import { Execution, getMetadataValue } from '@kubeflow/frontend'; import { Struct } from 'google-protobuf/google/protobuf/struct_pb'; import React from 'react'; import { useQuery } from 'react-query'; import { ErrorBoundary } from 'src/atoms/ErrorBoundary'; -import { color, commonCss, padding, spacing } from 'src/Css'; -import { getInputArtifactsInExecution, getOutputArtifactsInExecution } from 'src/lib/MlmdUtils'; +import { commonCss, padding } from 'src/Css'; +import { + filterEventWithInputArtifact, + filterEventWithOutputArtifact, + getLinkedArtifactsByExecution, + LinkedArtifact, +} from 'src/lib/MlmdUtils'; import { KeyValue } from 'src/lib/StaticGraphParser'; -import { stylesheet } from 'typestyle'; -import { ArtifactLink } from '../ArtifactLink'; +import ArtifactPreview from '../ArtifactPreview'; import Banner from '../Banner'; -import DetailsTable, { ValueComponentProps } from '../DetailsTable'; +import DetailsTable from '../DetailsTable'; import { ExecutionTitle } from './ExecutionTitle'; -const css = stylesheet({ - key: { - color: color.strong, - flex: '0 0 50%', - fontWeight: 'bold', - maxWidth: 300, - }, - row: { - borderBottom: `1px solid ${color.divider}`, - display: 'flex', - padding: `${spacing.units(-5)}px ${spacing.units(-6)}px`, - }, - valueText: { - flexGrow: 1, - overflow: 'hidden', - overflowWrap: 'break-word', - }, -}); - type ParamList = Array>; export interface IOTabProps { execution: Execution; + namespace: string | undefined; } -export function InputOutputTab({ execution }: IOTabProps) { +export function InputOutputTab({ execution, namespace }: IOTabProps) { const executionId = execution.getId(); // Retrieves input and output artifacts from Metadata store. - const { - isSuccess: isOutputArtifactSuccess, - error: outputArtifactError, - data: outputArtifactData, - } = useQuery( - ['execution_output_artifact', { id: executionId }], - () => getOutputArtifactsInExecution(execution), - { staleTime: Infinity }, - ); - - const { - isSuccess: isInputArtifactSuccess, - error: inputArtifactError, - data: inputArtifactData, - } = useQuery( - ['execution_input_artifact', { id: executionId }], - () => getInputArtifactsInExecution(execution), + const { isSuccess, error, data } = useQuery( + ['execution_artifact', { id: executionId, state: execution.getLastKnownState() }], + () => getLinkedArtifactsByExecution(execution), { staleTime: Infinity }, ); @@ -88,12 +53,10 @@ export function InputOutputTab({ execution }: IOTabProps) { const inputParams = extractInputFromExecution(execution); const outputParams = extractOutputFromExecution(execution); let inputArtifacts: ParamList = []; - if (isInputArtifactSuccess && inputArtifactData) { - inputArtifacts = getArtifactParamList(inputArtifactData); - } let outputArtifacts: ParamList = []; - if (isOutputArtifactSuccess && outputArtifactData) { - outputArtifacts = getArtifactParamList(outputArtifactData); + if (isSuccess && data) { + inputArtifacts = getArtifactParamList(filterEventWithInputArtifact(data)); + outputArtifacts = getArtifactParamList(filterEventWithOutputArtifact(data)); } return ( @@ -102,18 +65,11 @@ export function InputOutputTab({ execution }: IOTabProps) {
- {outputArtifactError && ( - - )} - {!outputArtifactError && inputArtifactError && ( + {error && ( )} @@ -124,10 +80,14 @@ export function InputOutputTab({ execution }: IOTabProps) { fields={inputParams} /> - key={`input-artifacts-${executionId}`} title='Artifacts' fields={inputArtifacts} + valueComponent={ArtifactPreview} + valueComponentProps={{ + namespace: namespace, + }} />
@@ -140,10 +100,14 @@ export function InputOutputTab({ execution }: IOTabProps) { fields={outputParams} /> - key={`output-artifacts-${executionId}`} title='Artifacts' fields={outputArtifacts} + valueComponent={ArtifactPreview} + valueComponentProps={{ + namespace: namespace, + }} />
@@ -194,41 +158,12 @@ function prettyPrintValue(value: string | number | Struct | undefined): string { return JSON.stringify(jsObject?.struct || jsObject?.list || jsObject, null, 2); } -function getArtifactParamList(inputArtifactData: Artifact[]): ParamList { - return inputArtifactData.map(artifact => [ - (getResourceProperty(artifact, ArtifactProperties.NAME) || - getResourceProperty(artifact, ArtifactCustomProperties.NAME, true) || - '') as string, - artifact.getUri(), +function getArtifactParamList(inputArtifacts: LinkedArtifact[]): ParamList { + return inputArtifacts.map(linkedArtifact => [ + linkedArtifact.event + .getPath() + ?.getStepsList()[0] + .getKey(), + linkedArtifact.artifact.getUri(), ]); } - -interface ArtifactPreviewProps extends ValueComponentProps { - fields: Array>; - title?: string; -} - -function ArtifactPreview(props: ArtifactPreviewProps) { - const { fields, title } = props; - return ( - <> - {!!title &&
{title}
} -
- {fields.map((f, i) => { - const [key, value] = f; - if (typeof value === 'string') { - return ( -
- {key} - - {value ? : `${value || ''}`} - -
- ); - } - return null; - })} -
- - ); -} diff --git a/frontend/src/lib/Apis.ts b/frontend/src/lib/Apis.ts index 2173f366c46..9391d97f98f 100644 --- a/frontend/src/lib/Apis.ts +++ b/frontend/src/lib/Apis.ts @@ -21,8 +21,8 @@ import { ApiVisualization, VisualizationServiceApi } from '../apis/visualization import { HTMLViewerConfig } from '../components/viewers/HTMLViewer'; import { PlotType } from '../components/viewers/Viewer'; import * as Utils from './Utils'; -import { StoragePath } from './WorkflowParser'; import { buildQuery } from './Utils'; +import { StoragePath } from './WorkflowParser'; const v1beta1Prefix = 'apis/v1beta1'; diff --git a/frontend/src/lib/MlmdUtils.ts b/frontend/src/lib/MlmdUtils.ts index d8a3859479a..baa39b75c14 100644 --- a/frontend/src/lib/MlmdUtils.ts +++ b/frontend/src/lib/MlmdUtils.ts @@ -177,32 +177,36 @@ function getStringValue(value?: string | number | Struct | null): string | undef return value; } -async function getArtifactsInExecution( - execution: Execution, - event_filter: Event.Type | undefined, -): Promise { +async function getEventByExecution(execution: Execution): Promise { const executionId = execution.getId(); if (!executionId) { throw new Error('Execution must have an ID'); } - const request = new GetEventsByExecutionIDsRequest(); - request.addExecutionIds(executionId); - let res: GetEventsByExecutionIDsResponse; + const request = new GetEventsByExecutionIDsRequest().addExecutionIds(executionId); + let response: GetEventsByExecutionIDsResponse; try { - res = await Api.getInstance().metadataStoreService.getEventsByExecutionIDs(request); + response = await Api.getInstance().metadataStoreService.getEventsByExecutionIDs(request); } catch (err) { err.message = 'Failed to getExecutionsByExecutionIDs: ' + err.message; throw err; } + return response.getEventsList(); +} + +// An artifact which has associated event. +// You can retrieve artifact name from event.path.steps[0].key +export interface LinkedArtifact { + event: Event; + artifact: Artifact; +} - const inputArtifactIds = res - .getEventsList() - .filter(event => (!event_filter || event.getType() === event_filter) && event.getArtifactId()) +async function getLinkedArtifactsByEvents(events: Event[]): Promise { + const artifactIds = events + .filter(event => event.getArtifactId()) .map(event => event.getArtifactId()); - const artifactsRequest = new GetArtifactsByIDRequest(); - artifactsRequest.setArtifactIdsList(inputArtifactIds); + const artifactsRequest = new GetArtifactsByIDRequest().setArtifactIdsList(artifactIds); let artifactsRes: GetArtifactsByIDResponse; try { artifactsRes = await Api.getInstance().metadataStoreService.getArtifactsByID(artifactsRequest); @@ -211,21 +215,40 @@ async function getArtifactsInExecution( throw artifactsErr; } - return artifactsRes.getArtifactsList(); + const artifactMap = artifactsRes.getArtifactsList().reduce((map, artifact) => { + map[artifact.getId()] = artifact; + return map; + }, {}); + + return events.map(event => { + const artifact = artifactMap[event.getArtifactId()]; + return { event: event, artifact: artifact }; + }); } -/** - * @throws error when network error or invalid data - */ -export async function getOutputArtifactsInExecution(execution: Execution): Promise { - return getArtifactsInExecution(execution, Event.Type.OUTPUT); +export async function getLinkedArtifactsByExecution( + execution: Execution, +): Promise { + const event = await getEventByExecution(execution); + return getLinkedArtifactsByEvents(event); +} + +export function filterEventWithInputArtifact(linkedArtifact: LinkedArtifact[]) { + return linkedArtifact.filter(obj => obj.event.getType() === Event.Type.INPUT); +} + +export function filterEventWithOutputArtifact(linkedArtifact: LinkedArtifact[]) { + return linkedArtifact.filter(obj => obj.event.getType() === Event.Type.OUTPUT); } /** * @throws error when network error or invalid data */ -export async function getInputArtifactsInExecution(execution: Execution): Promise { - return getArtifactsInExecution(execution, Event.Type.INPUT); +export async function getOutputArtifactsInExecution(execution: Execution): Promise { + const linkedArtifacts = await getLinkedArtifactsByExecution(execution); + return filterEventWithOutputArtifact(linkedArtifacts).map( + linkedArtifact => linkedArtifact.artifact, + ); } export async function getArtifactTypes(): Promise { diff --git a/frontend/src/pages/RunDetails.tsx b/frontend/src/pages/RunDetails.tsx index 014b4f18be0..f9570cd055d 100644 --- a/frontend/src/pages/RunDetails.tsx +++ b/frontend/src/pages/RunDetails.tsx @@ -149,6 +149,7 @@ interface RunDetailsState { mlmdRunContext?: Context; mlmdExecutions?: Execution[]; showReducedGraph?: boolean; + namespace?: string; } export const css = stylesheet({ @@ -427,7 +428,10 @@ class RunDetails extends Page { {sidepanelSelectedTab === SidePaneTab.INPUT_OUTPUT && isV2Pipeline(workflow) && selectedExecution && ( - + )} {sidepanelSelectedTab === SidePaneTab.TASK_DETAILS && ( @@ -697,8 +701,10 @@ class RunDetails extends Page { const relatedExperimentId = RunUtils.getFirstExperimentReferenceId(runDetail.run); let experiment: ApiExperiment | undefined; + let namespace: string | undefined; if (relatedExperimentId) { experiment = await Apis.experimentServiceApi.getExperiment(relatedExperimentId); + namespace = RunUtils.getNamespaceReferenceName(experiment); } const runMetadata = runDetail.run!; @@ -832,6 +838,7 @@ class RunDetails extends Page { workflow, mlmdRunContext, mlmdExecutions, + namespace, }); } catch (err) { await this.showPageError(`Error: failed to retrieve run: ${runId}.`, err); From c88aa995a5ab83ea86902286600117ed55889f52 Mon Sep 17 00:00:00 2001 From: James Liu Date: Thu, 24 Jun 2021 12:20:37 -0700 Subject: [PATCH 5/6] comment and UX rework --- frontend/src/components/ArtifactPreview.tsx | 2 +- .../components/tabs/InputOutputTab.test.tsx | 11 +- .../src/components/tabs/InputOutputTab.tsx | 117 ++++++++++++------ frontend/src/lib/MetricUtils.ts | 4 +- frontend/src/lib/MlmdUtils.ts | 10 +- frontend/src/lib/RunUtils.ts | 3 +- frontend/src/lib/StaticGraphParser.ts | 2 +- 7 files changed, 94 insertions(+), 55 deletions(-) diff --git a/frontend/src/components/ArtifactPreview.tsx b/frontend/src/components/ArtifactPreview.tsx index 385dcfcf6a7..be39f604f69 100644 --- a/frontend/src/components/ArtifactPreview.tsx +++ b/frontend/src/components/ArtifactPreview.tsx @@ -68,7 +68,7 @@ const ArtifactPreview: React.FC = ({ } const { isSuccess, isError, data, error } = useQuery( - ['artifact_preview', value], + ['artifact_preview', { value, namespace, maxbytes, maxlines }], () => getPreview(storage, namespace, maxbytes, maxlines), { staleTime: Infinity }, ); diff --git a/frontend/src/components/tabs/InputOutputTab.test.tsx b/frontend/src/components/tabs/InputOutputTab.test.tsx index b7d097adf44..49997fa2039 100644 --- a/frontend/src/components/tabs/InputOutputTab.test.tsx +++ b/frontend/src/components/tabs/InputOutputTab.test.tsx @@ -58,7 +58,7 @@ describe('InoutOutputTab', () => { screen.getByText(executionName, { selector: 'a', exact: false }); }); - it('shows Input/Output artifacts and parameters title', () => { + it("doesn't show Input/Output artifacts and parameters if no exists", async () => { jest .spyOn(Api.getInstance().metadataStoreService, 'getEventsByExecutionIDs') .mockResolvedValue(new GetEventsByExecutionIDsResponse()); @@ -71,10 +71,11 @@ describe('InoutOutputTab', () => { , ); - screen.getByText('Input'); - screen.getByText('Output'); - expect(screen.getAllByText('Parameters').length).toEqual(2); - expect(screen.getAllByText('Artifacts').length).toEqual(2); + await waitFor(() => screen.queryAllByText('Input Parameters').length == 0); + await waitFor(() => screen.queryAllByText('Input Artifacts').length == 0); + await waitFor(() => screen.queryAllByText('Output Parameters').length == 0); + await waitFor(() => screen.queryAllByText('Output Artifacts').length == 0); + await waitFor(() => screen.getByText('There is no input/output parameter or artifact.')); }); it('shows Input parameters with various types', async () => { diff --git a/frontend/src/components/tabs/InputOutputTab.tsx b/frontend/src/components/tabs/InputOutputTab.tsx index b85dc5fe84b..c37f88127d0 100644 --- a/frontend/src/components/tabs/InputOutputTab.tsx +++ b/frontend/src/components/tabs/InputOutputTab.tsx @@ -18,6 +18,7 @@ import { Execution, getMetadataValue } from '@kubeflow/frontend'; import { Struct } from 'google-protobuf/google/protobuf/struct_pb'; import React from 'react'; import { useQuery } from 'react-query'; +import { Link } from 'react-router-dom'; import { ErrorBoundary } from 'src/atoms/ErrorBoundary'; import { commonCss, padding } from 'src/Css'; import { @@ -30,6 +31,7 @@ import { KeyValue } from 'src/lib/StaticGraphParser'; import ArtifactPreview from '../ArtifactPreview'; import Banner from '../Banner'; import DetailsTable from '../DetailsTable'; +import { RoutePageFactory } from '../Router'; import { ExecutionTitle } from './ExecutionTitle'; type ParamList = Array>; @@ -59,6 +61,16 @@ export function InputOutputTab({ execution, namespace }: IOTabProps) { outputArtifacts = getArtifactParamList(filterEventWithOutputArtifact(data)); } + let isIoEmpty = false; + if ( + inputParams.length === 0 && + outputParams.length === 0 && + inputArtifacts.length === 0 && + outputArtifacts.length === 0 + ) { + isIoEmpty = true; + } + return (
@@ -73,42 +85,57 @@ export function InputOutputTab({ execution, namespace }: IOTabProps) { /> )} -
{'Input'}
- - - - key={`input-artifacts-${executionId}`} - title='Artifacts' - fields={inputArtifacts} - valueComponent={ArtifactPreview} - valueComponentProps={{ - namespace: namespace, - }} - /> - -
-
- -
{'Output'}
- - - - key={`output-artifacts-${executionId}`} - title='Artifacts' - fields={outputArtifacts} - valueComponent={ArtifactPreview} - valueComponentProps={{ - namespace: namespace, - }} - /> + {isSuccess && isIoEmpty && ( + + )} + + {inputParams.length > 0 && ( +
+ +
+ )} + + {inputArtifacts.length > 0 && ( +
+ + key={`input-artifacts-${executionId}`} + title='Input Artifacts' + fields={inputArtifacts} + valueComponent={ArtifactPreview} + valueComponentProps={{ + namespace: namespace, + }} + /> +
+ )} + + {outputParams.length > 0 && ( +
+ +
+ )} + + {outputArtifacts.length > 0 && ( +
+ + key={`output-artifacts-${executionId}`} + title='Output Artifacts' + fields={outputArtifacts} + valueComponent={ArtifactPreview} + valueComponentProps={{ + namespace: namespace, + }} + /> +
+ )}
@@ -159,11 +186,19 @@ function prettyPrintValue(value: string | number | Struct | undefined): string { } function getArtifactParamList(inputArtifacts: LinkedArtifact[]): ParamList { - return inputArtifacts.map(linkedArtifact => [ - linkedArtifact.event + return inputArtifacts.map(linkedArtifact => { + const key = linkedArtifact.event .getPath() ?.getStepsList()[0] - .getKey(), - linkedArtifact.artifact.getUri(), - ]); + .getKey(); + const artifactId = linkedArtifact.artifact.getId(); + const artifactElement = RoutePageFactory.artifactDetails(artifactId) ? ( + + {key} + + ) : ( + key + ); + return [artifactElement, linkedArtifact.artifact.getUri()]; + }); } diff --git a/frontend/src/lib/MetricUtils.ts b/frontend/src/lib/MetricUtils.ts index dd36b4cd417..692631d7574 100644 --- a/frontend/src/lib/MetricUtils.ts +++ b/frontend/src/lib/MetricUtils.ts @@ -28,6 +28,8 @@ function getMetricDisplayString(metric?: ApiRunMetric, decimalPlaces = 3): strin return metric.number_value.toFixed(decimalPlaces); } -export default { +const MetricUtils = { getMetricDisplayString, }; + +export default MetricUtils; diff --git a/frontend/src/lib/MlmdUtils.ts b/frontend/src/lib/MlmdUtils.ts index baa39b75c14..b4b695b81cd 100644 --- a/frontend/src/lib/MlmdUtils.ts +++ b/frontend/src/lib/MlmdUtils.ts @@ -215,13 +215,13 @@ async function getLinkedArtifactsByEvents(events: Event[]): Promise { - map[artifact.getId()] = artifact; - return map; - }, {}); + const artifactMap = new Map(); + for (const [, artifactEntry] of Object.entries(artifactsRes.getArtifactsList())) { + artifactMap.set(artifactEntry.getId(), artifactEntry); + } return events.map(event => { - const artifact = artifactMap[event.getArtifactId()]; + const artifact = artifactMap.get(event.getArtifactId()); return { event: event, artifact: artifact }; }); } diff --git a/frontend/src/lib/RunUtils.ts b/frontend/src/lib/RunUtils.ts index 9bb9dba3161..7a72ea48fab 100644 --- a/frontend/src/lib/RunUtils.ts +++ b/frontend/src/lib/RunUtils.ts @@ -195,7 +195,7 @@ function getRecurringRunName(run?: ApiRun): string { } // TODO: This file needs tests -export default { +const RunUtils = { extractMetricMetadata, getAllExperimentReferences, getFirstExperimentReference, @@ -213,3 +213,4 @@ export default { getWorkflowManifest, runsToMetricMetadataMap, }; +export default RunUtils; diff --git a/frontend/src/lib/StaticGraphParser.ts b/frontend/src/lib/StaticGraphParser.ts index 143986abcb0..3b85434498e 100644 --- a/frontend/src/lib/StaticGraphParser.ts +++ b/frontend/src/lib/StaticGraphParser.ts @@ -25,7 +25,7 @@ import { graphlib } from 'dagre'; export type nodeType = 'container' | 'resource' | 'dag' | 'unknown'; export interface KeyValue extends Array { - 0?: string; + 0?: string | JSX.Element; 1?: T; } From 2d4c8c82928e77d7edb6d3edecd280c293faf48b Mon Sep 17 00:00:00 2001 From: James Liu Date: Thu, 24 Jun 2021 14:18:01 -0700 Subject: [PATCH 6/6] downloadable link --- frontend/src/components/ArtifactPreview.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frontend/src/components/ArtifactPreview.tsx b/frontend/src/components/ArtifactPreview.tsx index be39f604f69..332eb7e82bd 100644 --- a/frontend/src/components/ArtifactPreview.tsx +++ b/frontend/src/components/ArtifactPreview.tsx @@ -90,7 +90,7 @@ const ArtifactPreview: React.FC = ({ return (
- + {linkText}