From 25a01c3950dabd7e02e6268e2aa8888215cbe2fa Mon Sep 17 00:00:00 2001 From: Matt Seddon Date: Tue, 16 Aug 2022 14:48:19 +1000 Subject: [PATCH 1/3] remove exp show and plots diff retry when git repo has no commits --- extension/src/cli/constants.ts | 2 ++ extension/src/cli/reader.test.ts | 34 +++++++++++++++++- extension/src/cli/reader.ts | 42 ++++++++++++++++++++--- extension/src/cli/retry.test.ts | 4 +-- extension/src/cli/retry.ts | 25 +++++++++++--- extension/src/experiments/data/collect.ts | 4 +-- extension/src/experiments/index.ts | 4 +++ extension/src/experiments/model/tree.ts | 15 ++++---- 8 files changed, 109 insertions(+), 21 deletions(-) diff --git a/extension/src/cli/constants.ts b/extension/src/cli/constants.ts index 36d13c97a6..6f2c90dc8a 100644 --- a/extension/src/cli/constants.ts +++ b/extension/src/cli/constants.ts @@ -2,6 +2,8 @@ export const MIN_CLI_VERSION = '2.11.0' export const LATEST_TESTED_CLI_VERSION = '2.13.0' export const MAX_CLI_VERSION = '3' +export const EMPTY_REPO_ERROR = 'unexpected error - Empty git repo' + export enum Command { ADD = 'add', CHECKOUT = 'checkout', diff --git a/extension/src/cli/reader.test.ts b/extension/src/cli/reader.test.ts index e5524aa124..4a19d59f33 100644 --- a/extension/src/cli/reader.test.ts +++ b/extension/src/cli/reader.test.ts @@ -2,7 +2,8 @@ import { join } from 'path' import { EventEmitter } from 'vscode' import { Disposable, Disposer } from '@hediet/std/disposable' import { CliResult, CliStarted } from '.' -import { CliReader } from './reader' +import { EMPTY_REPO_ERROR } from './constants' +import { CliReader, ExperimentsOutput } from './reader' import { createProcess } from '../processExecution' import { getFailingMockedProcess, getMockedProcess } from '../test/util/jest' import { getProcessEnv } from '../env' @@ -76,6 +77,37 @@ describe('CliReader', () => { executable: 'dvc' }) }) + + it('should return the default output if the cli returns an empty repo error', async () => { + const cwd = __dirname + mockedCreateProcess.mockImplementationOnce(() => { + throw new Error(EMPTY_REPO_ERROR) + }) + + const cliOutput = await cliReader.expShow(cwd) + expect(cliOutput).toStrictEqual({ workspace: { baseline: {} } }) + }) + + it('should retry the cli given any other type of error', async () => { + const cwd = __dirname + const mockOutput: ExperimentsOutput = { + workspace: { + baseline: { + data: { params: { 'params.yaml': { data: { epochs: 100000000 } } } } + } + } + } + mockedCreateProcess.mockImplementationOnce(() => { + throw new Error('error that should be retried') + }) + mockedCreateProcess.mockReturnValueOnce( + getMockedProcess(JSON.stringify(mockOutput)) + ) + + const cliOutput = await cliReader.expShow(cwd) + expect(cliOutput).toStrictEqual(mockOutput) + expect(mockedCreateProcess).toBeCalledTimes(2) + }) }) describe('diff', () => { diff --git a/extension/src/cli/reader.ts b/extension/src/cli/reader.ts index 7c2f6ec166..8ecf50b6a5 100644 --- a/extension/src/cli/reader.ts +++ b/extension/src/cli/reader.ts @@ -3,6 +3,7 @@ import { Cli, typeCheckCommands } from '.' import { Args, Command, + EMPTY_REPO_ERROR, ExperimentFlag, Flag, ListFlag, @@ -108,6 +109,10 @@ export interface ExperimentsOutput { } } +const defaultExperimentsOutput: ExperimentsOutput = { + workspace: { baseline: {} } +} + export interface PlotsOutput { [path: string]: Plot[] } @@ -132,9 +137,11 @@ export class CliReader extends Cli { cwd: string, ...flags: ExperimentFlag[] ): Promise { - return this.readProcessJson( + return this.readProcessJsonWithKnownErrors( cwd, Command.EXPERIMENT, + defaultExperimentsOutput, + [EMPTY_REPO_ERROR], SubCommand.SHOW, ...flags ) @@ -155,10 +162,12 @@ export class CliReader extends Cli { } public plotsDiff(cwd: string, ...revisions: string[]): Promise { - return this.readProcessJson( + return this.readProcessJsonWithKnownErrors( cwd, Command.PLOTS, - 'diff', + {}, + [EMPTY_REPO_ERROR], + Command.DIFF, ...revisions, Flag.OUTPUT_PATH, TEMP_PLOTS_DIR, @@ -184,11 +193,15 @@ export class CliReader extends Cli { cwd: string, formatter: typeof trimAndSplit | typeof JSON.parse | typeof trim, defaultValue: string, + nonRetryErrors: string[], ...args: Args ): Promise { const output = - (await retry(() => this.executeProcess(cwd, ...args), args.join(' '))) || - defaultValue + (await retry( + () => this.executeProcess(cwd, ...args), + args.join(' '), + nonRetryErrors + )) || defaultValue if (!formatter) { return output as unknown as T } @@ -200,6 +213,25 @@ export class CliReader extends Cli { cwd, JSON.parse, '{}', + [], + command, + ...args, + Flag.SHOW_JSON + ) + } + + private readProcessJsonWithKnownErrors( + cwd: string, + command: Command, + defaultValue: T, + nonRetryErrors: string[], + ...args: Args + ) { + return this.readProcess( + cwd, + JSON.parse, + JSON.stringify(defaultValue), + nonRetryErrors, command, ...args, Flag.SHOW_JSON diff --git a/extension/src/cli/retry.test.ts b/extension/src/cli/retry.test.ts index 0378833ae3..c052a6557d 100644 --- a/extension/src/cli/retry.test.ts +++ b/extension/src/cli/retry.test.ts @@ -17,7 +17,7 @@ describe('retry', () => { const promiseRefresher = jest.fn().mockImplementation(() => promise()) - const output = await retry(promiseRefresher, 'Definitely did not') + const output = await retry(promiseRefresher, 'Definitely did not', []) expect(output).toStrictEqual(returnValue) @@ -45,7 +45,7 @@ describe('retry', () => { .fn() .mockImplementation(() => unreliablePromise()) - await retry(promiseRefresher, 'Data update') + await retry(promiseRefresher, 'Data update', []) expect(promiseRefresher).toBeCalledTimes(4) expect(mockedDelay).toBeCalledTimes(3) diff --git a/extension/src/cli/retry.ts b/extension/src/cli/retry.ts index 3ce1ca3b29..371c8271f2 100644 --- a/extension/src/cli/retry.ts +++ b/extension/src/cli/retry.ts @@ -1,18 +1,35 @@ import { delay } from '../util/time' import { Logger } from '../common/logger' -export const retry = async ( - getNewPromise: () => Promise, +const isNonRetryError = ( + errorMessage: string, + nonRetryErrors: string[] +): boolean => { + for (const partialErrorMessage of nonRetryErrors) { + if (errorMessage.includes(partialErrorMessage)) { + return true + } + } + return false +} + +export const retry = async ( + getNewPromise: () => Promise, args: string, + nonRetryErrors: string[], waitBeforeRetry = 500 -): Promise => { +): Promise => { try { return await getNewPromise() } catch (error: unknown) { const errorMessage = (error as Error).message Logger.error(`${args} failed with ${errorMessage} retrying...`) + if (isNonRetryError(errorMessage, nonRetryErrors)) { + return '' + } + await delay(waitBeforeRetry) - return retry(getNewPromise, args, waitBeforeRetry * 2) + return retry(getNewPromise, args, nonRetryErrors, waitBeforeRetry * 2) } } diff --git a/extension/src/experiments/data/collect.ts b/extension/src/experiments/data/collect.ts index 227c8aa4ad..30add5506a 100644 --- a/extension/src/experiments/data/collect.ts +++ b/extension/src/experiments/data/collect.ts @@ -3,8 +3,8 @@ import { ExperimentsOutput } from '../../cli/reader' export const collectFiles = (data: ExperimentsOutput): string[] => { const files = new Set( Object.keys({ - ...data?.workspace.baseline?.data?.params, - ...data?.workspace.baseline?.data?.metrics + ...data?.workspace?.baseline?.data?.params, + ...data?.workspace?.baseline?.data?.metrics }).filter(Boolean) ) diff --git a/extension/src/experiments/index.ts b/extension/src/experiments/index.ts index 172715e3c3..18236a9326 100644 --- a/extension/src/experiments/index.ts +++ b/extension/src/experiments/index.ts @@ -277,6 +277,10 @@ export class Experiments extends BaseRepository { } public getExperimentCount() { + if (!this.columns.hasColumns()) { + return 0 + } + return this.experiments.getExperimentCount() } diff --git a/extension/src/experiments/model/tree.ts b/extension/src/experiments/model/tree.ts index b55eb6eac2..bfc5da4f95 100644 --- a/extension/src/experiments/model/tree.ts +++ b/extension/src/experiments/model/tree.ts @@ -268,7 +268,14 @@ export class ExperimentsTree private getDescription() { const dvcRoots = this.experiments.getDvcRoots() - if (!definedAndNonEmpty(dvcRoots)) { + + const total = sum( + dvcRoots.map(dvcRoot => + this.experiments.getRepository(dvcRoot).getExperimentCount() + ) + ) + + if (!total) { return } @@ -279,12 +286,6 @@ export class ExperimentsTree ) ) - const total = sum( - dvcRoots.map(dvcRoot => - this.experiments.getRepository(dvcRoot).getExperimentCount() - ) - ) - return `${selected} of ${total} (max ${MAX_SELECTED_EXPERIMENTS})` } From 3f0d7ed886bd496520ecf3811b26fb243b29efb7 Mon Sep 17 00:00:00 2001 From: Matt Seddon Date: Wed, 17 Aug 2022 10:19:51 +1000 Subject: [PATCH 2/3] any retry and return a default when error is unexpected --- extension/src/cli/constants.ts | 2 +- extension/src/cli/reader.test.ts | 7 ++++-- extension/src/cli/reader.ts | 41 +++++++------------------------- extension/src/cli/retry.test.ts | 4 ++-- extension/src/cli/retry.ts | 19 +++++---------- 5 files changed, 22 insertions(+), 51 deletions(-) diff --git a/extension/src/cli/constants.ts b/extension/src/cli/constants.ts index 6f2c90dc8a..5ff51a6897 100644 --- a/extension/src/cli/constants.ts +++ b/extension/src/cli/constants.ts @@ -2,7 +2,7 @@ export const MIN_CLI_VERSION = '2.11.0' export const LATEST_TESTED_CLI_VERSION = '2.13.0' export const MAX_CLI_VERSION = '3' -export const EMPTY_REPO_ERROR = 'unexpected error - Empty git repo' +export const UNEXPECTED_ERROR_CODE = 255 export enum Command { ADD = 'add', diff --git a/extension/src/cli/reader.test.ts b/extension/src/cli/reader.test.ts index 4a19d59f33..3956e6636c 100644 --- a/extension/src/cli/reader.test.ts +++ b/extension/src/cli/reader.test.ts @@ -2,7 +2,8 @@ import { join } from 'path' import { EventEmitter } from 'vscode' import { Disposable, Disposer } from '@hediet/std/disposable' import { CliResult, CliStarted } from '.' -import { EMPTY_REPO_ERROR } from './constants' +import { UNEXPECTED_ERROR_CODE } from './constants' +import { MaybeConsoleError } from './error' import { CliReader, ExperimentsOutput } from './reader' import { createProcess } from '../processExecution' import { getFailingMockedProcess, getMockedProcess } from '../test/util/jest' @@ -80,8 +81,10 @@ describe('CliReader', () => { it('should return the default output if the cli returns an empty repo error', async () => { const cwd = __dirname + const error = new Error('unexpected error - something something') + ;(error as MaybeConsoleError).exitCode = UNEXPECTED_ERROR_CODE mockedCreateProcess.mockImplementationOnce(() => { - throw new Error(EMPTY_REPO_ERROR) + throw error }) const cliOutput = await cliReader.expShow(cwd) diff --git a/extension/src/cli/reader.ts b/extension/src/cli/reader.ts index 8ecf50b6a5..889714a648 100644 --- a/extension/src/cli/reader.ts +++ b/extension/src/cli/reader.ts @@ -3,7 +3,6 @@ import { Cli, typeCheckCommands } from '.' import { Args, Command, - EMPTY_REPO_ERROR, ExperimentFlag, Flag, ListFlag, @@ -137,13 +136,14 @@ export class CliReader extends Cli { cwd: string, ...flags: ExperimentFlag[] ): Promise { - return this.readProcessJsonWithKnownErrors( + return this.readProcess( cwd, + JSON.parse, + JSON.stringify(defaultExperimentsOutput), Command.EXPERIMENT, - defaultExperimentsOutput, - [EMPTY_REPO_ERROR], SubCommand.SHOW, - ...flags + ...flags, + Flag.SHOW_JSON ) } @@ -162,11 +162,9 @@ export class CliReader extends Cli { } public plotsDiff(cwd: string, ...revisions: string[]): Promise { - return this.readProcessJsonWithKnownErrors( + return this.readProcessJson( cwd, Command.PLOTS, - {}, - [EMPTY_REPO_ERROR], Command.DIFF, ...revisions, Flag.OUTPUT_PATH, @@ -193,15 +191,11 @@ export class CliReader extends Cli { cwd: string, formatter: typeof trimAndSplit | typeof JSON.parse | typeof trim, defaultValue: string, - nonRetryErrors: string[], ...args: Args ): Promise { const output = - (await retry( - () => this.executeProcess(cwd, ...args), - args.join(' '), - nonRetryErrors - )) || defaultValue + (await retry(() => this.executeProcess(cwd, ...args), args.join(' '))) || + defaultValue if (!formatter) { return output as unknown as T } @@ -213,25 +207,6 @@ export class CliReader extends Cli { cwd, JSON.parse, '{}', - [], - command, - ...args, - Flag.SHOW_JSON - ) - } - - private readProcessJsonWithKnownErrors( - cwd: string, - command: Command, - defaultValue: T, - nonRetryErrors: string[], - ...args: Args - ) { - return this.readProcess( - cwd, - JSON.parse, - JSON.stringify(defaultValue), - nonRetryErrors, command, ...args, Flag.SHOW_JSON diff --git a/extension/src/cli/retry.test.ts b/extension/src/cli/retry.test.ts index c052a6557d..d034fef76d 100644 --- a/extension/src/cli/retry.test.ts +++ b/extension/src/cli/retry.test.ts @@ -17,7 +17,7 @@ describe('retry', () => { const promiseRefresher = jest.fn().mockImplementation(() => promise()) - const output = await retry(promiseRefresher, 'Definitely did not', []) + const output = await retry(promiseRefresher, 'Definitely did not') expect(output).toStrictEqual(returnValue) @@ -45,7 +45,7 @@ describe('retry', () => { .fn() .mockImplementation(() => unreliablePromise()) - await retry(promiseRefresher, 'Data update', []) + await retry(promiseRefresher, 'Data update') expect(promiseRefresher).toBeCalledTimes(4) expect(mockedDelay).toBeCalledTimes(3) diff --git a/extension/src/cli/retry.ts b/extension/src/cli/retry.ts index 371c8271f2..1995e3b5fd 100644 --- a/extension/src/cli/retry.ts +++ b/extension/src/cli/retry.ts @@ -1,22 +1,15 @@ +import { UNEXPECTED_ERROR_CODE } from './constants' +import { MaybeConsoleError } from './error' import { delay } from '../util/time' import { Logger } from '../common/logger' -const isNonRetryError = ( - errorMessage: string, - nonRetryErrors: string[] -): boolean => { - for (const partialErrorMessage of nonRetryErrors) { - if (errorMessage.includes(partialErrorMessage)) { - return true - } - } - return false +const isUnexpectedError = (error: unknown): boolean => { + return (error as MaybeConsoleError).exitCode === UNEXPECTED_ERROR_CODE } export const retry = async ( getNewPromise: () => Promise, args: string, - nonRetryErrors: string[], waitBeforeRetry = 500 ): Promise => { try { @@ -25,11 +18,11 @@ export const retry = async ( const errorMessage = (error as Error).message Logger.error(`${args} failed with ${errorMessage} retrying...`) - if (isNonRetryError(errorMessage, nonRetryErrors)) { + if (isUnexpectedError(error)) { return '' } await delay(waitBeforeRetry) - return retry(getNewPromise, args, nonRetryErrors, waitBeforeRetry * 2) + return retry(getNewPromise, args, waitBeforeRetry * 2) } } From 08a1eafd5f2e87a5af36fe9e91e39a8b2d7269dc Mon Sep 17 00:00:00 2001 From: Matt Seddon Date: Wed, 17 Aug 2022 11:29:13 +1000 Subject: [PATCH 3/3] self review --- extension/src/cli/reader.test.ts | 2 +- extension/src/cli/retry.ts | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/extension/src/cli/reader.test.ts b/extension/src/cli/reader.test.ts index 3956e6636c..683e21fa42 100644 --- a/extension/src/cli/reader.test.ts +++ b/extension/src/cli/reader.test.ts @@ -79,7 +79,7 @@ describe('CliReader', () => { }) }) - it('should return the default output if the cli returns an empty repo error', async () => { + it('should return the default output if the cli returns an unexpected error (255 exit code)', async () => { const cwd = __dirname const error = new Error('unexpected error - something something') ;(error as MaybeConsoleError).exitCode = UNEXPECTED_ERROR_CODE diff --git a/extension/src/cli/retry.ts b/extension/src/cli/retry.ts index 1995e3b5fd..6ed2e34d00 100644 --- a/extension/src/cli/retry.ts +++ b/extension/src/cli/retry.ts @@ -4,7 +4,7 @@ import { delay } from '../util/time' import { Logger } from '../common/logger' const isUnexpectedError = (error: unknown): boolean => { - return (error as MaybeConsoleError).exitCode === UNEXPECTED_ERROR_CODE + return (error as MaybeConsoleError)?.exitCode === UNEXPECTED_ERROR_CODE } export const retry = async (