Skip to content

Commit

Permalink
Show errors in plots ribbon (#3570)
Browse files Browse the repository at this point in the history
  • Loading branch information
@mattseddon
mattseddon authored Apr 2, 2023
1 parent 0a97521 commit ad944f2
Show file tree
Hide file tree
Showing 15 changed files with 223 additions and 39 deletions.
14 changes: 7 additions & 7 deletions extension/src/plots/errors/collect.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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', () => {
Expand Down Expand Up @@ -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`
)
})
})
Expand Down Expand Up @@ -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. |'
)
})

Expand Down Expand Up @@ -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'). |"
)
})
})
10 changes: 7 additions & 3 deletions extension/src/plots/errors/collect.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 = (
Expand Down
13 changes: 12 additions & 1 deletion extension/src/plots/errors/model.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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'
Expand Down Expand Up @@ -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()
}
Expand Down
59 changes: 59 additions & 0 deletions extension/src/plots/model/collect.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -259,6 +259,7 @@ describe('collectOverrideRevisionDetails', () => {
}
] as Experiment[]
}[id]),
() => undefined,
[]
)
expect(overrideComparison.map(({ revision }) => revision)).toStrictEqual([
Expand All @@ -270,6 +271,7 @@ describe('collectOverrideRevisionDetails', () => {
expect(overrideRevisions).toStrictEqual([
{
displayColor: '#4299e1',
errors: undefined,
fetched: true,
firstThreeColumns: [],
group: 'a',
Expand All @@ -278,6 +280,7 @@ describe('collectOverrideRevisionDetails', () => {
},
{
displayColor: '#13adc7',
errors: undefined,
fetched: true,
firstThreeColumns: [],
group: runningGroup,
Expand All @@ -286,6 +289,7 @@ describe('collectOverrideRevisionDetails', () => {
},
{
displayColor: '#48bb78',
errors: undefined,
fetched: true,
firstThreeColumns: [],
group: 'c',
Expand All @@ -295,6 +299,7 @@ describe('collectOverrideRevisionDetails', () => {
{
commit: 'Upgrade dependencies ...',
displayColor: '#f56565',
errors: undefined,
fetched: true,
firstThreeColumns: [],
group: 'd',
Expand Down Expand Up @@ -373,6 +378,7 @@ describe('collectOverrideRevisionDetails', () => {
}
] as Experiment[]
}[id]),
() => undefined,
[]
)
expect(overrideComparison.map(({ revision }) => revision)).toStrictEqual([
Expand All @@ -384,6 +390,7 @@ describe('collectOverrideRevisionDetails', () => {
expect(overrideRevisions).toStrictEqual([
{
displayColor: '#4299e1',
errors: undefined,
fetched: false,
firstThreeColumns: [],
group: 'a',
Expand All @@ -392,6 +399,7 @@ describe('collectOverrideRevisionDetails', () => {
},
{
displayColor: '#13adc7',
errors: undefined,
fetched: false,
firstThreeColumns: [],
group: undefined,
Expand All @@ -400,6 +408,7 @@ describe('collectOverrideRevisionDetails', () => {
},
{
displayColor: '#48bb78',
errors: undefined,
fetched: false,
firstThreeColumns: [],
group: 'c',
Expand All @@ -408,6 +417,7 @@ describe('collectOverrideRevisionDetails', () => {
},
{
displayColor: '#f56565',
errors: undefined,
fetched: false,
firstThreeColumns: [],
group: 'd',
Expand Down Expand Up @@ -486,6 +496,7 @@ describe('collectOverrideRevisionDetails', () => {
}
] as Experiment[]
}[id]),
() => undefined,
[]
)
expect(overrideComparison.map(({ revision }) => revision)).toStrictEqual([
Expand Down Expand Up @@ -525,6 +536,7 @@ describe('collectOverrideRevisionDetails', () => {
{ [justFinishedRunningId]: justFinishedRunningId },
(id: string) =>
({ [justFinishedRunningId]: [{ label: 'e' }] as Experiment[] }[id]),
() => undefined,
[]
)
expect(overrideComparison.map(({ revision }) => revision)).toStrictEqual([
Expand All @@ -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', () => {
Expand Down
35 changes: 25 additions & 10 deletions extension/src/plots/model/collect.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -555,12 +555,14 @@ const getOverrideRevision = (
displayColor: Color,
experiment: Experiment,
firstThreeColumns: string[],
fetchedRevs: Set<string>
fetchedRevs: Set<string>,
errors: string[] | undefined
): Revision => {
const { commit, displayNameOrParent, logicalGroupName, id, label } =
experiment
const revision: Revision = {
displayColor,
errors,
fetched: fetchedRevs.has(label),
firstThreeColumns: getRevisionFirstThreeColumns(
firstThreeColumns,
Expand All @@ -582,7 +584,8 @@ const overrideWithWorkspace = (
displayColor: Color,
label: string,
firstThreeColumns: string[],
fetchedRevs: Set<string>
fetchedRevs: Set<string>,
errors: string[] | undefined
): void => {
orderMapping[label] = EXPERIMENT_WORKSPACE_ID
selectedWithOverrides.push(
Expand All @@ -594,7 +597,8 @@ const overrideWithWorkspace = (
logicalGroupName: undefined
},
firstThreeColumns,
fetchedRevs
fetchedRevs,
errors
)
)
}
Expand All @@ -615,7 +619,8 @@ const getMostRecentFetchedCheckpointRevision = (
fetchedRevs: Set<string>,
revisionsWithData: Set<string>,
checkpoints: Experiment[] | undefined,
firstThreeColumns: string[]
firstThreeColumns: string[],
errors: string[] | undefined
): Revision => {
const mostRecent =
checkpoints?.find(({ label }) => revisionsWithData.has(label)) ||
Expand All @@ -624,7 +629,8 @@ const getMostRecentFetchedCheckpointRevision = (
selectedRevision.displayColor,
mostRecent,
firstThreeColumns,
fetchedRevs
fetchedRevs,
errors
)
}

Expand All @@ -635,7 +641,8 @@ const overrideRevisionDetail = (
fetchedRevs: Set<string>,
revisionsWithData: Set<string>,
checkpoints: Experiment[] | undefined,
firstThreeColumns: string[]
firstThreeColumns: string[],
errors: string[] | undefined
) => {
const { label } = selectedRevision

Expand All @@ -644,7 +651,8 @@ const overrideRevisionDetail = (
fetchedRevs,
revisionsWithData,
checkpoints,
firstThreeColumns
firstThreeColumns,
errors
)
orderMapping[label] = mostRecent.revision
selectedWithOverrides.push(mostRecent)
Expand All @@ -658,9 +666,11 @@ const collectRevisionDetail = (
revisionsWithData: Set<string>,
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) &&
Expand All @@ -672,7 +682,8 @@ const collectRevisionDetail = (
displayColor,
label,
firstThreeColumns,
fetchedRevs
fetchedRevs,
errors
)
}

Expand All @@ -691,7 +702,8 @@ const collectRevisionDetail = (
fetchedRevs,
revisionsWithData,
getCheckpoints(id),
firstThreeColumns
firstThreeColumns,
errors
)
}

Expand All @@ -701,7 +713,8 @@ const collectRevisionDetail = (
displayColor,
selectedRevision,
firstThreeColumns,
fetchedRevs
fetchedRevs,
errors
)
)
}
Expand All @@ -713,6 +726,7 @@ export const collectOverrideRevisionDetails = (
revisionsWithData: Set<string>,
unfinishedRunningExperiments: { [id: string]: string },
getCheckpoints: (id: string) => Experiment[] | undefined,
getErrors: (label: string) => string[] | undefined,
firstThreeColumns: string[]
): {
overrideComparison: Revision[]
Expand All @@ -730,6 +744,7 @@ export const collectOverrideRevisionDetails = (
revisionsWithData,
unfinishedRunningExperiments,
getCheckpoints,
getErrors,
firstThreeColumns
)
}
Expand Down
5 changes: 4 additions & 1 deletion extension/src/plots/model/index.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down
Loading

0 comments on commit ad944f2

Please sign in to comment.