Skip to content

Commit

Permalink
Do not process CLI errors thrown by plots diff
Browse files Browse the repository at this point in the history
  • Loading branch information
mattseddon committed Nov 29, 2022
1 parent 9397d1d commit 22e19f8
Show file tree
Hide file tree
Showing 7 changed files with 48 additions and 15 deletions.
2 changes: 2 additions & 0 deletions extension/src/cli/dvc/contract.ts
Original file line number Diff line number Diff line change
Expand Up @@ -93,3 +93,5 @@ export interface ExperimentsOutput {
export interface PlotsOutput {
[path: string]: Plot[]
}

export type PlotsOutputOrError = PlotsOutput | DvcError
10 changes: 6 additions & 4 deletions extension/src/cli/dvc/reader.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,8 @@ import {
DataStatusOutput,
DvcError,
ExperimentsOutput,
PlotsOutput
PlotsOutput,
PlotsOutputOrError
} from './contract'
import { getOptions } from './options'
import { typeCheckCommands } from '..'
Expand All @@ -29,7 +30,8 @@ export const isDvcError = <
T extends ExperimentsOutput | DataStatusOutput | PlotsOutput
>(
dataOrError: T | DvcError
): dataOrError is DvcError => !!(dataOrError as DvcError).error
): dataOrError is DvcError =>
!!(Object.keys(dataOrError).length === 1 && (dataOrError as DvcError).error)

export const autoRegisteredCommands = {
DATA_STATUS: 'dataStatus',
Expand Down Expand Up @@ -80,7 +82,7 @@ export class DvcReader extends DvcCli {
public plotsDiff(
cwd: string,
...revisions: string[]
): Promise<PlotsOutput | DvcError> {
): Promise<PlotsOutputOrError> {
return this.readProcessJson<PlotsOutput>(
cwd,
Command.PLOTS,
Expand Down Expand Up @@ -127,7 +129,7 @@ export class DvcReader extends DvcCli {
} catch (error: unknown) {
const msg =
(error as MaybeConsoleError).stderr || (error as Error).message
Logger.error(`${args} failed with ${msg} retrying...`)
Logger.error(`${args} failed with ${msg}`)
return { error: { msg, type: 'Caught error' } }
}
}
Expand Down
4 changes: 2 additions & 2 deletions extension/src/data/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,12 @@ import {
} from '../fileSystem/watcher'
import { ProcessManager } from '../processManager'
import { InternalCommands } from '../commands/internal'
import { ExperimentsOutput, PlotsOutput } from '../cli/dvc/contract'
import { ExperimentsOutput, PlotsOutputOrError } from '../cli/dvc/contract'
import { definedAndNonEmpty, sameContents, uniqueValues } from '../util/array'
import { DeferredDisposable } from '../class/deferred'

export abstract class BaseData<
T extends { data: PlotsOutput; revs: string[] } | ExperimentsOutput
T extends { data: PlotsOutputOrError; revs: string[] } | ExperimentsOutput
> extends DeferredDisposable {
public readonly onDidUpdate: Event<T>

Expand Down
17 changes: 12 additions & 5 deletions extension/src/plots/data/index.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import { EventEmitter } from 'vscode'
import { PlotsOutput } from '../../cli/dvc/contract'
import { PlotsOutputOrError } from '../../cli/dvc/contract'
import { isDvcError } from '../../cli/dvc/reader'
import { AvailableCommands, InternalCommands } from '../../commands/internal'
import { BaseData } from '../../data'
import {
Expand All @@ -9,7 +10,10 @@ import {
} from '../../util/array'
import { PlotsModel } from '../model'

export class PlotsData extends BaseData<{ data: PlotsOutput; revs: string[] }> {
export class PlotsData extends BaseData<{
data: PlotsOutputOrError
revs: string[]
}> {
private readonly model: PlotsModel

constructor(
Expand Down Expand Up @@ -49,7 +53,7 @@ export class PlotsData extends BaseData<{ data: PlotsOutput; revs: string[] }> {
}

const args = this.getArgs(revs)
const data = await this.internalCommands.executeCommand<PlotsOutput>(
const data = await this.internalCommands.executeCommand<PlotsOutputOrError>(
AvailableCommands.PLOTS_DIFF,
this.dvcRoot,
...args
Expand All @@ -66,8 +70,11 @@ export class PlotsData extends BaseData<{ data: PlotsOutput; revs: string[] }> {
return this.processManager.run('update')
}

public collectFiles({ data }: { data: PlotsOutput }) {
return Object.keys(data)
public collectFiles({ data }: { data: PlotsOutputOrError }) {
if (isDvcError(data)) {
return this.collectedFiles
}
return [...Object.keys(data), ...this.collectedFiles]
}

private getArgs(revs: string[]) {
Expand Down
9 changes: 7 additions & 2 deletions extension/src/plots/model/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ import {
SectionCollapsed,
PlotSizeNumber
} from '../webview/contract'
import { ExperimentsOutput, PlotsOutput } from '../../cli/dvc/contract'
import { ExperimentsOutput, PlotsOutputOrError } from '../../cli/dvc/contract'
import { Experiments } from '../../experiments'
import { getColorScale, truncateVerticalTitle } from '../vega/util'
import { definedAndNonEmpty, reorderObjectList } from '../../util/array'
Expand All @@ -39,6 +39,7 @@ import {
MultiSourceEncoding,
MultiSourceVariations
} from '../multiSource/collect'
import { isDvcError } from '../../cli/dvc/reader'

export class PlotsModel extends ModelWithPersistence {
private readonly experiments: Experiments
Expand Down Expand Up @@ -104,7 +105,11 @@ export class PlotsModel extends ModelWithPersistence {
return this.removeStaleData()
}

public async transformAndSetPlots(data: PlotsOutput, revs: string[]) {
public async transformAndSetPlots(data: PlotsOutputOrError, revs: string[]) {
if (isDvcError(data)) {
return
}

const cliIdToLabel = this.getCLIIdToLabel()

this.fetchedRevs = new Set([
Expand Down
12 changes: 12 additions & 0 deletions extension/src/plots/paths/model.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -257,4 +257,16 @@ describe('PathsModel', () => {
const noChildren = model.getChildren(logsLoss)
expect(noChildren).toStrictEqual([])
})

it('should not provide error as a path when the CLI throws an error', () => {
const model = new PathsModel(mockDvcRoot, buildMockMemento())
model.transformAndSet({
error: {
msg: 'UNEXPECTED ERROR: a strange thing happened',
type: 'Caught Error'
}
})

expect(model.getTerminalNodes()).toStrictEqual([])
})
})
9 changes: 7 additions & 2 deletions extension/src/plots/paths/model.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,12 @@ import {
PlotPath,
TemplateOrder
} from './collect'
import { PlotsOutput } from '../../cli/dvc/contract'
import { PathSelectionModel } from '../../path/selection/model'
import { PersistenceKey } from '../../persistence/constants'
import { performSimpleOrderedUpdate } from '../../util/array'
import { MultiSourceEncoding } from '../multiSource/collect'
import { isDvcError } from '../../cli/dvc/reader'
import { PlotsOutputOrError } from '../../cli/dvc/contract'

export class PathsModel extends PathSelectionModel<PlotPath> {
private templateOrder: TemplateOrder
Expand All @@ -26,7 +27,11 @@ export class PathsModel extends PathSelectionModel<PlotPath> {
)
}

public transformAndSet(data: PlotsOutput) {
public transformAndSet(data: PlotsOutputOrError) {
if (isDvcError(data)) {
return
}

const paths = collectPaths(this.data, data)

this.setNewStatuses(paths)
Expand Down

0 comments on commit 22e19f8

Please sign in to comment.