From ad944f271ad3ff9dde1fdd939a7b59a5c49a0944 Mon Sep 17 00:00:00 2001 From: Matt Seddon <37993418+mattseddon@users.noreply.github.com> Date: Sun, 2 Apr 2023 18:21:53 +1000 Subject: [PATCH] Show errors in plots ribbon (#3570) --- extension/src/plots/errors/collect.test.ts | 14 ++--- extension/src/plots/errors/collect.ts | 10 +++- extension/src/plots/errors/model.ts | 13 +++- extension/src/plots/model/collect.test.ts | 59 +++++++++++++++++++ extension/src/plots/model/collect.ts | 35 +++++++---- extension/src/plots/model/index.test.ts | 5 +- extension/src/plots/model/index.ts | 2 + extension/src/plots/webview/contract.ts | 9 +-- .../src/test/fixtures/plotsDiff/index.ts | 5 ++ webview/src/plots/components/App.test.tsx | 35 +++++++++++ .../plots/components/ribbon/RibbonBlock.tsx | 13 +++- .../components/ribbon/RibbonBlockTooltip.tsx | 16 +++-- .../components/ribbon/styles.module.scss | 9 ++- .../components/tooltip/ErrorTooltip.tsx | 16 +++-- webview/src/stories/Ribbon.stories.tsx | 21 +++++++ 15 files changed, 223 insertions(+), 39 deletions(-) diff --git a/extension/src/plots/errors/collect.test.ts b/extension/src/plots/errors/collect.test.ts index dfa9e2bc05..decfe6c12c 100644 --- a/extension/src/plots/errors/collect.test.ts +++ b/extension/src/plots/errors/collect.test.ts @@ -163,7 +163,7 @@ describe('collectImageErrors', () => { ] const error = collectImageErrors(path, EXPERIMENT_WORKSPACE_ID, errors) - expect(error).toStrictEqual(`FileNotFoundError: ${path} not found.`) + expect(error).toStrictEqual(`${path} not found.`) }) it('should concatenate errors together to give a single string', () => { @@ -195,7 +195,7 @@ describe('collectImageErrors', () => { const error = collectImageErrors(path, EXPERIMENT_WORKSPACE_ID, errors) expect(error).toStrictEqual( - `FileNotFoundError: ${path} not found.\nSomeError: catastrophic error\nUNEXPECTEDERRRRROR` + `${path} not found.\ncatastrophic error\nUNEXPECTEDERRRRROR` ) }) }) @@ -301,9 +301,9 @@ describe('collectPathErrorsTable', () => { 'Errors\n' + '|||\n' + '|--|--|\n' + - '| a-really... | SomeError: catastrophic error |\n' + '| a-really... | UNEXPECTEDERRRRROR |\n' + - '| workspace | FileNotFoundError: wat not found. |' + '| a-really... | catastrophic error |\n' + + '| workspace | wat not found. |' ) }) @@ -344,9 +344,9 @@ describe('collectPathErrorsTable', () => { 'Errors\n' + '|||\n' + '|--|--|\n' + - "| aa1401b | FieldNotFoundError: Could not find provided field ('acc_') in data fields ('step, acc'). |\n" + - "| test-plo... | FieldNotFoundError: Could not find provided field ('acc_') in data fields ('step, acc'). |\n" + - "| workspace | FieldNotFoundError: Could not find provided field ('acc_') in data fields ('step, acc'). |" + "| 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'). |" ) }) }) diff --git a/extension/src/plots/errors/collect.ts b/extension/src/plots/errors/collect.ts index 8b61863b59..2df6323d5f 100644 --- a/extension/src/plots/errors/collect.ts +++ b/extension/src/plots/errors/collect.ts @@ -30,13 +30,17 @@ export const collectErrors = ( ] } -const getMessage = (error: PlotError): string => { +export const getMessage = (error: PlotError): string => { const { msg, type, source } = error + if (msg) { + return msg + } + if (type === 'FileNotFoundError' && source && !msg) { - return `${type}: ${source} not found.` + return `${source} not found.` } - return [type, msg].filter(Boolean).join(': ') + return type } export const collectImageErrors = ( diff --git a/extension/src/plots/errors/model.ts b/extension/src/plots/errors/model.ts index 9aa588a9ac..11cb678652 100644 --- a/extension/src/plots/errors/model.ts +++ b/extension/src/plots/errors/model.ts @@ -2,7 +2,8 @@ import { join } from 'path' import { collectErrors, collectImageErrors, - collectPathErrorsTable + collectPathErrorsTable, + getMessage } from './collect' import { Disposable } from '../../class/dispose' import { DvcError, PlotError, PlotsOutputOrError } from '../../cli/dvc/contract' @@ -55,6 +56,16 @@ export class ErrorsModel extends Disposable { return acc } + public getRevisionErrors(rev: string) { + const errors: string[] = [] + for (const error of this.errors) { + if (error.rev === rev) { + errors.push(getMessage(error)) + } + } + return errors.length > 0 ? errors : undefined + } + public hasCliError() { return !!this.getCliError() } diff --git a/extension/src/plots/model/collect.test.ts b/extension/src/plots/model/collect.test.ts index e945f19c46..888395423f 100644 --- a/extension/src/plots/model/collect.test.ts +++ b/extension/src/plots/model/collect.test.ts @@ -259,6 +259,7 @@ describe('collectOverrideRevisionDetails', () => { } ] as Experiment[] }[id]), + () => undefined, [] ) expect(overrideComparison.map(({ revision }) => revision)).toStrictEqual([ @@ -270,6 +271,7 @@ describe('collectOverrideRevisionDetails', () => { expect(overrideRevisions).toStrictEqual([ { displayColor: '#4299e1', + errors: undefined, fetched: true, firstThreeColumns: [], group: 'a', @@ -278,6 +280,7 @@ describe('collectOverrideRevisionDetails', () => { }, { displayColor: '#13adc7', + errors: undefined, fetched: true, firstThreeColumns: [], group: runningGroup, @@ -286,6 +289,7 @@ describe('collectOverrideRevisionDetails', () => { }, { displayColor: '#48bb78', + errors: undefined, fetched: true, firstThreeColumns: [], group: 'c', @@ -295,6 +299,7 @@ describe('collectOverrideRevisionDetails', () => { { commit: 'Upgrade dependencies ...', displayColor: '#f56565', + errors: undefined, fetched: true, firstThreeColumns: [], group: 'd', @@ -373,6 +378,7 @@ describe('collectOverrideRevisionDetails', () => { } ] as Experiment[] }[id]), + () => undefined, [] ) expect(overrideComparison.map(({ revision }) => revision)).toStrictEqual([ @@ -384,6 +390,7 @@ describe('collectOverrideRevisionDetails', () => { expect(overrideRevisions).toStrictEqual([ { displayColor: '#4299e1', + errors: undefined, fetched: false, firstThreeColumns: [], group: 'a', @@ -392,6 +399,7 @@ describe('collectOverrideRevisionDetails', () => { }, { displayColor: '#13adc7', + errors: undefined, fetched: false, firstThreeColumns: [], group: undefined, @@ -400,6 +408,7 @@ describe('collectOverrideRevisionDetails', () => { }, { displayColor: '#48bb78', + errors: undefined, fetched: false, firstThreeColumns: [], group: 'c', @@ -408,6 +417,7 @@ describe('collectOverrideRevisionDetails', () => { }, { displayColor: '#f56565', + errors: undefined, fetched: false, firstThreeColumns: [], group: 'd', @@ -486,6 +496,7 @@ describe('collectOverrideRevisionDetails', () => { } ] as Experiment[] }[id]), + () => undefined, [] ) expect(overrideComparison.map(({ revision }) => revision)).toStrictEqual([ @@ -525,6 +536,7 @@ describe('collectOverrideRevisionDetails', () => { { [justFinishedRunningId]: justFinishedRunningId }, (id: string) => ({ [justFinishedRunningId]: [{ label: 'e' }] as Experiment[] }[id]), + () => undefined, [] ) expect(overrideComparison.map(({ revision }) => revision)).toStrictEqual([ @@ -540,6 +552,53 @@ describe('collectOverrideRevisionDetails', () => { 'd' ]) }) + + it('should override the revision errors for finished but unfetched checkpoint tips (based on available data)', () => { + const justFinishedRunningId = 'exp-was-running' + const errors = ["b's error"] + const { overrideComparison, overrideRevisions } = + collectOverrideRevisionDetails( + ['a', 'b', 'c', 'd'], + [ + { label: 'a' }, + { + checkpoint_tip: 'b', + displayColor: '#13adc7', + id: justFinishedRunningId, + label: 'b', + sha: 'b', + status: ExperimentStatus.SUCCESS + }, + { label: 'c' }, + { label: 'd' } + ] as SelectedExperimentWithColor[], + new Set([]), + new Set(['a', 'c', 'd', 'e']), + { [justFinishedRunningId]: justFinishedRunningId }, + (id: string) => + ({ [justFinishedRunningId]: [{ label: 'e' }] as Experiment[] }[id]), + (label: string) => { + if (label === 'b') { + return errors + } + }, + [] + ) + expect(overrideComparison.map(({ revision }) => revision)).toStrictEqual([ + 'a', + 'e', + 'c', + 'd' + ]) + expect( + overrideRevisions.map(({ revision, errors }) => ({ errors, revision })) + ).toStrictEqual([ + { errors: undefined, revision: 'a' }, + { errors, revision: 'e' }, + { errors: undefined, revision: 'c' }, + { errors: undefined, revision: 'd' } + ]) + }) }) describe('collectOrderedRevisions', () => { diff --git a/extension/src/plots/model/collect.ts b/extension/src/plots/model/collect.ts index 096d197ed6..7366dbae02 100644 --- a/extension/src/plots/model/collect.ts +++ b/extension/src/plots/model/collect.ts @@ -555,12 +555,14 @@ const getOverrideRevision = ( displayColor: Color, experiment: Experiment, firstThreeColumns: string[], - fetchedRevs: Set + fetchedRevs: Set, + errors: string[] | undefined ): Revision => { const { commit, displayNameOrParent, logicalGroupName, id, label } = experiment const revision: Revision = { displayColor, + errors, fetched: fetchedRevs.has(label), firstThreeColumns: getRevisionFirstThreeColumns( firstThreeColumns, @@ -582,7 +584,8 @@ const overrideWithWorkspace = ( displayColor: Color, label: string, firstThreeColumns: string[], - fetchedRevs: Set + fetchedRevs: Set, + errors: string[] | undefined ): void => { orderMapping[label] = EXPERIMENT_WORKSPACE_ID selectedWithOverrides.push( @@ -594,7 +597,8 @@ const overrideWithWorkspace = ( logicalGroupName: undefined }, firstThreeColumns, - fetchedRevs + fetchedRevs, + errors ) ) } @@ -615,7 +619,8 @@ const getMostRecentFetchedCheckpointRevision = ( fetchedRevs: Set, revisionsWithData: Set, checkpoints: Experiment[] | undefined, - firstThreeColumns: string[] + firstThreeColumns: string[], + errors: string[] | undefined ): Revision => { const mostRecent = checkpoints?.find(({ label }) => revisionsWithData.has(label)) || @@ -624,7 +629,8 @@ const getMostRecentFetchedCheckpointRevision = ( selectedRevision.displayColor, mostRecent, firstThreeColumns, - fetchedRevs + fetchedRevs, + errors ) } @@ -635,7 +641,8 @@ const overrideRevisionDetail = ( fetchedRevs: Set, revisionsWithData: Set, checkpoints: Experiment[] | undefined, - firstThreeColumns: string[] + firstThreeColumns: string[], + errors: string[] | undefined ) => { const { label } = selectedRevision @@ -644,7 +651,8 @@ const overrideRevisionDetail = ( fetchedRevs, revisionsWithData, checkpoints, - firstThreeColumns + firstThreeColumns, + errors ) orderMapping[label] = mostRecent.revision selectedWithOverrides.push(mostRecent) @@ -658,9 +666,11 @@ const collectRevisionDetail = ( revisionsWithData: Set, unfinishedRunningExperiments: { [id: string]: string }, getCheckpoints: (id: string) => Experiment[] | undefined, + getErrors: (label: string) => string[] | undefined, firstThreeColumns: string[] ) => { const { label, status, id, displayColor } = selectedRevision + const errors = getErrors(label) if ( !fetchedRevs.has(label) && @@ -672,7 +682,8 @@ const collectRevisionDetail = ( displayColor, label, firstThreeColumns, - fetchedRevs + fetchedRevs, + errors ) } @@ -691,7 +702,8 @@ const collectRevisionDetail = ( fetchedRevs, revisionsWithData, getCheckpoints(id), - firstThreeColumns + firstThreeColumns, + errors ) } @@ -701,7 +713,8 @@ const collectRevisionDetail = ( displayColor, selectedRevision, firstThreeColumns, - fetchedRevs + fetchedRevs, + errors ) ) } @@ -713,6 +726,7 @@ export const collectOverrideRevisionDetails = ( revisionsWithData: Set, unfinishedRunningExperiments: { [id: string]: string }, getCheckpoints: (id: string) => Experiment[] | undefined, + getErrors: (label: string) => string[] | undefined, firstThreeColumns: string[] ): { overrideComparison: Revision[] @@ -730,6 +744,7 @@ export const collectOverrideRevisionDetails = ( revisionsWithData, unfinishedRunningExperiments, getCheckpoints, + getErrors, firstThreeColumns ) } diff --git a/extension/src/plots/model/index.test.ts b/extension/src/plots/model/index.test.ts index 41595f1990..e15bc3cab5 100644 --- a/extension/src/plots/model/index.test.ts +++ b/extension/src/plots/model/index.test.ts @@ -42,7 +42,10 @@ describe('plotsModel', () => { getSelectedRevisions: mockedGetSelectedRevisions, isReady: () => Promise.resolve(undefined) } as unknown as Experiments, - { getImageErrors: () => undefined } as unknown as ErrorsModel, + { + getImageErrors: () => undefined, + getRevisionErrors: () => undefined + } as unknown as ErrorsModel, memento ) jest.clearAllMocks() diff --git a/extension/src/plots/model/index.ts b/extension/src/plots/model/index.ts index 5b2575dc8c..636700422c 100644 --- a/extension/src/plots/model/index.ts +++ b/extension/src/plots/model/index.ts @@ -223,6 +223,7 @@ export class PlotsModel extends ModelWithPersistence { ]), finishedExperiments, id => this.experiments.getCheckpoints(id), + label => this.errors.getRevisionErrors(label), this.experiments.getFirstThreeColumnOrder() ) } @@ -249,6 +250,7 @@ export class PlotsModel extends ModelWithPersistence { } = exp const revision: Revision = { displayColor, + errors: this.errors.getRevisionErrors(label), fetched: this.fetchedRevs.has(label), firstThreeColumns: getRevisionFirstThreeColumns( this.experiments.getFirstThreeColumnOrder(), diff --git a/extension/src/plots/webview/contract.ts b/extension/src/plots/webview/contract.ts index 90574c578d..f89c45813b 100644 --- a/extension/src/plots/webview/contract.ts +++ b/extension/src/plots/webview/contract.ts @@ -56,13 +56,14 @@ export type RevisionFirstThreeColumns = Array<{ }> export type Revision = { - id?: string - revision: string - group?: string + commit?: string displayColor: Color + errors?: string[] fetched: boolean - commit?: string firstThreeColumns: RevisionFirstThreeColumns + group?: string + id?: string + revision: string } export interface PlotsComparisonData { diff --git a/extension/src/test/fixtures/plotsDiff/index.ts b/extension/src/test/fixtures/plotsDiff/index.ts index b415576826..5c224294f5 100644 --- a/extension/src/test/fixtures/plotsDiff/index.ts +++ b/extension/src/test/fixtures/plotsDiff/index.ts @@ -540,6 +540,7 @@ export const getRevisions = (): Revision[] => { id: EXPERIMENT_WORKSPACE_ID, revision: EXPERIMENT_WORKSPACE_ID, displayColor: workspace, + errors: undefined, fetched: true, firstThreeColumns: [ { @@ -561,6 +562,7 @@ export const getRevisions = (): Revision[] => { group: undefined }, { + errors: undefined, fetched: true, firstThreeColumns: [ { @@ -585,6 +587,7 @@ export const getRevisions = (): Revision[] => { group: undefined }, { + errors: undefined, fetched: true, firstThreeColumns: [ { @@ -609,6 +612,7 @@ export const getRevisions = (): Revision[] => { group: '[exp-e7a67]' }, { + errors: undefined, fetched: true, firstThreeColumns: [ { @@ -633,6 +637,7 @@ export const getRevisions = (): Revision[] => { group: '[test-branch]' }, { + errors: undefined, fetched: true, firstThreeColumns: [ { diff --git a/webview/src/plots/components/App.test.tsx b/webview/src/plots/components/App.test.tsx index 7c2dc265f4..30f435186f 100644 --- a/webview/src/plots/components/App.test.tsx +++ b/webview/src/plots/components/App.test.tsx @@ -1787,6 +1787,7 @@ describe('App', () => { }) }) + // eslint-disable-next-line sonarjs/cognitive-complexity describe('Ribbon', () => { const getDisplayedRevisionOrder = () => { const ribbon = screen.getByTestId('ribbon') @@ -1877,6 +1878,40 @@ describe('App', () => { type: MessageFromWebviewType.REFRESH_REVISIONS }) }) + + it('should show an error indicator for each revision with an error', () => { + renderAppWithOptionalData({ + comparison: comparisonTableFixture, + selectedRevisions: plotsRevisionsFixture.map(rev => { + if (rev.revision === 'main') { + return { + ...rev, + errors: ['error'] + } + } + return rev + }) + }) + const errorIndicators = screen.getAllByText('!') + expect(errorIndicators).toHaveLength(1) + }) + + it('should not show an error indicator for a loading revision', () => { + renderAppWithOptionalData({ + comparison: comparisonTableFixture, + selectedRevisions: plotsRevisionsFixture.map(rev => { + if (rev.revision === 'main') { + return { + ...rev, + errors: ['error'], + fetched: false + } + } + return rev + }) + }) + expect(screen.queryByText('!')).not.toBeInTheDocument() + }) }) describe('Vega panels', () => { diff --git a/webview/src/plots/components/ribbon/RibbonBlock.tsx b/webview/src/plots/components/ribbon/RibbonBlock.tsx index c6a1703866..a8ec8e4626 100644 --- a/webview/src/plots/components/ribbon/RibbonBlock.tsx +++ b/webview/src/plots/components/ribbon/RibbonBlock.tsx @@ -18,6 +18,16 @@ export enum CopyTooltip { COPIED = 'Copied' } +const RevisionIcon: React.FC<{ fetched: boolean; errors?: string[] }> = ({ + fetched, + errors +}) => ( +
+ {fetched && errors && '!'} + {!fetched && } +
+) + export const RibbonBlock: React.FC = ({ revision, onClear @@ -25,6 +35,7 @@ export const RibbonBlock: React.FC = ({ const { firstThreeColumns, commit, + errors, fetched, group, id, @@ -47,7 +58,7 @@ export const RibbonBlock: React.FC = ({ {group &&
{rev}
}
- {!fetched && } +