diff --git a/extension/src/plots/errors/collect.test.ts b/extension/src/plots/errors/collect.test.ts index decfe6c12c..067a9fdcf9 100644 --- a/extension/src/plots/errors/collect.test.ts +++ b/extension/src/plots/errors/collect.test.ts @@ -1,9 +1,5 @@ import { join } from 'path' -import { - collectErrors, - collectImageErrors, - collectPathErrorsTable -} from './collect' +import { collectErrors, collectImageErrors, collectPathErrors } from './collect' import { EXPERIMENT_WORKSPACE_ID } from '../../cli/dvc/contract' describe('collectErrors', () => { @@ -14,10 +10,8 @@ describe('collectErrors', () => { [ { msg: 'unexpected error', - name: 'fun::plot', - rev: EXPERIMENT_WORKSPACE_ID, - source: 'metrics.json', - type: 'unexpected' + path: 'fun::plot', + rev: EXPERIMENT_WORKSPACE_ID } ], {} @@ -33,17 +27,13 @@ describe('collectErrors', () => { [ { msg: 'unexpected error', - name: 'fun::plot', - rev: EXPERIMENT_WORKSPACE_ID, - source: 'metrics.json', - type: 'unexpected' + path: 'fun::plot', + rev: EXPERIMENT_WORKSPACE_ID }, { msg: 'unexpected error', - name: 'fun::plot', - rev: 'main', - source: 'metrics.json', - type: 'unexpected' + path: 'fun::plot', + rev: 'main' } ], { [EXPERIMENT_WORKSPACE_ID]: EXPERIMENT_WORKSPACE_ID, ff2489c: 'main' } @@ -67,23 +57,21 @@ describe('collectErrors', () => { [ { msg: 'unexpected error', - name: 'fun::plot', - rev: EXPERIMENT_WORKSPACE_ID, - source: 'metrics.json', - type: 'unexpected' + path: 'fun::plot', + rev: EXPERIMENT_WORKSPACE_ID }, { msg: 'unexpected error', - name: 'fun::plot', - rev: 'main', - source: 'metrics.json', - type: 'unexpected' + path: 'fun::plot', + rev: 'main' } ], { [EXPERIMENT_WORKSPACE_ID]: EXPERIMENT_WORKSPACE_ID, ff2489c: 'main' } ) - expect(errors).toStrictEqual([{ ...newError, rev: 'main' }]) + expect(errors).toStrictEqual([ + { msg: 'new error', path: 'fun::plot', rev: 'main' } + ]) }) it('should collect new errors', () => { @@ -104,16 +92,16 @@ describe('collectErrors', () => { [ { msg: 'unexpected error', - name: 'fun::plot', - rev: EXPERIMENT_WORKSPACE_ID, - source: 'metrics.json', - type: 'unexpected' + path: 'fun::plot', + rev: EXPERIMENT_WORKSPACE_ID } ], { d7ad114: 'main' } ) - expect(errors).toStrictEqual([{ ...newError, rev: 'main' }]) + expect(errors).toStrictEqual([ + { msg: 'Blue screen of death', path: 'fun::plot', rev: 'main' } + ]) }) }) @@ -134,69 +122,57 @@ describe('collectImageErrors', () => { const errors = [ { msg: `${otherPath} - file type error\nOnly JSON, YAML, CSV and TSV formats are supported.`, - name: otherPath, - rev: EXPERIMENT_WORKSPACE_ID, - source: otherPath, - type: 'PlotMetricTypeError' + path: otherPath, + rev: EXPERIMENT_WORKSPACE_ID }, { msg: "Could not find provided field ('ste') in data fields ('step, test/acc').", - name: otherPath, - rev: EXPERIMENT_WORKSPACE_ID, - source: otherPath, - type: 'FieldNotFoundError' + path: otherPath, + rev: EXPERIMENT_WORKSPACE_ID }, { - msg: '', - name: path, - rev: EXPERIMENT_WORKSPACE_ID, - source: path, - type: 'FileNotFoundError' + msg: `${path} not found.`, + path, + rev: EXPERIMENT_WORKSPACE_ID }, { - msg: '', - name: path, - rev: 'main', - source: path, - type: 'FileNotFoundError' + msg: `${path} not found.`, + path, + rev: 'main' } ] const error = collectImageErrors(path, EXPERIMENT_WORKSPACE_ID, errors) - expect(error).toStrictEqual(`${path} not found.`) + expect(error).toStrictEqual([`${path} not found.`]) }) - it('should concatenate errors together to give a single string', () => { + it('should return an array of errors', () => { const path = join('training', 'plots', 'images', 'mispredicted.jpg') const errors = [ { - msg: '', - name: path, - rev: EXPERIMENT_WORKSPACE_ID, - source: path, - type: 'FileNotFoundError' + msg: `${path} not found.`, + path, + rev: EXPERIMENT_WORKSPACE_ID }, { msg: 'catastrophic error', - name: path, - rev: EXPERIMENT_WORKSPACE_ID, - source: path, - type: 'SomeError' + path, + rev: EXPERIMENT_WORKSPACE_ID }, { - msg: '', - name: path, - rev: EXPERIMENT_WORKSPACE_ID, - source: path, - type: 'UNEXPECTEDERRRRROR' + msg: 'UNEXPECTEDERRRRROR', + path, + rev: EXPERIMENT_WORKSPACE_ID } ] const error = collectImageErrors(path, EXPERIMENT_WORKSPACE_ID, errors) - expect(error).toStrictEqual( - `${path} not found.\ncatastrophic error\nUNEXPECTEDERRRRROR` - ) + expect(error).toStrictEqual([ + `${path} not found.`, + 'catastrophic error', + 'UNEXPECTEDERRRRROR' + ]) }) }) @@ -204,30 +180,24 @@ describe('collectPathErrorsTable', () => { it('should return undefined if the errors do not relate to selected revisions', () => { const rev = 'main' const path = 'wat' - const markdownTable = collectPathErrorsTable( + const markdownTable = collectPathErrors( path, [EXPERIMENT_WORKSPACE_ID], [ { - msg: '', - name: path, - rev, - source: path, - type: 'FileNotFoundError' + msg: `${path} not found.`, + path, + rev }, { msg: 'catastrophic error', - name: path, - rev, - source: path, - type: 'SomeError' + path, + rev }, { - msg: '', - name: path, - rev, - source: path, - type: 'UNEXPECTEDERRRRROR' + msg: 'UNEXPECTEDERRRRROR', + path, + rev } ] ) @@ -237,101 +207,91 @@ describe('collectPathErrorsTable', () => { it('should return undefined if the errors do not relate to the path', () => { const rev = 'main' const path = 'wat' - const markdownTable = collectPathErrorsTable( + const markdownTable = collectPathErrors( join('other', 'path'), [rev], [ { - msg: '', - name: path, - rev, - source: path, - type: 'FileNotFoundError' + msg: `${path} not found.`, + path, + rev }, { msg: 'catastrophic error', - name: path, - rev, - source: path, - type: 'SomeError' + path, + rev }, { - msg: '', - name: path, - rev, - source: path, - type: 'UNEXPECTEDERRRRROR' + msg: 'UNEXPECTEDERRRRROR', + path, + rev } ] ) expect(markdownTable).toBeUndefined() }) - it('should construct a markdown table with the error if they relate to the select revision and provided path', () => { + it('should return an array of objects containing error messages that relate to the select revision and provided path', () => { const rev = 'a-really-long-branch-name' const path = 'wat' - const markdownTable = collectPathErrorsTable( + const markdownTable = collectPathErrors( path, [EXPERIMENT_WORKSPACE_ID, rev], [ { - msg: '', - name: path, - rev: EXPERIMENT_WORKSPACE_ID, - source: path, - type: 'FileNotFoundError' + msg: `${path} not found.`, + path, + rev: EXPERIMENT_WORKSPACE_ID }, { msg: 'catastrophic error', - name: path, - rev, - source: path, - type: 'SomeError' + path, + rev + }, + { + msg: 'UNEXPECTEDERRRRROR', + path, + rev }, { - msg: '', - name: path, - rev, - source: path, - type: 'UNEXPECTEDERRRRROR' + msg: 'other path not found.', + path: 'other path', + rev: EXPERIMENT_WORKSPACE_ID } ] ) - expect(markdownTable).toStrictEqual( - 'Errors\n' + - '|||\n' + - '|--|--|\n' + - '| a-really... | UNEXPECTEDERRRRROR |\n' + - '| a-really... | catastrophic error |\n' + - '| workspace | wat not found. |' - ) + expect(markdownTable).toStrictEqual([ + { msg: 'wat not found.', rev: EXPERIMENT_WORKSPACE_ID }, + { msg: 'catastrophic error', rev }, + { msg: 'UNEXPECTEDERRRRROR', rev } + ]) }) it('should not duplicate entries in the table', () => { - const name = 'dvc.yaml::Accuracy' + const path = 'dvc.yaml::Accuracy' const msg = "Could not find provided field ('acc_') in data fields ('step, acc')." const type = 'FieldNotFoundError' const duplicateEntry = { msg, - name, + path, rev: 'aa1401b', type } - const markdownTable = collectPathErrorsTable( + const markdownTable = collectPathErrors( 'dvc.yaml::Accuracy', [EXPERIMENT_WORKSPACE_ID, 'main', 'test-plots-diff', 'aa1401b'], [ { msg, - name, + path, rev: 'workspace', type }, { msg, - name, + path, rev: 'test-plots-diff', type }, @@ -340,13 +300,16 @@ describe('collectPathErrorsTable', () => { ] ) - expect(markdownTable).toStrictEqual( - 'Errors\n' + - '|||\n' + - '|--|--|\n' + - "| aa1401b | Could not find provided field ('acc_') in data fields ('step, acc'). |\n" + - "| test-plo... | Could not find provided field ('acc_') in data fields ('step, acc'). |\n" + - "| workspace | Could not find provided field ('acc_') in data fields ('step, acc'). |" - ) + expect(markdownTable).toStrictEqual([ + { + msg, + rev: EXPERIMENT_WORKSPACE_ID + }, + { + msg, + rev: 'test-plots-diff' + }, + { msg, rev: 'aa1401b' } + ]) }) }) diff --git a/extension/src/plots/errors/collect.ts b/extension/src/plots/errors/collect.ts index 2df6323d5f..f173285a90 100644 --- a/extension/src/plots/errors/collect.ts +++ b/extension/src/plots/errors/collect.ts @@ -1,16 +1,26 @@ -import { - EXPERIMENT_WORKSPACE_ID, - PlotError, - PlotsOutput -} from '../../cli/dvc/contract' -import { truncate } from '../../util/string' +import { PlotError, PlotsOutput } from '../../cli/dvc/contract' + +export type Error = { path: string; rev: string; msg: string } + +const getMessage = (error: PlotError): string => { + const { msg, type, source } = error + + if (msg) { + return msg + } + + if (type === 'FileNotFoundError' && source && !msg) { + return `${source} not found.` + } + return type +} export const collectErrors = ( data: PlotsOutput, revs: string[], - errors: PlotError[], + errors: Error[], cliIdToLabel: { [id: string]: string } -) => { +): Error[] => { const fetchedRevs = new Set( revs.map((rev: string) => cliIdToLabel[rev] || rev) ) @@ -21,77 +31,67 @@ export const collectErrors = ( return [ ...existingErrors, ...newErrors.map(error => { - const { rev } = error + const { rev, name } = error return { - ...error, + msg: getMessage(error), + path: name, rev: cliIdToLabel[rev] || rev } }) ] } -export const getMessage = (error: PlotError): string => { - const { msg, type, source } = error - - if (msg) { - return msg - } - - if (type === 'FileNotFoundError' && source && !msg) { - return `${source} not found.` - } - return type -} - export const collectImageErrors = ( path: string, revision: string, - errors: PlotError[] -): string | undefined => { - const msgs: string[] = [] + errors: Error[] +): string[] | undefined => { + const acc: string[] = [] for (const error of errors) { - if (error.rev === revision && error.name === path) { - const msg = getMessage(error) - msgs.push(msg) + if (error.rev === revision && error.path === path) { + acc.push(error.msg) } } - if (msgs.length === 0) { + if (acc.length === 0) { return undefined } - return msgs.join('\n') + return acc } -const formatError = (acc: string[]): string | undefined => { - if (acc.length === 0) { - return - } - - acc.sort() - acc.unshift('Errors\n|||\n|--|--|') - - return acc.join('\n') -} +const isDuplicateError = ( + acc: { rev: string; msg: string }[], + rev: string, + msg: string +) => + acc.some( + ({ rev: existingRev, msg: existingMsg }) => + rev === existingRev && msg === existingMsg + ) -export const collectPathErrorsTable = ( +export const collectPathErrors = ( path: string, selectedRevisions: string[], - errors: PlotError[] -): string | undefined => { - const acc = new Set() + errors: Error[] +): { rev: string; msg: string }[] | undefined => { + const acc: { rev: string; msg: string }[] = [] for (const error of errors) { - const { name, rev } = error - if (name !== path || !selectedRevisions.includes(rev)) { + const { msg, rev } = error + if ( + error.path !== path || + !selectedRevisions.includes(rev) || + isDuplicateError(acc, rev, msg) + ) { continue } - const row = `| ${truncate( - rev, - EXPERIMENT_WORKSPACE_ID.length - )} | ${getMessage(error)} |` + acc.push({ msg, rev }) + } - acc.add(row) + if (acc.length === 0) { + return undefined } - return formatError([...acc]) + + return acc } diff --git a/extension/src/plots/errors/model.ts b/extension/src/plots/errors/model.ts index 11cb678652..cdb7f5c713 100644 --- a/extension/src/plots/errors/model.ts +++ b/extension/src/plots/errors/model.ts @@ -2,18 +2,18 @@ import { join } from 'path' import { collectErrors, collectImageErrors, - collectPathErrorsTable, - getMessage + collectPathErrors, + Error } from './collect' import { Disposable } from '../../class/dispose' -import { DvcError, PlotError, PlotsOutputOrError } from '../../cli/dvc/contract' +import { DvcError, PlotsOutputOrError } from '../../cli/dvc/contract' import { isDvcError } from '../../cli/dvc/reader' import { getCliErrorLabel } from '../../tree' export class ErrorsModel extends Disposable { private readonly dvcRoot: string - private errors: PlotError[] = [] + private errors: Error[] = [] private cliError: { error: string; path: string } | undefined constructor(dvcRoot: string) { @@ -39,7 +39,7 @@ export class ErrorsModel extends Disposable { } public getPathErrors(path: string, selectedRevisions: string[]) { - return collectPathErrorsTable(path, selectedRevisions, this.errors) + return collectPathErrors(path, selectedRevisions, this.errors) } public getErrorPaths(selectedRevisions: string[]) { @@ -48,22 +48,26 @@ export class ErrorsModel extends Disposable { } const acc = new Set() - for (const { name, rev } of this.errors) { + for (const { path, rev } of this.errors) { if (selectedRevisions.includes(rev)) { - acc.add(join(this.dvcRoot, name)) + acc.add(join(this.dvcRoot, path)) } } return acc } public getRevisionErrors(rev: string) { - const errors: string[] = [] + const acc = new Set() for (const error of this.errors) { if (error.rev === rev) { - errors.push(getMessage(error)) + acc.add(error.msg) } } - return errors.length > 0 ? errors : undefined + if (acc.size === 0) { + return undefined + } + + return [...acc] } public hasCliError() { diff --git a/extension/src/plots/model/index.ts b/extension/src/plots/model/index.ts index 45e09f0bea..4ec38349b8 100644 --- a/extension/src/plots/model/index.ts +++ b/extension/src/plots/model/index.ts @@ -515,12 +515,12 @@ export class PlotsModel extends ModelWithPersistence { for (const revision of selectedRevisions) { const image = this.comparisonData?.[revision]?.[path] - const error = this.errors.getImageErrors(path, revision) + const errors = this.errors.getImageErrors(path, revision) const fetched = this.fetchedRevs.has(revision) const url = collectImageUrl(image, fetched) const loading = !fetched && !url pathRevisions.revisions[revision] = { - error, + errors, loading, revision, url diff --git a/extension/src/plots/paths/collect.test.ts b/extension/src/plots/paths/collect.test.ts index a42a44c819..d54c2ac47c 100644 --- a/extension/src/plots/paths/collect.test.ts +++ b/extension/src/plots/paths/collect.test.ts @@ -3,6 +3,7 @@ import { VisualizationSpec } from 'react-vega' import isEqual from 'lodash.isequal' import { collectEncodingElements, + collectPathErrorsTable, collectPaths, collectTemplateOrder, EncodingType, @@ -594,3 +595,32 @@ describe('collectEncodingElements', () => { ]) }) }) + +describe('collectPathErrorsTable', () => { + it('should construct a markdown table with the error if they relate to the select revision and provided path', () => { + const rev = 'a-really-long-branch-name' + const path = 'wat' + const markdownTable = collectPathErrorsTable([ + { + msg: `${path} not found.`, + rev: EXPERIMENT_WORKSPACE_ID + }, + { + msg: 'catastrophic error', + rev + }, + { + msg: 'UNEXPECTEDERRRRROR', + rev + } + ]) + expect(markdownTable).toStrictEqual( + 'Errors\n' + + '|||\n' + + '|--|--|\n' + + '| a-really... | UNEXPECTEDERRRRROR |\n' + + '| a-really... | catastrophic error |\n' + + '| workspace | wat not found. |' + ) + }) +}) diff --git a/extension/src/plots/paths/collect.ts b/extension/src/plots/paths/collect.ts index 3bb1e868bd..997c12a480 100644 --- a/extension/src/plots/paths/collect.ts +++ b/extension/src/plots/paths/collect.ts @@ -5,7 +5,12 @@ import { TemplatePlot, TemplatePlotGroup } from '../webview/contract' -import { PlotError, PlotsData, PlotsOutput } from '../../cli/dvc/contract' +import { + EXPERIMENT_WORKSPACE_ID, + PlotError, + PlotsData, + PlotsOutput +} from '../../cli/dvc/contract' import { getParent, getPath, getPathArray } from '../../fileSystem/util' import { splitMatchedOrdered, definedAndNonEmpty } from '../../util/array' import { isMultiViewPlot } from '../vega/util' @@ -18,6 +23,7 @@ import { } from '../multiSource/constants' import { MultiSourceEncoding } from '../multiSource/collect' import { CLIRevisionIdToLabel } from '../model/collect' +import { truncate } from '../../util/string' export enum PathType { COMPARISON = 'comparison', @@ -465,3 +471,28 @@ export const collectEncodingElements = ( return acc } + +const formatError = (acc: string[]): string | undefined => { + if (acc.length === 0) { + return + } + + acc.sort() + acc.unshift('Errors\n|||\n|--|--|') + + return acc.join('\n') +} + +export const collectPathErrorsTable = ( + errors: { rev: string; msg: string }[] +): string | undefined => { + const acc = new Set() + for (const error of errors) { + const { msg, rev } = error + + const row = `| ${truncate(rev, EXPERIMENT_WORKSPACE_ID.length)} | ${msg} |` + + acc.add(row) + } + return formatError([...acc]) +} diff --git a/extension/src/plots/paths/model.ts b/extension/src/plots/paths/model.ts index 5b546e1802..388ff312ec 100644 --- a/extension/src/plots/paths/model.ts +++ b/extension/src/plots/paths/model.ts @@ -1,5 +1,6 @@ import { Memento } from 'vscode' import { + collectPathErrorsTable, collectPaths, collectTemplateOrder, PathType, @@ -199,7 +200,13 @@ export class PathsModel extends PathSelectionModel { } private getTooltip(path: string) { - const error = this.errors.getPathErrors(path, this.selectedRevisions) + const errors = this.errors.getPathErrors(path, this.selectedRevisions) + + if (!errors?.length) { + return + } + + const error = collectPathErrorsTable(errors) return error ? getErrorTooltip(error) : undefined } } diff --git a/extension/src/plots/webview/contract.ts b/extension/src/plots/webview/contract.ts index f89c45813b..6ef4b9971b 100644 --- a/extension/src/plots/webview/contract.ts +++ b/extension/src/plots/webview/contract.ts @@ -173,7 +173,7 @@ export interface TemplatePlotsData { export type ComparisonPlot = { url: string | undefined revision: string - error: string | undefined + errors: string[] | undefined loading: boolean } diff --git a/extension/src/test/fixtures/plotsDiff/index.ts b/extension/src/test/fixtures/plotsDiff/index.ts index 5c224294f5..dbc75bddc6 100644 --- a/extension/src/test/fixtures/plotsDiff/index.ts +++ b/extension/src/test/fixtures/plotsDiff/index.ts @@ -705,7 +705,7 @@ export const getComparisonWebviewMessage = ( revisionsAcc[revision] = { url: `${url}?${MOCK_IMAGE_MTIME}`, revision, - error: undefined, + errors: undefined, loading: false } } diff --git a/webview/src/plots/components/App.test.tsx b/webview/src/plots/components/App.test.tsx index 30f435186f..974727f81e 100644 --- a/webview/src/plots/components/App.test.tsx +++ b/webview/src/plots/components/App.test.tsx @@ -249,7 +249,7 @@ describe('App', () => { path: 'training/plots/images/misclassified.jpg', revisions: { ad2b5ec: { - error: undefined, + errors: undefined, loading: true, revision: 'ad2b5ec', url: undefined diff --git a/webview/src/plots/components/comparisonTable/ComparisonTable.test.tsx b/webview/src/plots/components/comparisonTable/ComparisonTable.test.tsx index b5061d923b..a7d2a73cec 100644 --- a/webview/src/plots/components/comparisonTable/ComparisonTable.test.tsx +++ b/webview/src/plots/components/comparisonTable/ComparisonTable.test.tsx @@ -273,7 +273,7 @@ describe('ComparisonTable', () => { revisions: { ...revisions, [revisionWithNoData]: { - error: undefined, + errors: undefined, loading: false, revision: revisionWithNoData, url: undefined @@ -306,7 +306,7 @@ describe('ComparisonTable', () => { revisions: { ...revisions, [revisionWithNoData]: { - error: 'this is an error', + errors: ['this is an error'], loading: false, revision: revisionWithNoData, url: undefined diff --git a/webview/src/plots/components/comparisonTable/ComparisonTableCell.tsx b/webview/src/plots/components/comparisonTable/ComparisonTableCell.tsx index e2a9ab0a34..ae1c78fc77 100644 --- a/webview/src/plots/components/comparisonTable/ComparisonTableCell.tsx +++ b/webview/src/plots/components/comparisonTable/ComparisonTableCell.tsx @@ -13,9 +13,9 @@ type ComparisonTableCellProps = { const MissingPlotTableCell: React.FC<{ plot: ComparisonPlot }> = ({ plot }) => (
- {plot.error ? ( + {plot.errors?.length ? ( <> - +
diff --git a/webview/src/stories/ComparisonTable.stories.tsx b/webview/src/stories/ComparisonTable.stories.tsx index bafce1f85e..bf5806457b 100644 --- a/webview/src/stories/ComparisonTable.stories.tsx +++ b/webview/src/stories/ComparisonTable.stories.tsx @@ -83,9 +83,9 @@ const removeImages = ( revision === EXPERIMENT_WORKSPACE_ID ) { filteredRevisionData[revision] = { - error: + errors: revision === 'main' - ? `FileNotFoundError: ${path} not found.` + ? [`FileNotFoundError: ${path} not found.`] : undefined, loading: false, revision,